| Message ID | 1654770559-101375-3-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | blk-mq/libata/scsi: SCSI driver tagging improvements | expand |
On Thu, Jun 09, 2022 at 06:29:03PM +0800, John Garry wrote:
> This reverts commit 6bd49b1a8d43ec118c55f3aaa7577729b52bde15.
Please add an actual text describing why you are doing this and how
insteasd of this completely pointless revert line.
On 14/06/2022 07:44, Christoph Hellwig wrote: > On Thu, Jun 09, 2022 at 06:29:03PM +0800, John Garry wrote: >> This reverts commit 6bd49b1a8d43ec118c55f3aaa7577729b52bde15. > > Please add an actual text describing why you are doing this and how > insteasd of this completely pointless revert line. > > > . OK. And in hindsight it would have been a good opportunity to mention something which I am undecided on - that is which scsi_device to use for these reserved commands? In this series I use the scsi shost sdev for all reserved commands, but maybe we should use the target sdev. Pros of using scsi host sdev: - don't need to worry about request queue freezing - don't need to worry about running out of request queue budget - available when scsi host is added - libata adds target sdev after some internal commands are sent, and this would be a bit painful to change, however adding the sdev earlier would seem to be a good change to make Cons: - generally better to use same scsi device as target (or is it?). For example, it seems better to have reserved scsi_cmnd.device actually set to the target sdev. - don't need to add stuff like ata_is_scmd_ata_internal() later in this series Prob other reasons which I have forgot about. Please let me know if you have any thoughts on this. Cheers, John
On 6/9/22 03:29, John Garry wrote: > +/** > + * scsi_get_host_dev - Create a scsi_device that points to the host adapter itself ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ What does this mean? That part of the function description is not clear to me. > + * @shost: Host that needs a scsi_device ^^^^^^^^^^^^^ This is not detailed enough. Consider changing "a scsi_device" into "a scsi device for allocating reserved commands from". > + * > + * Lock status: None assumed. > + * > + * Returns: The scsi_device or NULL > + * > + * Notes: > + * Attach a single scsi_device to the Scsi_Host - this should > + * be made to look like a "pseudo-device" that points to the > + * HA itself. > + * > + * Note - this device is not accessible from any high-level > + * drivers (including generics), which is probably not > + * optimal. We can add hooks later to attach. The "which is probably not optimal. We can add hooks later to attach." part probably should be moved to the patch description. > + */ > +struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost) > +{ > + struct scsi_device *sdev = NULL; > + struct scsi_target *starget; > + > + mutex_lock(&shost->scan_mutex); > + if (!scsi_host_scan_allowed(shost)) > + goto out; > + starget = scsi_alloc_target(&shost->shost_gendev, 0, shost->this_id); ^^^^^^^^^^^^^^^^^^ Is it guaranteed that this channel / id combination will not be used for any other SCSI device? What if shost->this_id == -1? > + if (!starget) > + goto out; > + > + sdev = scsi_alloc_sdev(starget, 0, NULL); > + if (sdev) > + sdev->borken = 0; > + else > + scsi_target_reap(starget); > + put_device(&starget->dev); > + out: > + mutex_unlock(&shost->scan_mutex); > + return sdev; > +} > +EXPORT_SYMBOL(scsi_get_host_dev); Elsewhere in the SCSI core "get..dev" means increment the reference count of a SCSI device. Maybe scsi_alloc_host_dev() is a better name? > +/* > + * These two functions are used to allocate and free a pseudo device > + * which will connect to the host adapter itself rather than any > + * physical device. You must deallocate when you are done with the > + * thing. This physical pseudo-device isn't real and won't be available > + * from any high-level drivers. > + */ Please keep function comments in .c files because that makes it more likely that the comment and the implementation will remain in sync. Thanks, Bart.
On 14/06/2022 20:33, Bart Van Assche wrote: Hi Bart, > On 6/9/22 03:29, John Garry wrote: >> +/** >> + * scsi_get_host_dev - Create a scsi_device that points to the host >> adapter itself > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > What does this mean? That part of the function description is not > clear to me. > Agreed, this text is just as it was before (it was originally deleted) but I can fix it up to make sense. >> + * @shost: Host that needs a scsi_device > ^^^^^^^^^^^^^ > This is not detailed enough. Consider changing "a scsi_device" into > "a scsi device for allocating reserved commands from". > >> + * >> + * Lock status: None assumed. >> + * >> + * Returns: The scsi_device or NULL >> + * >> + * Notes: >> + * Attach a single scsi_device to the Scsi_Host - this should >> + * be made to look like a "pseudo-device" that points to the >> + * HA itself. >> + * >> + * Note - this device is not accessible from any high-level >> + * drivers (including generics), which is probably not >> + * optimal. We can add hooks later to attach. > > The "which is probably not optimal. We can add hooks later to attach." > part probably should be moved to the patch description. ok > >> + */ >> +struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost) >> +{ >> + struct scsi_device *sdev = NULL; >> + struct scsi_target *starget; >> + >> + mutex_lock(&shost->scan_mutex); >> + if (!scsi_host_scan_allowed(shost)) >> + goto out; >> + starget = scsi_alloc_target(&shost->shost_gendev, 0, >> shost->this_id); > > ^^^^^^^^^^^^^^^^^^ > Is it guaranteed that this channel / id combination will not be used for > any other SCSI device? Does it matter if the parent device is different? > > What if shost->this_id == -1? > >> + if (!starget) >> + goto out; >> + >> + sdev = scsi_alloc_sdev(starget, 0, NULL); >> + if (sdev) >> + sdev->borken = 0; >> + else >> + scsi_target_reap(starget); >> + put_device(&starget->dev); >> + out: >> + mutex_unlock(&shost->scan_mutex); >> + return sdev; >> +} >> +EXPORT_SYMBOL(scsi_get_host_dev); > > Elsewhere in the SCSI core "get..dev" means increment the reference > count of > a SCSI device. Maybe scsi_alloc_host_dev() is a better name? I think that the intention is to only use this once for a shost, i.e. get or allocate that scsi_device once and use it for the lifetime of the shost. But I can rename if you think it's better. > >> +/* >> + * These two functions are used to allocate and free a pseudo device >> + * which will connect to the host adapter itself rather than any >> + * physical device. You must deallocate when you are done with the >> + * thing. This physical pseudo-device isn't real and won't be available >> + * from any high-level drivers. >> + */ > > Please keep function comments in .c files because that makes it more likely > that the comment and the implementation will remain in sync. > fine, I can relocate this. Thanks, John
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index 91ac901a6682..925fe63fa370 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -1978,3 +1978,60 @@ void scsi_forget_host(struct Scsi_Host *shost) spin_unlock_irqrestore(shost->host_lock, flags); } +/** + * scsi_get_host_dev - Create a scsi_device that points to the host adapter itself + * @shost: Host that needs a scsi_device + * + * Lock status: None assumed. + * + * Returns: The scsi_device or NULL + * + * Notes: + * Attach a single scsi_device to the Scsi_Host - this should + * be made to look like a "pseudo-device" that points to the + * HA itself. + * + * Note - this device is not accessible from any high-level + * drivers (including generics), which is probably not + * optimal. We can add hooks later to attach. + */ +struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost) +{ + struct scsi_device *sdev = NULL; + struct scsi_target *starget; + + mutex_lock(&shost->scan_mutex); + if (!scsi_host_scan_allowed(shost)) + goto out; + starget = scsi_alloc_target(&shost->shost_gendev, 0, shost->this_id); + if (!starget) + goto out; + + sdev = scsi_alloc_sdev(starget, 0, NULL); + if (sdev) + sdev->borken = 0; + else + scsi_target_reap(starget); + put_device(&starget->dev); + out: + mutex_unlock(&shost->scan_mutex); + return sdev; +} +EXPORT_SYMBOL(scsi_get_host_dev); + +/** + * scsi_free_host_dev - Free a scsi_device that points to the host adapter itself + * @sdev: Host device to be freed + * + * Lock status: None assumed. + * + * Returns: Nothing + */ +void scsi_free_host_dev(struct scsi_device *sdev) +{ + BUG_ON(sdev->id != sdev->host->this_id); + + __scsi_remove_device(sdev); +} +EXPORT_SYMBOL(scsi_free_host_dev); + diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 667d889b92b5..59aef1f178f5 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -790,6 +790,16 @@ void scsi_host_busy_iter(struct Scsi_Host *, struct class_container; +/* + * These two functions are used to allocate and free a pseudo device + * which will connect to the host adapter itself rather than any + * physical device. You must deallocate when you are done with the + * thing. This physical pseudo-device isn't real and won't be available + * from any high-level drivers. + */ +extern void scsi_free_host_dev(struct scsi_device *); +extern struct scsi_device *scsi_get_host_dev(struct Scsi_Host *); + /* * DIF defines the exchange of protection information between * initiator and SBC block device.
This reverts commit 6bd49b1a8d43ec118c55f3aaa7577729b52bde15. Signed-off-by: John Garry <john.garry@huawei.com> --- drivers/scsi/scsi_scan.c | 57 ++++++++++++++++++++++++++++++++++++++++ include/scsi/scsi_host.h | 10 +++++++ 2 files changed, 67 insertions(+)