* re: PM / OPP: Add debugfs support
@ 2015-09-22 12:34 Dan Carpenter
2015-10-08 9:17 ` Viresh Kumar
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2015-09-22 12:34 UTC (permalink / raw)
To: viresh.kumar; +Cc: linux-pm
Hello Viresh Kumar,
The patch 5cb5fdbf3877: "PM / OPP: Add debugfs support" from Sep 4,
2015, leads to a bunch of static checker warnings:
drivers/base/power/opp/debugfs.c:41 opp_debug_create_one() error: format string overflow. buf_size: 15 length: 24
drivers/base/power/opp/debugfs.c:48 opp_debug_create_one() warn: passing casted pointer '&opp->available' to 'debugfs_create_bool()' 1 vs 32.
drivers/base/power/opp/debugfs.c:52 opp_debug_create_one() warn: passing casted pointer '&opp->dynamic' to 'debugfs_create_bool()' 1 vs 32.
drivers/base/power/opp/debugfs.c:55 opp_debug_create_one() warn: passing casted pointer '&opp->turbo' to 'debugfs_create_bool()' 1 vs 32.
drivers/base/power/opp/debugfs.c:58 opp_debug_create_one() warn: passing casted pointer '&opp->rate' to 'debugfs_create_u32()' 64 vs 32.
drivers/base/power/opp/debugfs.c:61 opp_debug_create_one() warn: passing casted pointer '&opp->u_volt' to 'debugfs_create_u32()' 64 vs 32.
drivers/base/power/opp/debugfs.c:65 opp_debug_create_one() warn: passing casted pointer '&opp->u_volt_min' to 'debugfs_create_u32()' 64 vs 32.
drivers/base/power/opp/debugfs.c:69 opp_debug_create_one() warn: passing casted pointer '&opp->u_volt_max' to 'debugfs_create_u32()' 64 vs 32.
drivers/base/power/opp/debugfs.c:73 opp_debug_create_one() warn: passing casted pointer '&opp->u_amp' to 'debugfs_create_u32()' 64 vs 32.
drivers/base/power/opp/debugfs.c:76 opp_debug_create_one() warn: passing casted pointer '&opp->clock_latency_ns' to 'debugfs_create_u32()' 64 vs 32.
drivers/base/power/opp/debugfs.c
34 int opp_debug_create_one(struct dev_pm_opp *opp, struct device_opp *dev_opp)
35 {
36 struct dentry *pdentry = dev_opp->dentry;
37 struct dentry *d;
38 char name[15];
39
40 /* Rate is unique to each OPP, use it to give opp-name */
41 sprintf(name, "opp:%lu", opp->rate);
opp->rate is unsigned long so the static checker says this can overflow
on 64 bit systems. The static checker tries to do cross function
analysis, but it's actually wrong. There is a later bug which cancels
out this bug.
Anyway, we should make the buffer larger and or change it to use
snprintf().
42
43 /* Create per-opp directory */
44 d = debugfs_create_dir(name, pdentry);
45 if (!d)
46 return -ENOMEM;
47
48 if (!debugfs_create_bool("available", S_IRUGO, d,
49 (u32 *)&opp->available))
debugfs_create_bool() writes 32 bits, but we only have 8 bits of space
so this corrupts memory.
50 return -ENOMEM;
51
52 if (!debugfs_create_bool("dynamic", S_IRUGO, d, (u32 *)&opp->dynamic))
53 return -ENOMEM;
Same.
54
55 if (!debugfs_create_bool("turbo", S_IRUGO, d, (u32 *)&opp->turbo))
56 return -ENOMEM;
Same.
57
58 if (!debugfs_create_u32("rate_hz", S_IRUGO, d, (u32 *)&opp->rate))
59 return -ENOMEM;
This only writes to the first 32 bits of the unsigned long. Which will
not work on big endian systems.
I don't know why is there no debugfs_create_ul() function. It seems
like it wouldn't be that complicated to create one.
It's the same for the rest of this funciton.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: PM / OPP: Add debugfs support
2015-09-22 12:34 PM / OPP: Add debugfs support Dan Carpenter
@ 2015-10-08 9:17 ` Viresh Kumar
2015-10-08 9:25 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Viresh Kumar @ 2015-10-08 9:17 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-pm
Hi Dan,
Sorry for being too late. Was in the middle of a conference earlier,
and that followed with 10 days of leave.
On 22-09-15, 15:34, Dan Carpenter wrote:
> Hello Viresh Kumar,
>
> The patch 5cb5fdbf3877: "PM / OPP: Add debugfs support" from Sep 4,
> 2015, leads to a bunch of static checker warnings:
>
> drivers/base/power/opp/debugfs.c:41 opp_debug_create_one() error: format string overflow. buf_size: 15 length: 24
> drivers/base/power/opp/debugfs.c:48 opp_debug_create_one() warn: passing casted pointer '&opp->available' to 'debugfs_create_bool()' 1 vs 32.
> drivers/base/power/opp/debugfs.c:52 opp_debug_create_one() warn: passing casted pointer '&opp->dynamic' to 'debugfs_create_bool()' 1 vs 32.
> drivers/base/power/opp/debugfs.c:55 opp_debug_create_one() warn: passing casted pointer '&opp->turbo' to 'debugfs_create_bool()' 1 vs 32.
> drivers/base/power/opp/debugfs.c:58 opp_debug_create_one() warn: passing casted pointer '&opp->rate' to 'debugfs_create_u32()' 64 vs 32.
> drivers/base/power/opp/debugfs.c:61 opp_debug_create_one() warn: passing casted pointer '&opp->u_volt' to 'debugfs_create_u32()' 64 vs 32.
> drivers/base/power/opp/debugfs.c:65 opp_debug_create_one() warn: passing casted pointer '&opp->u_volt_min' to 'debugfs_create_u32()' 64 vs 32.
> drivers/base/power/opp/debugfs.c:69 opp_debug_create_one() warn: passing casted pointer '&opp->u_volt_max' to 'debugfs_create_u32()' 64 vs 32.
> drivers/base/power/opp/debugfs.c:73 opp_debug_create_one() warn: passing casted pointer '&opp->u_amp' to 'debugfs_create_u32()' 64 vs 32.
> drivers/base/power/opp/debugfs.c:76 opp_debug_create_one() warn: passing casted pointer '&opp->clock_latency_ns' to 'debugfs_create_u32()' 64 vs 32.
>
> drivers/base/power/opp/debugfs.c
> 34 int opp_debug_create_one(struct dev_pm_opp *opp, struct device_opp *dev_opp)
> 35 {
> 36 struct dentry *pdentry = dev_opp->dentry;
> 37 struct dentry *d;
> 38 char name[15];
> 39
> 40 /* Rate is unique to each OPP, use it to give opp-name */
> 41 sprintf(name, "opp:%lu", opp->rate);
>
> opp->rate is unsigned long so the static checker says this can overflow
> on 64 bit systems. The static checker tries to do cross function
> analysis, but it's actually wrong. There is a later bug which cancels
> out this bug.
I did change it to snprintf in my local repository and yes it should
be fixed. About the length, yes I missed the u64 type for 64bit
systems and so that's a bug.
> Anyway, we should make the buffer larger and or change it to use
> snprintf().
>
> 42
> 43 /* Create per-opp directory */
> 44 d = debugfs_create_dir(name, pdentry);
> 45 if (!d)
> 46 return -ENOMEM;
> 47
> 48 if (!debugfs_create_bool("available", S_IRUGO, d,
> 49 (u32 *)&opp->available))
>
> debugfs_create_bool() writes 32 bits, but we only have 8 bits of space
> so this corrupts memory.
Right, so I fixed this in a different way. I have changed
debugfs_create_bool() API to accept a bool and update memory only for
a bool, instead of 8 or 32 bits. That patch is merged now and so the
cast can be dropped right away.
Over that, the $Subject patch is dropped by Rafael now, due to
conflicts with the debugfs_create_bool() series that got merged.
And so I need to resend it anyway.
> 57
> 58 if (!debugfs_create_u32("rate_hz", S_IRUGO, d, (u32 *)&opp->rate))
> 59 return -ENOMEM;
>
> This only writes to the first 32 bits of the unsigned long. Which will
> not work on big endian systems.
>
> I don't know why is there no debugfs_create_ul() function. It seems
> like it wouldn't be that complicated to create one.
I am fighting hard to get one, but Greg isn't liking the idea very
much :)
--
viresh
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-10-08 11:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-22 12:34 PM / OPP: Add debugfs support Dan Carpenter
2015-10-08 9:17 ` Viresh Kumar
2015-10-08 9:25 ` Dan Carpenter
2015-10-08 11:15 ` Viresh Kumar
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).