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 ?
