Re: [PATCH v2 3/3] scsi: simplify scsi_stop_queue()

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

On 6/7/23 08:38, Martin Wilck wrote:
> On Wed, 2023-06-07 at 07:05 -0700, Bart Van Assche wrote:
>> On 6/7/23 02:26, Martin Wilck wrote:
>>> On Wed, 2023-06-07 at 07:27 +0200, Christoph Hellwig wrote:
>>>> On Tue, Jun 06, 2023 at 09:38:45PM +0200, mwilck@suse.com wrote:
>>>>>    scsi_target_block(struct device *dev)
>>>>>    {
>>>>> +       struct Scsi_Host *shost = dev_to_shost(dev);
>>>>> +
>>>>>           if (scsi_is_target_device(dev))
>>>>>                   starget_for_each_device(to_scsi_target(dev),
>>>>> NULL,
>>>>>                                           device_block);
>>>>>           else
>>>>>                   device_for_each_child(dev, NULL,
>>>>> target_block);
>>>>> +
>>>>> +       /* Wait for ongoing scsi_queue_rq() calls to finish. */
>>>>> +       if (!WARN_ON_ONCE(!shost))
>>>>
>>>> How could host ever be NULL here?  I can't see why we'd want this
>>>> check.
>>>
>>> The reason is simple: I wasn't certain if dev_to_shost() could
>>> return
>>> NULL, and preferred skipping the wait over an Oops. I hear you say
>>> that
>>> dev_to_shost() can't go wrong, so I'll remove the NULL test.
>>
>> I propose to pass shost as the first argument to scsi_target_block()
>> instead of using dev_to_shost() inside scsi_target_block(). Except in
>> __iscsi_block_session(), shost is already available as a local
>> variable.
> 
> If we do this, it might actually be cleaner to just pass the tag set to
> wait for.

Wouldn't that be close to a layering violation? Shouldn't SCSI APIs accept
pointers to SCSI objects instead of pointers to block layer abstractions?

Thanks,

Bart.