On 7/20/24 07:02, Igor Pylypiv wrote:
> On Thu, Jul 18, 2024 at 07:33:58PM +0900, Damien Le Moal wrote:
>
> Hi Damien,
>
>> Introduce the function ata_dev_print_horkage() to print the horkage
>> flags that will be used for a device. This new function is called from
>> ata_dev_horkage() when a match on a device model or device model and
>> revision is found for a device in the ata_dev_horkages array.
>>
>> To implement this function, the ATA_HORKAGE_ flags are redefined using
>> the new enum ata_horkage which defines the bit shift for each horkage
>> flag. The array of strings ata_horkage_names is used to define the name
>> of each flag, which are printed by ata_dev_print_horkage().
>>
>> Example output for a device listed in the ata_dev_horkages array and
>> which has the ATA_HORKAGE_DISABLE flag applied:
>>
>> [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>> [10193.469190] ata1.00: Model ASMT109x- Config, applying horkages: disable
>> [10193.469195] ata1.00: unsupported device, disabling
>> [10193.481564] ata1.00: disable device
>>
>> And while at it, make sure to use the unsigned int type for horkage
>> flags as struct ata_device->horkage is an unsugned int.
>
>
> It looks like we're soon going to run out of bits to use for the horkage
> flags. I think it might be better to change ata_device->horkage type to
> unsigned long. Given that the horkage bit numbers will be implicitly
> handled by the ata_horkage enum, it is more likely for someone to add
> a new horkage types/flags that will exceed 32 bits.
Yes, we are using 29 bits out of 32 available with unsigned int. But unsigned
long is still 32-bits on 32-bits arch, so that would not be an appropriate
solution. Furthermore, if one defines a horkage bit past value 31, you get a
compilation warning:
In file included from drivers/pnp/resource.c:20:
./include/linux/libata.h:439:30: warning: left shift count >= width of type
[-Wshift-count-overflow]
439 | ATA_HORKAGE_32 = (1U << __ATA_HORKAGE_32),
| ^~
which would be enough to signal to something needs to be done about the lack of
bits. So we can deal with that when/if it becomes needed.
We can also trivially add build checks to generate a compilation error:
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9cf0fe84186f..ef81aa0c1520 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4335,6 +4335,9 @@ static unsigned int ata_dev_horkage(const struct
ata_device *dev)
unsigned char model_rev[ATA_ID_FW_REV_LEN + 1];
const struct ata_dev_horkage_entry *ad = ata_dev_horkages;
+ /* dev->horkage is an unsigned int. */
+ BUILD_BUG_ON(__ATA_HORKAGE_MAX > 31);
+
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));
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5378d364c0f2..f9e922fb6446 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -58,6 +58,8 @@
/*
* Horkage types. May be set by libata or controller on drives.
* Some horkage may be drive/controller pair dependent.
+ * ata_device->horkage is an unsigned int, so __ATA_HORKAGE_MAX must not
+ * exceed 31.
*/
enum ata_horkage {
__ATA_HORKAGE_DIAGNOSTIC, /* Failed boot diag */
@@ -91,6 +93,8 @@ enum ata_horkage {
__ATA_HORKAGE_NO_ID_DEV_LOG, /* Identify device log missing */
__ATA_HORKAGE_NO_LOG_DIR, /* Do not read log directory */
__ATA_HORKAGE_NO_FUA, /* Do not use FUA */
+
+ __ATA_HORKAGE_MAX,
};
enum {
So I think I will respin the series and add this.
--
Damien Le Moal
Western Digital Research
