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: iy9ebTzSVrt94Escn0VyMw0dZlFLZ/fytJ/1S0YVJTb8MviF6te9laZwC+hc0nBXL5e3fsxqZ+
 0K5LG+R7F0Qii0ZCIVhKgHw0mAjdJxORFFCrcsVlCfSNoLB89RzusagSNA79iRLx5XjD41enhx
 pOjsn1mGMctiRLwMLsl07NC42EcZhyuuB4nniI1cMI4VDNrMWq65CdC3NGtdVRA17RoEpyjRhW
 OicV72g3FVGXq0oTSxWaanLU6zivPnA9XfKHs2+R7Cz2hDWb4Jr+348ZpleKyarkwavv0uvsUr
 n2N941JRVuAlxUP3iInn8L5a
IronPort-SDR: rEMlbAG68CAtizGBaR23fiYROnWu6XzWFAaCREFnQ/NEEI5BeN1fu16VJxuWXpVHAkaGkJXdt9
 2oEHKJDw6BpuMLl//Wd2r16T4uZ+UsOsBiTsBTBib/rl0El3kEMNd48kRLqfuCeBZaSNFJVLM2
 q5viSI0AuiKEy98AZsaaZAlt9uxnJ7lQDYBfbym/0p/ENcYiq8FCRlnuW3VmYx5Bl2ImZ/sLv0
 PD4t/x+mqwYkcqECCrNQ5DDDRIS2d9TxvGUyIV9p/EOHWOIMNKw9wfzgFZqutylqB3oqIXKrhJ
 cWU=
WDCIronportException: Internal
Message-ID: <28712bad-8215-4246-7370-42d204488aa3@opensource.wdc.com>
Date:   Fri, 7 Oct 2022 07:20:45 +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>
 <24a48f71-8a79-6311-1e43-494df0458a32@opensource.wdc.com>
 <7ecf20b7-794a-39d8-0b03-8f19d9167efd@maciej.szmigiero.name>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <7ecf20b7-794a-39d8-0b03-8f19d9167efd@maciej.szmigiero.name>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/6/22 22:06, Maciej S. Szmigiero wrote:
> On 6.10.2022 01:38, Damien Le Moal wrote:
>> 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.
> 
> A specific Kconfig option is more structured than a free-form
> CONFIG_CMDLINE (which is also technically a per-arch option, but seems
> to be widely supported across arches).
> 
> That's why there is a lot (100+) of similar Kconfig default-changing
> options, a quick sample of these (in no particular order):
> SOUND_OSS_CORE_PRECLAIM, SND_INTEL_BYT_PREFER_SOF, LSM,
> SECURITY_SELINUX_CHECKREQPROT_VALUE, SECURITY_LOADPIN_ENFORCE,
> SECURITY_APPARMOR_DEBUG_MESSAGES, IP_VS_TAB_BITS, IP_SET_MAX,
> MAC80211_HAS_RC, SLUB_DEBUG_ON, KFENCE_SAMPLE_INTERVAL, PRINTK_TIME,
> DEBUG_OBJECTS_ENABLE_DEFAULT, RCU_NOCB_CPU_DEFAULT_ALL, ...
> 
> libata currently has only one similar option: SATA_MOBILE_LPM_POLICY,
> so it's not like a person performing kernel configuration is
> overloaded with questions here.
> 
> But at the same time, I respect your decision as a maintainer of
> this code.

I am not dead set on pushing back on this, but as usual, whenever we can
avoid adding config options, we should. Given that libata has had fua
disabled forever, I am not convinced yet that there is a strong need for
that new option. But if distros prefer the config option approach, we can
make that happen.

If anything, I would be tempted to switch fua support to on by default
after some time if we do not get many reports of broken drives. You did
mention that old mac minis drives did not like it, but these are not even
blacklisted in libata-scsi. They should. Only one model of maxtor drives
is. We should add an ATA_HORKAGE_NO_FUA flag and start a proper blacklist
of drives not liking fua. Without that in place, switching to on by
default as your config option allows could break many (old) systems.


> 
>>
>> Patch 1 looks good. I will queue it up once rc1 is out.
> 
> Thanks,
> Maciej
> 
> 

-- 
Damien Le Moal
Western Digital Research