On Mon, Jun 27, 2016 at 09:12:11PM +0000, Don Brace wrote: [...] > > > --- > > > > [...] > > > > > + */ > > > + > > > +#if !defined(_SMARTPQI_H) > > > +#define _SMARTPQI_H > > > + > > > +#pragma pack(1) > > > > I know GCC does understand #pragma pack() to be compatible with MSVC > > but please > > use the kernel's __packed convention on structs you want to have packed. > > > > We prefer to leave this "as is" for these reasons: > > We do not use __packed on other Microsemi Linux drivers. > > There's not consistency in the usage of __packed. Just a quick survey of the kernel source reveals at least four styles: > So your making a 5th version, c.f. https://xkcd.com/927/ Anyway, a IMHO more important question > > > +static void pqi_complete_queued_requests(struct pqi_ctrl_info *ctrl_info, > > > + struct pqi_scsi_dev_t *device_in_reset) > > > +{ > > > + unsigned int i; > > > + unsigned int path; > > > + struct pqi_queue_group *queue_group; > > > + unsigned long flags; > > > + struct pqi_io_request *io_request; > > > + struct pqi_io_request *next; > > > + struct scsi_cmnd *scmd; > > > + struct pqi_scsi_dev_t *device; > > > + > > > + for (i = 0; i < ctrl_info->num_queue_groups; i++) { > > > + queue_group = &ctrl_info->queue_groups[i]; > > > + > > > + for (path = 0; path < 2; path++) { > > > + spin_lock_irqsave(&queue_group->submit_lock[path], > > > + flags); > > > + > > > + list_for_each_entry_safe(io_request, next, > > > + &queue_group->request_list[path], > > > + request_list_entry) { > > > + scmd = io_request->scmd; > > > + if (scmd) { > > > + device = scmd->device->hostdata; > > > + if (device == device_in_reset) { > > > + set_host_byte(scmd, DID_RESET); > > > + pqi_scsi_done(scmd); > > > + list_del(&io_request-> > > > + request_list_entry); > > > + } > > > + } > > > + } > > > + > > > + spin_unlock_irqrestore(&queue_group->submit_lock[path], > > > + flags); > > > + } > > > > If you'd factor the inner for loop into an own small (inline) function, it > > could drastically improve readability here. > > > > something like > > > > static void pqi_complete_queue_group(struct pqi_queue_group *qg, struct > > pqi_device_t *device_in_reset) > > { > > unsigned int path; > > unsigned long flags; > > struct qpi_scsi_dev_t *dev; > > struct pqi_io_request *ior, *next; > > > > for (path = 0; path < 2; path++) { > > spin_lock_irqsave(&qg->submit_lock[path], flags); > > > > list_for_each_entry_save(ior, next, &qg->request_list[path], > > request_list_entry) { > > scmd = ior->scmnd; > > if (!scmd) > > continue; > > > > device = scmnd->device->hostdata; > > if (dev == device_in_reset) { > > set_host_byte(scmnd, DID_RESET); > > pqi_scsi_done(scmnd); > > list_del(&ior->request_list_entry); > > } > > } > > spin_unlock_irqresetore(&qg->submit_lock[path], flags); > > } > > } Does this have to be under a spin_lock and then even the irqsave variant? From my understanding scsi-mq/blk-mq is supposed to place the queues per-cpu. So if you're now deactivating local IRQs here you're throwing all optimizations out of the window. Can't you have the request lists and queues per-cpu so this code can run lockless? Thanks, Johannes -- Johannes Thumshirn Storage jthumshirn@xxxxxxx +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
