Re: [PATCH RFC v3 16/22] ata: libata-scsi: Allocate sdev early in port probe

From: Damien Le Moal
Date: Wed Oct 26 2022 - 21:35:28 EST


On 10/25/22 19:18, John Garry wrote:
> Currently the per-ata device sdev is allocated as part of the scsi target
> scan, which is after the ata port probe.
>
> However it is useful to have the sdev available in the port probe. As an
> example of an advantage, if the request queue is available in the probe
> (which it would be if the sdev is available), then it is possible to use
> a SCSI cmnd for ATA internal commands. The benefit of this is then we can
> put the ATA qc structure in the SCSI cmnd private data. It will also be
> useful if we want to send ATA internal commands as requests.
>
> Export scsi_target_reap() so that it can be used to put the extra
> reference we get when allocating the sdev.
>
> Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
> ---
> drivers/ata/libata-eh.c | 1 +
> drivers/ata/libata-scsi.c | 23 +++++++++--------------
> drivers/scsi/scsi_scan.c | 1 +
> 3 files changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 08e11bc312c2..1ed5b1b64792 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3446,6 +3446,7 @@ static int ata_eh_schedule_probe(struct ata_device *dev)
>
> ata_eh_detach_dev(dev);
> ata_dev_init(dev);
> + ata_scsi_setup_sdev(dev);
> ehc->did_probe_mask |= (1 << dev->devno);
> ehc->i.action |= ATA_EH_RESET;
> ehc->saved_xfer_mode[dev->devno] = 0;
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index efdba852e363..476e0ef4bd29 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1109,7 +1109,12 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
> if (dev->flags & ATA_DFLAG_TRUSTED)
> sdev->security_supported = 1;
>
> - dev->sdev = sdev;
> + /*
> + * Put extra reference which we get when allocating the starget
> + * initially
> + */
> + scsi_target_reap(scsi_target(sdev));
> +
> return 0;
> }
>
> @@ -4289,26 +4294,16 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
> repeat:
> ata_for_each_link(link, ap, EDGE) {
> ata_for_each_dev(dev, link, ENABLED) {
> - struct scsi_device *sdev;
> + struct Scsi_Host *shost = ap->scsi_host;
> int channel = 0, id = 0;
>
> - if (dev->sdev)
> - continue;
> -
> if (ata_is_host_link(link))
> id = dev->devno;
> else
> channel = link->pmp;
>
> - sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
> - NULL);
> - if (!IS_ERR(sdev)) {
> - dev->sdev = sdev;
> - ata_scsi_assign_ofnode(dev, ap);

Is there something equivalent to what this function does inside
scsi_scan_target() ? I had a quick look but did not see anything...

> - scsi_device_put(sdev);
> - } else {
> - dev->sdev = NULL;
> - }
> + scsi_scan_target(&shost->shost_gendev, channel, id,
> + 0, SCSI_SCAN_INITIAL);
> }
> }
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index b795c138f2c1..da7bc14b030c 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -598,6 +598,7 @@ void scsi_target_reap(struct scsi_target *starget)
> BUG_ON(starget->state == STARGET_DEL);
> scsi_target_reap_ref_put(starget);
> }
> +EXPORT_SYMBOL_GPL(scsi_target_reap);
>
> /**
> * scsi_sanitize_inquiry_string - remove non-graphical chars from an

--
Damien Le Moal
Western Digital Research