lkml.org 
[lkml]   [2012]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V2 3/6] thermal: Add generic cpuhotplug cooling implementation
    On 03/19/2012 11:47 AM, Amit Daniel Kachhap wrote:

    > This patch adds support for generic cpu thermal cooling low level
    > implementations using cpuhotplug based on the thermal level requested
    > from user. Different cpu related cooling devices can be registered by the
    > user and the binding of these cooling devices to the corresponding
    > trip points can be easily done as the registration APIs return the
    > cooling device pointer. The user of these APIs are responsible for
    > passing the cpumask.
    >


    I am really not aware of the cpu thermal cooling stuff, but since this patch
    deals with CPU Hotplug (which I am interested in), I have some questions
    below..


    > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
    > +
    > +static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
    > + unsigned long *state)
    > +{
    > + int ret = -EINVAL;
    > + struct hotplug_cooling_device *hotplug_dev;
    > +
    > + mutex_lock(&cooling_cpuhotplug_lock);
    > + list_for_each_entry(hotplug_dev, &cooling_cpuhotplug_list, node) {
    > + if (hotplug_dev && hotplug_dev->cool_dev == cdev) {
    > + *state = hotplug_dev->hotplug_state;
    > + ret = 0;
    > + break;
    > + }
    > + }
    > + mutex_unlock(&cooling_cpuhotplug_lock);
    > + return ret;
    > +}
    > +
    > +/*This cooling may be as ACTIVE type*/
    > +static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
    > + unsigned long state)
    > +{
    > + int cpuid, this_cpu = smp_processor_id();


    What prevents this task from being migrated to another CPU?
    IOW, what ensures that 'this_cpu' remains valid throughout this function?

    I see that you are acquiring mutex locks below.. So this is definitely not
    a preempt disabled section.. so I guess my question above is valid..

    Or is this code bound to a particular cpu?

    > + struct hotplug_cooling_device *hotplug_dev;
    > +
    > + mutex_lock(&cooling_cpuhotplug_lock);
    > + list_for_each_entry(hotplug_dev, &cooling_cpuhotplug_list, node)
    > + if (hotplug_dev && hotplug_dev->cool_dev == cdev)
    > + break;
    > +
    > + mutex_unlock(&cooling_cpuhotplug_lock);
    > + if (!hotplug_dev || hotplug_dev->cool_dev != cdev)
    > + return -EINVAL;
    > +
    > + if (hotplug_dev->hotplug_state == state)
    > + return 0;
    > +
    > + /*
    > + * This cooling device may be of type ACTIVE, so state field can
    > + * be 0 or 1
    > + */
    > + if (state == 1) {
    > + for_each_cpu(cpuid, hotplug_dev->allowed_cpus) {
    > + if (cpu_online(cpuid) && (cpuid != this_cpu))


    What prevents the cpu numbered cpuid from being taken down right at this moment?
    Don't you need explicit synchronization with CPU Hotplug using something like
    get_online_cpus()/put_online_cpus() here?

    > + cpu_down(cpuid);
    > + }
    > + } else if (state == 0) {
    > + for_each_cpu(cpuid, hotplug_dev->allowed_cpus) {
    > + if (!cpu_online(cpuid) && (cpuid != this_cpu))


    Same here.

    > + cpu_up(cpuid);
    > + }
    > + } else {
    > + return -EINVAL;
    > + }
    > +
    > + hotplug_dev->hotplug_state = state;
    > +
    > + return 0;
    > +}
    > +/* bind hotplug callbacks to cpu hotplug cooling device */
    > +static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
    > + .get_max_state = cpuhotplug_get_max_state,
    > + .get_cur_state = cpuhotplug_get_cur_state,
    > + .set_cur_state = cpuhotplug_set_cur_state,
    > +};
    > +



    \
     
     \ /
      Last update: 2012-03-19 12:49    [W:0.025 / U:151.744 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site