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

[Hannes Reinecke]

On 10/27/22 11:16, Damien Le Moal wrote:
> On 10/27/22 17:11, Hannes Reinecke wrote:
> > On 10/27/22 03:34, Damien Le Moal wrote:
> > > 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...
> > Typically, the SCSI layer has two ways of scanning.
> > One it the old-style serial scanning (originating in the old SCSI
> > parallel model):
> > The scanning code will blindly scan _all_ devices up to max_luns, and
> > attach every device for which the scanning code returns 'OK'.
> > The other one is to issue REPORT_LUNS and scan all LUNs returned there.
> >
> > Mapped to libata we would need to figure out a stable SCSI enumeration,
> > given that we have PMP and slave devices to content with.
> > To my knowledge we have ATA ports, each can have either a 'master' and
> > 'slave' device, _or_ it be a PMP port when it can support up to 16
> > devices, right?
>
> yes
>
> > Point being, master/slave and PMP are exclusive, right?
>
> Never heard of pmp with ide cable :)
See?

> > So we can make the master as LUN 0, and the slave as LUN 1.
>
> Yes, but isn't that a little wrong ? That would assume that the ata port
> is the device and the ata devices the luns of that device. But beside
> the "link busy" stuff that needs to be dealt with, master and slave are
> independent devices, unlike LUNs. No ?
> Well; technically, yes.

But we already enumerate the ata ports (which is typically done by the 
hardware/PCI layer etc), and if we were try to model slave devices as 
independent ports we would either have to insert a numbering (awkward) 
or add a number at the en (even more awkward).

And the one key takeaway from the 'multiple actuators' discussion is 
that LUNs _are_ independent (cf all the hoops they had to jump through 
to define a command spanning several LUNs ...)(which, incidentally, we 
could leverage here, too ...), and the target port really only serves as 
an enumeration thingie to address the LUNs.

So we _could_ map the master device on LUN 0 and the slave device on LUN 
1 with no loss of functionality, _but_ enable a consistent SCSI enumeration.

Cheers,

Hannes
--
Dr. Hannes Reinecke                Kernel Storage Architect
hare@xxxxxxx                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman