Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: P7jIjsaMcYW7dfbyD8cn1QWTbbHaLVuEL0XpkKRe1xduq5iv7boDv9xbUAupLcYzAqXMnW/URL
 NXcZRoKd+POQbzL98xoW1Iu1KHVSyiqLnCCOzCvSoeJU90pZOCB489Fh7eu2DK9NH/UA1KpYpF
 ZG5F4qIQUZiFviyBBFJoDXxP8pkpEdTwd8TqPptK0sgg/qDv3INcu0sLfDdu/p3PM2stVSjbP+
 00MfcoWQUQr0v6vTT8c4SkHsj0rRqf5w7fPg1MsC4w3B1Jo8kVrmkMCG950I96JRPOy9Y3ygwl
 tyjOecHwL2snAMmBf3b29czG
IronPort-SDR: 90S8hEIAPYRDO+ZCqyyyQ62nqe5X7zftmwQPdDu+tOY7mtRPMDUR5T9wXywfKn+gJnFHcyGmDa
 j8XgUb4+dy1GJAe+isYXvUf0imy9aM3ryZczw7j2Fh4EYxN7x7b8oqjXHLmz+9CpIiACF3wndW
 z3ou3pMp42kqT+5QFcK1VAbCDhmvrx96UF//nhm/68O5gN6o/+nZenpUIQn27A3OGJgTNmR/qE
 KZ8+MeCM0TrmtzwvoMaQmIuGze5PQqWf4jgNMuwGKQmTjAU0Q4TkouicVkFssCg2Yux5cl83gB
 n2w=
WDCIronportException: Internal
Message-ID: <24a48f71-8a79-6311-1e43-494df0458a32@opensource.wdc.com>
Date:   Thu, 6 Oct 2022 08:38:38 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig
Content-Language: en-US
To:     "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>
Cc:     linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org
References: <ed1c179cff84b49e22cf229cc69751fa4d357d9a.1664221825.git.maciej.szmigiero@oracle.com>
 <df8701f3905c1a394863e57c7a2d30c5b5dc3503.1664221825.git.maciej.szmigiero@oracle.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <df8701f3905c1a394863e57c7a2d30c5b5dc3503.1664221825.git.maciej.szmigiero@oracle.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/27/22 04:51, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.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@oracle.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