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

2023-05-14 13:13:19

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

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 <[email protected]>
---
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

2023-05-15 07:59:25

by Dan Carpenter

[permalink] [raw]

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

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 <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Link: https://lore.kernel.org/r/[email protected]/

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

2023-05-18 14:42:26

by Prathu Baronia

[permalink] [raw]

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

Hi Dan,

Thanks for pointing these introduced in an earlier ---
- Prathu

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

2023-05-18 15:19:17

by Prathu Baronia

[permalink] [raw]

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

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 <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
Link: https://lore.kernel.org/r/[email protected]/
Signed-off-by: Prathu Baronia <[email protected]>
---
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

2023-05-27 07:44:45

by Greg Kroah-Hartman

[permalink] [raw]

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

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 <[email protected]>

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

2023-05-27 09:39:23

by Prathu Baronia

[permalink] [raw]

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

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 <[email protected]>
>
> 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

2023-05-27 11:55:10

by Prathu Baronia

[permalink] [raw]

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

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 <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
Link: https://lore.kernel.org/r/[email protected]/
Signed-off-by: Prathu Baronia <[email protected]>
---
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

2023-05-27 12:28:00

by Prathu Baronia

[permalink] [raw]

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

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 <[email protected]>
---
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

2023-05-27 12:30:45

by Prathu Baronia

[permalink] [raw]

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

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

Signed-off-by: Prathu Baronia <[email protected]>
---
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

2023-05-27 22:35:31

by Uwe Kleine-König

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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/[email protected]/
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/ |

Attachments:

(No filename) (2.23 kB)
signature.asc (499.00 B)
Download all attachments

2023-05-28 09:13:43

by Greg Kroah-Hartman

[permalink] [raw]

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

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 <[email protected]>
> > ---
> > 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/[email protected]/
> 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

2023-05-28 09:43:54

by Greg Kroah-Hartman

[permalink] [raw]

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

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 <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]/
> Signed-off-by: Prathu Baronia <[email protected]>
> ---
> 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

2023-05-28 10:00:28

by Greg Kroah-Hartman

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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

2023-06-13 16:27:50

by Prathu Baronia

[permalink] [raw]

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

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

2023-06-13 16:41:00

by Prathu Baronia

[permalink] [raw]

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

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/[email protected]/
> 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

2023-06-13 16:47:18

by Prathu Baronia

[permalink] [raw]

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

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

2023-06-13 16:58:56

by Prathu Baronia

[permalink] [raw]

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

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

2023-06-16 15:46:19

by Prathu Baronia

[permalink] [raw]

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

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 <[email protected]>
---
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

2023-06-16 16:02:31

by Prathu Baronia

[permalink] [raw]

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

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 <[email protected]>
---
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

2023-06-17 14:50:30

by Greg Kroah-Hartman

[permalink] [raw]

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

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

2023-06-19 07:07:25

by Prathu Baronia

[permalink] [raw]

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

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

2023-06-19 16:57:21

by Prathu Baronia

[permalink] [raw]

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

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 <[email protected]>
---
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

2023-06-20 05:37:12

by Dan Carpenter

[permalink] [raw]

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

2023-06-20 09:44:51

by Greg Kroah-Hartman

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 :(