Re: [PATCH v6 7/7] ata: libata: Enable fua support by default

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Tue, Nov 08, 2022 at 02:55:44PM +0900, Damien Le Moal wrote:
> Change the default value of the fua module parameter to 1 to enable fua
> support by default for all devices supporting it.
> 
> FUA support can be disabled for individual drives using the
> force=[ID]nofua libata module argument.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/ata/libata-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 97ade977b830..2967671131d2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -127,9 +127,9 @@ int atapi_passthru16 = 1;
>  module_param(atapi_passthru16, int, 0444);
>  MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");
>  
> -int libata_fua = 0;
> +int libata_fua = 1;
>  module_param_named(fua, libata_fua, int, 0444);
> -MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
> +MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])");
>  
>  static int ata_ignore_hpa;
>  module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);
> -- 
> 2.38.1
> 

Before this commit:
-For a SCSI mode sense:
 FUA bit will not get set in the simulated SCSI cmd output buffer,
 since ATA_DFLAG_FUA is not be set, since libata_fua == 0.
-For a SCSI WRITE_16 / WRITE_16 command:
 ata_scsi_rw_xlat() sets ATA_TFLAG_FUA, regardless if ATA_DFLAG_FUA
 is set or not. ata_set_rwcmd_protocol() will return a FUA command
 as long as ATA_TFLAG_FUA is set.

After this commit:
-For a SCSI mode sense:
 FUA bit will get set in the simulated SCSI cmd output buffer,
 since ATA_DFLAG_FUA is set, since libata_fua == 1.
-For a SCSI WRITE_16 / WRITE_16 command:
 ata_scsi_rw_xlat() sets ATA_TFLAG_FUA, regardless if ATA_DFLAG_FUA
 is set or not. ata_set_rwcmd_protocol() will return a FUA command
 as long as ATA_TFLAG_FUA is set.


Perhaps this commit should more clearly say that this commit only affects
the simulated output for a SCSI mode sense command?

Currently, the commit message is a bit misleading, since it makes the
reader think that a SCSI write command with the FUA bit was not propagated
to the device before this commit, which AFAICT, is not true.

If the intention of this series was to only send a ATA write command to
the device, if the device has the ATA_DFLAG_FUA flag set, then perhaps the
series should include a new commit which either:
-Adds a check to ata_scsi_rw_xlat() so that it only sets ATA_TFLAG_FUA if
ATA_DFLAG_FUA is set;
or
-Adds a check to ata_scsi_rw_xlat() which returns an error if the SCSI
command has the FUA bit set, but ATA_DFLAG_FUA is not set.


Kind regards,
Niklas