| Message ID | 20240724054539.182655-3-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Some cleanup, renaming and horkage improvements | expand |
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
On 7/26/24 01:43, Niklas Cassel wrote: > 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) Yep, when writing these patches, I thought about doing such renaming but end choosing the lazy path :) I completely agree about it, so will add patches to do that.
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) { unsigned char model_num[ATA_ID_PROD_LEN + 1]; unsigned char model_rev[ATA_ID_FW_REV_LEN + 1]; - const struct ata_blacklist_entry *ad = ata_device_blacklist; + const struct ata_dev_horkage_entry *ad = ata_dev_horkages; ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num)); ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev)); @@ -4372,8 +4372,7 @@ static int cable_is_40wire(struct ata_port *ap) * * Compute supported xfermask of @dev and store it in * dev->*_mask. This function is responsible for applying all - * known limits including host controller limits, device - * blacklist, etc... + * known limits including host controller limits, device horkages, etc... * * LOCKING: * None.