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.
>
> Cheers,
>
> Hannes
--
Damien Le Moal
Western Digital Research
