Re: [PATCH 3/3] scsi: Improve CDL control

[Igor Pylypiv <ipylypiv@google.com>]

On Thu, Apr 17, 2025 at 08:08:00PM +0900, Damien Le Moal wrote:
> On 4/17/25 12:37, Igor Pylypiv wrote:
> > On Wed, Apr 16, 2025 at 05:42:38PM +0900, Damien Le Moal wrote:
> >> With ATA devices supporting the CDL feature, using CDL requires that the
> >> feature be enabled with a SET FEATURES command. This command is issued
> >> as the translated command for the MODE SELECT command issued by
> >> scsi_cdl_enable() when the user enables CDL through the device
> >> cdl_enable sysfs attribute.
> >>
> >> However, the implementation of scsi_cdl_enable() always issues a MODE
> >> SELECT command for ATA devices when the enable argument is true, even if
> >> CDL is already enabled on the device. While this does not cause any
> >> issue with using CDL descriptors with read/write commands (the CDL
> >> feature will be enabled on the drive), issuing the MODE SELECT command
> >> even when the device CDL feature is already enabled will cause a reset
> >> of the ATA device CDL statistics log page (as defined in ACS, any CDL
> >> enable action must reset the device statistics).
> >>
> >> Avoid this needless actions (and the implied statistics log page reset)
> >> by modifying scsi_cdl_enable() to issue the MODE SELECT command to
> >> enable CDL if and only if CDL is not reported as already enabled on the
> >> device.
> > 
> > Hi Damien,
> > 
> > What happens when a drive spins up with CDL enabled? Last year you sent
> > a patch to make sure that CDL gets disabled by default. Is that still
> > the case?
> 
> Yes, that is unchanged so that we keep being consistent with the fact that the
> scsi layer starts with the sysfs cdl_enabled attribute set to false. So if an
> ATA device starts with CDL enabled, it will be disabled.
> 
> That does not cause any issue with the CDL statistics log page because that page
> is not persistent and cleared) on power-on-reset events and this change has no
> effect on that. The CDL statistics log page will always be cleared on boot/reboot.
> 

Sounds good. Thank you for confirming, Damien!

Reviewed-by: Igor Pylypiv <ipylypiv@google.com>

> 
> -- 
> Damien Le Moal
> Western Digital Research