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

[Bart Van Assche <bvanassche@acm.org>]

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.

Thanks,

Bart.