[PATCH] axis-fifo: use devm_kasprintf for device name

[PATCH] axis-fifo: use devm_kasprintf for device name

[Prathu Baronia]

- Replaces devm_kzalloc and snprintf combo.
- Also made the fops alignment proper.

Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
 drivers/staging/axis-fifo/axis-fifo.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index dfd2b357f484..8b46699efb34 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -720,11 +720,11 @@ static int axis_fifo_close(struct inode *inod, struct file *f)
 }
 
 static const struct file_operations fops = {
-	.owner = THIS_MODULE,
-	.open = axis_fifo_open,
+	.owner   = THIS_MODULE,
+	.open    = axis_fifo_open,
 	.release = axis_fifo_close,
-	.read = axis_fifo_read,
-	.write = axis_fifo_write
+	.read    = axis_fifo_read,
+	.write   = axis_fifo_write
 };
 
 /* read named property from the device tree */
@@ -820,10 +820,6 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	 * ----------------------------
 	 */
 
-	device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
-	if (!device_name)
-		return -ENOMEM;
-
 	/* allocate device wrapper memory */
 	fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
 	if (!fifo)
@@ -861,7 +857,9 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
 
 	/* create unique device name */
-	snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
+	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%pa", DRIVER_NAME, &r_mem->start);
+	if (!device_name)
+		return -ENOMEM;
 	dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
 
 	/* ----------------------------
-- 
2.34.1

Re: [PATCH] axis-fifo: use devm_kasprintf for device name

[Greg Kroah-Hartman]

On Sun, May 14, 2023 at 03:10:27AM +0530, Prathu Baronia wrote:
> - Replaces devm_kzalloc and snprintf combo.
> - Also made the fops alignment proper.
> 
> Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
> ---
>  drivers/staging/axis-fifo/axis-fifo.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> index dfd2b357f484..8b46699efb34 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -720,11 +720,11 @@ static int axis_fifo_close(struct inode *inod, struct file *f)
>  }
>  
>  static const struct file_operations fops = {
> -	.owner = THIS_MODULE,
> -	.open = axis_fifo_open,
> +	.owner   = THIS_MODULE,
> +	.open    = axis_fifo_open,
>  	.release = axis_fifo_close,
> -	.read = axis_fifo_read,
> -	.write = axis_fifo_write
> +	.read    = axis_fifo_read,
> +	.write   = axis_fifo_write
>  };
>  
>  /* read named property from the device tree */
> @@ -820,10 +820,6 @@ static int axis_fifo_probe(struct platform_device *pdev)
>  	 * ----------------------------
>  	 */
>  
> -	device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
> -	if (!device_name)
> -		return -ENOMEM;
> -
>  	/* allocate device wrapper memory */
>  	fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
>  	if (!fifo)
> @@ -861,7 +857,9 @@ static int axis_fifo_probe(struct platform_device *pdev)
>  	dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
>  
>  	/* create unique device name */
> -	snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
> +	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%pa", DRIVER_NAME, &r_mem->start);
> +	if (!device_name)
> +		return -ENOMEM;
>  	dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
>  
>  	/* ----------------------------
> -- 
> 2.34.1
> 
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch did many different things all at once, making it difficult
  to review.  All Linux kernel patches need to only do one thing at a
  time.  If you need to do multiple things (such as clean up all coding
  style issues in a file/driver), do it in a sequence of patches, each
  one doing only one thing.  This will make it easier to review the
  patches to ensure that they are correct, and to help alleviate any
  merge issues that larger patches can cause.

- You did not specify a description of why the patch is needed, or
  possibly, any description at all, in the email body.  Please read the
  section entitled "The canonical patch format" in the kernel file,
  Documentation/process/submitting-patches.rst for what is needed in
  order to properly describe the change.

- You did not write a descriptive Subject: for the patch, allowing Greg,
  and everyone else, to know what this patch is all about.  Please read
  the section entitled "The canonical patch format" in the kernel file,
  Documentation/process/submitting-patches.rst for what a proper
  Subject: line should look like.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

(no subject)

[Prathu Baronia]

Thanks for the feedback. I have gone through
Documentation/process/submitting_patches.rst and have split v1 into
logical commits and added appropriate commit messages. Please let me
know if there any changes required in this.

Prathu

[PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Prathu Baronia]

In various places, string buffers of a fixed size are allocated, and
filled using snprintf() with the same fixed size, which is error-prone.

Replace this by calling devm_kasprintf() instead, which always uses the
appropriate size.

Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
 drivers/staging/axis-fifo/axis-fifo.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index 7a21f2423204..7b3080202b31 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -816,10 +816,6 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	 * ----------------------------
 	 */
 
-	device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
-	if (!device_name)
-		return -ENOMEM;
-
 	/* allocate device wrapper memory */
 	fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
 	if (!fifo)
@@ -857,7 +853,9 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
 
 	/* create unique device name */
