linux-kernel - Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <24a48f71-8a79-6311-1e43-494df0458a32@opensource.wdc.com>
Date:   Thu, 6 Oct 2022 08:38:38 +0900
From:   Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To:     "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
Cc:     linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig

On 9/27/22 04:51, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@...cle.com>
> 
> Currently, if one wants to make use of FUA support in libata it is
> necessary to provide an explicit kernel command line parameter in order to
> enable it (for drives that report such support).
> 
> In terms of Git archaeology: FUA support was enabled by default in early
> libata versions but was disabled soon after.
> Since then there were a few attempts to enable this support by default:
> [1] (for NCQ drives only), [2] (for all drives).
> However, the second change had to be reverted after a report came of
> an incompatibility with the HDD in 2011 Mac Mini.
> 
> Enabling FUA avoids having to emulate it by issuing an extra drive cache
> flush for every request that have this flag set.
> Since FUA support is required by the ATA/ATAPI spec for any drive that
> supports LBA48 and so these days should be pretty widespread let's provide
> an ability to enable it by default in Kconfig.

This can be done by adding "libata.fua=1" to the CONFIG_CMDLINE option. So
I do not see the need to add yet another config option.

Patch 1 looks good. I will queue it up once rc1 is out.

> 
> [1]: https://lore.kernel.org/lkml/45CFFF82.4030301@shaw.ca/
> [2]: https://lore.kernel.org/lkml/1336447443-4685-1-git-send-email-wenqing.lz@taobao.com/
> 
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@...cle.com>
> ---
>  drivers/ata/Kconfig       | 15 +++++++++++++++
>  drivers/ata/libata-core.c |  5 +++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 36833a8629980..fd39bb22963a3 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -102,6 +102,21 @@ config SATA_PMP
>  	  This option adds support for SATA Port Multipliers
>  	  (the SATA version of an ethernet hub, or SAS expander).
>  
> +config ATA_FUA
> +	bool "ATA FUA support"
> +	help
> +	  Enables Forced Unit Access (FUA) support by default for drives that
> +	  have it.
> +	  Otherwise the FUA flag has to be emulated by flushing the drive cache.
> +
> +	  Regardless of this option, you can enable or disable such support at
> +	  kernel boot time by providing libata.fua=1 or libata.fua=0 kernel
> +	  command line parameter.
> +
> +	  If building a kernel for yourself, say Y.
> +	  If building a kernel for a distro that's supposed to run out of the
> +	  box on old and broken hardware say N.
> +
>  if HAS_DMA
>  
>  comment "Controllers with non-SFF native interface"
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index b322006c85806..941836c4b5eda 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -127,9 +127,10 @@ 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 = IS_ENABLED(CONFIG_ATA_FUA);
>  module_param_named(fua, libata_fua, int, 0644);
> -MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
> +MODULE_PARM_DESC(fua,
> +		 "FUA support (0=off, 1=on), default " __stringify(IS_ENABLED(CONFIG_ATA_FUA)));
>  
>  static int ata_ignore_hpa;
>  module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);

-- 
Damien Le Moal
Western Digital Research