Re: [PATCH v2 05/20] scsi: core: Add support for internal commands

Hannes Reinecke
2021-11-24T06:33:21Z

On 11/23/21 8:18 PM, Bart Van Assche wrote:
> On 11/23/21 9:46 AM, Bart Van Assche wrote:
>> On 11/23/21 12:13 AM, Hannes Reinecke wrote:
>>> It's actually a bit more involved.
>>>
>>> The biggest issue is that the SCSI layer is littered with the assumption
>>> that there _will_ be a ->device pointer in struct scsi_cmnd.
>>> If we make up a scsi_cmnd structure _without_ that we'll have to audit
>>> the entire stack to ensure we're not tripping over a NULL device
>>> pointer.
>>> And to make matters worse, we also need to audit the completion path in
>>> the driver, which typically have the same 'issue'.
>>>
>>> Case in point:
>>>
>>> # git grep -- '->device' drivers/scsi | wc --lines
>>> 2712
>>>
>>> Which was the primary reason for adding a stub device to the SCSI Host;
>>> simply to avoid all the pointless churn and have a valid device for all
>>> commands.
>>>
>>> The only way I can see how to avoid getting dragged down into that
>>> rat-hole is to _not_ returning a scsi_cmnd, but rather something else
>>> entirely; that's the avenue I've exploited with my last patchset which
>>> would just return a tag number.
>>> But as there are drivers which really need a scsi_cmnd I can't se how we
>>> can get away with not having a stub scsi_device for the scsi host.
>>>
>>> And that won't even show up in sysfs if we assign it a LUN number beyond
>>> the addressable range; 'max_id':0 tends to be a safe choice here.
>>
>> There is no risk that the scsi_cmnd.device member will be dereferenced
>> for
>> internal requests allocated by the UFS driver. But I understand from your
>> email that making sure that the scsi_cmnd.device member is not NULL is
>> important for other SCSI LLDs. I will look into the approach of
>> associating
>> a stub SCSI device with internal requests.
>
> (replying to my own email)
>
> Hi Hannes,
>
> Allocating a struct scsi_device for internal requests seems tricky to
> me. The
> most straightforward approach would be to call scsi_alloc_sdev().
> However, that
> function accepts a scsi_target pointer and calls .slave_alloc(). So a
> scsi_target structure would have to be set up before that function is
> called and
> SCSI LLDs would have to be audited to verify that .slave_alloc() works
> fine for
> the H:C:I:L tuple assigned to the fake SCSI device. Additionally, how
> should the
> inquiry data be initialized that is filled in by scsi_add_lun()?
>
> Since I do not use SCSI hardware that needs a scsi_device to be
> associated with
> internal requests, I prefer that this functionality is implemented in a
> future
> patch series. Changing the hba->host->internal_queue occurrences in the UFS
> driver into something like hba->host->internal_sdev->request_queue once
> this
> functionality is implemented should be easy.
>
Well, I still do have the patchset for implementing it.
Will be reposting it.

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: Felix Imendörffer