drm/amd: cleanup get_mfd_cell_dev()

PATCH Applied Dan Carpenter 3 participants 5 messages Feb 25, 2016 – Feb 27, 2016

Subsystems (3)

DRM drivers Kernel janitors Radeon and amdgpu DRM drivers

lore

Share & tools

lore

Dan Carpenter

author

2016-02-25 07:47 UTC #0 lore raw

PATCH 1/1 drm/amd: cleanup get_mfd_cell_dev()

It's simpler to just use snprintf() to print this to one buffer instead of using strcpy() and strcat(). Also using snprintf() is slightly safer than using sprintf().

Signed-off-by Dan Carpenter <[email protected]>

drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +2 -4

index 9f8cfaa..d6b0bff 100644
@@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain *genpd)
 static struct device *get_mfd_cell_dev(const char *device_name, int r)
 {
 	char auto_dev_name[25];
-	char buf[8];
 	struct device *dev;
 
-	sprintf(buf, ".%d.auto", r);
-	strcpy(auto_dev_name, device_name);
-	strcat(auto_dev_name, buf);
+	snprintf(auto_dev_name, sizeof(auto_dev_name),
+		 "%s.%d.auto", device_name, r);
 	dev = bus_find_device_by_name(&platform_bus_type, NULL, auto_dev_name);
 	dev_info(dev, "device %s added to pm domain\n", auto_dev_name);

Alex Deucher

Applied

2016-02-26 17:46 UTC #1 lore raw

Quoted 5 lines · Dan Carpenter

It's simpler to just use snprintf() to print this to one buffer instead of using strcpy() and strcat(). Also using snprintf() is slightly safer than using sprintf().
Signed-off-by Dan Carpenter <[email protected]>

Applied. Thanks!

Quoted 24 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +2 -4

index 9f8cfaa..d6b0bff 100644
@@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain *genpd)
 static struct device *get_mfd_cell_dev(const char *device_name, int r)
 {
        char auto_dev_name[25];
-       char buf[8];
        struct device *dev;
 
-       sprintf(buf, ".%d.auto", r);
-       strcpy(auto_dev_name, device_name);
-       strcat(auto_dev_name, buf);
+       snprintf(auto_dev_name, sizeof(auto_dev_name),
+                "%s.%d.auto", device_name, r);
        dev = bus_find_device_by_name(&platform_bus_type, NULL, auto_dev_name);
        dev_info(dev, "device %s added to pm domain\n", auto_dev_name);

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

walter harms

2016-02-27 09:50 UTC #2 lore raw

In reply to Dan Carpenter

Am 25.02.2016 08:47, schrieb Dan Carpenter:

Quoted 15 lines

It's simpler to just use snprintf() to print this to one buffer instead of using strcpy() and strcat(). Also using snprintf() is slightly safer than using sprintf().
Signed-off-by Dan Carpenter <[email protected]>
drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +0 -1
index 9f8cfaa..d6b0bff 100644
@@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain *genpd)
 static struct device *get_mfd_cell_dev(const char *device_name, int r)
 {
 	char auto_dev_name[25];
-	char buf[8];

drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c static int acp_poweron(struct generic_pm_domain *genpd) · line 249

Quoted 10 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +2 -3

 	struct device *dev;
 
-	sprintf(buf, ".%d.auto", r);
-	strcpy(auto_dev_name, device_name);
-	strcat(auto_dev_name, buf);
+	snprintf(auto_dev_name, sizeof(auto_dev_name),
+		 "%s.%d.auto", device_name, r);
 	dev = bus_find_device_by_name(&platform_bus_type, NULL, auto_dev_name);
 	dev_info(dev, "device %s added to pm domain\n", auto_dev_name);

hi,
i tried to understand what is the base for char auto_dev_name[25]. It is not clear from these snipped if that is large or small.

(To be aware i assume that get_mfd_cell_dev("terrible_long_and_Stupid_name",1234567899346712) will never happen but i could find no reason)

A small comment that explains the magic 25 would be nice.

re,
wh

Dan Carpenter

author

2016-02-27 10:40 UTC #3 lore raw

Quoted 33 lines · walter harms

Am 25.02.2016 08:47, schrieb Dan Carpenter:
Quoted 15 lines
It's simpler to just use snprintf() to print this to one buffer instead of using strcpy() and strcat(). Also using snprintf() is slightly safer than using sprintf().
Signed-off-by Dan Carpenter <[email protected]>
drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +0 -1
index 9f8cfaa..d6b0bff 100644
@@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain *genpd)
 static struct device *get_mfd_cell_dev(const char *device_name, int r)
 {
 	char auto_dev_name[25];
-	char buf[8];
drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c static int acp_poweron(struct generic_pm_domain *genpd) · line 249
Quoted 10 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +2 -3
 	struct device *dev;
 
-	sprintf(buf, ".%d.auto", r);
-	strcpy(auto_dev_name, device_name);
-	strcat(auto_dev_name, buf);
+	snprintf(auto_dev_name, sizeof(auto_dev_name),
+		 "%s.%d.auto", device_name, r);
 	dev = bus_find_device_by_name(&platform_bus_type, NULL, auto_dev_name);
 	dev_info(dev, "device %s added to pm domain\n", auto_dev_name);
 
hi,
i tried to understand what is the base for char auto_dev_name[25]. It is not clear from these snipped if that is large or small.

Quoted 5 lines

(To be aware i assume that get_mfd_cell_dev("terrible_long_and_Stupid_name",1234567899346712) will never happen but i could find no reason)
A small comment that explains the magic 25 would be nice.

I have no idea, either of course. For example, mc13xxx_add_subdevice_pdata() assumes device_name by itself can be 30 characters. Hence the change to snprintf.

regards,
dan carpenter

walter harms

2016-02-27 11:18 UTC #4 lore raw

Am 27.02.2016 11:40, schrieb Dan Carpenter:

Quoted 38 lines · walter harms

Quoted 37 lines
Am 25.02.2016 08:47, schrieb Dan Carpenter:
Quoted 15 lines
It's simpler to just use snprintf() to print this to one buffer instead of using strcpy() and strcat(). Also using snprintf() is slightly safer than using sprintf().
Signed-off-by Dan Carpenter <[email protected]>
drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +0 -1
index 9f8cfaa..d6b0bff 100644
@@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain *genpd)
 static struct device *get_mfd_cell_dev(const char *device_name, int r)
 {
 	char auto_dev_name[25];
-	char buf[8];
drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c static int acp_poweron(struct generic_pm_domain *genpd) · line 249
Quoted 10 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +2 -3
 	struct device *dev;
 
-	sprintf(buf, ".%d.auto", r);
-	strcpy(auto_dev_name, device_name);
-	strcat(auto_dev_name, buf);
+	snprintf(auto_dev_name, sizeof(auto_dev_name),
+		 "%s.%d.auto", device_name, r);
 	dev = bus_find_device_by_name(&platform_bus_type, NULL, auto_dev_name);
 	dev_info(dev, "device %s added to pm domain\n", auto_dev_name);
 
hi,
i tried to understand what is the base for char auto_dev_name[25]. It is not clear from these snipped if that is large or small.
(To be aware i assume that get_mfd_cell_dev("terrible_long_and_Stupid_name",1234567899346712) will never happen but i could find no reason)

Quoted 6 lines

Quoted 1 line
A small comment that explains the magic 25 would be nice.
I have no idea, either of course. For example, mc13xxx_add_subdevice_pdata() assumes device_name by itself can be 30 characters. Hence the change to snprintf.

Hi Dan,
i also think that this limit is artificial, one of those "it works" things.
The problem is that i have seen changes in the naming als ready done, like the
shift from /dev/sda to things like /dev/disk/by-id/scsi-SATA_WDC_WD5000AAKS-_WD-WCASY5869245-part1 and you are out of the game here. snprintf will cut the tail and the %d.auto stuff is dead in the water. To make it clear, I do not thing that is security related issue but it could result in annoying failures. (the solution is obviously to use asprintf() and free the mem later :) ).

re,
wh