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

[Hannes Reinecke <hare@xxxxxxx>]

On 4/25/22 08:08, Damien Le Moal wrote:
> On 4/25/22 14:56, Hannes Reinecke wrote:
> > 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@xxxxxxxxxxxxxxxxxx>
> > > Reviewed-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
> > > ---
> > >    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 ...
>
> Problem is that when parsing the options and gathering the horkages/flags
> about the drive, nothing is known about it yet, so we do not have the link
> flags to directly modify based on the option name. So we have to keep the
> information about which flag to set and which to reset. Generating a
> "flags_mask" variable with all the correct flags set based on the option
> is easy, but we still need to know the operation to do with that mask on
> the device/link flags.
>
> > 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?
>
> For the existing libata.force option, yes, all this is cosmetic.
> For the new flags, we still have to deal with the reversed flags (the
> ATA_XXX_NO_...). And for these, the onoff is definitely reversed from the
> non reversed flags. And given that some of the reversed flags can already
> be set with libata.force, I find this way the cleanest to add new flags
> without breaking the manipulation of existing ones.
Ok.

Reviewed-by: Hannes Reinecke <hare@xxxxxxx>

Cheers,

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