Re: [PATCH v2 3/3] ata: libata: Print horkages applied to devices

[Igor Pylypiv <ipylypiv@google.com>]

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.

> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-core.c |  79 ++++++++++++++++++++++++++--
>  include/linux/libata.h    | 108 +++++++++++++++++++++++++-------------
>  2 files changed, 145 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index ee958d2893e6..398f0aa10b1b 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_horkage(const struct ata_device *dev);
> +static unsigned int ata_dev_horkage(const struct ata_device *dev);
>  
>  static DEFINE_IDA(ata_ida);
>  
> @@ -3987,10 +3987,73 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
>  	return rc;
>  }
>  
> +static const char *ata_horkage_names[] = {
> +	[__ATA_HORKAGE_DIAGNOSTIC]	= "diagnostic",
> +	[__ATA_HORKAGE_NODMA]		= "nodma",
> +	[__ATA_HORKAGE_NONCQ]		= "noncq",
> +	[__ATA_HORKAGE_MAX_SEC_128]	= "maxsec128",
> +	[__ATA_HORKAGE_BROKEN_HPA]	= "brokenhpa",
> +	[__ATA_HORKAGE_DISABLE]		= "disable",
> +	[__ATA_HORKAGE_HPA_SIZE]	= "hpasize",
> +	[__ATA_HORKAGE_IVB]		= "ivb",
> +	[__ATA_HORKAGE_STUCK_ERR]	= "stuckerr",
> +	[__ATA_HORKAGE_BRIDGE_OK]	= "bridgeok",
> +	[__ATA_HORKAGE_ATAPI_MOD16_DMA]	= "atapimod16dma",
> +	[__ATA_HORKAGE_FIRMWARE_WARN]	= "firmwarewarn",
> +	[__ATA_HORKAGE_1_5_GBPS]	= "1.5gbps",
> +	[__ATA_HORKAGE_NOSETXFER]	= "nosetxfer",
> +	[__ATA_HORKAGE_BROKEN_FPDMA_AA]	= "brokenfpdmaaa",
> +	[__ATA_HORKAGE_DUMP_ID]		= "dumpid",
> +	[__ATA_HORKAGE_MAX_SEC_LBA48]	= "maxseclba48",
> +	[__ATA_HORKAGE_ATAPI_DMADIR]	= "atapidmadir",
> +	[__ATA_HORKAGE_NO_NCQ_TRIM]	= "noncqtrim",
> +	[__ATA_HORKAGE_NOLPM]		= "nolpm",
> +	[__ATA_HORKAGE_WD_BROKEN_LPM]	= "wdbrokenlpm",
> +	[__ATA_HORKAGE_ZERO_AFTER_TRIM]	= "zeroaftertrim",
> +	[__ATA_HORKAGE_NO_DMA_LOG]	= "nodmalog",
> +	[__ATA_HORKAGE_NOTRIM]		= "notrim",
> +	[__ATA_HORKAGE_MAX_SEC_1024]	= "maxsec1024",
> +	[__ATA_HORKAGE_MAX_TRIM_128M]	= "maxtrim128m",
> +	[__ATA_HORKAGE_NO_NCQ_ON_ATI]	= "noncqonati",
> +	[__ATA_HORKAGE_NO_ID_DEV_LOG]	= "noiddevlog",
> +	[__ATA_HORKAGE_NO_LOG_DIR]	= "nologdir",
> +	[__ATA_HORKAGE_NO_FUA]		= "nofua",
> +};
> +
> +static void ata_dev_print_horkage(const struct ata_device *dev,
> +				  const char *model, const char *rev,
> +				  unsigned int horkage)
> +{
> +	int n = 0, i;
> +	size_t sz;
> +	char *str;
> +
> +	if (!horkage)
> +		return;
> +
> +	sz = 64 + ARRAY_SIZE(ata_horkage_names) * 16;
> +	str = kmalloc(sz, GFP_KERNEL);
> +	if (!str)
> +		return;
> +
> +	n = snprintf(str, sz, "Model %s%s%s, applying horkages:",
> +		     model, rev ? ", rev" : "", rev ? rev : "");
> +
> +	for (i = 0; i < ARRAY_SIZE(ata_horkage_names); i++) {
> +		if (horkage & (1U << i))
> +			n += snprintf(str + n, sz - n,
> +				      " %s", ata_horkage_names[i]);
> +	}
> +
> +	ata_dev_warn(dev, "%s", str);
> +
> +	kfree(str);
> +}
> +
>  struct ata_dev_horkage_entry {
>  	const char *model_num;
>  	const char *model_rev;
> -	unsigned long horkage;
> +	unsigned int horkage;
>  };
>  
>  static const struct ata_dev_horkage_entry ata_dev_horkages[] = {
> @@ -4266,7 +4329,7 @@ static const struct ata_dev_horkage_entry ata_dev_horkages[] = {
>  	{ }
>  };
>  
> -static unsigned long ata_dev_horkage(const struct ata_device *dev)
> +static unsigned int 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];
> @@ -4277,10 +4340,16 @@ static unsigned long ata_dev_horkage(const struct ata_device *dev)
>  
>  	while (ad->model_num) {
>  		if (glob_match(ad->model_num, model_num)) {
> -			if (ad->model_rev == NULL)
> +			if (ad->model_rev == NULL) {
> +				ata_dev_print_horkage(dev, model_num,
> +						      NULL, ad->horkage);
>  				return ad->horkage;
> -			if (glob_match(ad->model_rev, model_rev))
> +			}
> +			if (glob_match(ad->model_rev, model_rev)) {
> +				ata_dev_print_horkage(dev, model_num,
> +						      model_rev, ad->horkage);
>  				return ad->horkage;
> +			}
>  		}
>  		ad++;
>  	}
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 17394098bee9..5378d364c0f2 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -55,6 +55,44 @@
>  /* defines only for the constants which don't work well as enums */
>  #define ATA_TAG_POISON		0xfafbfcfdU
>  
> +/*
> + * Horkage types. May be set by libata or controller on drives.
> + * Some horkage may be drive/controller pair dependent.
> + */
> +enum ata_horkage {
> +	__ATA_HORKAGE_DIAGNOSTIC,	/* Failed boot diag */
> +	__ATA_HORKAGE_NODMA,		/* DMA problems */
> +	__ATA_HORKAGE_NONCQ,		/* Don't use NCQ */
> +	__ATA_HORKAGE_MAX_SEC_128,	/* Limit max sects to 128 */
> +	__ATA_HORKAGE_BROKEN_HPA,	/* Broken HPA */
> +	__ATA_HORKAGE_DISABLE,		/* Disable it */
> +	__ATA_HORKAGE_HPA_SIZE,		/* native size off by one */
> +	__ATA_HORKAGE_IVB,		/* cbl det validity bit bugs */
> +	__ATA_HORKAGE_STUCK_ERR,	/* stuck ERR on next PACKET */
> +	__ATA_HORKAGE_BRIDGE_OK,	/* no bridge limits */
> +	__ATA_HORKAGE_ATAPI_MOD16_DMA,	/* use ATAPI DMA for commands
> +					    not multiple of 16 bytes */
> +	__ATA_HORKAGE_FIRMWARE_WARN,	/* firmware update warning */
> +	__ATA_HORKAGE_1_5_GBPS,		/* force 1.5 Gbps */
> +	__ATA_HORKAGE_NOSETXFER,		/* skip SETXFER, SATA only */
> +	__ATA_HORKAGE_BROKEN_FPDMA_AA,	/* skip AA */
> +	__ATA_HORKAGE_DUMP_ID,		/* dump IDENTIFY data */
> +	__ATA_HORKAGE_MAX_SEC_LBA48,	/* Set max sects to 65535 */
> +	__ATA_HORKAGE_ATAPI_DMADIR,	/* device requires dmadir */
> +	__ATA_HORKAGE_NO_NCQ_TRIM,	/* don't use queued TRIM */
> +	__ATA_HORKAGE_NOLPM,		/* don't use LPM */
> +	__ATA_HORKAGE_WD_BROKEN_LPM,	/* some WDs have broken LPM */
> +	__ATA_HORKAGE_ZERO_AFTER_TRIM,	/* guarantees zero after trim */
> +	__ATA_HORKAGE_NO_DMA_LOG,	/* don't use DMA for log read */
> +	__ATA_HORKAGE_NOTRIM,		/* don't use TRIM */
> +	__ATA_HORKAGE_MAX_SEC_1024,	/* Limit max sects to 1024 */
> +	__ATA_HORKAGE_MAX_TRIM_128M,	/* Limit max trim size to 128M */
> +	__ATA_HORKAGE_NO_NCQ_ON_ATI,	/* Disable NCQ on ATI chipset */
> +	__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 */
> +};
> +
>  enum {
>  	/* various global constants */
>  	LIBATA_MAX_PRD		= ATA_MAX_PRD / 2,
> @@ -362,43 +400,39 @@ enum {
>  	 */
>  	ATA_EH_CMD_TIMEOUT_TABLE_SIZE = 8,
>  
> -	/* Horkage types. May be set by libata or controller on drives
> -	   (some horkage may be drive/controller pair dependent */
> -
> -	ATA_HORKAGE_DIAGNOSTIC	= (1 << 0),	/* Failed boot diag */
> -	ATA_HORKAGE_NODMA	= (1 << 1),	/* DMA problems */
> -	ATA_HORKAGE_NONCQ	= (1 << 2),	/* Don't use NCQ */
> -	ATA_HORKAGE_MAX_SEC_128	= (1 << 3),	/* Limit max sects to 128 */
> -	ATA_HORKAGE_BROKEN_HPA	= (1 << 4),	/* Broken HPA */
> -	ATA_HORKAGE_DISABLE	= (1 << 5),	/* Disable it */
> -	ATA_HORKAGE_HPA_SIZE	= (1 << 6),	/* native size off by one */
> -	ATA_HORKAGE_IVB		= (1 << 8),	/* cbl det validity bit bugs */
> -	ATA_HORKAGE_STUCK_ERR	= (1 << 9),	/* stuck ERR on next PACKET */
> -	ATA_HORKAGE_BRIDGE_OK	= (1 << 10),	/* no bridge limits */
> -	ATA_HORKAGE_ATAPI_MOD16_DMA = (1 << 11), /* use ATAPI DMA for commands
> -						    not multiple of 16 bytes */
> -	ATA_HORKAGE_FIRMWARE_WARN = (1 << 12),	/* firmware update warning */
> -	ATA_HORKAGE_1_5_GBPS	= (1 << 13),	/* force 1.5 Gbps */
> -	ATA_HORKAGE_NOSETXFER	= (1 << 14),	/* skip SETXFER, SATA only */
> -	ATA_HORKAGE_BROKEN_FPDMA_AA	= (1 << 15),	/* skip AA */
> -	ATA_HORKAGE_DUMP_ID	= (1 << 16),	/* dump IDENTIFY data */
> -	ATA_HORKAGE_MAX_SEC_LBA48 = (1 << 17),	/* Set max sects to 65535 */
> -	ATA_HORKAGE_ATAPI_DMADIR = (1 << 18),	/* device requires dmadir */
> -	ATA_HORKAGE_NO_NCQ_TRIM	= (1 << 19),	/* don't use queued TRIM */
> -	ATA_HORKAGE_NOLPM	= (1 << 20),	/* don't use LPM */
> -	ATA_HORKAGE_WD_BROKEN_LPM = (1 << 21),	/* some WDs have broken LPM */
> -	ATA_HORKAGE_ZERO_AFTER_TRIM = (1 << 22),/* guarantees zero after trim */
> -	ATA_HORKAGE_NO_DMA_LOG	= (1 << 23),	/* don't use DMA for log read */
> -	ATA_HORKAGE_NOTRIM	= (1 << 24),	/* don't use TRIM */
> -	ATA_HORKAGE_MAX_SEC_1024 = (1 << 25),	/* Limit max sects to 1024 */
> -	ATA_HORKAGE_MAX_TRIM_128M = (1 << 26),	/* Limit max trim size to 128M */
> -	ATA_HORKAGE_NO_NCQ_ON_ATI = (1 << 27),	/* Disable NCQ on ATI chipset */
> -	ATA_HORKAGE_NO_ID_DEV_LOG = (1 << 28),	/* Identify device log missing */
> -	ATA_HORKAGE_NO_LOG_DIR	= (1 << 29),	/* Do not read log directory */
> -	ATA_HORKAGE_NO_FUA	= (1 << 30),	/* Do not use FUA */
> -
> -	 /* DMA mask for user DMA control: User visible values; DO NOT
> -	    renumber */
> +	/* Horkage flags */
> +	ATA_HORKAGE_DIAGNOSTIC      = (1U << __ATA_HORKAGE_DIAGNOSTIC),
> +	ATA_HORKAGE_NODMA           = (1U << __ATA_HORKAGE_NODMA),
> +	ATA_HORKAGE_NONCQ           = (1U << __ATA_HORKAGE_NONCQ),
> +	ATA_HORKAGE_MAX_SEC_128     = (1U << __ATA_HORKAGE_MAX_SEC_128),
> +	ATA_HORKAGE_BROKEN_HPA      = (1U << __ATA_HORKAGE_BROKEN_HPA),
> +	ATA_HORKAGE_DISABLE         = (1U << __ATA_HORKAGE_DISABLE),
> +	ATA_HORKAGE_HPA_SIZE        = (1U << __ATA_HORKAGE_HPA_SIZE),
> +	ATA_HORKAGE_IVB             = (1U << __ATA_HORKAGE_IVB),
> +	ATA_HORKAGE_STUCK_ERR       = (1U << __ATA_HORKAGE_STUCK_ERR),
> +	ATA_HORKAGE_BRIDGE_OK       = (1U << __ATA_HORKAGE_BRIDGE_OK),
> +	ATA_HORKAGE_ATAPI_MOD16_DMA = (1U << __ATA_HORKAGE_ATAPI_MOD16_DMA),
> +	ATA_HORKAGE_FIRMWARE_WARN   = (1U << __ATA_HORKAGE_FIRMWARE_WARN),
> +	ATA_HORKAGE_1_5_GBPS        = (1U << __ATA_HORKAGE_1_5_GBPS),
> +	ATA_HORKAGE_NOSETXFER       = (1U << __ATA_HORKAGE_NOSETXFER),
> +	ATA_HORKAGE_BROKEN_FPDMA_AA = (1U << __ATA_HORKAGE_BROKEN_FPDMA_AA),
> +	ATA_HORKAGE_DUMP_ID         = (1U << __ATA_HORKAGE_DUMP_ID),
> +	ATA_HORKAGE_MAX_SEC_LBA48   = (1U << __ATA_HORKAGE_MAX_SEC_LBA48),
> +	ATA_HORKAGE_ATAPI_DMADIR    = (1U << __ATA_HORKAGE_ATAPI_DMADIR),
> +	ATA_HORKAGE_NO_NCQ_TRIM     = (1U << __ATA_HORKAGE_NO_NCQ_TRIM),
> +	ATA_HORKAGE_NOLPM           = (1U << __ATA_HORKAGE_NOLPM),
> +	ATA_HORKAGE_WD_BROKEN_LPM   = (1U << __ATA_HORKAGE_WD_BROKEN_LPM),
> +	ATA_HORKAGE_ZERO_AFTER_TRIM = (1U << __ATA_HORKAGE_ZERO_AFTER_TRIM),
> +	ATA_HORKAGE_NO_DMA_LOG      = (1U << __ATA_HORKAGE_NO_DMA_LOG),
> +	ATA_HORKAGE_NOTRIM          = (1U << __ATA_HORKAGE_NOTRIM),
> +	ATA_HORKAGE_MAX_SEC_1024    = (1U << __ATA_HORKAGE_MAX_SEC_1024),
> +	ATA_HORKAGE_MAX_TRIM_128M   = (1U << __ATA_HORKAGE_MAX_TRIM_128M),
> +	ATA_HORKAGE_NO_NCQ_ON_ATI   = (1U << __ATA_HORKAGE_NO_NCQ_ON_ATI),
> +	ATA_HORKAGE_NO_ID_DEV_LOG   = (1U << __ATA_HORKAGE_NO_ID_DEV_LOG),
> +	ATA_HORKAGE_NO_LOG_DIR      = (1U << __ATA_HORKAGE_NO_LOG_DIR),
> +	ATA_HORKAGE_NO_FUA          = (1U << __ATA_HORKAGE_NO_FUA),
> +
> +	/* User visible DMA mask for DMA control. DO NOT renumber. */
>  	ATA_DMA_MASK_ATA	= (1 << 0),	/* DMA on ATA Disk */
>  	ATA_DMA_MASK_ATAPI	= (1 << 1),	/* DMA on ATAPI */
>  	ATA_DMA_MASK_CFA	= (1 << 2),	/* DMA on CF Card */
> -- 
> 2.45.2
> 
>
Thank you,
Igor