On Fri, 6 Dec 2019 at 16:14, Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> wrote: > > On Fri, Dec 06, 2019 at 03:26:16PM +0100, Ulf Hansson wrote: > > [...] > > > > You can merge it as it is but that's how things stand and adding > > > a comment to the *code* would help understand its logic. > > > > Okay, how about adding a comment along the lines of this: > > > > "Using the deepest state for the CPU to trigger a potential selection > > of a shared state for the domain, assumes the domain states are all > > deeper states". > > Just this it should be fine (I trimmed it a bit). Great, I add that! > > > > > So, unless I am missing your point, I think the above code does > > > > exactly what you want, no? > > > > > > > > In regards to the "arbitrary choice" of what cpuidle state to use, > > > > there are more details about why that is, in the changelog. > > > > > > > > > > > > > > This inizialization though does not belong in here, it is done at driver > > > > > level, it should not be done in this per-cpu path. IIUC the logic the > > > > > enter pointer should only be overridden if and only if all cpus managed > > > > > by the driver have a corresponding device associated. > > > > > > > > I think you have overlooked the fact that there are one cpuidle driver > > > > registered per CPU. The above doesn't make sense to me, sorry. > > > > > > You are calling psci_dt_cpu_init_idle() for every possibile cpu. > > > > > > Every time psci_dt_attach_cpu() is called, we check dev and override > > > the idle driver enter method. There is one driver, what I am saying > > > is that it is not correct to check dev and override the enter pointer > > > for *every* cpu that we try to attach to a power domain. This must > > > be done once for all by checking that *all* devices could be attached > > > to a power domain. > > > > Ah, now I think get your point. > > > > You want me to re-iterate through all the registered cpuidle drivers, > > which means one per CPU - and then override the enter callback for > > each of them, but only if all devices was successfully attached to a > > PM domain. Is that correct? > > > > My only worries with this, is that we have already registered the > > cpuidle drivers and I don't think it's a good idea to update the enter > > callbacks, beyond that point. > > > > Perhaps another option is to track whether the first CPU gets attached > > (and then update the enter callback), but after that require all the > > remaining CPUs to be attached as well - else bail out with an error > > code, failing to register all the driver instances. > > > > What do you think about that? > > I was confused - now we have one cpuidle driver per cpu so this > comment was bogus from this perspective (I was still reasoning > wit a *single* cpuidle driver across cpus. Apologies). No worries! We agreed on the way forward, so I am happy. :-) > > Sudeep will follow up on this but please forget this specific > comment - I was wrong. Alright, thanks! Does that also mean I can add your ack for the rest of the patches in the series (besides the last hotplug patch) or is there any other issues you want to raise? Have a nice weekend! Kind regards Uffe
