On Monday, July 20, 2015 01:14:41 PM Pan Xinhui wrote:
> hi, Rafael
> thanks for your reply :)
> On 2015年07月18日 08:34, Rafael J. Wysocki wrote:
> > On Tuesday, July 14, 2015 10:52:35 AM Pan Xinhui wrote:
> >> hi, Rafael,
> >> let me do more explanation :)
> >>
> >> On 2015年07月14日 10:09, Pan Xinhui wrote:
> >>> hi, Rafael,
> >>> thanks for you reply :)
> >>> On 2015年07月14日 07:26, Rafael J. Wysocki wrote:
> >>>> On Monday, July 13, 2015 02:33:08 PM Pan Xinhui wrote:
> >>>>> hi, Rafeal
> >>>>> thanks for your reply. :)
> >>>>>
> >>>>> On 2015年07月11日 04:44, Rafael J. Wysocki wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On Fri, Jul 10, 2015 at 7:50 AM, Pan Xinhui <xinhuix....@intel.com>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>> If CONFIG_X86_ACPI_CPUFREQ_CPB has not been defined, the placeholder
> >>>>>>> for
> >>>>>>> cpb is not needed. Add ifdef around it.
> >>>>>>>
> >>>>>>> Signed-off-by: Pan Xinhui <xinhuix....@intel.com>
> >>>>>>> ---
> >>>>>>> drivers/cpufreq/acpi-cpufreq.c | 2 ++
> >>>>>>> 1 file changed, 2 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/cpufreq/acpi-cpufreq.c
> >>>>>>> b/drivers/cpufreq/acpi-cpufreq.c
> >>>>>>> index e7fcaa6..314a19e 100644
> >>>>>>> --- a/drivers/cpufreq/acpi-cpufreq.c
> >>>>>>> +++ b/drivers/cpufreq/acpi-cpufreq.c
> >>>>>>> @@ -884,7 +884,9 @@ static int acpi_cpufreq_resume(struct
> >>>>>>> cpufreq_policy *policy)
> >>>>>>> static struct freq_attr *acpi_cpufreq_attr[] = {
> >>>>>>> &cpufreq_freq_attr_scaling_available_freqs,
> >>>>>>> &freqdomain_cpus,
> >>>>>>> +#ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
> >>>>>>> NULL, /* this is a placeholder for cpb, do not remove */
> >>>>>>> +#endif
> >>>>>>
> >>>>>> Adding the ifdef here doesn't change anything, because the next NULL
> >>>>>> will play the role of the one you've just #ifdefed and the structure
> >>>>>> will be filled with zeros from that point on anyway.
> >>>>>>
> >>>>> Yes, adding ifdef here does not change any binary codes. But I want to
> >>>>> make the codes more readable. :)
> >>>>> Patch author has noticed two *NULL* here would confuse people,
> >>>>> especially who first read this acpi-cpufreq.c file
> >>>>> From code style point, it would be better to have #ifdef around it.
> >>>>
> >>>> Not really.
> >>>>
> >>>> Why don't you simply drop *both* NULLs?
> >>>>
> >>> Just like string end with *NULL* :)
> >>>
> >>> 1021 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy,
> >>> 1022 struct device *dev)
> >>> 1023 {
> >>> 1024 struct freq_attr **drv_attr;
> >>> 1025 int ret = 0;
> >>> 1026
> >>> 1027 /* set up files for this cpu device */
> >>> 1028 drv_attr = cpufreq_driver->attr;
> >>> 1029 while (drv_attr && *drv_attr) {
> >>> 1030 ret = sysfs_create_file(&policy->kobj, &((*drv_attr)->attr));
> >>> 1031 if (ret)
> >>> 1032 return ret;
> >>> 1033 drv_attr++;
> >>> 1034 }
> >>> If struct freq_attr *acpi_cpufreq_attr[] did not end with NULL, line 1033
> >>> will access invalid data area.
> >>> If *drv_attr(the data after struct freq_attr * array[]) happened to be
> >>> not NULL. panic may hit in sysfs_create_file :(
> >>> So at least one *NULL* must be in the end of freq_attr *array[].
> >
> > OK, so the array is NULL-terminated and one NULL is needed to mark the end
> > of it.
> >
> >
> >>>
> >>> Actually in acpi-cpufreq.c, in acpi_cpufreq_init function.
> >>> 957 struct freq_attr **iter;
> >>> 958
> >>> 959 pr_debug("adding sysfs entry for cpb\n");
> >>> 960
> >>> 961 for (iter = acpi_cpufreq_attr; *iter != NULL; iter++)
> >>> 962 ;
> >>> 963
> >>> 964 /* make sure there is a terminator behind it */
> >>> 965 if (iter[1] == NULL)
> >>> 966 *iter = &cpb;
> >>> 967 }
> >>> line965, check of iter[1] is not needed. Maybe the patch author was
> >>> afraid of an unexpected remove of first *NULL*.
> >>> It might be a better solution to add ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
> >>> around that *NULL*, and remove this !iter[1] check.
> >
> > Ah, so that is an exceptionally ugly piece of code.
> >
> > What about the patch below?
> >
> agree, seems a little better than two-NULLs. I just have one minor question
> listed below.
>
> > ---
> > drivers/cpufreq/acpi-cpufreq.c | 21 +++++++++++----------
> > 1 file changed, 11 insertions(+), 10 deletions(-)
> >
> > Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c
> > +++ linux-pm/drivers/cpufreq/acpi-cpufreq.c
> > @@ -884,7 +884,9 @@ static int acpi_cpufreq_resume(struct cp
> > static struct freq_attr *acpi_cpufreq_attr[] = {
> > &cpufreq_freq_attr_scaling_available_freqs,
> > &freqdomain_cpus,
> > - NULL, /* this is a placeholder for cpb, do not remove */
> > +#ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
> > + &cpb,
> > +#endif
> > NULL,
> > };
> >
> such definition may hide a fact that it might be set to NULL if cpb is not
> supported.
> So if that happen, other member of this array whose index is large than cpb
> might not registered.
> for example
>
> +#ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
> + &cpb,
> +#endif
> +#ifdef CONFIG_NEW_XXXXXX
> &new_cpufreq_attrs,
> #endif
> NULL
> }
> anyway, at that time, people could work out a new solution. if they really
> have to add such new cpufreq attr. :)
Well, they just need to put their new stuff above the CPB attribute.
>
> it seems good to me. thanks for your patch :)
OK, thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
