Re: [PATCH v3 6/8] ata: libata: Add support to parse equal sign in libata.force

[Damien Le Moal <dlemoal@kernel.org>]

On 12/1/25 21:35, Niklas Cassel wrote:
> Currently, no libata.force parameter supports an arbitrary value.
> 
> All allowed values, e.g. udma/16, udma/25, udma/33, udma/44, udma/66,
> udma/100, udma/133 have hardcoded entries in the force_tbl table.
> 
> Add code to allow a libata.force param with the format
> libata.force=param=param_value, where param_value can be an arbitrary
> value.
> 
> This code will be used in a follow up commit.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

A couple of nits below.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> ---
>  drivers/ata/libata-core.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 697188b990dd6..7f59eca2a65be 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -6461,6 +6461,13 @@ EXPORT_SYMBOL_GPL(ata_platform_remove_one);
>  	{ "no" #name,	.quirk_on	= (flag) },	\
>  	{ #name,	.quirk_off	= (flag) }
>  
> +/*
> + * If the ata_force_param struct member 'name' ends with '=', then the value
> + * after the equal sign will be parsed as an u64, and will be saved in the
> + * ata_force_param struct member 'value'. This works because each libata.force
> + * entry (struct ata_force_ent) is separated by commas, so each entry represents
> + * a single quirk, and can thus only have a single value.

s/have/has

> + */
>  static const struct ata_force_param force_tbl[] __initconst = {
>  	force_cbl(40c,			ATA_CBL_PATA40),
>  	force_cbl(80c,			ATA_CBL_PATA80),
> @@ -6548,8 +6555,9 @@ static int __init ata_parse_force_one(char **cur,
>  				      const char **reason)
>  {
>  	char *start = *cur, *p = *cur;
> -	char *id, *val, *endp;
> +	char *id, *val, *endp, *equalsign, *char_after_equalsign;
>  	const struct ata_force_param *match_fp = NULL;
> +	u64 val_after_equalsign;
>  	int nr_matches = 0, i;
>  
>  	/* find where this param ends and update *cur */
> @@ -6592,10 +6600,36 @@ static int __init ata_parse_force_one(char **cur,
>  	}
>  
>   parse_val:
> -	/* parse val, allow shortcuts so that both 1.5 and 1.5Gbps work */
> +	equalsign = strchr(val, '=');
> +	if (equalsign) {
> +		char_after_equalsign = equalsign + 1;
> +		if (!strlen(char_after_equalsign) ||
> +		    kstrtoull(char_after_equalsign, 10, &val_after_equalsign)) {
> +			*reason = "invalid value after equal sign";
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* parse val */

	/* Parse the parameter value. */

>  	for (i = 0; i < ARRAY_SIZE(force_tbl); i++) {
>  		const struct ata_force_param *fp = &force_tbl[i];
>  
> +		/*
> +		 * If val contains equal sign, match has to be exact, i.e.
> +		 * shortcuts are not supported.
> +		 */
> +		if (equalsign &&
> +		    (strncasecmp(val, fp->name,
> +				 char_after_equalsign - val) == 0)) {
> +			force_ent->param = *fp;
> +			force_ent->param.value = val_after_equalsign;
> +			return 0;
> +		}
> +
> +		/*
> +		 * If val does not contain equal sign, allow shortcuts so that
> +		 * both 1.5 and 1.5Gbps work.
> +		 */
>  		if (strncasecmp(val, fp->name, strlen(val)))
>  			continue;
>  


-- 
Damien Le Moal
Western Digital Research