On Tue, Apr 11, 2023 at 01:59:44PM +0200, 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. In addition to: https://github.com/torvalds/linux/blame/v6.3-rc6/drivers/scsi/sd.c#L3066-L3074 checking for the existence of VPD page89 is also currently done by e.g.: https://github.com/torvalds/linux/blob/v6.3-rc6/drivers/scsi/mpt3sas/mpt3sas_scsih.c#L12607 and https://github.com/torvalds/linux/blob/v6.3-rc6/drivers/hwmon/drivetemp.c#L340 Kind regards, Niklas
