Messages in this thread | | | Date | Thu, 27 Oct 2022 11:51:34 +0200 | Subject | Re: [PATCH RFC v3 16/22] ata: libata-scsi: Allocate sdev early in port probe | From | 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@huawei.com> >>>> --- >>>> 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@suse.de +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
| |