Re: [PATCH v5 2/4] ata: libata: Rename ata_dev_blacklisted()

[Niklas Cassel <cassel@kernel.org>]

On Wed, Jul 24, 2024 at 02:45:37PM +0900, Damien Le Moal wrote:
> Rename the function ata_dev_blacklisted() to ata_dev_horkage() as this
> new name:
> 1) Does not use an expression that can be considered as negatively loaded.
> 2) The name does not reflect what the function actually does, which is
>    returning a set of horkage flag for the device, of which only one
>    flag will completely disable the device.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-core.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index a35bce4236d3..272770f09609 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -84,7 +84,7 @@ static unsigned int ata_dev_init_params(struct ata_device *dev,
>  					u16 heads, u16 sectors);
>  static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
>  static void ata_dev_xfermask(struct ata_device *dev);
> -static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
> +static unsigned long ata_dev_horkage(const struct ata_device *dev);
>  
>  static DEFINE_IDA(ata_ida);
>  
> @@ -2223,7 +2223,7 @@ static inline u8 ata_dev_knobble(struct ata_device *dev)
>  {
>  	struct ata_port *ap = dev->link->ap;
>  
> -	if (ata_dev_blacklisted(dev) & ATA_HORKAGE_BRIDGE_OK)
> +	if (ata_dev_horkage(dev) & ATA_HORKAGE_BRIDGE_OK)
>  		return 0;
>  
>  	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev->id)));
> @@ -2830,7 +2830,7 @@ int ata_dev_configure(struct ata_device *dev)
>  	}
>  
>  	/* set horkage */
> -	dev->horkage |= ata_dev_blacklisted(dev);
> +	dev->horkage |= ata_dev_horkage(dev);
>  	ata_force_horkage(dev);
>  
>  	if (dev->horkage & ATA_HORKAGE_DISABLE) {
> @@ -3987,13 +3987,13 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
>  	return rc;
>  }
>  
> -struct ata_blacklist_entry {
> +struct ata_dev_horkage_entry {
>  	const char *model_num;
>  	const char *model_rev;
>  	unsigned long horkage;
>  };
>  
> -static const struct ata_blacklist_entry ata_device_blacklist [] = {
> +static const struct ata_dev_horkage_entry ata_dev_horkages[] = {
>  	/* Devices with DMA related problems under Linux */
>  	{ "WDC AC11000H",	NULL,		ATA_HORKAGE_NODMA },
>  	{ "WDC AC22100H",	NULL,		ATA_HORKAGE_NODMA },
> @@ -4111,7 +4111,7 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
>  
>  	/* Devices which get the IVB wrong */
>  	{ "QUANTUM FIREBALLlct10 05", "A03.0900", ATA_HORKAGE_IVB },
> -	/* Maybe we should just blacklist TSSTcorp... */
> +	/* Maybe we should just add all TSSTcorp devices... */
>  	{ "TSSTcorp CDDVDW SH-S202[HJN]", "SB0[01]",  ATA_HORKAGE_IVB },
>  
>  	/* Devices that do not need bridging limits applied */
> @@ -4266,11 +4266,11 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
>  	{ }
>  };
>  
> -static unsigned long ata_dev_blacklisted(const struct ata_device *dev)
> +static unsigned long ata_dev_horkage(const struct ata_device *dev)

So it turns that the term "horkage" is only used by libata:
$ git grep -i horkage

See also:
https://unix.stackexchange.com/questions/752755/what-is-horkage

I have mixed emotions about this. In one way I want to preserve the legacy,
but in one way, the term quirk/quirks is used everywhere else in the kernel,
so it is way more intuitive for other kernel developers to understand what
this is if we just use the common terminology.

I do think that the name ata_dev_horkage() is not good because it sounds
like you can just have a single horkage, but in fact a devices can have
serveral quirks. So should the name be ata_dev_horkages() ?

Search the kernel tree gives zero results for this before your patch:
$ git grep -i horkages origin/master  | wc -l
0

So maybe the plural of horkage is horkage???

To me, this again suggests that quirk/quirks is way clearer.
(And we do also use the term quirk in libata:
$ git grep -i quirk drivers/ata
so in one way renaming would also make us more consistent...)

I do see that you have used "horkages" in this patch however.

So either migrate to quirk/quirks, or we continue to make up our own words.

I prefer the former.
dev->quirks |= ata_dev_quirks(dev);


If we choose the latter, may I suggest that we change this line:
dev->horkage |= ata_dev_horkage(dev);
to:
dev->horkages |= ata_dev_horkages(dev);

to make it clearer that we can have several *whatever the plural of horkage*.

see e.g. how nvme does it:
ctrl->quirks = quirks;
and
if (ns->ctrl->quirks & NVME_QUIRK_DEALLOCATE_ZEROES)


Kind regards,
Niklas