diff mbox series

[RFC,v2,02/18] scsi: core: Resurrect scsi_{get,free}_host_dev()

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

Commit Message

John Garry June 9, 2022, 10:29 a.m. UTC
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(+)

Comments

Christoph Hellwig June 14, 2022, 6:44 a.m. UTC | #1
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.
John Garry June 14, 2022, 9:33 a.m. UTC | #2
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
Bart Van Assche June 14, 2022, 7:33 p.m. UTC | #3
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.
John Garry June 15, 2022, 1:44 p.m. UTC | #4
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 mbox series

Patch

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.