Re: [PATCH 1/5] Thermal: do bind operation after thermal zone orcooling device register returns.

[Francesco Lavra]

Hi,
On 10/23/2012 10:23 AM, Hongbo Zhang wrote:
> Hi Francesco,
> I found out more points about this issue.
> 
> [1]
> cdev should be ready when get_max_state callback be called, otherwise
> parameter cdev is useless, imagine there may be cases that
> get_max_state call back is shared by more than one cooling devices of
> same kind, like this:
> common_get_max_state(*cdev, *state)
> {
>     if (cdev == cdev1)  *state = 3;
>     else if (cdev == cdev)  *state = 5;
>     else
> }
> 
> [2]
> In my cpufreq cooling case(in fact cdev is not used to calculate
> max_state), the cooling_cpufreq_list should be ready when
> get_max_state call back is called. In this patch I defer the binding
> when registration finished, but this is not perfect now, see this
> routine in cpufreq_cooling_register:
> 
> thermal_cooling_device_register;
> at this time: thermal_bind_work -> get_max_state -> get NULL
> cooling_cpufreq_list
> and then: list_add_tail(&cpufreq_dev->node, &cooling_cpufreq_list)
> This is due to we cannot know exactly when the bind work is executed.
> (and this can be fixed by moving mutex_lock(&cooling_cpufreq_lock)
> aheadof thermal_cooling_device_register and other corresponding
> modifications, but I found another way as below)
> 
> [3]
> Root cause of this problem is calling get_max_state in register -> bind routine.
> Better solution is to add another parameter in cooling device register
> function, also add a max_state member in struct cdev, like:
> thermal_cooling_device_register(char *type, void *devdata, const
> struct thermal_cooling_device_ops *ops, unsigned long max_state)
> and then in the bind function:
> if(cdev->max_state)
>     max_state = cdev->max_state;
> else
>     cdev->get_max_state(cdev, &max_state)
> 
> It is common sense that the cooling driver should know its cooling
> max_state(ability) before registration, and it can be offered when
> register.
> I think this way doesn't change both thermal and cooling layer much,
> it is more clear.
> I will update this patch soon.

I still believe the thermal layer doesn't need any change to work around
this problem, and I still believe that when a cooling device is being
registered, all of its ops should be fully functional.
The problem with the cpufreq cooling device driver is that its callbacks
use the internal list of devices to retrieve the struct
cpufreq_cooling_device instance corresponding to a given struct
thermal_cooling_device. This is not necessary, because the struct
thermal_cooling_device has a private data pointer (devdata) which in
this case is exactly a reference to the struct cpufreq_cooling_device
instance the callbacks are looking for. In fact, I think the
cooling_cpufreq_list is not necessary at all, and should be removed from
the cpufreq cooling driver.
So the 3 callbacks, instead of iterating through the device list, should
have something like:
struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
That would do the trick.

--
Francesco
--
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/