Page MenuHomeFreeBSD

Microsemi smartpqi SCSI driver for PQI controllers
ClosedPublic

Authored by deepak.ukey_microsemi.com on Feb 26 2018, 8:58 AM.

Details

Summary

The driver provides support for the new generation of PQI controllers from Microsemi. This driver is the first SCSI driver to implement the PQI queuing model and it will replace the aacraid driver for Adaptec Series 9 controllers.
HARDWARE Controllers supported by the driver include:

  1. HPE Gen10 Smart Array Controller Family
  2. OEM Controllers based on the Microsemi Chipset.
Test Plan

Microsemi Product verification group has developed the test plan and executed them so that there is maximum code path coverage.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deepak.ukey_microsemi.com created this object with visibility "Custom Policy".

Hi,

This is the Microsemi Smartpqi SCSI driver for PQI controllers. Please review it.

Regards,
Deepak

This revision is now accepted and ready to land.Feb 26 2018, 9:03 AM
This revision now requires review to proceed.Mar 7 2018, 10:05 AM

This review has been restricted from general visibility; is this intentional?

This review has been restricted from general visibility; is this intentional?

Currently we created a custom policy so that only reviewer can see it. Once review completes, we will make it public.

I've added a bunch of review comments. Nothing super major.

The man page should indicate what loader.conf or sysctl.conf tuneables if any are available. It should also indicate what compile time options if any are available for this driver.

