Re: [PATCH v6 09/19] scsi: allow enabling and disabling command duration limits

[Damien Le Moal <dlemoal@kernel.org>]

On 4/11/23 20:59, Niklas Cassel wrote:
> On Tue, Apr 11, 2023 at 04:38:48PM +0900, Damien Le Moal wrote:
>> On 4/11/23 16:23, Christoph Hellwig wrote:
>>> On Tue, Apr 11, 2023 at 04:09:34PM +0900, Damien Le Moal wrote:
>>>> But yes, I guess we could just unconditionally enable CDL for ATA on device scan
>>>> to be on par with scsi, which has CDL always enabled.
>>>
>>> I'd prefer that.  With a module option to not enable it just to be
>>> safe.
>>
>> Then it may be better to move the cdl_enable attribute store definition for ATA
>> devices to libata. That would be less messy that way. Let me see if that can be
>> done cleanly.
> 
> I don't think that the SCSI mode select can just be replaced with simple
> SET FEATURES in libata.
> 
> If we move the SET FEATURES call to a function in libata, and then use a
> function pointer in the scsi_host_template, and let only libata set this
> function pointer (similar to e.g. how the queue_depth sysfs attribute works),
> then the code will no longer work for SATA devices connected to a SAS HBA.
> 
> 
> 
> The current code simply checks if VPD page89 (the ATA Information VPD
> page - which is defined in the SCSI to ATA Translation (SAT) standard)
> exists. This page (and thus the sdev->vpd_pg89 pointer) will only exist
> if either:
> 1) An ATA device is connected to a SATA controller. This page will then
> be implemented by libata.
> 2) An ATA device is connected to a SAS HBA. The SAS HB will then provide
> this page. (The page will not exist for SCSI devices connected to the
> same SAS HBA.)
> 
> For case 1) with the current code, scsi.c will call scsi_mode_select()
> which will be translated by libata before being sent down to the device.
> 
> For case 2) with the current code, scsi.c will send down a SCSI mode
> select to the SAS HBA, and the SAS HBA will be responsible for translating
> the command before sending it to the device.
> 
> So I actually think that the current way to check if vpd page89 exists
> is probably the "cleanest" solution that I can think of.
> 
> If you have a better suggestion that will work for both case 1) and
> case 2), I will be happy to change the code accordingly.

Good point. If we move the code for cdl_enable to libata, then we will not be
covering the SAS HBA cases.

Christoph,

I do not see a cleaner solution... Can we keep this patch as is ? Any other idea ?