Re: [PATCH] scsi: mpi3mr: Fix SATA NCQ priority support

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

On 6/6/24 18:31, Christoph Hellwig wrote:
> On Thu, Jun 06, 2024 at 02:47:49PM +0900, Damien Le Moal wrote:
>> The function mpi3mr_qcmd() of the mpi3mr driver is able to indicate to
>> the HBA if a read or write command directed at a SATA device should be
>> executed using NCQ high priority, if the request uses the RT priority
>> class and the user has enabled NCQ priority through sysfs.
>>
>> However, unlike the mpt3sas driver, the mpi3mr driver does not define
>> the sas_ncq_prio_supported and sas_ncq_prio_enable sysfs attributes, so
>> the ncq_prio_enable field of struct mpi3mr_sdev_priv_data is never
>> actually set and NCQ Priority cannot ever be used.
>>
>> Fix this by defining these missing atributes to allow a user to check if
>> a device supports NCQ priority and to enable/disable the use of NCQ
>> priority. To do this, lift the function scsih_ncq_prio_supp() out of the
>> mpt3sas driver and make it the generic scsi device function
>> scsi_ncq_prio_supported(). Nothing in that function is hardware
>> specific, so this function can be used for both the mpt3sas driver and
>> the mpi3mr driver.
> 
> Shouldn't this move into the SAS transport class instead then?
> 
>> +/**
>> + * scsi_ncq_prio_supported - Check for NCQ command priority support
>> + * @sdev: SCSI device
>> + *
>> + * Check if a (SATA) device supports NCQ priority. For non-SATA devices,
>> + * this always return false.
>> + */
>> +bool scsi_ncq_prio_supported(struct scsi_device *sdev)
>> +{
>> +	struct scsi_vpd *vpd;
>> +	bool ncq_prio_supported = false;
>> +
>> +	rcu_read_lock();
>> +	vpd = rcu_dereference(sdev->vpd_pg89);
>> +	if (vpd && vpd->len >= 214)
>> +		ncq_prio_supported = (vpd->data[213] >> 4) & 1;
>> +	rcu_read_unlock();
>> +
>> +	return ncq_prio_supported;
>> +}
>> +EXPORT_SYMBOL_GPL(scsi_ncq_prio_supported);
> 
> This also feels kinda out of place in the core SCSI code and more in
> scope for the SAS transport class, even if the other code can't
> move there for whatever reason.

"also" ? your previous point was not about this function ?

But I think I get it. I will move this to scsi_transport_sas.c and rename it to
sas_ata_ncq_prio_supported().

-- 
Damien Le Moal
Western Digital Research