Microsemi smartpqi SCSI driver for PQI controllers

[deepak.ukey_microsemi.com]

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.

[deepak.ukey_microsemi.com]

Hi,

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

Regards,
Deepak

[emaste]

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

[deepak.ukey_microsemi.com]

In D14514#308148, @emaste wrote:

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.

[sbruno]

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

2

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

26

This syntax should be reset to:

.\" $FreeBSD$

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

27

Date should be updated.

101

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

sys/dev/smartpqi/smartpqi_cam.c

3

Update copyright date to 2018.

58

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

3

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_defines.h

3

Update copyright date to 2018

sys/dev/smartpqi/smartpqi_discovery.c

3

Update copyright date to 2018

1349

Hmmm? What does this comment indicate?

sys/dev/smartpqi/smartpqi_event.c

3

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_helper.c

3

Update copyright date to 2018.

216

Is this missing an entry for "RAID 6"?

sys/dev/smartpqi/smartpqi_includes.h

3

Update copyright date to 2018.

27

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

3

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_intr.c

3

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_ioctl.c

3

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_ioctl.h

3

Update copyright date to 2018.

69

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

3

Update copyright date to 2018.

120

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

3

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_misc.c

3

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_prototypes.h

3

Update copyright to 2018.

sys/dev/smartpqi/smartpqi_queue.c

3

Update copyright date to 2018.

81

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?

293

Stray commented out code? Should it be deleted?

378

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

sys/dev/smartpqi/smartpqi_request.c

3

Update copyright date to 2018.

727

Hrm ... Is this debugging or something else ?

740

More debugging to be removed?

747

More debugging?

sys/dev/smartpqi/smartpqi_response.c

3

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_sis.c

3

Update copyright date to 2018.

245

Debugging to be removed?

sys/dev/smartpqi/smartpqi_structures.h

3

Update copyright date to 2018.

sys/dev/smartpqi/smartpqi_tag.c

3

Update copyright date to 2018.

[deepak.ukey_microsemi.com]

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.

2. > 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]

In D14514#314579, @deepak.ukey_microsemi.com wrote:

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.

2. > 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?

[deepak.ukey_microsemi.com]

sys/dev/smartpqi/smartpqi_ioctl.h

69

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

120

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

[deepak.ukey_microsemi.com]

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]

share/man/man4/smartpqi.4

26

Please reset this syntax to:

.\" $FreeBSD$

101

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

sys/dev/smartpqi/smartpqi_queue.c

75

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.

[deepak.ukey_microsemi.com]

Updated the diff. Please review it.

sys/dev/smartpqi/smartpqi_queue.c

75

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.

[sbruno]

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

[deepak.ukey_microsemi.com]

In D14514#317149, @sbruno wrote:

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]

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

[deepak.ukey_microsemi.com]

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]

In D14514#319855, @deepak.ukey_microsemi.com wrote:

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

[deepak.ukey_microsemi.com]

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