share/man/man4/smartpqi.4
1 ↗(On Diff #39736)

Did Scott write this man page? Or is this from Microsemi?

25 ↗(On Diff #39736)

This syntax should be reset to:

.\" $FreeBSD$

This will allow the subversion server to insert the correct information.

26 ↗(On Diff #39736)

Date should be updated.

100 ↗(On Diff #39736)

I certainly don't *remember* writing this man page. :-) Who is the actual author here?

sys/dev/smartpqi/smartpqi_cam.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

57 ↗(On Diff #39736)

Is this really the "base" transfer speed of this device? aacraid(4) advertises this as its base speed, but this device driver is much newer.

sys/dev/smartpqi/smartpqi_cmd.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_defines.h
2 ↗(On Diff #39736)

Update copyright date to 2018

sys/dev/smartpqi/smartpqi_discovery.c
2 ↗(On Diff #39736)

Update copyright date to 2018

1348 ↗(On Diff #39736)

Hmmm? What does this comment indicate?

sys/dev/smartpqi/smartpqi_event.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_helper.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

215 ↗(On Diff #39736)

Is this missing an entry for "RAID 6"?

sys/dev/smartpqi/smartpqi_includes.h
2 ↗(On Diff #39736)

Update copyright date to 2018.

26 ↗(On Diff #39736)

This file seems like an overuse of include files. Since this is included in almost every file in the driver, it seems like we are including the header files many, many times. This does lead to bloat in the driver but is functionally harmless.

sys/dev/smartpqi/smartpqi_init.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_intr.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_ioctl.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_ioctl.h
2 ↗(On Diff #39736)

Update copyright date to 2018.

68 ↗(On Diff #39736)

I suspect that this will not work on Big Endian architectures (but I doubt microsemi is supporting non-x86 architectures with this driver). Should we add a restriction to only AMD64 and I386 for this driver?

sys/dev/smartpqi/smartpqi_main.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

119 ↗(On Diff #39736)

Have any of these devices ever been implemented in any other device in FreeBSD? If they have, we might have to remove devices from them.

sys/dev/smartpqi/smartpqi_mem.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_misc.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_prototypes.h
2 ↗(On Diff #39736)

Update copyright to 2018.

sys/dev/smartpqi/smartpqi_queue.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

80 ↗(On Diff #39736)

I don't mind a hard sleep in drivers, but do you think this should be a try/fail/sleep type of loop instead? Is it permitted to try and copy/validate the response before this timeout?

292 ↗(On Diff #39736)

Stray commented out code? Should it be deleted?

377 ↗(On Diff #39736)

This looks like a debug statement here. What's going on here?

sys/dev/smartpqi/smartpqi_request.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

726 ↗(On Diff #39736)

Hrm ... Is this debugging or something else ?

739 ↗(On Diff #39736)

More debugging to be removed?

746 ↗(On Diff #39736)

More debugging?

sys/dev/smartpqi/smartpqi_response.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_sis.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

244 ↗(On Diff #39736)

Debugging to be removed?

sys/dev/smartpqi/smartpqi_structures.h
2 ↗(On Diff #39736)

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_tag.c
2 ↗(On Diff #39736)

Update copyright date to 2018.

I agree all the other review comments and fixed them. But need some clarifications on the below.

  1. > smartpqi_includes.h:26

+ */
+
+#ifndef _PQI_INCLUDES_H

This file seems like an overuse of include files. Since this is included in almost every file in the driver, it seems like we are including the header files many, many times. This does lead to bloat in the driver but is functionally harmless.

----> We actually have two sets of header files – OS specific and PQI protocol specific. We tried to consolidate them in one header file and use it. So could you please help us out by elaborating what could be the best solution here.

  1. > smartpqi_ioctl.h:68

+ } LogUnit;
+
+}OS_ATTRIBUTE_PACKED SCSI3Addr_struct;

I suspect that this will not work on Big Endian architectures (but I doubt microsemi is supporting non-x86 architectures with this driver). Should we add a restriction to only AMD64 and I386 for this driver?

------>SCSI3Addr_struct etc are the objects used for pass-through ioctl between management software and Firmware. So from driver perspective, ideally we should not be facing the issue. Please correct if we are wrong.

sbruno added a comment.Apr 4 2018, 3:26 PM

I agree all the other review comments and fixed them. But need some clarifications on the below.

  1. > smartpqi_includes.h:26

+ */
+
+#ifndef _PQI_INCLUDES_H

This file seems like an overuse of include files. Since this is included in almost every file in the driver, it seems like we are including the header files many, many times. This does lead to bloat in the driver but is functionally harmless.

----> We actually have two sets of header files – OS specific and PQI protocol specific. We tried to consolidate them in one header file and use it. So could you please help us out by elaborating what could be the best solution here.

I don't think its an "important" review comment. It would be better if each file only included freebsd headers that it needed so that it could be easier to determine what is being used in each file. However, since you are maintaining a common code base in an effort to reduce work, I think this is fine.

  1. > smartpqi_ioctl.h:68

+ } LogUnit;
+
+}OS_ATTRIBUTE_PACKED SCSI3Addr_struct;

I suspect that this will not work on Big Endian architectures (but I doubt microsemi is supporting non-x86 architectures with this driver). Should we add a restriction to only AMD64 and I386 for this driver?

------>SCSI3Addr_struct etc are the objects used for pass-through ioctl between management software and Firmware. So from driver perspective, ideally we should not be facing the issue. Please correct if we are wrong.

In a big-endian system, you would get a struct passed down that would be in an inverted order, right? If you are using the bit operators to divide up your elements, they wouldn't be in the right place. Its not important if you do not wish to support big-endian CPUs, but you should indicate that by only allowing your driver to be used on little-endian CPUs. Does this make sense?

sys/dev/smartpqi/smartpqi_ioctl.h
68 ↗(On Diff #39736)

We don’t support Big Endian architecture at present. so we have added a restriction to only AMD64 and I386 for now.

sys/dev/smartpqi/smartpqi_main.c
119 ↗(On Diff #39736)

ARC used 0x28b, 0x28c, 0x28c and smartpqi uses 0x28f, so there is no overlap of device IDs.

I have Updated the diff with the changes according to comments. Please review it.

About sysctl.conf, Currently we don’t have sysctl.conf tumble parameters. We may add it in future patch.

sbruno added inline comments.Apr 6 2018, 4:30 PM
share/man/man4/smartpqi.4
25 ↗(On Diff #41171)

Please reset this syntax to:

.\" $FreeBSD$
100 ↗(On Diff #39736)

I'm still not an "author" of this man page.

sys/dev/smartpqi/smartpqi_queue.c
74 ↗(On Diff #41171)

Oh, you removed the OS_SLEEP(). Is that ok? I had commented that it might be better to do a while(try); sleep; done type loop, but I didn't mean that it should be completely removed if it is required for functionality.

Updated the diff. Please review it.

sys/dev/smartpqi/smartpqi_queue.c
74 ↗(On Diff #41171)

Yes. It is ok. Earlier we added sleep because of one firmware issue of reading resister. Now it is fixed that's why we removed it.

deepak.ukey_microsemi.com changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbruno accepted this revision.Apr 12 2018, 10:44 PM

I'm running a universe build. After that, would you like this committed?

This revision is now accepted and ready to land.Apr 12 2018, 10:44 PM

I'm running a universe build. After that, would you like this committed?

Yes Sure. Thanks for your help for reviewing and make it open.

sbruno requested changes to this revision.Apr 23 2018, 11:07 PM

Huh, ok. This fails to build on TARGET=i386. I'm not sure if you want to fix this or restrict it to AMD64 only:

make buildkernel TARGET=i386
...
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_init.c:339:21: error: implicit declaration of function 'bus_space_read_8' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                signature = LE_64(PCI_MEM_GET64(softs, &softs->pqi_reg->signature, PQI_SIGNATURE));
                                  ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:874:5: note: expanded from macro 'PCI_MEM_GET64'
    bus_space_read_8(_softs->pci_mem_handle.pqi_btag, \
    ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_init.c:339:21: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:874:5: note: expanded from macro 'PCI_MEM_GET64'
    bus_space_read_8(_softs->pci_mem_handle.pqi_btag, \
    ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_init.c:359:13: error: implicit declaration of function 'bus_space_read_8' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        COND_WAIT((PCI_MEM_GET64(softs, &softs->pqi_reg->admin_q_config,
                   ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:874:5: note: expanded from macro 'PCI_MEM_GET64'
    bus_space_read_8(_softs->pci_mem_handle.pqi_btag, \
    ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_init.c:476:8: error: implicit declaration of function 'bus_space_read_8' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        val = PCI_MEM_GET64(softs, &softs->pqi_reg->pqi_dev_adminq_cap, PQI_ADMINQ_CAP);
              ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:874:5: note: expanded from macro 'PCI_MEM_GET64'
    bus_space_read_8(_softs->pci_mem_handle.pqi_btag, \
    ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_init.c:805:2: error: incompatible pointer types passing 'OS_ATOMIC64_T *' (aka 'volatile unsigned long long *') to parameter of type 'volatile u_long *' (aka 'volatile unsigned long *') [-Werror,-Wincompatible-pointer-types]
        OS_ATOMIC64_SET(softs, num_intrs, 0);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:864:62: note: expanded from macro 'OS_ATOMIC64_SET'
#define OS_ATOMIC64_SET(_softs, target, val)    atomic_set_long(&(_softs)->target, val)
                                                                ^~~~~~~~~~~~~~~~~
./machine/atomic.h:603:1: note: passing argument to parameter 'p' here
ATOMIC_ASM(set,      long,  "orl %1,%0",   "ir",  v);
^
./machine/atomic.h:109:48: note: expanded from macro 'ATOMIC_ASM'
void atomic_##NAME##_##TYPE(volatile u_##TYPE *p, u_##TYPE v);  \
                                               ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_init.c:819:33: error: implicit declaration of function 'bus_space_read_8' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                softs->prev_heartbeat_count = CTRLR_HEARTBEAT_CNT(softs) - OS_FW_HEARTBEAT_TIMER_INTERVAL;
                                              ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:267:43: note: expanded from macro 'CTRLR_HEARTBEAT_CNT'
#define CTRLR_HEARTBEAT_CNT(softs)              LE_64(PCI_MEM_GET64(softs, softs->heartbeat_counter_abs_addr, softs->heartbeat_counter_off))
                                                      ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:874:5: note: expanded from macro 'PCI_MEM_GET64'
    bus_space_read_8(_softs->pci_mem_handle.pqi_btag, \
    ^
6 errors generated.
--- smartpqi_init.o ---
*** [smartpqi_init.o] Error code 1

make[4]: stopped in /home/sbruno/bsd/fbsd_head/sys/modules/smartpqi
A failure has been detected in another branch of the parallel make

make[4]: stopped in /home/sbruno/bsd/fbsd_head/sys/modules/sn
--- all_subdir_sn ---
*** [all_subdir_sn] Error code 2

make[3]: stopped in /home/sbruno/bsd/fbsd_head/sys/modules
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_queue.c:111:14: error: implicit declaration of function 'bus_space_read_8' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        val = LE_64(PCI_MEM_GET64(softs, &softs->pqi_reg->pqi_dev_adminq_cap, PQI_ADMINQ_CAP));
                    ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:874:5: note: expanded from macro 'PCI_MEM_GET64'
    bus_space_read_8(_softs->pci_mem_handle.pqi_btag, \
    ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_queue.c:111:14: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:874:5: note: expanded from macro 'PCI_MEM_GET64'
    bus_space_read_8(_softs->pci_mem_handle.pqi_btag, \
    ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_queue.c:233:2: error: implicit declaration of function 'bus_space_write_8' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        PCI_MEM_PUT64(softs, &softs->pqi_reg->admin_q_config, PQI_ADMINQ_CONFIG, LE_64(cmd));
        ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:882:5: note: expanded from macro 'PCI_MEM_PUT64'
    bus_space_write_8(_softs->pci_mem_handle.pqi_btag, \
    ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_queue.c:233:2: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:882:5: note: expanded from macro 'PCI_MEM_PUT64'
    bus_space_write_8(_softs->pci_mem_handle.pqi_btag, \
    ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_queue.c:241:13: error: implicit declaration of function 'bus_space_read_8' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        COND_WAIT((PCI_MEM_GET64(softs, &softs->pqi_reg->admin_q_config, PQI_ADMINQ_CONFIG) ==
                   ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:874:5: note: expanded from macro 'PCI_MEM_GET64'
    bus_space_read_8(_softs->pci_mem_handle.pqi_btag, \
    ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_queue.c:302:2: error: implicit declaration of function 'bus_space_write_8' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        PCI_MEM_PUT64(softs, &softs->pqi_reg->admin_ibq_elem_array_addr, 
        ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:882:5: note: expanded from macro 'PCI_MEM_PUT64'
    bus_space_write_8(_softs->pci_mem_handle.pqi_btag, \
    ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_queue.c:337:9: error: implicit declaration of function 'bus_space_read_8' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        PCI_MEM_GET64(softs, &softs->pqi_reg->admin_ibq_pi_offset, PQI_ADMIN_IBQ_PI_OFFSET));
        ^
/home/sbruno/bsd/fbsd_head/sys/modules/smartpqi/../../dev/smartpqi/smartpqi_defines.h:874:5: note: expanded from macro 'PCI_MEM_GET64'
    bus_space_read_8(_softs->pci_mem_handle.pqi_btag, \
    ^
7 errors generated.
--- smartpqi_queue.o ---
*** [smartpqi_queue.o] Error code 1

make[4]: stopped in /home/sbruno/bsd/fbsd_head/sys/modules/smartpqi
2 errors

make[4]: stopped in /home/sbruno/bsd/fbsd_head/sys/modules/smartpqi
--- all_subdir_smartpqi ---
*** [all_subdir_smartpqi] Error code 2

make[3]: stopped in /home/sbruno/bsd/fbsd_head/sys/modules
A failure has been detected in another branch of the parallel make

make[4]: stopped in /home/sbruno/bsd/fbsd_head/sys/modules/smbfs
--- all_subdir_smbfs ---
*** [all_subdir_smbfs] Error code 2

make[3]: stopped in /home/sbruno/bsd/fbsd_head/sys/modules
A failure has been detected in another branch of the parallel make

make[4]: stopped in /home/sbruno/bsd/fbsd_head/sys/modules/rtwn
--- all_subdir_rtwn ---
*** [all_subdir_rtwn] Error code 2

make[3]: stopped in /home/sbruno/bsd/fbsd_head/sys/modules
4 errors

make[3]: stopped in /home/sbruno/bsd/fbsd_head/sys/modules
--- modules-all ---
*** [modules-all] Error code 2

make[2]: stopped in /var/tmp/home/sbruno/bsd/fbsd_head/i386.i386/sys/GENERIC-NODEBUG
1 error

make[2]: stopped in /var/tmp/home/sbruno/bsd/fbsd_head/i386.i386/sys/GENERIC-NODEBUG
--- buildkernel ---
*** [buildkernel] Error code 2

make[1]: stopped in /home/sbruno/bsd/fbsd_head
1 error

make[1]: stopped in /home/sbruno/bsd/fbsd_head
--- buildkernel ---
*** [buildkernel] Error code 2

make: stopped in /home/sbruno/bsd/fbsd_head
1 error

make: stopped in /home/sbruno/bsd/fbsd_head
This revision now requires changes to proceed.Apr 23 2018, 11:07 PM

Currently Smartpqi driver is not qualified with i386 architecture so for now it will support only AMD64. We are planning to add support for i386 in future.

sbruno added a comment.EditedApr 24 2018, 2:36 PM

Currently Smartpqi driver is not qualified with i386 architecture so for now it will support only AMD64. We are planning to add support for i386 in future.

Ok, so how about something like this.

Move the smartpqi entries from sys/conf/files to sys/conf/files.amd64 and restrict sys/modules/Makefile to amd64 only:

Index: sys/modules/Makefile
===================================================================
--- sys/modules/Makefile	(revision 332894)
+++ sys/modules/Makefile	(working copy)
@@ -355,6 +355,7 @@
 	siis \
 	sis \
 	sk \
+	${_smartpqi} \
 	smbfs \
 	sn \
 	snp \
@@ -729,6 +730,7 @@
 _sfxge=		sfxge
 _sgx=		sgx
 _sgx_linux=	sgx_linux
+_smartpqi=      smartpqi
 
 .if ${MK_BHYVE} != "no" || defined(ALL_MODULES)
 _vmm=		vmm

As suggested Restricted the driver to only amd64 and also Moved the smartpqi entries from sys/conf/files to sys/conf/files.amd64.

sbruno accepted this revision.Apr 26 2018, 2:52 PM
This revision is now accepted and ready to land.Apr 26 2018, 2:52 PM
This revision was automatically updated to reflect the committed changes.