| Message ID | 5706174.BUsqiXljxZ@wuerfel (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Arnd, On 07-10-15, 09:35, Arnd Bergmann wrote: > The recently added debugfs support for OPP creates files using the > debugfs_create_bool() and debugfs_create_u32() interfaces, but > casts the data argument to u32*, which is broken on some architectures. > > In case of debugfs_create_bool(), the API has changed as of 621a5f7ad9cd > ("debugfs: Pass bool pointer to debugfs_create_bool()"), so we now get > a warning about the new interface in linux-next, which contains both > patches. Removing the cast makes it work in linux-next, and makes it > warn in cases where it does not work. > > For debugfs_create_u32(), the current usage is broken on 64-bit > architectures when the values exceed the range of 32-bit variables > (which should not happen), or when the kernel is built for as > big-endian. This patch removes the casts and changes the types > to u32 to make them match and print the correct value on all > architectures. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Fixes: 5cb5fdbf38770 ("PM / OPP: Add debugfs support") > --- > Found while building ARM allmodconfig Thanks. I just got back from vacations and this was the first thing I was going to look at. :) So, there are two problems here and I feel they should be fixed separately. The bool thing is simple to fix, just remove the cast as you did. Though it will keep warning if we use pm/linux-next branch directly, but will work well with linux-next. The second problem isn't that trivial. Just making unsigned longs as u32 doesn't look like the right solution to me (at least). They are unsigned long, to match the expected types with other frameworks like clock and regulator. What about adding something like debugfs_create_unsigned_long() for such cases? @Greg ??
On 07-10-15, 16:29, Viresh Kumar wrote: > What about adding something like debugfs_create_unsigned_long() for > such cases? And that can be as simple (or ugly, not sure if its buggy) as: debugfs_create_unsigned_long() { if (sizeof(unsigned long) == sizeof(u32)) return debugfs_create_u32(); if (sizeof(unsigned long) == sizeof(u64)) return debugfs_create_u64(); return -EINVAL; }
On Wed, Oct 07, 2015 at 04:33:02PM +0530, Viresh Kumar wrote: > On 07-10-15, 16:29, Viresh Kumar wrote: > > What about adding something like debugfs_create_unsigned_long() for > > such cases? > > And that can be as simple (or ugly, not sure if its buggy) as: > > debugfs_create_unsigned_long() > { > if (sizeof(unsigned long) == sizeof(u32)) > return debugfs_create_u32(); > if (sizeof(unsigned long) == sizeof(u64)) > return debugfs_create_u64(); > > return -EINVAL; > } Why would you be wanting to create a "unsigned long" as an api anyway? Just force it to be u64 all the time, can't you do that? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07-10-15, 12:07, Greg Kroah-Hartman wrote: > Why would you be wanting to create a "unsigned long" as an api anyway? > Just force it to be u64 all the time, can't you do that? Okay, so the variable in question (lets say frequency) is an 'unsigned long' and that's how all the APIs of clock framework expect/define it. And you are probably saying that we do this: unsigned long freq; debugfs_create_u64((u64 *)&freq); Right? Or are you asking to update clock APIs to be converted to u64? This will break things on a 32 bit architecture where unsigned long is 32 bits long, as doing this will overwrite the next 32 bits: *freq = XYZ;
On Wed, Oct 07, 2015 at 04:51:49PM +0530, Viresh Kumar wrote: > On 07-10-15, 12:07, Greg Kroah-Hartman wrote: > > Why would you be wanting to create a "unsigned long" as an api anyway? > > Just force it to be u64 all the time, can't you do that? > > Okay, so the variable in question (lets say frequency) is an 'unsigned > long' and that's how all the APIs of clock framework expect/define > it. > > And you are probably saying that we do this: > > unsigned long freq; > > debugfs_create_u64((u64 *)&freq); > > Right? Or are you asking to update clock APIs to be converted to u64? Yes, they should be u64 as I doubt you want to debug problems that you have in the driver where it works on a 64bit system but doesn't on a 32bit one. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Cc'ing Mike and Stephen.. On 07-10-15, 13:57, Greg Kroah-Hartman wrote: > On Wed, Oct 07, 2015 at 04:51:49PM +0530, Viresh Kumar wrote: > > On 07-10-15, 12:07, Greg Kroah-Hartman wrote: > > > Why would you be wanting to create a "unsigned long" as an api anyway? > > > Just force it to be u64 all the time, can't you do that? > > > > Okay, so the variable in question (lets say frequency) is an 'unsigned > > long' and that's how all the APIs of clock framework expect/define > > it. > > > > And you are probably saying that we do this: > > > > unsigned long freq; > > > > debugfs_create_u64((u64 *)&freq); > > > > Right? Or are you asking to update clock APIs to be converted to u64? > > Yes, they should be u64 as I doubt you want to debug problems that you > have in the driver where it works on a 64bit system but doesn't on a > 32bit one. Firstly changing the clock API (and other similar APIs) to make frequency u64 instead of 'unsigned long', looks like a giant effort. There are too many users of those API, etc.. Over that, it might be good performance wise to use u32 for 32 bit systems and u64 for 64 bit one, to represent clock frequency and maybe that's why we chose unsigned long there.
On Wed, Oct 07, 2015 at 04:51:49PM +0530, Viresh Kumar wrote: > On 07-10-15, 12:07, Greg Kroah-Hartman wrote: > > Why would you be wanting to create a "unsigned long" as an api anyway? > > Just force it to be u64 all the time, can't you do that? > > Okay, so the variable in question (lets say frequency) is an 'unsigned > long' and that's how all the APIs of clock framework expect/define > it. > > And you are probably saying that we do this: > > unsigned long freq; > > debugfs_create_u64((u64 *)&freq); > > Right? Or are you asking to update clock APIs to be converted to u64? Don't you even think about doing that - that's totally broken no matter what, and this is a good example of why casts are Bad. debugfs_create_u64() will create a debugfs object that will want to access a 64-bit value, but the value pointed to is only 32-bit. The net result is that the debugfs file ends up reading or writing the neigbouring 32-bits, which may potentially be unaligned. The variable pointed to for debugfs_create_u64() must be a 64-bit value. No casts allowed. An alternative would be to have debugfs_create_ulong() or similar which has accessors for standard C types. I don't think forcing subsystems wanting to use debugfs to have to use u{8,16,32,64} types for internal data is on - these are for things that we want to say "we want this to be a 8, 16, 32 or 64-bit type respectively" which is not what most subsystems need to do with internal data. This looks like a debugfs cockup to me. :)
On 07-10-15, 17:33, Russell King - ARM Linux wrote: > Don't you even think about doing that - that's totally broken no matter > what, and this is a good example of why casts are Bad. > > debugfs_create_u64() will create a debugfs object that will want to > access a 64-bit value, but the value pointed to is only 32-bit. The > net result is that the debugfs file ends up reading or writing the > neigbouring 32-bits, which may potentially be unaligned. Yeah, that's what I was also saying. Its broken. > The variable pointed to for debugfs_create_u64() must be a 64-bit > value. No casts allowed. > > An alternative would be to have debugfs_create_ulong() And that's what I suggested.
On Wed, Oct 07, 2015 at 06:33:24PM +0530, Viresh Kumar wrote: > Cc'ing Mike and Stephen.. > > On 07-10-15, 13:57, Greg Kroah-Hartman wrote: > > On Wed, Oct 07, 2015 at 04:51:49PM +0530, Viresh Kumar wrote: > > > On 07-10-15, 12:07, Greg Kroah-Hartman wrote: > > > > Why would you be wanting to create a "unsigned long" as an api anyway? > > > > Just force it to be u64 all the time, can't you do that? > > > > > > Okay, so the variable in question (lets say frequency) is an 'unsigned > > > long' and that's how all the APIs of clock framework expect/define > > > it. > > > > > > And you are probably saying that we do this: > > > > > > unsigned long freq; > > > > > > debugfs_create_u64((u64 *)&freq); > > > > > > Right? Or are you asking to update clock APIs to be converted to u64? > > > > Yes, they should be u64 as I doubt you want to debug problems that you > > have in the driver where it works on a 64bit system but doesn't on a > > 32bit one. > > Firstly changing the clock API (and other similar APIs) to make > frequency u64 instead of 'unsigned long', looks like a giant effort. > There are too many users of those API, etc.. > > Over that, it might be good performance wise to use u32 for 32 bit > systems and u64 for 64 bit one, to represent clock frequency and maybe > that's why we chose unsigned long there. Ok, then stop exporting it in debugfs and everyone will be happy :) greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07-10-15, 18:19, Greg Kroah-Hartman wrote:
> Ok, then stop exporting it in debugfs and everyone will be happy :)
Hehe, not really. See I am not happy :(
So, what do you suggest? Should we create debugfs_create_ulong()? And
how to implement that? With existing u32/u64 APIs or from scratch?
On Wed, Oct 07, 2015 at 10:57:01PM +0530, Viresh Kumar wrote: > On 07-10-15, 18:19, Greg Kroah-Hartman wrote: > > Ok, then stop exporting it in debugfs and everyone will be happy :) > > Hehe, not really. See I am not happy :( Why not? Why does this have to be exported in debugfs? Just delete it, who cares about it? It's just "debugging". > So, what do you suggest? Should we create debugfs_create_ulong()? Ick, do we _really_ need and want that? > And how to implement that? With existing u32/u64 APIs or from scratch? It's probably easy to just do in a few lines "from scratch", if you really need/want it, right? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07-10-15, 18:39, Greg Kroah-Hartman wrote: > Why not? Why does this have to be exported in debugfs? Just delete it, > who cares about it? It's just "debugging". It is really useful to see all the freq/voltage combinations on which a device can work on, specially the CPU. And there are some serious users of it as well.. Android has hacked the cpufreq layer today, to export something similar in a hacky way, as they wanted to estimate power ratings based on Hz/uV values. This will kill them as well.. And its really useful for debugging as well, to see if all went well. Maybe debugfs is not really the best place for this, but yeah, this is very useful.
On Wednesday 07 October 2015 18:33:24 Viresh Kumar wrote: > Cc'ing Mike and Stephen.. > > On 07-10-15, 13:57, Greg Kroah-Hartman wrote: > > On Wed, Oct 07, 2015 at 04:51:49PM +0530, Viresh Kumar wrote: > > > On 07-10-15, 12:07, Greg Kroah-Hartman wrote: > > > > Why would you be wanting to create a "unsigned long" as an api anyway? > > > > Just force it to be u64 all the time, can't you do that? > > > > > > Okay, so the variable in question (lets say frequency) is an 'unsigned > > > long' and that's how all the APIs of clock framework expect/define > > > it. > > > > > > And you are probably saying that we do this: > > > > > > unsigned long freq; > > > > > > debugfs_create_u64((u64 *)&freq); > > > > > > Right? Or are you asking to update clock APIs to be converted to u64? > > > > Yes, they should be u64 as I doubt you want to debug problems that you > > have in the driver where it works on a 64bit system but doesn't on a > > 32bit one. > > Firstly changing the clock API (and other similar APIs) to make > frequency u64 instead of 'unsigned long', looks like a giant effort. > There are too many users of those API, etc.. > > Over that, it might be good performance wise to use u32 for 32 bit > systems and u64 for 64 bit one, to represent clock frequency and maybe > that's why we chose unsigned long there. I think it clearly makes sense to have a fixed length for each of these members: either 32 bit is enough to represent all possible values, then there is no need to make them 'long' on 64-bit architectures, or 32 bit is not enough and then the code is broken on 32-bit architectures today and should be fixed. In my patch, I assumed that if 32-bit architectures work fine today, then we don't need more range on 64-bit architectures either. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07-10-15, 21:12, Arnd Bergmann wrote: > I think it clearly makes sense to have a fixed length for each of these > members: > > either 32 bit is enough to represent all possible values, then > there is no need to make them 'long' on 64-bit architectures, or 32 bit > is not enough and then the code is broken on 32-bit architectures today > and should be fixed. I agree. But I am not 100% sure why it was done this way to start with. Probably this is the logic behind that: - Max clock rate supported by a u32 is ~ 4.295 GHz - People expected that, we will not reach this rate for 32 bit systems but for 64 bit ones. - If above is true, then making it u64 for all will generate not very optimized code for 32bit systems, as we need to fetch two 32bit values everytime then. - And making it u32 for 64 bit systems wouldn't be great as well, as we need to mask out half of the read value. Ofcourse, Mike and Stephen can correct me here :) > In my patch, I assumed that if 32-bit architectures work fine today, then > we don't need more range on 64-bit architectures either. The problem here is that we haven't fixed it properly. - clock framework expects it to be unsigned long - DT is sending a 64 bit value in Hz - But we are storing and exposing it in u32 That's weird, isn't it? So, either we update clock API and other similar APIs to u64 or u32 (which may not be the right thing to do), Or we keep it unsigned long here as well and add debugfs_create_ulong().
On Thu, 8 Oct 2015, Viresh Kumar wrote: > > In my patch, I assumed that if 32-bit architectures work fine today, then > > we don't need more range on 64-bit architectures either. > > The problem here is that we haven't fixed it properly. > - clock framework expects it to be unsigned long > - DT is sending a 64 bit value in Hz > - But we are storing and exposing it in u32 > > That's weird, isn't it? > > So, either we update clock API and other similar APIs to u64 or u32 > (which may not be the right thing to do), Or we keep it unsigned long > here as well and add debugfs_create_ulong(). I don't see why debugfs can't accomodate C's builtin types, rather than insisting on predetermined sizes. After, the printf family of functions does that: %u vs. %lu. In fact, there's no way to tell printf that a particular value is 32 bits or 64 bits. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Viresh Kumar (2015-10-08 00:48:28) > On 07-10-15, 21:12, Arnd Bergmann wrote: > > I think it clearly makes sense to have a fixed length for each of these > > members: > > > > either 32 bit is enough to represent all possible values, then > > there is no need to make them 'long' on 64-bit architectures, or 32 bit > > is not enough and then the code is broken on 32-bit architectures today > > and should be fixed. > > I agree. > > But I am not 100% sure why it was done this way to start with. > Probably this is the logic behind that: > - Max clock rate supported by a u32 is ~ 4.295 GHz > - People expected that, we will not reach this rate for 32 bit systems > but for 64 bit ones. > - If above is true, then making it u64 for all will generate not very > optimized code for 32bit systems, as we need to fetch two 32bit > values everytime then. > - And making it u32 for 64 bit systems wouldn't be great as well, as > we need to mask out half of the read value. > > Ofcourse, Mike and Stephen can correct me here :) I chose unsigned long for the common clock framework _implemenation_, because the long-standing clk.h _api_ returns this type for clk_get_rate and passes this type in for clk_set_rate and clk_round_rate. > > > In my patch, I assumed that if 32-bit architectures work fine today, then > > we don't need more range on 64-bit architectures either. > > The problem here is that we haven't fixed it properly. > - clock framework expects it to be unsigned long > - DT is sending a 64 bit value in Hz > - But we are storing and exposing it in u32 Don't forget cpufreq is using unsigned int for KHz. > > That's weird, isn't it? Yes it is. > > So, either we update clock API and other similar APIs to u64 or u32 > (which may not be the right thing to do), Or we keep it unsigned long > here as well and add debugfs_create_ulong(). This comes up every now and then. I'm still trying to figure out if sub-Hertz quantities should be considered (e.g. representing freqs in milliHertz). Regards, Mike > > -- > viresh -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19, 2015 at 08:40:09AM -0700, Michael Turquette wrote: > Quoting Viresh Kumar (2015-10-08 00:48:28) > > On 07-10-15, 21:12, Arnd Bergmann wrote: > > > I think it clearly makes sense to have a fixed length for each of these > > > members: > > > > > > either 32 bit is enough to represent all possible values, then > > > there is no need to make them 'long' on 64-bit architectures, or 32 bit > > > is not enough and then the code is broken on 32-bit architectures today > > > and should be fixed. > > > > I agree. > > > > But I am not 100% sure why it was done this way to start with. > > Probably this is the logic behind that: > > - Max clock rate supported by a u32 is ~ 4.295 GHz The limit is actually half that when you consider clk_round_rate() returns a long, because clk_round_rate() needs to be able to return errors. The use of long and unsigned long was done in the knowledge that it'd restrict to 2GHz the upper limit, partially because 64-bit math on 32-bit systems is inefficient and expensive, and I initially wanted people to use the thing, so it had to be lean and fast. (If I'd started out with something as complex as the CCF, it would've been laughed out as being "far too complex" and "we don't need this"). It also started out as a way to convey clock information to drivers for peripherals, which commonly don't need clocks up in the GHz range, indeed, it was rare at that time the API was designed for peripheral clocks to be much above 100MHz.
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 25f3efef24db..1cbc0acf831a 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -696,7 +696,7 @@ static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, break; /* Duplicate OPPs */ - dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", + dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %u, volt: %u, enabled: %d. New: freq: %u, volt: %u, enabled: %d\n", __func__, opp->rate, opp->u_volt, opp->available, new_opp->rate, new_opp->u_volt, new_opp->available); @@ -898,7 +898,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) /* OPP to select on device suspend */ if (of_property_read_bool(np, "opp-suspend")) { if (dev_opp->suspend_opp) - dev_warn(dev, "%s: Multiple suspend OPPs found (%lu %lu)\n", + dev_warn(dev, "%s: Multiple suspend OPPs found (%u %u)\n", __func__, dev_opp->suspend_opp->rate, new_opp->rate); else @@ -910,7 +910,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) mutex_unlock(&dev_opp_list_lock); - pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n", + pr_debug("%s: turbo:%d rate:%u uv:%u uvmin:%u uvmax:%u latency:%u\n", __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt, new_opp->u_volt_min, new_opp->u_volt_max, new_opp->clock_latency_ns); diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c index 865cbfa24640..45436dabae8d 100644 --- a/drivers/base/power/opp/debugfs.c +++ b/drivers/base/power/opp/debugfs.c @@ -38,43 +38,42 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct device_opp *dev_opp) char name[15]; /* Rate is unique to each OPP, use it to give opp-name */ - sprintf(name, "opp:%lu", opp->rate); + sprintf(name, "opp:%u", opp->rate); /* Create per-opp directory */ d = debugfs_create_dir(name, pdentry); if (!d) return -ENOMEM; - if (!debugfs_create_bool("available", S_IRUGO, d, - (u32 *)&opp->available)) + if (!debugfs_create_bool("available", S_IRUGO, d, &opp->available)) return -ENOMEM; - if (!debugfs_create_bool("dynamic", S_IRUGO, d, (u32 *)&opp->dynamic)) + if (!debugfs_create_bool("dynamic", S_IRUGO, d, &opp->dynamic)) return -ENOMEM; - if (!debugfs_create_bool("turbo", S_IRUGO, d, (u32 *)&opp->turbo)) + if (!debugfs_create_bool("turbo", S_IRUGO, d, &opp->turbo)) return -ENOMEM; - if (!debugfs_create_u32("rate_hz", S_IRUGO, d, (u32 *)&opp->rate)) + if (!debugfs_create_u32("rate_hz", S_IRUGO, d, &opp->rate)) return -ENOMEM; if (!debugfs_create_u32("u_volt_target", S_IRUGO, d, - (u32 *)&opp->u_volt)) + &opp->u_volt)) return -ENOMEM; if (!debugfs_create_u32("u_volt_min", S_IRUGO, d, - (u32 *)&opp->u_volt_min)) + &opp->u_volt_min)) return -ENOMEM; if (!debugfs_create_u32("u_volt_max", S_IRUGO, d, - (u32 *)&opp->u_volt_max)) + &opp->u_volt_max)) return -ENOMEM; - if (!debugfs_create_u32("u_amp", S_IRUGO, d, (u32 *)&opp->u_amp)) + if (!debugfs_create_u32("u_amp", S_IRUGO, d, &opp->u_amp)) return -ENOMEM; if (!debugfs_create_u32("clock_latency_ns", S_IRUGO, d, - (u32 *)&opp->clock_latency_ns)) + &opp->clock_latency_ns)) return -ENOMEM; opp->dentry = d; diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index 0e4c27fb0c4a..ef573e650773 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -71,13 +71,13 @@ struct dev_pm_opp { bool available; bool dynamic; bool turbo; - unsigned long rate; + u32 rate; - unsigned long u_volt; - unsigned long u_volt_min; - unsigned long u_volt_max; - unsigned long u_amp; - unsigned long clock_latency_ns; + u32 u_volt; + u32 u_volt_min; + u32 u_volt_max; + u32 u_amp; + u32 clock_latency_ns; struct device_opp *dev_opp; struct rcu_head rcu_head;
The recently added debugfs support for OPP creates files using the debugfs_create_bool() and debugfs_create_u32() interfaces, but casts the data argument to u32*, which is broken on some architectures. In case of debugfs_create_bool(), the API has changed as of 621a5f7ad9cd ("debugfs: Pass bool pointer to debugfs_create_bool()"), so we now get a warning about the new interface in linux-next, which contains both patches. Removing the cast makes it work in linux-next, and makes it warn in cases where it does not work. For debugfs_create_u32(), the current usage is broken on 64-bit architectures when the values exceed the range of 32-bit variables (which should not happen), or when the kernel is built for as big-endian. This patch removes the casts and changes the types to u32 to make them match and print the correct value on all architectures. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Fixes: 5cb5fdbf38770 ("PM / OPP: Add debugfs support") --- Found while building ARM allmodconfig -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html