-	snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
+	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%pa", DRIVER_NAME, &r_mem->start);
+	if (!device_name)
+		return -ENOMEM;
 	dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
 
 	/* ----------------------------
-- 
2.34.1

Re: [PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Dan Carpenter]

Hi Prathu,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Prathu-Baronia/axis-fifo-cleanup-space-issues-with-fops-struct/20230514-220201
base:   staging/staging-testing
patch link:    https://lore.kernel.org/r/20230514130148.138624-2-prathubaronia2011%40gmail.com
patch subject: [PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
config: i386-randconfig-m021
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202305150358.nt1BkbXz-lkp@intel.com/

New smatch warnings:
drivers/staging/axis-fifo/axis-fifo.c:858 axis_fifo_probe() warn: missing unwind goto?

Old smatch warnings:
drivers/staging/axis-fifo/axis-fifo.c:907 axis_fifo_probe() error: '%pa' expects argument of type 'phys_addr_t*', argument 4 has type 'void**'

vim +858 drivers/staging/axis-fifo/axis-fifo.c

4a965c5f89decd Jacob Feder        2018-07-22  806  static int axis_fifo_probe(struct platform_device *pdev)
4a965c5f89decd Jacob Feder        2018-07-22  807  {
4a965c5f89decd Jacob Feder        2018-07-22  808  	struct resource *r_mem; /* IO mem resources */
4a965c5f89decd Jacob Feder        2018-07-22  809  	struct device *dev = &pdev->dev; /* OS device (from device tree) */
4a965c5f89decd Jacob Feder        2018-07-22  810  	struct axis_fifo *fifo = NULL;
d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  811  	char *device_name;
4a965c5f89decd Jacob Feder        2018-07-22  812  	int rc = 0; /* error return value */
4a965c5f89decd Jacob Feder        2018-07-22  813  
4a965c5f89decd Jacob Feder        2018-07-22  814  	/* ----------------------------
4a965c5f89decd Jacob Feder        2018-07-22  815  	 *     init wrapper device
4a965c5f89decd Jacob Feder        2018-07-22  816  	 * ----------------------------
4a965c5f89decd Jacob Feder        2018-07-22  817  	 */
4a965c5f89decd Jacob Feder        2018-07-22  818  
4a965c5f89decd Jacob Feder        2018-07-22  819  	/* allocate device wrapper memory */
d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  820  	fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
4a965c5f89decd Jacob Feder        2018-07-22  821  	if (!fifo)
4a965c5f89decd Jacob Feder        2018-07-22  822  		return -ENOMEM;
4a965c5f89decd Jacob Feder        2018-07-22  823  
4a965c5f89decd Jacob Feder        2018-07-22  824  	dev_set_drvdata(dev, fifo);
4a965c5f89decd Jacob Feder        2018-07-22  825  	fifo->dt_device = dev;
4a965c5f89decd Jacob Feder        2018-07-22  826  
4a965c5f89decd Jacob Feder        2018-07-22  827  	init_waitqueue_head(&fifo->read_queue);
4a965c5f89decd Jacob Feder        2018-07-22  828  	init_waitqueue_head(&fifo->write_queue);
4a965c5f89decd Jacob Feder        2018-07-22  829  
0443b3f4436321 Quentin Deslandes  2020-01-21  830  	mutex_init(&fifo->read_lock);
0443b3f4436321 Quentin Deslandes  2020-01-21  831  	mutex_init(&fifo->write_lock);
4a965c5f89decd Jacob Feder        2018-07-22  832  
4a965c5f89decd Jacob Feder        2018-07-22  833  	/* ----------------------------
4a965c5f89decd Jacob Feder        2018-07-22  834  	 *   init device memory space
4a965c5f89decd Jacob Feder        2018-07-22  835  	 * ----------------------------
4a965c5f89decd Jacob Feder        2018-07-22  836  	 */
4a965c5f89decd Jacob Feder        2018-07-22  837  
4a965c5f89decd Jacob Feder        2018-07-22  838  	/* get iospace for the device */
4a965c5f89decd Jacob Feder        2018-07-22  839  	r_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
4a965c5f89decd Jacob Feder        2018-07-22  840  	if (!r_mem) {
4a965c5f89decd Jacob Feder        2018-07-22  841  		dev_err(fifo->dt_device, "invalid address\n");
4a965c5f89decd Jacob Feder        2018-07-22  842  		rc = -ENODEV;
4a965c5f89decd Jacob Feder        2018-07-22  843  		goto err_initial;
4a965c5f89decd Jacob Feder        2018-07-22  844  	}
4a965c5f89decd Jacob Feder        2018-07-22  845  
4a965c5f89decd Jacob Feder        2018-07-22  846  	/* request physical memory */
354e27a86b4c64 Quentin Deslandes  2019-11-01  847  	fifo->base_addr = devm_ioremap_resource(fifo->dt_device, r_mem);
6a20d283ed6867 Quentin Deslandes  2019-11-01  848  	if (IS_ERR(fifo->base_addr)) {
6a20d283ed6867 Quentin Deslandes  2019-11-01  849  		rc = PTR_ERR(fifo->base_addr);
4a965c5f89decd Jacob Feder        2018-07-22  850  		goto err_initial;
4a965c5f89decd Jacob Feder        2018-07-22  851  	}
4a965c5f89decd Jacob Feder        2018-07-22  852  
4a965c5f89decd Jacob Feder        2018-07-22  853  	dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
4a965c5f89decd Jacob Feder        2018-07-22  854  
4a965c5f89decd Jacob Feder        2018-07-22  855  	/* create unique device name */
c5cab4f62648eb Prathu Baronia     2023-05-14  856  	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%pa", DRIVER_NAME, &r_mem->start);
c5cab4f62648eb Prathu Baronia     2023-05-14  857  	if (!device_name)
c5cab4f62648eb Prathu Baronia     2023-05-14 @858  		return -ENOMEM;

		rc = -ENOMEM;
		goto err_initial;

4a965c5f89decd Jacob Feder        2018-07-22  859  	dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
4a965c5f89decd Jacob Feder        2018-07-22  860  
4a965c5f89decd Jacob Feder        2018-07-22  861  	/* ----------------------------
4a965c5f89decd Jacob Feder        2018-07-22  862  	 *          init IP
4a965c5f89decd Jacob Feder        2018-07-22  863  	 * ----------------------------
4a965c5f89decd Jacob Feder        2018-07-22  864  	 */
4a965c5f89decd Jacob Feder        2018-07-22  865  
ed6daf2b2832d9 Quentin Deslandes  2019-11-01  866  	rc = axis_fifo_parse_dt(fifo);
4a965c5f89decd Jacob Feder        2018-07-22  867  	if (rc)
6a20d283ed6867 Quentin Deslandes  2019-11-01  868  		goto err_initial;
4a965c5f89decd Jacob Feder        2018-07-22  869  
4a965c5f89decd Jacob Feder        2018-07-22  870  	reset_ip_core(fifo);
4a965c5f89decd Jacob Feder        2018-07-22  871  
4a965c5f89decd Jacob Feder        2018-07-22  872  	/* ----------------------------
4a965c5f89decd Jacob Feder        2018-07-22  873  	 *    init device interrupts
4a965c5f89decd Jacob Feder        2018-07-22  874  	 * ----------------------------
4a965c5f89decd Jacob Feder        2018-07-22  875  	 */
4a965c5f89decd Jacob Feder        2018-07-22  876  
4a965c5f89decd Jacob Feder        2018-07-22  877  	/* get IRQ resource */
790ada0e6ec33e Lad Prabhakar      2021-12-24  878  	rc = platform_get_irq(pdev, 0);
790ada0e6ec33e Lad Prabhakar      2021-12-24  879  	if (rc < 0)
6a20d283ed6867 Quentin Deslandes  2019-11-01  880  		goto err_initial;
4a965c5f89decd Jacob Feder        2018-07-22  881  
4a965c5f89decd Jacob Feder        2018-07-22  882  	/* request IRQ */
790ada0e6ec33e Lad Prabhakar      2021-12-24  883  	fifo->irq = rc;
6a20d283ed6867 Quentin Deslandes  2019-11-01  884  	rc = devm_request_irq(fifo->dt_device, fifo->irq, &axis_fifo_irq, 0,
6a20d283ed6867 Quentin Deslandes  2019-11-01  885  			      DRIVER_NAME, fifo);
4a965c5f89decd Jacob Feder        2018-07-22  886  	if (rc) {
4a965c5f89decd Jacob Feder        2018-07-22  887  		dev_err(fifo->dt_device, "couldn't allocate interrupt %i\n",
4a965c5f89decd Jacob Feder        2018-07-22  888  			fifo->irq);
6a20d283ed6867 Quentin Deslandes  2019-11-01  889  		goto err_initial;
4a965c5f89decd Jacob Feder        2018-07-22  890  	}
4a965c5f89decd Jacob Feder        2018-07-22  891  
4a965c5f89decd Jacob Feder        2018-07-22  892  	/* ----------------------------
4a965c5f89decd Jacob Feder        2018-07-22  893  	 *      init char device
4a965c5f89decd Jacob Feder        2018-07-22  894  	 * ----------------------------
4a965c5f89decd Jacob Feder        2018-07-22  895  	 */
4a965c5f89decd Jacob Feder        2018-07-22  896  
d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  897  	/* create character device */
d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  898  	fifo->miscdev.fops = &fops;
d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  899  	fifo->miscdev.minor = MISC_DYNAMIC_MINOR;
d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  900  	fifo->miscdev.name = device_name;
d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  901  	fifo->miscdev.groups = axis_fifo_attrs_groups;
d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  902  	fifo->miscdev.parent = dev;
d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  903  	rc = misc_register(&fifo->miscdev);
4a965c5f89decd Jacob Feder        2018-07-22  904  	if (rc < 0)
6a20d283ed6867 Quentin Deslandes  2019-11-01  905  		goto err_initial;
4a965c5f89decd Jacob Feder        2018-07-22  906  
d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  907  	dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%pa, irq=%i\n",
                                                                                                                      ^^^^^
