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

[Hannes Reinecke <hare@suse.de>]

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