Re: [PATCH v2 4/5] ata: libata-core: Allow forcing most horkage flags

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 4/25/22 15:00, Hannes Reinecke wrote:
> On 4/25/22 03:34, Damien Le Moal wrote:
>> To facilitate debugging of drive issues in the field without kernel
>> changes (e.g. temporary test patches), add an entry for most horkage
>> flags in the force_tbl array to allow controlling these horkage
>> settings with the libata.force kernel boot parameter.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> ---
>>   drivers/ata/libata-core.c | 22 ++++++++++++++++++++--
>>   1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index e5a0e73b39d3..f68cb5639ec4 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -6230,9 +6230,27 @@ static const struct ata_force_param force_tbl[] __initconst = {
>>   	force_horkage_onoff(ncqtrim,	ATA_HORKAGE_NO_NCQ_TRIM),
>>   	force_horkage_onoff(ncqati,	ATA_HORKAGE_NO_NCQ_ON_ATI),
>>   
>> -	force_horkage_on(dump_id,	ATA_HORKAGE_DUMP_ID),
>> +	force_horkage_onoff(trim,	ATA_HORKAGE_NOTRIM),
>> +	force_horkage_on(trim_zero,	ATA_HORKAGE_ZERO_AFTER_TRIM),
>> +	force_horkage_on(max_trim_128m, ATA_HORKAGE_MAX_TRIM_128M),
>> +
>> +	force_horkage_onoff(dma,	ATA_HORKAGE_NODMA),
>>   	force_horkage_on(atapi_dmadir,	ATA_HORKAGE_ATAPI_DMADIR),
>> -	force_horkage_on(disable,	ATA_HORKAGE_DISABLE)
>> +	force_horkage_on(atapi_mod16_dma, ATA_HORKAGE_ATAPI_MOD16_DMA),
>> +
>> +	force_horkage_onoff(dmalog,	ATA_HORKAGE_NO_DMA_LOG),
>> +	force_horkage_onoff(iddevlog,	ATA_HORKAGE_NO_ID_DEV_LOG),
>> +	force_horkage_onoff(logdir,	ATA_HORKAGE_NO_LOG_DIR),
>> +
>> +	force_horkage_on(max_sec_128,	ATA_HORKAGE_MAX_SEC_128),
>> +	force_horkage_on(max_sec_1024,	ATA_HORKAGE_MAX_SEC_1024),
>> +	force_horkage_on(max_sec_lba48,	ATA_HORKAGE_MAX_SEC_LBA48),
>> +
>> +	force_horkage_onoff(lpm,	ATA_HORKAGE_NOLPM),
>> +	force_horkage_onoff(setxfer,	ATA_HORKAGE_NOSETXFER),
>> +	force_horkage_on(dump_id,	ATA_HORKAGE_DUMP_ID),
>> +
>> +	force_horkage_on(disable,	ATA_HORKAGE_DISABLE),
> 
> ... and this exemplifies my concerns with the 'onoff' mechanism:
> Why is 'disable' just marked as 'on' ?

Yeah, I can add the off side of it too. Fairly useless though as these are
off by default, except for the few cases where we already know that the
flag is needed, in which case turning it off would be a bad idea. So I do
not allow it by having the "on" only.

> Sure we can set it to 'off' (we have to, otherwise that flag would 
> always be set). And if we can set it to 'off', where's the different to 
> 'onoff' ?

Because of the reversed definition of the flag. E.g. nodmalog means *set*
ATA_HORKAGE_NO_DMA_LOG flags. so the "no" option means set. If we add the
off version for non reversed flags, then the "no" option would clear the
flag, not set it. It is a mess. That is the cleanest way I could think of
without making things even more messy.

At best, we can allow everything to be set/cleared using 2 macros:
onoff and offon, depending on the flag meaning polarity (i.e. a NO flag or
not).

> 
> Style-differences apart it looks good.
> 
> Cheers,
> 
> Hannes


-- 
Damien Le Moal
Western Digital Research