Re: [PATCH] ata: sata_fsl: fix sscanf() and sysfs_emit() format strings

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2/8/22 15:46, Damien Le Moal wrote:
> Use the %u format for unsigned int parameters handling with sscanf() and
> sysfs_emit() to avoid compilation warnings. In
> fsl_sata_rx_watermark_store(), the call to sscanf() to parse a single
> argument is replaced with a call to kstrtouint().
> 
> While at it, also replace the printk(KERN_ERR) calls with dev_err()
> calls and fix blank lines in fsl_sata_rx_watermark_store().
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/ata/sata_fsl.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index da0152116d9f..556034a15430 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -322,7 +322,7 @@ static void fsl_sata_set_irq_coalescing(struct ata_host *host,
>  static ssize_t fsl_sata_intr_coalescing_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	return sysfs_emit(buf, "%d	%d\n",
> +	return sysfs_emit(buf, "%u	%u\n",
>  			intr_coalescing_count, intr_coalescing_ticks);
>  }
>  
> @@ -332,10 +332,8 @@ static ssize_t fsl_sata_intr_coalescing_store(struct device *dev,
>  {
>  	unsigned int coalescing_count,	coalescing_ticks;
>  
> -	if (sscanf(buf, "%d%d",
> -				&coalescing_count,
> -				&coalescing_ticks) != 2) {
> -		printk(KERN_ERR "fsl-sata: wrong parameter format.\n");
> +	if (sscanf(buf, "%u%u", &coalescing_count, &coalescing_ticks) != 2) {

PPC folks,

The "%u%u" sscanf() format above seems totally bogus to me. How could 2
unsigned int without a separating characters be parsed as such ? Surely,
a separation character is needed, no ?

I cannot find any documentation on what the intr_coalescing sysfs
attribute format should be... Please have a look.


> +		dev_err(dev, "fsl-sata: wrong parameter format.\n");
>  		return -EINVAL;
>  	}
>  
> @@ -359,7 +357,7 @@ static ssize_t fsl_sata_rx_watermark_show(struct device *dev,
>  	rx_watermark &= 0x1f;
>  	spin_unlock_irqrestore(&host->lock, flags);
>  
> -	return sysfs_emit(buf, "%d\n", rx_watermark);
> +	return sysfs_emit(buf, "%u\n", rx_watermark);
>  }
>  
>  static ssize_t fsl_sata_rx_watermark_store(struct device *dev,
> @@ -373,8 +371,8 @@ static ssize_t fsl_sata_rx_watermark_store(struct device *dev,
>  	void __iomem *csr_base = host_priv->csr_base;
>  	u32 temp;
>  
> -	if (sscanf(buf, "%d", &rx_watermark) != 1) {
> -		printk(KERN_ERR "fsl-sata: wrong parameter format.\n");
> +	if (kstrtouint(buf, 10, &rx_watermark) < 0) {
> +		dev_err(dev, "fsl-sata: wrong parameter format.\n");
>  		return -EINVAL;
>  	}
>  
> @@ -382,8 +380,8 @@ static ssize_t fsl_sata_rx_watermark_store(struct device *dev,
>  	temp = ioread32(csr_base + TRANSCFG);
>  	temp &= 0xffffffe0;
>  	iowrite32(temp | rx_watermark, csr_base + TRANSCFG);
> -
>  	spin_unlock_irqrestore(&host->lock, flags);
> +
>  	return strlen(buf);
>  }
>  


-- 
Damien Le Moal
Western Digital Research