Re: How to facilitate the cpuidle drivers to go to the same direction (Was: Re: [PATCH 4/9] ARM: OMAP4: cpuidle: fix wrong driver initialization)

[Deepthi Dharwar <deepthi-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>]

On 03/29/2013 09:20 PM, Daniel Lezcano wrote:
> On 03/29/2013 04:10 PM, Santosh Shilimkar wrote:
>> On Friday 29 March 2013 06:20 PM, Amit Kucheria wrote:
>>> On Fri, Mar 29, 2013 at 5:50 PM, Santosh Shilimkar
>>> <santosh.shilimkar-l0cyMroinI0@public.gmane.org> wrote:
>>>> On Friday 29 March 2013 05:26 PM, Amit Kucheria wrote:
>>>>> On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano
>>>>> <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>> On 03/29/2013 11:38 AM, Santosh Shilimkar wrote:
>>>>>>> On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
>>>>>>>> The driver is initialized several times. This is wrong and if the
>>>>>>>> return code of the function was checked, it will return -EINVAL.
>>>>>>>>
>>>>>>>> Move this initialization out of the loop.
>>>>>>>>
>>>>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>>>> ---
>>>>>>> Fix for this is already and v2 of the patch is here [1]
>>>>>>
>>>>>> Ah, ok. Thanks for reviewing the patch.
>>>>>>
>>>>>> Can we find a solution to have a single entry point to sumbit patches
>>>>>> for all the cpuidle drivers ?
>>>>>>
>>>>>> Otherwise, consolidating them is a pain: a patch for the samsung tree,
>>>>>> another one for the at91 tree, etc ... and wait for all the trees to
>>>>>> sync before continuing to consolidate the code.
>>>>>>
>>>>>> Wouldn't be worth to move these drivers under the PM umbrella instead of
>>>>>> the SoC specific code ?
>>>>>>
>>>>>> Any idea to simplify the cpuidle consolidation and maintenance ?
>>>>>
>>>>> Adding Arnd and Olof to this discussion since atleast the ARM drivers
>>>>> go through their arm-soc tree.
>>>>>
>>>>> Given the work you're putting in to consolidate the drivers, perhaps
>>>>> they can insist that idle drivers get acked by you?
>>>>>
>>>> Not to create controversy but as a general rule there is nothing
>>>> like *insisting* ack on patches for merge apart from the official
>>>> maintainers(gate keepers).
>>>>
>>>> Having said that, its always good to get more reviews and acks so
>>>> that better code gets merged.
>>>>
>>>> This just my personal opinion.
>>>
>>> I'm not asking for special treatment here. :) I'm requesting one set
>>> of maintainers (arm-soc maintainers) to push back on changes that
>>> don't get platform idle drivers in sync with the consolidation work
>>> that is currently ongoing.
>>>
>>> This will speed up the process since it is hard to track every
>>> SoC-specific list for these changes. Some platform maintainers might
>>> not even be aware of it (those that Daniel hasn't modified yet). A
>>> similar approach seems to have worked for common clock, DT, pinmux,
>>> etc.
>>>
>> Every patch gets pulled into arm-soc/arm-core has to be posted on
>> LAKML. So as long as everybody follows that rule, there is no need to
>> track every SoC lists. And what I have seen so far every this rule
>> has been followed well.
> 
> (Added Benjamin, Deepthi and Paul)
> 
> I don't think everybody is following this rule, patches go to the SoC
> maintainer's tree without sometimes going through lakml.
> 
> Furthermore, there is not only ARM, there is also acpi_idle, intel_idle,
> pseries_idle and sh_idle, respectively located in:
> 
> drivers/acpi/processor_idle.c
> drivers/acpi/processor_driver.c
> drivers/idle/intel_idle.c
> 
> These ones above are under linux-pm, that is Rafael, like cpuidle, even
> if it is not marked in the MAINTAINERS file, so that should be ok.
> 
> And there is also:
> 
> arch/sh/kernel/cpu/shmobile/cpuidle.c
> arch/powerpc/platforms/pseries/processor_idle.c
> 
> And hopefully, some others in the right place, calxeda_idle and
> kirwood_idle located in drivers/cpuidle.
> 
> In the maintainer file, there is no information about cpuidle at all.
> 
> For example, if someone modify the cpuidle framework allowing to
> consolidate the code across the different drivers, we have to wait for
> the merge before using the new api into the different drivers.
> If we follow strictly the path of the merge tree we fall into a scenario
> where consolidating the drivers takes a loooooong time.
> 
> From my POV, *all* the cpuidle drivers must go under drivers/cpuidle,
> like cpufreq and pass through a single entry point to apply the patches,
> so the cpuidle framework and the drivers are always synced.

I can very much relate to the above mentioned problem, where code
modifications done as a part of cpuidle framework needs to be
reflected in every arch back-end cpuidle driver.
We do have added advantages in moving
all the back-end drivers into drivers/cpuidle.
This would help us achieve better reviews, easier consolidations
and more importantly maintaining sync btw drivers and framework and
the up-streaming process.

But then, this means we get all the
arch specific code out under drivers/cpuidle
which can be very messy. Also instances where the changes
are specifically tied only to the  architecture of the back-end driver
(SoC specific), it is absolutely necessary to get SoC maintainer's review.

Cheers,
Deepthi

> If everyone agree and we reach this consensus, then we can work to move
> these drivers to a single place.
> 
> I think Amit was suggesting to Cc me in the meantime, so while we are
> moving these drivers to this place, I can help to ensure we go to the
> same direction.
> 
> For example, Arnd Cc'ed me about the zynq cpuidle driver when it has
> been posted and, after review, it appeared it was totally obsolete wrt
> the modifications we did this year.
> 
> I propose first to add an entry in MAINTAINERS:
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4cf5fd3..5b5ab87 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2206,6 +2206,15 @@ S:       Maintained
>  F:     drivers/cpufreq/
>  F:     include/linux/cpufreq.h
> 
> +CPU IDLE DRIVERS
> +M:      Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> +L:      cpuidle-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +L:      linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S:      Maintained
> +F:      drivers/cpuidle/
> +F:      include/linux/cpuidle.h
> +
>  CPUID/MSR DRIVER
>  M:     "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
>  S:     Maintained
> 
> Does it make sense ?
> 
> Thanks
>   -- Daniel
> 
>