* [PATCH] axis-fifo: use devm_kasprintf for device name
@ 2023-05-13 21:40 Prathu Baronia
2023-05-14 6:37 ` Greg Kroah-Hartman
0 siblings, 1 reply; 30+ messages in thread
From: Prathu Baronia @ 2023-05-13 21:40 UTC (permalink / raw)
To: Greg Kroah-Hartman, Prathu Baronia, linux-staging, linux-kernel
- 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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] axis-fifo: use devm_kasprintf for device name
2023-05-13 21:40 [PATCH] axis-fifo: use devm_kasprintf for device name Prathu Baronia
@ 2023-05-14 6:37 ` Greg Kroah-Hartman
2023-05-14 13:01 ` Prathu Baronia
0 siblings, 1 reply; 30+ messages in thread
From: Greg Kroah-Hartman @ 2023-05-14 6:37 UTC (permalink / raw)
To: Prathu Baronia; +Cc: linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* (no subject)
2023-05-14 6:37 ` Greg Kroah-Hartman
@ 2023-05-14 13:01 ` Prathu Baronia
2023-05-14 13:01 ` [PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
2023-05-14 13:01 ` [PATCH v2 2/2] axis-fifo: cleanup space issues with fops struct Prathu Baronia
0 siblings, 2 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-05-14 13:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, linux-staging, prathubaronia2011
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-05-14 13:01 ` Prathu Baronia
@ 2023-05-14 13:01 ` Prathu Baronia
2023-05-15 7:42 ` Dan Carpenter
2023-05-14 13:01 ` [PATCH v2 2/2] axis-fifo: cleanup space issues with fops struct Prathu Baronia
1 sibling, 1 reply; 30+ messages in thread
From: Prathu Baronia @ 2023-05-14 13:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, linux-staging, prathubaronia2011
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 2/2] axis-fifo: cleanup space issues with fops struct
2023-05-14 13:01 ` Prathu Baronia
2023-05-14 13:01 ` [PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
@ 2023-05-14 13:01 ` Prathu Baronia
1 sibling, 0 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-05-14 13:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, linux-staging, prathubaronia2011
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-05-14 13:01 ` [PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
@ 2023-05-15 7:42 ` Dan Carpenter
2023-05-18 14:31 ` Prathu Baronia
0 siblings, 1 reply; 30+ messages in thread
From: Dan Carpenter @ 2023-05-15 7:42 UTC (permalink / raw)
To: oe-kbuild, Prathu Baronia, gregkh
Cc: lkp, oe-kbuild-all, linux-kernel, linux-staging, prathubaronia2011
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-05-15 7:42 ` Dan Carpenter
@ 2023-05-18 14:31 ` Prathu Baronia
2023-05-18 14:51 ` [PATCH v3 " Prathu Baronia
0 siblings, 1 reply; 30+ messages in thread
From: Prathu Baronia @ 2023-05-18 14:31 UTC (permalink / raw)
To: Dan Carpenter
Cc: oe-kbuild, gregkh, lkp, oe-kbuild-all, linux-kernel, linux-staging
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
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-05-18 14:31 ` Prathu Baronia
@ 2023-05-18 14:51 ` Prathu Baronia
2023-05-18 14:51 ` [PATCH v3 2/2] axis-fifo: cleanup space issues with fops struct Prathu Baronia
2023-05-27 7:35 ` [PATCH v3 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Greg KH
0 siblings, 2 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-05-18 14:51 UTC (permalink / raw)
To: prathubaronia2011
Cc: dan.carpenter, gregkh, linux-kernel, linux-staging, lkp,
oe-kbuild-all, oe-kbuild, Dan Carpenter
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 2/2] axis-fifo: cleanup space issues with fops struct
2023-05-18 14:51 ` [PATCH v3 " Prathu Baronia
@ 2023-05-18 14:51 ` Prathu Baronia
2023-05-27 7:35 ` [PATCH v3 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Greg KH
1 sibling, 0 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-05-18 14:51 UTC (permalink / raw)
To: prathubaronia2011
Cc: dan.carpenter, gregkh, linux-kernel, linux-staging, lkp,
oe-kbuild-all, oe-kbuild
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-05-18 14:51 ` [PATCH v3 " Prathu Baronia
2023-05-18 14:51 ` [PATCH v3 2/2] axis-fifo: cleanup space issues with fops struct Prathu Baronia
@ 2023-05-27 7:35 ` Greg KH
2023-05-27 8:53 ` Prathu Baronia
1 sibling, 1 reply; 30+ messages in thread
From: Greg KH @ 2023-05-27 7:35 UTC (permalink / raw)
To: Prathu Baronia
Cc: dan.carpenter, linux-kernel, linux-staging, lkp, oe-kbuild-all,
oe-kbuild, Dan Carpenter
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-05-27 7:35 ` [PATCH v3 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Greg KH
@ 2023-05-27 8:53 ` Prathu Baronia
2023-05-27 11:50 ` [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier Prathu Baronia
0 siblings, 1 reply; 30+ messages in thread
From: Prathu Baronia @ 2023-05-27 8:53 UTC (permalink / raw)
To: Greg KH
Cc: dan.carpenter, linux-kernel, linux-staging, lkp, oe-kbuild-all,
oe-kbuild, Dan Carpenter
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier
2023-05-27 8:53 ` Prathu Baronia
@ 2023-05-27 11:50 ` Prathu Baronia
2023-05-27 11:50 ` [PATCH v4 2/3] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
` (2 more replies)
0 siblings, 3 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-05-27 11:50 UTC (permalink / raw)
To: prathubaronia2011, Greg Kroah-Hartman, Uwe Kleine-König,
Khadija Kamran, Fabio M. De Francesco, linux-staging,
linux-kernel
Cc: dan.carpenter, error27, lkp, oe-kbuild-all, oe-kbuild
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 2/3] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-05-27 11:50 ` [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier Prathu Baronia
@ 2023-05-27 11:50 ` Prathu Baronia
2023-05-28 9:00 ` Greg Kroah-Hartman
2023-05-27 11:51 ` [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct Prathu Baronia
2023-05-28 8:59 ` [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier Greg Kroah-Hartman
2 siblings, 1 reply; 30+ messages in thread
From: Prathu Baronia @ 2023-05-27 11:50 UTC (permalink / raw)
To: prathubaronia2011, Greg Kroah-Hartman, Uwe Kleine-König,
Fabio M. De Francesco, Khadija Kamran, linux-staging,
linux-kernel
Cc: dan.carpenter, error27, lkp, oe-kbuild-all, oe-kbuild
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct
2023-05-27 11:50 ` [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier Prathu Baronia
2023-05-27 11:50 ` [PATCH v4 2/3] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
@ 2023-05-27 11:51 ` Prathu Baronia
2023-05-27 22:31 ` Uwe Kleine-König
2023-05-28 8:59 ` [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier Greg Kroah-Hartman
2 siblings, 1 reply; 30+ messages in thread
From: Prathu Baronia @ 2023-05-27 11:51 UTC (permalink / raw)
To: prathubaronia2011, Greg Kroah-Hartman, Uwe Kleine-König,
Fabio M. De Francesco, Khadija Kamran, linux-staging,
linux-kernel
Cc: dan.carpenter, error27, lkp, oe-kbuild-all, oe-kbuild
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct
2023-05-27 11:51 ` [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct Prathu Baronia
@ 2023-05-27 22:31 ` Uwe Kleine-König
2023-05-28 8:57 ` Greg Kroah-Hartman
2023-06-13 15:56 ` Prathu Baronia
0 siblings, 2 replies; 30+ messages in thread
From: Uwe Kleine-König @ 2023-05-27 22:31 UTC (permalink / raw)
To: Prathu Baronia
Cc: Greg Kroah-Hartman, Fabio M. De Francesco, Khadija Kamran,
linux-staging, linux-kernel, dan.carpenter, error27, lkp,
oe-kbuild-all, oe-kbuild
[-- 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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct
2023-05-27 22:31 ` Uwe Kleine-König
@ 2023-05-28 8:57 ` Greg Kroah-Hartman
2023-06-13 16:01 ` Prathu Baronia
2023-06-13 15:56 ` Prathu Baronia
1 sibling, 1 reply; 30+ messages in thread
From: Greg Kroah-Hartman @ 2023-05-28 8:57 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Prathu Baronia, Fabio M. De Francesco, Khadija Kamran,
linux-staging, linux-kernel, dan.carpenter, error27, lkp,
oe-kbuild-all, oe-kbuild
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier
2023-05-27 11:50 ` [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier Prathu Baronia
2023-05-27 11:50 ` [PATCH v4 2/3] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
2023-05-27 11:51 ` [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct Prathu Baronia
@ 2023-05-28 8:59 ` Greg Kroah-Hartman
2023-06-13 16:02 ` Prathu Baronia
2 siblings, 1 reply; 30+ messages in thread
From: Greg Kroah-Hartman @ 2023-05-28 8:59 UTC (permalink / raw)
To: Prathu Baronia
Cc: Uwe Kleine-König, Khadija Kamran, Fabio M. De Francesco,
linux-staging, linux-kernel, dan.carpenter, error27, lkp,
oe-kbuild-all, oe-kbuild
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/3] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-05-27 11:50 ` [PATCH v4 2/3] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
@ 2023-05-28 9:00 ` Greg Kroah-Hartman
2023-06-13 16:03 ` Prathu Baronia
0 siblings, 1 reply; 30+ messages in thread
From: Greg Kroah-Hartman @ 2023-05-28 9:00 UTC (permalink / raw)
To: Prathu Baronia
Cc: Uwe Kleine-König, Fabio M. De Francesco, Khadija Kamran,
linux-staging, linux-kernel, dan.carpenter, error27, lkp,
oe-kbuild-all, oe-kbuild
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct
2023-05-27 22:31 ` Uwe Kleine-König
2023-05-28 8:57 ` Greg Kroah-Hartman
@ 2023-06-13 15:56 ` Prathu Baronia
1 sibling, 0 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-06-13 15:56 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Fabio M. De Francesco, Khadija Kamran,
linux-staging, linux-kernel, dan.carpenter, error27, lkp,
oe-kbuild-all, oe-kbuild
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct
2023-05-28 8:57 ` Greg Kroah-Hartman
@ 2023-06-13 16:01 ` Prathu Baronia
0 siblings, 0 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-06-13 16:01 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Uwe Kleine-König, Fabio M. De Francesco, Khadija Kamran,
linux-staging, linux-kernel, dan.carpenter, error27, lkp,
oe-kbuild-all, oe-kbuild
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier
2023-05-28 8:59 ` [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier Greg Kroah-Hartman
@ 2023-06-13 16:02 ` Prathu Baronia
2023-06-16 15:25 ` [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
0 siblings, 1 reply; 30+ messages in thread
From: Prathu Baronia @ 2023-06-13 16:02 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Uwe Kleine-König, Khadija Kamran, Fabio M. De Francesco,
linux-staging, linux-kernel, dan.carpenter, error27, lkp,
oe-kbuild-all, oe-kbuild
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/3] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-05-28 9:00 ` Greg Kroah-Hartman
@ 2023-06-13 16:03 ` Prathu Baronia
0 siblings, 0 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-06-13 16:03 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Uwe Kleine-König, Fabio M. De Francesco, Khadija Kamran,
linux-staging, linux-kernel, dan.carpenter, error27, lkp,
oe-kbuild-all, oe-kbuild
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-06-13 16:02 ` Prathu Baronia
@ 2023-06-16 15:25 ` Prathu Baronia
2023-06-16 15:26 ` [PATCH v5 2/2] axis-fifo: remove the unnecessary dev_info() Prathu Baronia
2023-06-17 14:00 ` [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Greg Kroah-Hartman
0 siblings, 2 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-06-16 15:25 UTC (permalink / raw)
To: prathubaronia2011, Greg Kroah-Hartman, Fabio M. De Francesco,
Khadija Kamran, Uwe Kleine-König, linux-staging,
linux-kernel
Cc: dan.carpenter, error27, lkp, oe-kbuild-all, oe-kbuild
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 2/2] axis-fifo: remove the unnecessary dev_info()
2023-06-16 15:25 ` [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
@ 2023-06-16 15:26 ` Prathu Baronia
2023-06-17 14:00 ` [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Greg Kroah-Hartman
1 sibling, 0 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-06-16 15:26 UTC (permalink / raw)
To: prathubaronia2011, Greg Kroah-Hartman, Fabio M. De Francesco,
Khadija Kamran, Uwe Kleine-König, linux-staging,
linux-kernel
Cc: dan.carpenter, error27, lkp, oe-kbuild-all, oe-kbuild
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-06-16 15:25 ` [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
2023-06-16 15:26 ` [PATCH v5 2/2] axis-fifo: remove the unnecessary dev_info() Prathu Baronia
@ 2023-06-17 14:00 ` Greg Kroah-Hartman
2023-06-19 6:43 ` Prathu Baronia
1 sibling, 1 reply; 30+ messages in thread
From: Greg Kroah-Hartman @ 2023-06-17 14:00 UTC (permalink / raw)
To: Prathu Baronia
Cc: Fabio M. De Francesco, Khadija Kamran, Uwe Kleine-König,
linux-staging, linux-kernel, dan.carpenter, error27, lkp,
oe-kbuild-all, oe-kbuild
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-06-17 14:00 ` [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Greg Kroah-Hartman
@ 2023-06-19 6:43 ` Prathu Baronia
2023-06-19 16:22 ` [PATCH v6 " Prathu Baronia
0 siblings, 1 reply; 30+ messages in thread
From: Prathu Baronia @ 2023-06-19 6:43 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Fabio M. De Francesco, Khadija Kamran, Uwe Kleine-König,
linux-staging, linux-kernel, dan.carpenter, error27, lkp,
oe-kbuild-all, oe-kbuild
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v6 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-06-19 6:43 ` Prathu Baronia
@ 2023-06-19 16:22 ` Prathu Baronia
2023-06-19 16:22 ` [PATCH v6 2/2] axis-fifo: change device name by assigning unique device id Prathu Baronia
` (2 more replies)
0 siblings, 3 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-06-19 16:22 UTC (permalink / raw)
To: prathubaronia2011, Greg Kroah-Hartman, Fabio M. De Francesco,
Khadija Kamran, Uwe Kleine-König, linux-staging,
linux-kernel
Cc: dan.carpenter, error27, lkp, oe-kbuild-all, oe-kbuild
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v6 2/2] axis-fifo: change device name by assigning unique device id
2023-06-19 16:22 ` [PATCH v6 " Prathu Baronia
@ 2023-06-19 16:22 ` Prathu Baronia
2023-06-20 5:18 ` [PATCH v6 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Dan Carpenter
2023-06-20 9:24 ` Greg Kroah-Hartman
2 siblings, 0 replies; 30+ messages in thread
From: Prathu Baronia @ 2023-06-19 16:22 UTC (permalink / raw)
To: prathubaronia2011, Greg Kroah-Hartman, Uwe Kleine-König,
Fabio M. De Francesco, Khadija Kamran, linux-staging,
linux-kernel
Cc: dan.carpenter, error27, lkp, oe-kbuild-all, oe-kbuild
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-06-19 16:22 ` [PATCH v6 " Prathu Baronia
2023-06-19 16:22 ` [PATCH v6 2/2] axis-fifo: change device name by assigning unique device id Prathu Baronia
@ 2023-06-20 5:18 ` Dan Carpenter
2023-06-20 9:24 ` Greg Kroah-Hartman
2 siblings, 0 replies; 30+ messages in thread
From: Dan Carpenter @ 2023-06-20 5:18 UTC (permalink / raw)
To: Prathu Baronia
Cc: Greg Kroah-Hartman, Fabio M. De Francesco, Khadija Kamran,
Uwe Kleine-König, linux-staging, linux-kernel, error27, lkp,
oe-kbuild-all, oe-kbuild
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings
2023-06-19 16:22 ` [PATCH v6 " Prathu Baronia
2023-06-19 16:22 ` [PATCH v6 2/2] axis-fifo: change device name by assigning unique device id Prathu Baronia
2023-06-20 5:18 ` [PATCH v6 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Dan Carpenter
@ 2023-06-20 9:24 ` Greg Kroah-Hartman
2 siblings, 0 replies; 30+ messages in thread
From: Greg Kroah-Hartman @ 2023-06-20 9:24 UTC (permalink / raw)
To: Prathu Baronia
Cc: Fabio M. De Francesco, Khadija Kamran, Uwe Kleine-König,
linux-staging, linux-kernel, dan.carpenter, error27, lkp,
oe-kbuild-all, oe-kbuild
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 :(
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2023-06-20 9:24 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-13 21:40 [PATCH] axis-fifo: use devm_kasprintf for device name Prathu Baronia
2023-05-14 6:37 ` Greg Kroah-Hartman
2023-05-14 13:01 ` Prathu Baronia
2023-05-14 13:01 ` [PATCH v2 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
2023-05-15 7:42 ` Dan Carpenter
2023-05-18 14:31 ` Prathu Baronia
2023-05-18 14:51 ` [PATCH v3 " Prathu Baronia
2023-05-18 14:51 ` [PATCH v3 2/2] axis-fifo: cleanup space issues with fops struct Prathu Baronia
2023-05-27 7:35 ` [PATCH v3 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Greg KH
2023-05-27 8:53 ` Prathu Baronia
2023-05-27 11:50 ` [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier Prathu Baronia
2023-05-27 11:50 ` [PATCH v4 2/3] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
2023-05-28 9:00 ` Greg Kroah-Hartman
2023-06-13 16:03 ` Prathu Baronia
2023-05-27 11:51 ` [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct Prathu Baronia
2023-05-27 22:31 ` Uwe Kleine-König
2023-05-28 8:57 ` Greg Kroah-Hartman
2023-06-13 16:01 ` Prathu Baronia
2023-06-13 15:56 ` Prathu Baronia
2023-05-28 8:59 ` [PATCH v4 1/3] axis-fifo: fix smatch warning for format specifier Greg Kroah-Hartman
2023-06-13 16:02 ` Prathu Baronia
2023-06-16 15:25 ` [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Prathu Baronia
2023-06-16 15:26 ` [PATCH v5 2/2] axis-fifo: remove the unnecessary dev_info() Prathu Baronia
2023-06-17 14:00 ` [PATCH v5 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Greg Kroah-Hartman
2023-06-19 6:43 ` Prathu Baronia
2023-06-19 16:22 ` [PATCH v6 " Prathu Baronia
2023-06-19 16:22 ` [PATCH v6 2/2] axis-fifo: change device name by assigning unique device id Prathu Baronia
2023-06-20 5:18 ` [PATCH v6 1/2] axis-fifo: use devm_kasprintf() for allocating formatted strings Dan Carpenter
2023-06-20 9:24 ` Greg Kroah-Hartman
2023-05-14 13:01 ` [PATCH v2 2/2] axis-fifo: cleanup space issues with fops struct Prathu Baronia
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).