Re: [PATCH v2 2/2] cpufreq: powernv: Register for OCC related opal_message notification

[Viresh Kumar]

On 28 April 2015 at 13:48, Shilpasri G Bhat
<shilpa.bhat@xxxxxxxxxxxxxxxxxx> wrote:
> My bad I haven't added explicit comment to state reason behind this change.
>
> I modified the definition of *throttle_check() to match the function definition
> to be called via smp_call() instead of adding an additional wrapper around
> *throttle_check().
>
> OCC is a chip entity and any local throttle state changes should be associated
> to cpus belonging to that chip. The *throttle_check() will read the core
> register PMSR to verify throttling. All the cores in a chip will have the same
> throttled state as they are managed by a the same OCC in that chip.
>
> smp_call() is required to ensure *throttle_check() is called on a cpu belonging
> to the chip for which we have received throttled/unthrottled notification. We
> could be handling throttled/unthrottled notification of 'chip1' in 'chip2' so do
> an smp_call() on 'chip1'.

Okay. Lets talk about the code that is already present in mainline. Isn't that
suffering from this issue ? If yes, then you need to bugfix that separately.

> We are irq_disabled in powernv_cpufreq_occ_msg() the notification handler.
> Thus the use of kworker to do an smp_call and restore policy->cur.
>
> OCC_RESET is global event it affects frequency of all chips. Pmax capping is
> local event, it affects the frequency of a chip.
>

>> That's a lot. I am not an expert here and so really can't comment on
>> the internals of ppc. But, is it patch solving a single problem ? I don't
>> know, I somehow got the impression that it can be split into multiple
>> (smaller & review-able) patches. Only if it makes sense. Your call.
>
> All the changes introduced in this patch is centered around opal_message
> notification handler powernv_cpufreq_occ_msg(). I can split it into multiple
> patches but it all will be relevant only to solve the above problem.

And that's what I meant here. Yes, this all is solving a central problem, but
a patch must be divided into separate, independently working, entities.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/