d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  908  		 &r_mem->start, &fifo->base_addr, fifo->irq);
                                                                                ^^^^^^^^^^^^^^^^
I think Smatch is correct that it should just be %p instead of %pa but
I don't know the rules here too well.

4a965c5f89decd Jacob Feder        2018-07-22  909  
4a965c5f89decd Jacob Feder        2018-07-22  910  	return 0;
4a965c5f89decd Jacob Feder        2018-07-22  911  
4a965c5f89decd Jacob Feder        2018-07-22  912  err_initial:
4a965c5f89decd Jacob Feder        2018-07-22  913  	dev_set_drvdata(dev, NULL);
4a965c5f89decd Jacob Feder        2018-07-22  914  	return rc;
4a965c5f89decd Jacob Feder        2018-07-22  915  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Prathu Baronia]

Hi Dan,

Thanks for pointing these out. The older warning seems to be
introduced in an earlier commit. I will fix both of these in v3.
---
- Prathu


On Mon, May 15, 2023 at 1:13 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Hi Prathu,
>
> kernel test robot noticed the following build warnings:
>
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Prathu-Baronia/axis-fifo-cleanup-space-issues-with-fops-struct/20230514-220201
> base:   staging/staging-testing
> patch link:    https://lore.kernel.org/r/20230514130148.138624-2-prathubaronia2011%40gmail.com
> patch subject: [PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
> config: i386-randconfig-m021
> compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> | Link: https://lore.kernel.org/r/202305150358.nt1BkbXz-lkp@intel.com/
>
> New smatch warnings:
> drivers/staging/axis-fifo/axis-fifo.c:858 axis_fifo_probe() warn: missing unwind goto?
>
> Old smatch warnings:
> drivers/staging/axis-fifo/axis-fifo.c:907 axis_fifo_probe() error: '%pa' expects argument of type 'phys_addr_t*', argument 4 has type 'void**'
>
> vim +858 drivers/staging/axis-fifo/axis-fifo.c
>
> 4a965c5f89decd Jacob Feder        2018-07-22  806  static int axis_fifo_probe(struct platform_device *pdev)
> 4a965c5f89decd Jacob Feder        2018-07-22  807  {
> 4a965c5f89decd Jacob Feder        2018-07-22  808       struct resource *r_mem; /* IO mem resources */
> 4a965c5f89decd Jacob Feder        2018-07-22  809       struct device *dev = &pdev->dev; /* OS device (from device tree) */
> 4a965c5f89decd Jacob Feder        2018-07-22  810       struct axis_fifo *fifo = NULL;
> d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  811       char *device_name;
> 4a965c5f89decd Jacob Feder        2018-07-22  812       int rc = 0; /* error return value */
> 4a965c5f89decd Jacob Feder        2018-07-22  813
> 4a965c5f89decd Jacob Feder        2018-07-22  814       /* ----------------------------
> 4a965c5f89decd Jacob Feder        2018-07-22  815        *     init wrapper device
> 4a965c5f89decd Jacob Feder        2018-07-22  816        * ----------------------------
> 4a965c5f89decd Jacob Feder        2018-07-22  817        */
> 4a965c5f89decd Jacob Feder        2018-07-22  818
> 4a965c5f89decd Jacob Feder        2018-07-22  819       /* allocate device wrapper memory */
> d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  820       fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
> 4a965c5f89decd Jacob Feder        2018-07-22  821       if (!fifo)
> 4a965c5f89decd Jacob Feder        2018-07-22  822               return -ENOMEM;
> 4a965c5f89decd Jacob Feder        2018-07-22  823
> 4a965c5f89decd Jacob Feder        2018-07-22  824       dev_set_drvdata(dev, fifo);
> 4a965c5f89decd Jacob Feder        2018-07-22  825       fifo->dt_device = dev;
> 4a965c5f89decd Jacob Feder        2018-07-22  826
> 4a965c5f89decd Jacob Feder        2018-07-22  827       init_waitqueue_head(&fifo->read_queue);
> 4a965c5f89decd Jacob Feder        2018-07-22  828       init_waitqueue_head(&fifo->write_queue);
> 4a965c5f89decd Jacob Feder        2018-07-22  829
> 0443b3f4436321 Quentin Deslandes  2020-01-21  830       mutex_init(&fifo->read_lock);
> 0443b3f4436321 Quentin Deslandes  2020-01-21  831       mutex_init(&fifo->write_lock);
> 4a965c5f89decd Jacob Feder        2018-07-22  832
> 4a965c5f89decd Jacob Feder        2018-07-22  833       /* ----------------------------
> 4a965c5f89decd Jacob Feder        2018-07-22  834        *   init device memory space
> 4a965c5f89decd Jacob Feder        2018-07-22  835        * ----------------------------
> 4a965c5f89decd Jacob Feder        2018-07-22  836        */
> 4a965c5f89decd Jacob Feder        2018-07-22  837
> 4a965c5f89decd Jacob Feder        2018-07-22  838       /* get iospace for the device */
> 4a965c5f89decd Jacob Feder        2018-07-22  839       r_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 4a965c5f89decd Jacob Feder        2018-07-22  840       if (!r_mem) {
> 4a965c5f89decd Jacob Feder        2018-07-22  841               dev_err(fifo->dt_device, "invalid address\n");
> 4a965c5f89decd Jacob Feder        2018-07-22  842               rc = -ENODEV;
> 4a965c5f89decd Jacob Feder        2018-07-22  843               goto err_initial;
> 4a965c5f89decd Jacob Feder        2018-07-22  844       }
> 4a965c5f89decd Jacob Feder        2018-07-22  845
> 4a965c5f89decd Jacob Feder        2018-07-22  846       /* request physical memory */
> 354e27a86b4c64 Quentin Deslandes  2019-11-01  847       fifo->base_addr = devm_ioremap_resource(fifo->dt_device, r_mem);
> 6a20d283ed6867 Quentin Deslandes  2019-11-01  848       if (IS_ERR(fifo->base_addr)) {
> 6a20d283ed6867 Quentin Deslandes  2019-11-01  849               rc = PTR_ERR(fifo->base_addr);
> 4a965c5f89decd Jacob Feder        2018-07-22  850               goto err_initial;
> 4a965c5f89decd Jacob Feder        2018-07-22  851       }
> 4a965c5f89decd Jacob Feder        2018-07-22  852
> 4a965c5f89decd Jacob Feder        2018-07-22  853       dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
> 4a965c5f89decd Jacob Feder        2018-07-22  854
> 4a965c5f89decd Jacob Feder        2018-07-22  855       /* create unique device name */
> c5cab4f62648eb Prathu Baronia     2023-05-14  856       device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%pa", DRIVER_NAME, &r_mem->start);
> c5cab4f62648eb Prathu Baronia     2023-05-14  857       if (!device_name)
> c5cab4f62648eb Prathu Baronia     2023-05-14 @858               return -ENOMEM;
>
>                 rc = -ENOMEM;
>                 goto err_initial;
>
> 4a965c5f89decd Jacob Feder        2018-07-22  859       dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
> 4a965c5f89decd Jacob Feder        2018-07-22  860
> 4a965c5f89decd Jacob Feder        2018-07-22  861       /* ----------------------------
> 4a965c5f89decd Jacob Feder        2018-07-22  862        *          init IP
> 4a965c5f89decd Jacob Feder        2018-07-22  863        * ----------------------------
> 4a965c5f89decd Jacob Feder        2018-07-22  864        */
> 4a965c5f89decd Jacob Feder        2018-07-22  865
> ed6daf2b2832d9 Quentin Deslandes  2019-11-01  866       rc = axis_fifo_parse_dt(fifo);
> 4a965c5f89decd Jacob Feder        2018-07-22  867       if (rc)
> 6a20d283ed6867 Quentin Deslandes  2019-11-01  868               goto err_initial;
> 4a965c5f89decd Jacob Feder        2018-07-22  869
> 4a965c5f89decd Jacob Feder        2018-07-22  870       reset_ip_core(fifo);
> 4a965c5f89decd Jacob Feder        2018-07-22  871
> 4a965c5f89decd Jacob Feder        2018-07-22  872       /* ----------------------------
> 4a965c5f89decd Jacob Feder        2018-07-22  873        *    init device interrupts
> 4a965c5f89decd Jacob Feder        2018-07-22  874        * ----------------------------
> 4a965c5f89decd Jacob Feder        2018-07-22  875        */
> 4a965c5f89decd Jacob Feder        2018-07-22  876
> 4a965c5f89decd Jacob Feder        2018-07-22  877       /* get IRQ resource */
> 790ada0e6ec33e Lad Prabhakar      2021-12-24  878       rc = platform_get_irq(pdev, 0);
> 790ada0e6ec33e Lad Prabhakar      2021-12-24  879       if (rc < 0)
> 6a20d283ed6867 Quentin Deslandes  2019-11-01  880               goto err_initial;
> 4a965c5f89decd Jacob Feder        2018-07-22  881
> 4a965c5f89decd Jacob Feder        2018-07-22  882       /* request IRQ */
> 790ada0e6ec33e Lad Prabhakar      2021-12-24  883       fifo->irq = rc;
> 6a20d283ed6867 Quentin Deslandes  2019-11-01  884       rc = devm_request_irq(fifo->dt_device, fifo->irq, &axis_fifo_irq, 0,
> 6a20d283ed6867 Quentin Deslandes  2019-11-01  885                             DRIVER_NAME, fifo);
> 4a965c5f89decd Jacob Feder        2018-07-22  886       if (rc) {
> 4a965c5f89decd Jacob Feder        2018-07-22  887               dev_err(fifo->dt_device, "couldn't allocate interrupt %i\n",
> 4a965c5f89decd Jacob Feder        2018-07-22  888                       fifo->irq);
> 6a20d283ed6867 Quentin Deslandes  2019-11-01  889               goto err_initial;
> 4a965c5f89decd Jacob Feder        2018-07-22  890       }
> 4a965c5f89decd Jacob Feder        2018-07-22  891
> 4a965c5f89decd Jacob Feder        2018-07-22  892       /* ----------------------------
> 4a965c5f89decd Jacob Feder        2018-07-22  893        *      init char device
> 4a965c5f89decd Jacob Feder        2018-07-22  894        * ----------------------------
> 4a965c5f89decd Jacob Feder        2018-07-22  895        */
> 4a965c5f89decd Jacob Feder        2018-07-22  896
> d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  897       /* create character device */
> d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  898       fifo->miscdev.fops = &fops;
> d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  899       fifo->miscdev.minor = MISC_DYNAMIC_MINOR;
> d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  900       fifo->miscdev.name = device_name;
> d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  901       fifo->miscdev.groups = axis_fifo_attrs_groups;
> d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  902       fifo->miscdev.parent = dev;
> d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  903       rc = misc_register(&fifo->miscdev);
> 4a965c5f89decd Jacob Feder        2018-07-22  904       if (rc < 0)
> 6a20d283ed6867 Quentin Deslandes  2019-11-01  905               goto err_initial;
> 4a965c5f89decd Jacob Feder        2018-07-22  906
> d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  907       dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%pa, irq=%i\n",
>                                                                                                                       ^^^^^
> d2d7aa53891ea1 Greg Kroah-Hartman 2021-09-07  908                &r_mem->start, &fifo->base_addr, fifo->irq);
>                                                                                 ^^^^^^^^^^^^^^^^
> I think Smatch is correct that it should just be %p instead of %pa but
> I don't know the rules here too well.
>
> 4a965c5f89decd Jacob Feder        2018-07-22  909
> 4a965c5f89decd Jacob Feder        2018-07-22  910       return 0;
> 4a965c5f89decd Jacob Feder        2018-07-22  911
> 4a965c5f89decd Jacob Feder        2018-07-22  912  err_initial:
> 4a965c5f89decd Jacob Feder        2018-07-22  913       dev_set_drvdata(dev, NULL);
> 4a965c5f89decd Jacob Feder        2018-07-22  914       return rc;
> 4a965c5f89decd Jacob Feder        2018-07-22  915  }
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests
>

[PATCH v3 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Prathu Baronia]

In various places, string buffers of a fixed size are allocated, and
filled using snprintf() with the same fixed size, which is error-prone.

Replace this by calling devm_kasprintf() instead, which always uses the
appropriate size.

Also fix an old smatch warning reported by lkp introduced by commit
d2d7aa53891e. In the mentioned commit we had used "%pa" format specifier
for a void* type and hence smatch complained about its use instead of
"%p".

Fixes: d2d7aa53891e ("staging: axis-fifo: convert to use miscdevice")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Link: https://lore.kernel.org/r/202305150358.nt1BkbXz-lkp@intel.com/
Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
V2 -> V3: Fix smatch warnings from kernel test robot
V1 -> V2: Split into logical commits and fix commit message

 drivers/staging/axis-fifo/axis-fifo.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index 7a21f2423204..d71bdc6dd961 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -816,10 +816,6 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	 * ----------------------------
 	 */
 
-	device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
-	if (!device_name)
-		return -ENOMEM;
-
 	/* allocate device wrapper memory */
 	fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
 	if (!fifo)
@@ -857,7 +853,11 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
 
 	/* create unique device name */
-	snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
+	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%pa", DRIVER_NAME, &r_mem->start);
+	if (!device_name) {
+		rc = -ENOMEM;
+		goto err_initial;
+	}
 	dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
 
 	/* ----------------------------
@@ -906,8 +906,8 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	if (rc < 0)
 		goto err_initial;
 
-	dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%pa, irq=%i\n",
-		 &r_mem->start, &fifo->base_addr, fifo->irq);
+	dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%p, irq=%i\n",
+		 &r_mem->start, fifo->base_addr, fifo->irq);
 
 	return 0;
 

base-commit: 4d6d4c7f541d7027beed4fb86eb2c451bd8d6fff
-- 
2.34.1

[PATCH v3 2/2] axis-fifo: cleanup space issues with fops struct

[Prathu Baronia]

Add required spaces for proper formatting of fops members for better
readability.

Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
 drivers/staging/axis-fifo/axis-fifo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index d71bdc6dd961..59e962467a3d 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -716,11 +716,11 @@ static int axis_fifo_close(struct inode *inod, struct file *f)
 }
 
 static const struct file_operations fops = {
-	.owner = THIS_MODULE,
-	.open = axis_fifo_open,
+	.owner   = THIS_MODULE,
+	.open    = axis_fifo_open,
 	.release = axis_fifo_close,
-	.read = axis_fifo_read,
-	.write = axis_fifo_write
+	.read    = axis_fifo_read,
+	.write   = axis_fifo_write
 };
 
 /* read named property from the device tree */
-- 
2.34.1

Re: [PATCH v3 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Greg KH]

On Thu, May 18, 2023 at 08:21:53PM +0530, Prathu Baronia wrote:
> In various places, string buffers of a fixed size are allocated, and
> filled using snprintf() with the same fixed size, which is error-prone.

Maybe error-prone, but all is fine with the original code, right?

> Replace this by calling devm_kasprintf() instead, which always uses the
> appropriate size.
> 
> Also fix an old smatch warning reported by lkp introduced by commit
> d2d7aa53891e. In the mentioned commit we had used "%pa" format specifier
> for a void* type and hence smatch complained about its use instead of
> "%p".

When you have "also" in a changelog commit, that usually means this
needs to be split out into a separate patch.  And that's the case here,
make the first patch of the series fix the problem.  Then do your
cleanups on later patches.

> Fixes: d2d7aa53891e ("staging: axis-fifo: convert to use miscdevice")

changing to a different string function does not fix anything.

> Reported-by: kernel test robot <lkp@intel.com>

It did not report that you need to replace a string function, right?

See, things got messy when you mixed in changes into one.  Please break
these up.

thanks,

greg k-h

Re: [PATCH v3 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Prathu Baronia]

On Sat, May 27, 2023 at 08:35:57AM +0100, Greg KH wrote:
> 
> Maybe error-prone, but all is fine with the original code, right?
> 
Yes, all is fine with the original code. Replaced it only because it was error-prone.

> When you have "also" in a changelog commit, that usually means this
> needs to be split out into a separate patch.  And that's the case here,
> make the first patch of the series fix the problem.  Then do your
> cleanups on later patches.
> 
Point taken, will split it in v4.

> > Fixes: d2d7aa53891e ("staging: axis-fifo: convert to use miscdevice")
> 
> changing to a different string function does not fix anything.
Right, this should only be part of the smatch warning fixing commit.

> 
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> It did not report that you need to replace a string function, right?
> 
> See, things got messy when you mixed in changes into one.  Please break
> these up.
Understood, will do.

Prathu

[PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier

[Prathu Baronia]

Fix an old smatch warning reported by lkp introduced by commit
d2d7aa53891e. In the mentioned commit we had used "%pa" format specifier
for a void* type and hence smatch complained about its use instead of
"%p".

Fixes: d2d7aa53891e ("staging: axis-fifo: convert to use miscdevice")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Link: https://lore.kernel.org/r/202305150358.nt1BkbXz-lkp@intel.com/
Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
V3 -> V4: Split into warning fixing and cleanup commits
V2 -> V3: Fix smatch warnings from kernel test robot
V1 -> V2: Split into logical commits and fix commit message

 drivers/staging/axis-fifo/axis-fifo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index 7a21f2423204..271cab805cad 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -906,8 +906,8 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	if (rc < 0)
 		goto err_initial;
 
-	dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%pa, irq=%i\n",
-		 &r_mem->start, &fifo->base_addr, fifo->irq);
+	dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%p, irq=%i\n",
+		 &r_mem->start, fifo->base_addr, fifo->irq);
 
 	return 0;
 

base-commit: 44c026a73be8038f03dbdeef028b642880cf1511
-- 
2.34.1

[PATCH v4 2/3] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Prathu Baronia]

In various places, string buffers of a fixed size are allocated, and
filled using snprintf() with the same fixed size, which is error-prone.

Replace this by calling devm_kasprintf() instead, which always uses the
appropriate size.

Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
 drivers/staging/axis-fifo/axis-fifo.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index 271cab805cad..d71bdc6dd961 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -816,10 +816,6 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	 * ----------------------------
 	 */
 
-	device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
-	if (!device_name)
-		return -ENOMEM;
-
 	/* allocate device wrapper memory */
 	fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
 	if (!fifo)
@@ -857,7 +853,11 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
 
 	/* create unique device name */
-	snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
+	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%pa", DRIVER_NAME, &r_mem->start);
+	if (!device_name) {
+		rc = -ENOMEM;
+		goto err_initial;
+	}
 	dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
 
 	/* ----------------------------
-- 
2.34.1

Re: [PATCH v4 2/3] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Greg Kroah-Hartman]

On Sat, May 27, 2023 at 05:20:59PM +0530, Prathu Baronia wrote:
> In various places, string buffers of a fixed size are allocated, and
> filled using snprintf() with the same fixed size, which is error-prone.
> 
> Replace this by calling devm_kasprintf() instead, which always uses the
> appropriate size.
> 
> Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
> ---
>  drivers/staging/axis-fifo/axis-fifo.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> index 271cab805cad..d71bdc6dd961 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -816,10 +816,6 @@ static int axis_fifo_probe(struct platform_device *pdev)
>  	 * ----------------------------
>  	 */
>  
> -	device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
> -	if (!device_name)
> -		return -ENOMEM;
> -
>  	/* allocate device wrapper memory */
>  	fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
>  	if (!fifo)
> @@ -857,7 +853,11 @@ static int axis_fifo_probe(struct platform_device *pdev)
>  	dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
>  
>  	/* create unique device name */
> -	snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
> +	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%pa", DRIVER_NAME, &r_mem->start);

we should not be using a kernel address as a device name, please fix
this up to just use a unique number instead.

thanks,

greg k-h

Re: [PATCH v4 2/3] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Prathu Baronia]

On Sun, May 28, 2023 at 10:00:14AM +0100, Greg Kroah-Hartman wrote:
> we should not be using a kernel address as a device name, please fix
> this up to just use a unique number instead.
Agreed, will be fixed in v5.

Prathu

[PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct

[Prathu Baronia]

Add required spaces for proper formatting of fops members for better
readability.

Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
 drivers/staging/axis-fifo/axis-fifo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index d71bdc6dd961..59e962467a3d 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -716,11 +716,11 @@ static int axis_fifo_close(struct inode *inod, struct file *f)
 }
 
 static const struct file_operations fops = {
-	.owner = THIS_MODULE,
-	.open = axis_fifo_open,
+	.owner   = THIS_MODULE,
+	.open    = axis_fifo_open,
 	.release = axis_fifo_close,
-	.read = axis_fifo_read,
-	.write = axis_fifo_write
+	.read    = axis_fifo_read,
+	.write   = axis_fifo_write
 };
 
 /* read named property from the device tree */
-- 
2.34.1

Re: [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]

Hello,

