Re: [PATCH v2 3/5] ata: libata-core: Improve link flags forced settings

[Hannes Reinecke <hare@suse.de>]

On 4/25/22 03:34, Damien Le Moal wrote:
> Similarly to the horkage flags, introduce the force_lflag_onoff() macro
> to define struct ata_force_param entries of the force_tbl array that
> allow turning on or off a link flag using the libata.force boot
> parameter. To be consistent with naming, the macro force_lflag() is
> renamed to force_lflag_on().
> 
> Using force_lflag_onoff(), define a new force_tbl entry for the
> ATA_LFLAG_NO_DEBOUNCE_DELAY link flag, thus allowing testing if an
> adapter requires a link debounce delay or not.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
>   drivers/ata/libata-core.c | 32 ++++++++++++++++++++++----------
>   1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index dfdb23c2bbd6..e5a0e73b39d3 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -96,7 +96,8 @@ struct ata_force_param {
>   	unsigned long	xfer_mask;
>   	unsigned int	horkage_on;
>   	unsigned int	horkage_off;
> -	u16		lflags;
> +	u16		lflags_on;
> +	u16		lflags_off;
>   };
>   
>   struct ata_force_ent {
> @@ -386,11 +387,17 @@ static void ata_force_link_limits(struct ata_link *link)
>   		}
>   
>   		/* let lflags stack */
> -		if (fe->param.lflags) {
> -			link->flags |= fe->param.lflags;
> +		if (fe->param.lflags_on) {
> +			link->flags |= fe->param.lflags_on;
>   			ata_link_notice(link,
>   					"FORCE: link flag 0x%x forced -> 0x%x\n",
> -					fe->param.lflags, link->flags);
> +					fe->param.lflags_on, link->flags);
> +		}
> +		if (fe->param.lflags_off) {
> +			link->flags &= ~fe->param.lflags_off;
> +			ata_link_notice(link,
> +				"FORCE: link flag 0x%x cleared -> 0x%x\n",
> +				fe->param.lflags_off, link->flags);
>   		}
>   	}
>   }
> @@ -6153,8 +6160,12 @@ EXPORT_SYMBOL_GPL(ata_platform_remove_one);
>   #define force_xfer(mode, shift)				\
>   	{ #mode,	.xfer_mask	= (1UL << (shift)) }
>   
> -#define force_lflag(name, flags)			\
> -	{ #name,	.lflags		= (flags) }
> +#define force_lflag_on(name, flags)			\
> +	{ #name,	.lflags_on	= (flags) }
> +
> +#define force_lflag_onoff(name, flags)			\
> +	{ "no" #name,	.lflags_on	= (flags) },	\
> +	{ #name,	.lflags_off	= (flags) }
>   
>   #define force_horkage_on(name, flag)			\
>   	{ #name,	.horkage_on	= (flag) }
> @@ -6209,10 +6220,11 @@ static const struct ata_force_param force_tbl[] __initconst = {
>   	force_xfer(udma/133,		ATA_SHIFT_UDMA + 6),
>   	force_xfer(udma7,		ATA_SHIFT_UDMA + 7),
>   
> -	force_lflag(nohrst,		ATA_LFLAG_NO_HRST),
> -	force_lflag(nosrst,		ATA_LFLAG_NO_SRST),
> -	force_lflag(norst,		ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST),
> -	force_lflag(rstonce,		ATA_LFLAG_RST_ONCE),
> +	force_lflag_on(nohrst,		ATA_LFLAG_NO_HRST),
> +	force_lflag_on(nosrst,		ATA_LFLAG_NO_SRST),
> +	force_lflag_on(norst,		ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST),
> +	force_lflag_on(rstonce,		ATA_LFLAG_RST_ONCE),
> +	force_lflag_onoff(dbdelay,	ATA_LFLAG_NO_DEBOUNCE_DELAY),
>   
>   	force_horkage_onoff(ncq,	ATA_HORKAGE_NONCQ),
>   	force_horkage_onoff(ncqtrim,	ATA_HORKAGE_NO_NCQ_TRIM),

Hmm.

Personally, I'm not a fan of distinct 'flags_on' and 'flags_off'; I 
always wonder what does happen if one sets the same value for both ...

And do we really need this? All settings above are just simple flags 
which can be set or unset.
Sure, some flags are inverted, but still they are flags.
So why do we need the separation here?
Isn't it rather cosmetical?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer