> -----Original Message----- > From: Hannes Reinecke [mailto:hare@xxxxxxx] > Sent: Friday, June 10, 2016 4:57 AM > To: Don Brace; jejb@xxxxxxxxxxxxxxxxxx; Viswas G; Mahesh Rajashekhara; > hch@xxxxxxxxxxxxx; Scott Teel; Kevin Barnett; Justin Lindley; Scott Benesh; > elliott@xxxxxxx > Cc: linux-scsi@xxxxxxxxxxxxxxx > Subject: Re: [PATCH V2 0/2] initial submit of Microsemi smartpqi driver > > EXTERNAL EMAIL > > > On 06/09/2016 08:50 PM, Don Brace wrote: > > These patches are based on Linus's tree > > > > - add smartpqi to kernel.org > > - remove PCI IDs from aacraid driver > > - Depends on adoption of smartpqi driver > > > > Changes since initial upload > > - Forgot to give correct ownership to the author. > > Changes since V1 > > - Corrected make ARCH=i386 kbuild test robot issue. > > > > --- > > > > Don Brace (1): > > aacraid: remove wildcard for series 9 controllers > > > > Kevin Barnett (1): > > smartpqi: initial commit of Microsemi smartpqi driver > > > > > > drivers/scsi/smartpqi/Kconfig | 50 > > drivers/scsi/smartpqi/Makefile | 3 > > drivers/scsi/smartpqi/smartpqi.h | 1135 ++++ > > drivers/scsi/smartpqi/smartpqi_init.c | 6817 > ++++++++++++++++++++++++ > > drivers/scsi/smartpqi/smartpqi_sas_transport.c | 350 + > > drivers/scsi/smartpqi/smartpqi_sis.c | 394 + > > drivers/scsi/smartpqi/smartpqi_sis.h | 32 > > 7 files changed, 8781 insertions(+) > > create mode 100644 drivers/scsi/smartpqi/Kconfig > > create mode 100644 drivers/scsi/smartpqi/Makefile > > create mode 100644 drivers/scsi/smartpqi/smartpqi.h > > create mode 100644 drivers/scsi/smartpqi/smartpqi_init.c > > create mode 100644 drivers/scsi/smartpqi/smartpqi_sas_transport.c > > create mode 100644 drivers/scsi/smartpqi/smartpqi_sis.c > > create mode 100644 drivers/scsi/smartpqi/smartpqi_sis.h > > > Some general comments: > - Wouldn't it be better to split off CISS specific definitions from > smartpqi.h into a separate header file? I guess this even could be > shared with hpsa, right? -- The hpsa driver is now in maintenance mode, so eventually it will no longer be in the kernel. Additionally the ciss.h header is associated specifically with Compaq and Hewlett-Packard Enterprise devices, while the smartpqi driver is from Microsemi, and will be used with devices from multiple vendors. > - In the same vein: Maybe it's worth considering splitting off the PQI > specific ones into a separate header file, too, and just keeping the > smartpqi.h file for the driver specific definitions. -- It's possible, but the driver uses PQI to communicate with the controllers, so we wanted to keep the PQI structures in the controller header. > - The device rescan / device mapping functionality is positively > horrible. I still think it was a mistake having that in hpsa, but I'd be > loath to see this one repeated in smartpqi. Why can't you use the LUN > numbers directly? It's _quite_ easy to have the RAID and passthrough > devices on a separate SCSI bus, so that shouldn't be an issue. -- The driver must maintain a representation of the devices because the controllers operate in multiple modes (HBA, local RAID, pass-through RAID, IO acceleration, encryption, etc). The driver is responsible for enforcing certain limitations in some of these modes, and for enabling certain functionality in others. -- The state of enablement of the feature and the application of limitations is done on a per device level, so the driver must maintain an internal map of these devices. For example, in the IO accel mode, the driver maintains extensive knowledge of the underlying physical disks of a RAID volume: to determine the eligibility of each IO request for the IO accelerated path, and maintains state information per volume with respect to IO accel path enablement on that device. This enablement can get temporarily disabled while controller completes certain background functions like RAID level migration, rebuilds, etc. The driver must coordinate with the controller during these path state transitions for each device. -- The storage administrator assigns LUN and sometimes target numbering from the storage system's configuration app. The driver uses those assigned LUN and target numbers so that addresses are consistent across multiple servers, and persistent across reboots to comply with requirements for clustering and shared storage. > - The 'devices' sysfs attribute is a sysfs abuse. As per definition each > sysfs attribute should be a single entry. If you want/need to have a > listing consider adding a sysfs directory for the devices. > But then again, if you drop the device rescan/mapping functionality and > use the LUN numbers directly none of these would matter and the > 'devices' attribute could be dropped, too. -- We will remove the devices sysfs attribute > - Statistics. Using atomics for each variable and enabling them by > default is not a good idea. I'd prefer to have it optional; have you > checked using debugfs for this? -- We will be investigating this. Especially during performance tuning. > - 'host_wellness' is cute. Do we get a Spa to go with it? -- No > - pqi_alloc_io_request() has this weird 'while' loop. I was under the > impression that the block layer would guarantee tag uniqueness, so why > don't you use the request tag to identify the command from the request > pool (and split the number of tags per hardware queue)? -- Kevin Barnett: We should probably do this differently. I was planning on revisiting this as part of performance tuning. > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Teamlead Storage & Networking > hare@xxxxxxx +49 911 74053 688 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton > HRB 21284 (AG Nürnberg) ��.n��������+%������w��{.n�����{������ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
