> -----Original Message----- > From: Christoph Hellwig [mailto:hch@xxxxxxxxxxxxx] > Sent: Monday, February 23, 2015 2:15 PM > To: Don Brace > Cc: Scott Teel; Kevin Barnett; james.bottomley@xxxxxxxxxxxxx; > hch@xxxxxxxxxxxxx; Justin Lindley; brace; linux-scsi@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 01/43] hpsa: add masked physical devices into h->dev[] > array > > > -/* link sdev->hostdata to our per-device structure. */ > > static int hpsa_slave_alloc(struct scsi_device *sdev) > > { > > struct hpsa_scsi_dev_t *sd; > > unsigned long flags; > > struct ctlr_info *h; > > + int queue_depth; > > > > h = sdev_to_hba(sdev); > > spin_lock_irqsave(&h->devlock, flags); > > sd = lookup_hpsa_scsi_dev(h, sdev_channel(sdev), > > sdev_id(sdev), sdev->lun); > > - if (sd != NULL) { > > - sdev->hostdata = sd; > > - if (sd->queue_depth) > > + > > + if (likely(sd)) { > > scsi_change_queue_depth(sdev, sd->queue_depth); > > Looks like the indentation is incorrect here. Corrected. > > > atomic_set(&sd->ioaccel_cmds_out, 0); > > + sdev->hostdata = (sd->expose_state & HPSA_SCSI_ADD) ? sd : > NULL; > > + queue_depth = sd->queue_depth != 0 ? > > + sd->queue_depth : sdev->host->can_queue; > > + } else { > > + sdev->hostdata = NULL; > > + queue_depth = sdev->host->can_queue; > > } > > + > > + if (shost_use_blk_mq(sdev->host)) > > + scsi_change_queue_depth(sdev, queue_depth); > > But why do you have another call here only for the mq case? > Also in general I'd really prefer if you'd only change the queue > depth in ->slave_configure as most drivers do unless you have a good > reason to do it differently. Thanks, I corrected the code. Also, I asked Webb about setting up the queue Depth in slave_alloc. His reply was " We needed to get tagging set up during allocation so that we could use it in the lead-up to configuration, which is why it was done in hpsa_slave_alloc(); and, given that the code there was then already messing about with queue depth, it made sense to also deal with the rest of it there." But given your next comment, perhaps I need to clean this up more? > > > + else /* We depend on tags for cmd allocation. */ > > + sdev->tagged_supported = 1; > > Since 3.19-rc1 you'll always get tags from the first command sent, as > we now moved the block tagging configuration earlier. > > > +/* configure scsi device based on internal per-device structure */ > > +static int hpsa_slave_configure(struct scsi_device *sdev) > > +{ > > + struct hpsa_scsi_dev_t *sd; > > + unsigned long flags; > > + struct ctlr_info *h; > > + > > + h = sdev_to_hba(sdev); > > + spin_lock_irqsave(&h->devlock, flags); > > + sd = sdev->hostdata; > > There should be no need to have this dereference inside the spinlock. > > Also can you explain somewhere why some devices might not have this > hostdata set? For our mixed mode controllers, we have started reporting all devices. The masked devices can be part of a logical volume, or it did not respond during the device scan. These do not get their hostdata set. -- 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
