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

[Hannes Reinecke <hare@xxxxxxx>]

On 4/25/22 08:15, Damien Le Moal wrote:
> 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@xxxxxxxxxxxxxxxxxx>
> > > Reviewed-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
> > > ---
> > >    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).

Hmm. Yeah, I see. Ugly, but probably the easiest for now.

Reviewed-by: Hannes Reinecke <hare@xxxxxxx>

Cheers,

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