On Sat, May 27, 2023 at 05:21:00PM +0530, Prathu Baronia wrote:
> Add required spaces for proper formatting of fops members for better
> readability.
> 
> Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
> ---
>  drivers/staging/axis-fifo/axis-fifo.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> index d71bdc6dd961..59e962467a3d 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -716,11 +716,11 @@ static int axis_fifo_close(struct inode *inod, struct file *f)
>  }
>  
>  static const struct file_operations fops = {
> -	.owner = THIS_MODULE,
> -	.open = axis_fifo_open,
> +	.owner   = THIS_MODULE,
> +	.open    = axis_fifo_open,
>  	.release = axis_fifo_close,
> -	.read = axis_fifo_read,
> -	.write = axis_fifo_write
> +	.read    = axis_fifo_read,
> +	.write   = axis_fifo_write

Note this is only subjectively better. IMHO with just a single space
this is perfectly readable. Aligning the = might look nice, but it's
also annoying at times. When you add another member (e.g.
.iterate_shared) you either add a line that doesn't match all others, or
you have to touch all other lines of that struct which (objectively?)
hurts readability of that patch. Also for generated patches this kind of
alignment yields extra work. (See for example
https://lore.kernel.org/lkml/20230525205840.734432-1-u.kleine-koenig@pengutronix.de/
which required semi-manual fixup to keep the alignment after coccinelle
generated the patch.)

If you still think this is a good idea, I'd ask you to stick to one
style for the whole file. e.g. axis_fifo_driver uses inconsistent
and different indention.

A thing that IMHO is more useful to change here, is the name fops; I'd
suggest something like axis_fifo_fops (and also use prefixes for some
other symbols like "get_dts_property"). In 6.4-rc1 my ctags file knows
about 64 different places that define something called "fops".

Just my 0.02€,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct

[Greg Kroah-Hartman]

On Sun, May 28, 2023 at 12:31:11AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Sat, May 27, 2023 at 05:21:00PM +0530, Prathu Baronia wrote:
> > Add required spaces for proper formatting of fops members for better
> > readability.
> > 
> > Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
> > ---
> >  drivers/staging/axis-fifo/axis-fifo.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> > index d71bdc6dd961..59e962467a3d 100644
> > --- a/drivers/staging/axis-fifo/axis-fifo.c
> > +++ b/drivers/staging/axis-fifo/axis-fifo.c
> > @@ -716,11 +716,11 @@ static int axis_fifo_close(struct inode *inod, struct file *f)
> >  }
> >  
> >  static const struct file_operations fops = {
> > -	.owner = THIS_MODULE,
> > -	.open = axis_fifo_open,
> > +	.owner   = THIS_MODULE,
> > +	.open    = axis_fifo_open,
> >  	.release = axis_fifo_close,
> > -	.read = axis_fifo_read,
> > -	.write = axis_fifo_write
> > +	.read    = axis_fifo_read,
> > +	.write   = axis_fifo_write
> 
> Note this is only subjectively better. IMHO with just a single space
> this is perfectly readable. Aligning the = might look nice, but it's
> also annoying at times. When you add another member (e.g.
> .iterate_shared) you either add a line that doesn't match all others, or
> you have to touch all other lines of that struct which (objectively?)
> hurts readability of that patch. Also for generated patches this kind of
> alignment yields extra work. (See for example
> https://lore.kernel.org/lkml/20230525205840.734432-1-u.kleine-koenig@pengutronix.de/
> which required semi-manual fixup to keep the alignment after coccinelle
> generated the patch.)
> 
> If you still think this is a good idea, I'd ask you to stick to one
> style for the whole file. e.g. axis_fifo_driver uses inconsistent
> and different indention.

I agree, there is no "requirement" that these fields are aligned at all,
so I would stick to the real fixes that are needed for this code to be
able to be moved out of staging instead.

thanks,

greg k-h

Re: [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct

[Prathu Baronia]

On Sun, May 28, 2023 at 09:57:52AM +0100, Greg Kroah-Hartman wrote:
> I agree, there is no "requirement" that these fields are aligned at all,
> so I would stick to the real fixes that are needed for this code to be
> able to be moved out of staging instead.
Sure greg, leaving out the cleanup fixes for later.

Prathu

Re: [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct

[Prathu Baronia]

On Sun, May 28, 2023 at 12:31:11AM +0200, Uwe Kleine-König wrote:
> Note this is only subjectively better. IMHO with just a single space
> this is perfectly readable. Aligning the = might look nice, but it's
> also annoying at times. When you add another member (e.g.
> .iterate_shared) you either add a line that doesn't match all others, or
> you have to touch all other lines of that struct which (objectively?)
> hurts readability of that patch. Also for generated patches this kind of
> alignment yields extra work. (See for example
> https://lore.kernel.org/lkml/20230525205840.734432-1-u.kleine-koenig@pengutronix.de/
> which required semi-manual fixup to keep the alignment after coccinelle
> generated the patch.)
> 
Agreed, that this would create more troubles than benefits.

> If you still think this is a good idea, I'd ask you to stick to one
> style for the whole file. e.g. axis_fifo_driver uses inconsistent
> and different indention.
> 
> A thing that IMHO is more useful to change here, is the name fops; I'd
> suggest something like axis_fifo_fops (and also use prefixes for some
> other symbols like "get_dts_property"). In 6.4-rc1 my ctags file knows
> about 64 different places that define something called "fops".
> 
Agreed. Upon further walkthrough I think this driver requires a lot of cleanup
so I will leave that cleanup for another dedicated patch series for now.

Prathu

Re: [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier

[Greg Kroah-Hartman]

On Sat, May 27, 2023 at 05:20:58PM +0530, Prathu Baronia wrote:
> Fix an old smatch warning reported by lkp introduced by commit
> d2d7aa53891e. In the mentioned commit we had used "%pa" format specifier
> for a void* type and hence smatch complained about its use instead of
> "%p".
> 
> Fixes: d2d7aa53891e ("staging: axis-fifo: convert to use miscdevice")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Link: https://lore.kernel.org/r/202305150358.nt1BkbXz-lkp@intel.com/
> Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
> ---
> V3 -> V4: Split into warning fixing and cleanup commits
> V2 -> V3: Fix smatch warnings from kernel test robot
> V1 -> V2: Split into logical commits and fix commit message
> 
>  drivers/staging/axis-fifo/axis-fifo.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> index 7a21f2423204..271cab805cad 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -906,8 +906,8 @@ static int axis_fifo_probe(struct platform_device *pdev)
>  	if (rc < 0)
>  		goto err_initial;
>  
> -	dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%pa, irq=%i\n",
> -		 &r_mem->start, &fifo->base_addr, fifo->irq);
> +	dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%p, irq=%i\n",
> +		 &r_mem->start, fifo->base_addr, fifo->irq);

In looking at this further, this whole dev_info() line should just be
removed.  When drivers work properly, they are quiet, otherwise the
kernel log would look even worse than it currently is.

So please just remove this entirely.  Also, attempting to print memory
addresses to the kernel log is very suspect and generally frowned apon,
which is another reason this shouldn't be done.

thanks,

greg k-h

Re: [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier

[Prathu Baronia]

On Sun, May 28, 2023 at 09:59:27AM +0100, Greg Kroah-Hartman wrote:
> So please just remove this entirely.  Also, attempting to print memory
> addresses to the kernel log is very suspect and generally frowned apon,
> which is another reason this shouldn't be done.
Agreed, will be removed in v5.

Prathu

[PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Prathu Baronia]

In various places, string buffers of a fixed size are allocated, and
filled using snprintf() with the same fixed size, which is error-prone.

Replace this by calling devm_kasprintf() instead, which always uses the
appropriate size. Allocate the device name with a unique identifier
instead of a kernel address.

Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
V4 -> V5: Remove the dev_info() and use a unique identifier for dev name
V3 -> V4: Split into warning fixing and cleanup commits
V2 -> V3: Fix smatch warnings from kernel test robot
V1 -> V2: Split into logical commits and fix commit message

 drivers/staging/axis-fifo/axis-fifo.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index 7a21f2423204..5e070e00e27a 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -816,10 +816,6 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	 * ----------------------------
 	 */
 
-	device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
-	if (!device_name)
-		return -ENOMEM;
-
 	/* allocate device wrapper memory */
 	fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
 	if (!fifo)
@@ -857,7 +853,11 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
 
 	/* create unique device name */
-	snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
+	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%d", DRIVER_NAME, pdev->id);
+	if (!device_name) {
+		rc = -ENOMEM;
+		goto err_initial;
+	}
 	dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
 
 	/* ----------------------------

base-commit: fb054096aea0576f0c0a61c598e5e9676443ee86
-- 
2.34.1

[PATCH v5 2/2] axis-fifo: remove the unnecessary dev_info()

[Prathu Baronia]

This dev_info() statement is not needed since drivers need to be quiet
under normal operation and its not a good idea to print addresses in
kernel log.

Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
 drivers/staging/axis-fifo/axis-fifo.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index 5e070e00e27a..d1ce8b9e32eb 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -906,9 +906,6 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	if (rc < 0)
 		goto err_initial;
 
-	dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%pa, irq=%i\n",
-		 &r_mem->start, &fifo->base_addr, fifo->irq);
-
 	return 0;
 
 err_initial:
-- 
2.34.1

Re: [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Greg Kroah-Hartman]

On Fri, Jun 16, 2023 at 08:55:59PM +0530, Prathu Baronia wrote:
> In various places, string buffers of a fixed size are allocated, and
> filled using snprintf() with the same fixed size, which is error-prone.
> 
> Replace this by calling devm_kasprintf() instead, which always uses the
> appropriate size. Allocate the device name with a unique identifier
> instead of a kernel address.

You are doing two different things here, one is changing the allocation
way, and the other is the name.  If one of those things turns out to
break something, we have to revert this whole thing.

So please make this two different changes, one to change to use
devm_kasprintf() and the second to change the naming scheme, ESPECIALLY
as you do not mention the name change in the subject line.  And that's
going to be a user-visible change, so you need to make that VERY
obvious.

thanks,

greg k-h

Re: [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Prathu Baronia]

On Sat, Jun 17, 2023 at 04:00:02PM +0200, Greg Kroah-Hartman wrote:
> 
> You are doing two different things here, one is changing the allocation
> way, and the other is the name.  If one of those things turns out to
> break something, we have to revert this whole thing.
> 
> So please make this two different changes, one to change to use
> devm_kasprintf() and the second to change the naming scheme, ESPECIALLY
> as you do not mention the name change in the subject line.  And that's
> going to be a user-visible change, so you need to make that VERY
> obvious.
Makes sense greg, will do in v6.

Prathu

[PATCH v6 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Prathu Baronia]

In various places, string buffers of a fixed size are allocated, and
filled using snprintf() with the same fixed size, which is error-prone.

Replace this by calling devm_kasprintf() instead, which always uses the
appropriate size.

Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
V5 -> V6: Split into api change and name change commits
V4 -> V5: Remove the dev_info() and use a unique identifier for dev name
V3 -> V4: Split into warning fixing and cleanup commits
V2 -> V3: Fix smatch warnings from kernel test robot
V1 -> V2: Split into logical commits and fix commit message

 drivers/staging/axis-fifo/axis-fifo.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index 7a21f2423204..7d8da9ce2db8 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -816,10 +816,6 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	 * ----------------------------
 	 */
 
-	device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
-	if (!device_name)
-		return -ENOMEM;
-
 	/* allocate device wrapper memory */
 	fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
 	if (!fifo)
@@ -857,7 +853,11 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
 
 	/* create unique device name */
-	snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
+	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%p", DRIVER_NAME, &r_mem->start);
+	if (!device_name) {
+		rc = -ENOMEM;
+		goto err_initial;
+	}
 	dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
 
 	/* ----------------------------

base-commit: 45a3e24f65e90a047bef86f927ebdc4c710edaa1
-- 
2.34.1

[PATCH v6 2/2] axis-fifo: change device name by assigning unique device id

[Prathu Baronia]

Allocating the device name with a unique identifier
instead of a kernel address.

Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
 drivers/staging/axis-fifo/axis-fifo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index 7d8da9ce2db8..5e070e00e27a 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -853,7 +853,7 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
 
 	/* create unique device name */
-	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%p", DRIVER_NAME, &r_mem->start);
+	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%d", DRIVER_NAME, pdev->id);
 	if (!device_name) {
 		rc = -ENOMEM;
 		goto err_initial;
-- 
2.34.1

Re: [PATCH v6 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Dan Carpenter]

On Mon, Jun 19, 2023 at 09:52:44PM +0530, Prathu Baronia wrote:
> In various places, string buffers of a fixed size are allocated, and
> filled using snprintf() with the same fixed size, which is error-prone.
> 
> Replace this by calling devm_kasprintf() instead, which always uses the
> appropriate size.
> 
> Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
> ---
> V5 -> V6: Split into api change and name change commits
> V4 -> V5: Remove the dev_info() and use a unique identifier for dev name
> V3 -> V4: Split into warning fixing and cleanup commits
> V2 -> V3: Fix smatch warnings from kernel test robot
> V1 -> V2: Split into logical commits and fix commit message
> 

> -	snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
                                      ^^^

> +	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%p", DRIVER_NAME, &r_mem->start);
                                                          ^^

This is a sneaky fix which Greg already kind of complained about if I
remember correctly...

regards,
dan carpenter

Re: [PATCH v6 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings

[Greg Kroah-Hartman]

On Mon, Jun 19, 2023 at 09:52:44PM +0530, Prathu Baronia wrote:
> In various places, string buffers of a fixed size are allocated, and
> filled using snprintf() with the same fixed size, which is error-prone.
> 
> Replace this by calling devm_kasprintf() instead, which always uses the
> appropriate size.
> 
> Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
> ---
> V5 -> V6: Split into api change and name change commits
> V4 -> V5: Remove the dev_info() and use a unique identifier for dev name
> V3 -> V4: Split into warning fixing and cleanup commits
> V2 -> V3: Fix smatch warnings from kernel test robot
> V1 -> V2: Split into logical commits and fix commit message
> 
>  drivers/staging/axis-fifo/axis-fifo.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> index 7a21f2423204..7d8da9ce2db8 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -816,10 +816,6 @@ static int axis_fifo_probe(struct platform_device *pdev)
>  	 * ----------------------------
>  	 */
>  
> -	device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
> -	if (!device_name)
> -		return -ENOMEM;
> -
>  	/* allocate device wrapper memory */
>  	fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
>  	if (!fifo)
> @@ -857,7 +853,11 @@ static int axis_fifo_probe(struct platform_device *pdev)
>  	dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
>  
>  	/* create unique device name */
> -	snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
> +	device_name = devm_kasprintf(dev, GFP_KERNEL, "%s_%p", DRIVER_NAME, &r_mem->start);

As Dan points out, you are changing the name here :(

[PATCH v2 2/2] axis-fifo: cleanup space issues with fops struct

[Prathu Baronia]

Add required spaces for proper formatting of fops members for better
readability.

Signed-off-by: Prathu Baronia <prathubaronia2011@gmail.com>
---
 drivers/staging/axis-fifo/axis-fifo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index 7b3080202b31..2692fda89583 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -716,11 +716,11 @@ static int axis_fifo_close(struct inode *inod, struct file *f)
 }
 
 static const struct file_operations fops = {
-	.owner = THIS_MODULE,
-	.open = axis_fifo_open,
+	.owner   = THIS_MODULE,
+	.open    = axis_fifo_open,
 	.release = axis_fifo_close,
-	.read = axis_fifo_read,
-	.write = axis_fifo_write
+	.read    = axis_fifo_read,
+	.write   = axis_fifo_write
 };
 
 /* read named property from the device tree */
-- 
2.34.1