| Message ID | 78187ea173460c871eef31432ec2a80ec657fe30.1395643393.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Viresh, > Ideally, .driver_data field of struct cpufreq_frequency_table must > not be used by core at all. But during a recent change if its value > is same as CPUFREQ_BOOST_FREQ macro, then it is treated specially by > core. > > The value of this macro was set to ~2 earlier, i.e. 0xFFFFFFFD. In > case some driver is using this field for its own data and sets this > field to -3, then with two's complement that value will also become > 0xFFFFFFFD. > > To fix this issue, lets change value of this flag to a very uncommon > value which shouldn't be used by any driver unless they want to use > BOOST feature. > > Along with this update comments to make this more clear. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > > Gautham/Vaidy: I hope this fixes the problem we discussed for your > patchset. > I think that a name of the patchset, for which the current setting causes problem would be welcome here. Otherwise, I can only ask why it is not possible to add #define SPECIAL_SETTING ~3 ? > include/linux/cpufreq.h | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index c48e595..9f25d9d 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -455,12 +455,18 @@ extern struct cpufreq_governor > cpufreq_gov_conservative; > * FREQUENCY TABLE HELPERS > * > *********************************************************************/ > +/* Special Values of .frequency field */ > #define CPUFREQ_ENTRY_INVALID ~0 > #define CPUFREQ_TABLE_END ~1 > -#define CPUFREQ_BOOST_FREQ ~2 > +/* Special Values of .driver_data field */ > +#define CPUFREQ_BOOST_FREQ 0xABABABAB > > struct cpufreq_frequency_table { > - unsigned int driver_data; /* driver specific data, > not used by core */ > + /* > + * driver specific data, not used by core unless it is set to > + * CPUFREQ_BOOST_FREQ. > + */ > + unsigned int driver_data; > unsigned int frequency; /* kHz - doesn't need to be > in ascending > * order */ > };
On 24 March 2014 13:51, Lukasz Majewski <l.majewski@samsung.com> wrote: > I think that a name of the patchset, for which the current setting > causes problem would be welcome here. I couldn't find a lkml link to the exact message earlier, but it was discussed here: https://patchwork.kernel.org/patch/3865481/ > Otherwise, I can only ask why it is not possible to add > #define SPECIAL_SETTING ~3 ? Driver wants to use this field for +ve and -ve values and it should be free to do so.... And ~2 was creating problems when he used -3 for this field.. If we make it ~3, then he will face problem for -4.. So, I used a special value 0xABABABAB here whose integer value is: -1414812757 The only other option we have is to add another field "bool boost" in cpufreq_frequency_table, which is the last thing I wanted to do. And that's the point Gautham was trying to make in one of our IRC chats as well.. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, Mar 24, 2014 at 12:18:14PM +0530, Viresh Kumar wrote: > Ideally, .driver_data field of struct cpufreq_frequency_table must not be used > by core at all. But during a recent change if its value is same as > CPUFREQ_BOOST_FREQ macro, then it is treated specially by core. > > The value of this macro was set to ~2 earlier, i.e. 0xFFFFFFFD. In case some > driver is using this field for its own data and sets this field to -3, then with > two's complement that value will also become 0xFFFFFFFD. > > To fix this issue, lets change value of this flag to a very uncommon value which > shouldn't be used by any driver unless they want to use BOOST > feature > > Along with this update comments to make this more clear. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > > Gautham/Vaidy: I hope this fixes the problem we discussed for your > patchset. Since the type of .driver_data is "unsigned int", the cpufreq core seems to be assuming that the value cannot be -ve. So, drivers should be storing -ve values in these fields at their own risk. Because apart from determining whether the corresponding frequency is a boost frequency or not, the cpufreq core seems to be using the .driver_data field in pr_debugs(). The value of .driver_data formatted as "%u" is not useful in these pr_debugs, if the driver stores -ve value in this field. On the other hand, if we change the type of .driver_data to "int" then restricting the driver to not use specific values is unreasonable since .driver_data field is supposed to be private to the driver and the core is not supposed to intepret it. In which case we should be having a separate field for determining if the frequency is a BOOST frequency or not. So while it fixes the problem for us, I don't think this patch fixes the problem in general for the reasons mentioned above. -- Thanks and Regards gautham. > > include/linux/cpufreq.h | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index c48e595..9f25d9d 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -455,12 +455,18 @@ extern struct cpufreq_governor cpufreq_gov_conservative; > * FREQUENCY TABLE HELPERS * > *********************************************************************/ > > +/* Special Values of .frequency field */ > #define CPUFREQ_ENTRY_INVALID ~0 > #define CPUFREQ_TABLE_END ~1 > -#define CPUFREQ_BOOST_FREQ ~2 > +/* Special Values of .driver_data field */ > +#define CPUFREQ_BOOST_FREQ 0xABABABAB > > struct cpufreq_frequency_table { > - unsigned int driver_data; /* driver specific data, not used by core */ > + /* > + * driver specific data, not used by core unless it is set to > + * CPUFREQ_BOOST_FREQ. > + */ > + unsigned int driver_data; > unsigned int frequency; /* kHz - doesn't need to be in ascending > * order */ > }; > -- > 1.7.12.rc2.18.g61b472e > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 24 March 2014 12:18, Viresh Kumar <viresh.kumar@linaro.org> wrote: > Ideally, .driver_data field of struct cpufreq_frequency_table must not be used > by core at all. But during a recent change if its value is same as > CPUFREQ_BOOST_FREQ macro, then it is treated specially by core. > > The value of this macro was set to ~2 earlier, i.e. 0xFFFFFFFD. In case some > driver is using this field for its own data and sets this field to -3, then with > two's complement that value will also become 0xFFFFFFFD. > > To fix this issue, lets change value of this flag to a very uncommon value which > shouldn't be used by any driver unless they want to use BOOST feature. > > Along with this update comments to make this more clear. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- Please discard this patch now as I have sent a replacement patch. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index c48e595..9f25d9d 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -455,12 +455,18 @@ extern struct cpufreq_governor cpufreq_gov_conservative; * FREQUENCY TABLE HELPERS * *********************************************************************/ +/* Special Values of .frequency field */ #define CPUFREQ_ENTRY_INVALID ~0 #define CPUFREQ_TABLE_END ~1 -#define CPUFREQ_BOOST_FREQ ~2 +/* Special Values of .driver_data field */ +#define CPUFREQ_BOOST_FREQ 0xABABABAB struct cpufreq_frequency_table { - unsigned int driver_data; /* driver specific data, not used by core */ + /* + * driver specific data, not used by core unless it is set to + * CPUFREQ_BOOST_FREQ. + */ + unsigned int driver_data; unsigned int frequency; /* kHz - doesn't need to be in ascending * order */ };
Ideally, .driver_data field of struct cpufreq_frequency_table must not be used by core at all. But during a recent change if its value is same as CPUFREQ_BOOST_FREQ macro, then it is treated specially by core. The value of this macro was set to ~2 earlier, i.e. 0xFFFFFFFD. In case some driver is using this field for its own data and sets this field to -3, then with two's complement that value will also become 0xFFFFFFFD. To fix this issue, lets change value of this flag to a very uncommon value which shouldn't be used by any driver unless they want to use BOOST feature. Along with this update comments to make this more clear. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- Gautham/Vaidy: I hope this fixes the problem we discussed for your patchset. include/linux/cpufreq.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)