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

From: Hannes Reinecke <hare@suse.de>
To: John Garry <john.g.garry@oracle.com>,
	Damien Le Moal <damien.lemoal@opensource.wdc.com>,
	John Garry <john.garry@huawei.com>,
	axboe@kernel.dk, jejb@linux.ibm.com, martin.petersen@oracle.com,
	jinpu.wang@cloud.ionos.com, bvanassche@acm.org, hch@lst.de,
	ming.lei@redhat.com, niklas.cassel@wdc.com
Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-ide@vger.kernel.org, linux-scsi@vger.kernel.org,
	linuxarm@huawei.com
Subject: Re: [PATCH RFC v3 16/22] ata: libata-scsi: Allocate sdev early in port probe
Date: Mon, 7 Nov 2022 11:20:05 +0100	[thread overview]
Message-ID: <f4c4b350-70da-9f28-8675-d3319217e471@suse.de> (raw)
In-Reply-To: <59947ba8-d1a1-2e50-6199-f3974221f8fb@oracle.com>

On 11/7/22 11:09, John Garry wrote:
> 
>>>>>>    @@ -4289,26 +4294,16 @@ void ata_scsi_scan_host(struct ata_port
>>>>>> *ap, int sync)
>>>>>>     repeat:
> 
> I've been trying to follow this thread, below, but got a bit lost ....
> 
>>>>>>        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...
>>>>>
> 
> So are we discussing below whether we can have fixed channel, id, lun 
> per ATA sdev per shost? If so, I don't think it would work as libsas 
> uses dynamic target ids per host.
> 
Not static. target port enumeration would still be left to the driver / 
transport, so it can do whatever it wants here.
Idea was to match the 'ata_port' structure to the scsi target port; 
well, not exactly 'match' but have a 1:1 relationship between them.
_And_ to have a defined way on how the devices are enumerated; PATA 
drives would always have LUN 0 and LUN 1(for the slave), and everything 
else would use REPORT LUNs. For which we need an emulation for libata, 
but that should be trivially to implement.

With that we would know exactly how many scsi target ports we'll have, 
and can create them based on hardware details before probing starts.
Whether or not a _device_ is attached to that port is being determined 
during scanning via the 'slave_alloc' or 'slave_configure' callbacks, so 
we still would be able to blank out any not implemented or not connected 
ports.
_But_ it would align better with the SCSI layer, such that we can work 
on integrating scanning and resetting, and make things like 
'sd_revalidate' actually work for libata.

And for libsas it might be worthwhile to check the 'scan_start' and 
'scan_finished' callbacks; that seems to be the ticket here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer