* [BUGFIX 2/2] PCI/PM: Resume device before shutdown
From: Huang Ying @ 2012-10-24 6:54 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki, Huang Ying
In-Reply-To: <1351061654-8339-1-git-send-email-ying.huang@intel.com>
Some actions during shutdown need device to be in D0 state, such as
MSI shutdown etc, so resume device before shutdown.
Signed-off-by: Huang Ying <ying.huang@intel.com>
---
drivers/pci/pci-driver.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -398,6 +398,8 @@ static void pci_device_shutdown(struct d
struct pci_dev *pci_dev = to_pci_dev(dev);
struct pci_driver *drv = pci_dev->driver;
+ pm_runtime_resume(dev);
+
if (drv && drv->shutdown)
drv->shutdown(pci_dev);
pci_msi_shutdown(pci_dev);
@@ -408,16 +410,6 @@ static void pci_device_shutdown(struct d
* continue to do DMA
*/
pci_disable_device(pci_dev);
-
- /*
- * Devices may be enabled to wake up by runtime PM, but they need not
- * be supposed to wake up the system from its "power off" state (e.g.
- * ACPI S5). Therefore disable wakeup for all devices that aren't
- * supposed to wake up the system at this point. The state argument
- * will be ignored by pci_enable_wake().
- */
- if (!device_may_wakeup(dev))
- pci_enable_wake(pci_dev, PCI_UNKNOWN, false);
}
#ifdef CONFIG_PM
^ permalink raw reply
* Re: drivers/cpufreq/cpufreq_governor.c:46:53: sparse: incorrect type in argument 2 (different modifiers)
From: Viresh Kumar @ 2012-10-24 6:35 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Fengguang Wu, linux-pm, Lists linaro-dev
In-Reply-To: <1574211.OHQpWS6lzF@vostro.rjw.lan>
On 24 October 2012 06:03, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Tuesday 23 of October 2012 10:55:51 Viresh Kumar wrote:
>> @Rafael: Most of these are due to mixed use of u64 and cputime64_t. Both of
>> which are u64 if i am not wrong. Any specific reason that we used cputime64_t
>> instead of u64? Or can i make everything u64 instead in governors?
>
> Well, there might be a reason in the past, but I'm not sure it's still there. :-)
>
> I _think_ you can use u64 everywhere. It would be more correct than the current
> situation anyway.
Great. I will send another set for these warnings..
--
viresh
^ permalink raw reply
* Re: [PATCH 2/2] cpufreq: governors: remove redundant code
From: Viresh Kumar @ 2012-10-24 6:07 UTC (permalink / raw)
To: rjw, cpufreq, linux-pm, linux-kernel, linux-arm-kernel,
linaro-dev, patches, pdsw-power-team, arvind.chauhan
In-Reply-To: <CAKohpo=Jsy9az4PgJ-mkq_U49EOpHGES+-DaROanCOTn-L2s3Q@mail.gmail.com>
On 22 October 2012 14:16, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 20 October 2012 01:42, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> Initially ondemand governor was written and then using its code conservative
>> governor is written. It used a lot of code from ondemand governor, but copy of
>> code was created instead of using the same routines from both governors. Which
>> increased code redundancy, which is difficult to manage.
>>
>> This patch is an attempt to move common part of both the governors to
>> cpufreq_governor.c file to come over above mentioned issues.
>>
>> This shouldn't change anything from functionality point of view.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
For everybody else, this patch is already pushed by Rafael in his linux-next
branch.
--
viresh
^ permalink raw reply
* Re: [PATCH 5/5] Thermal: Add ST-Ericsson db8500 thermal dirver.
From: Hongbo Zhang @ 2012-10-24 4:40 UTC (permalink / raw)
To: Francesco Lavra
Cc: linaro-dev, linux-kernel, linux-pm, STEricsson_nomadik_linux,
kernel, linaro-kernel, hongbo.zhang, patches
In-Reply-To: <508595A0.4040604@gmail.com>
On 23 October 2012 02:51, Francesco Lavra <francescolavra.fl@gmail.com> wrote:
> On 10/22/2012 02:02 PM, Hongbo Zhang wrote:
> [...]
>>>> +static irqreturn_t prcmu_low_irq_handler(int irq, void *irq_data)
>>>> +{
>>>> + struct db8500_thermal_zone *pzone = irq_data;
>>>> + struct db8500_thsens_platform_data *ptrips;
>>>> + unsigned long next_low, next_high;
>>>> + unsigned int idx;
>>>> +
>>>> + ptrips = pzone->trip_tab;
>>>> + idx = pzone->cur_index;
>>>> + if (unlikely(idx == 0))
>>>> + /* Meaningless for thermal management, ignoring it */
>>>> + return IRQ_HANDLED;
>>>> +
>>>> + if (idx == 1) {
>>>> + next_high = ptrips->trip_points[0].temp;
>>>> + next_low = PRCMU_DEFAULT_LOW_TEMP;
>>>> + } else {
>>>> + next_high = ptrips->trip_points[idx-1].temp;
>>>> + next_low = ptrips->trip_points[idx-2].temp;
>>>> + }
>>>> +
>>>> + pzone->cur_index -= 1;
>>>> + pzone->cur_temp_pseudo = (next_high + next_low)/2;
>>>> +
>>>> + prcmu_stop_temp_sense();
>>>> + prcmu_config_hotmon((u8)(next_low/1000), (u8)(next_high/1000));
>>>> + prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
>>>> +
>>>> + pr_debug("PRCMU set max %ld, set min %ld\n", next_high, next_low);
>>>> +
>>>> + pzone->trend = THERMAL_TREND_DROPPING;
>>>> + schedule_work(&pzone->therm_work);
>>>> +
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static irqreturn_t prcmu_high_irq_handler(int irq, void *irq_data)
>>>> +{
>>>> + struct db8500_thermal_zone *pzone = irq_data;
>>>> + struct db8500_thsens_platform_data *ptrips;
>>>> + unsigned long next_low, next_high;
>>>> + unsigned int idx;
>>>> +
>>>> + ptrips = pzone->trip_tab;
>>>> + idx = pzone->cur_index;
>>>> +
>>>> + if (idx < ptrips->num_trips - 1) {
>>>> + next_high = ptrips->trip_points[idx+1].temp;
>>>> + next_low = ptrips->trip_points[idx].temp;
>>>> +
>>>> + pzone->cur_index += 1;
>>>> + pzone->cur_temp_pseudo = (next_high + next_low)/2;
>>>> +
>>>> + prcmu_stop_temp_sense();
>>>> + prcmu_config_hotmon((u8)(next_low/1000), (u8)(next_high/1000));
>>>> + prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
>>>> +
>>>> + pr_debug("PRCMU set max %ld, min %ld\n", next_high, next_low);
>>>> + }
>>>> +
>>>> + if (idx == ptrips->num_trips - 1)
>>>
>>> } else if ()
>> There is no else condition here, because it it the highest critical
>> trip point, system will be shut down in thermal_work.
>> But I'd like to add some comments here to state this situation.
>
> I didn't mean adding a new else condition, what I meant is that if the
> first condition (idx < ptrips->num_trips - 1) is verified, then there is
> no need to check for the second condition (idx == ptrips->num_trips -
> 1). So I was thinking of changing the code to:
>
> if (idx < ptrips->num_trips - 1)
> ...
> else if (idx == ptrips->num_trips - 1)
> ...
>
> Sorry if I wasn't clear.
Got it, thanks.
>
>>>
>>>> + pzone->cur_temp_pseudo = ptrips->trip_points[idx].temp + 1;
>>>> +
>>>> + pzone->trend = THERMAL_TREND_RAISING;
>>>> + schedule_work(&pzone->therm_work);
>>>> +
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static void db8500_thermal_work(struct work_struct *work)
>>>> +{
>>>> + enum thermal_device_mode cur_mode;
>>>> + struct db8500_thermal_zone *pzone;
>>>> +
>>>> + pzone = container_of(work, struct db8500_thermal_zone, therm_work);
>>>> +
>>>> + mutex_lock(&pzone->th_lock);
>>>> + cur_mode = pzone->mode;
>>>> + mutex_unlock(&pzone->th_lock);
>>>> +
>>>> + if (cur_mode == THERMAL_DEVICE_DISABLED) {
>>>> + pr_warn("Warning: thermal function disabled.\n");
>>>> + return;
>>>> + }
>>>> +
>>>> + thermal_zone_device_update(pzone->therm_dev);
>>>> + pr_debug("db8500_thermal_work finished.\n");
>>>> +}
>>>> +
>>>> +static int __devinit db8500_thermal_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct db8500_thermal_zone *pzone = NULL;
>>>> + struct db8500_thsens_platform_data *ptrips;
>>>> + int low_irq, high_irq, ret = 0;
>>>> + unsigned long dft_low, dft_high;
>>>> +
>>>> + pzone = devm_kzalloc(&pdev->dev,
>>>> + sizeof(struct db8500_thermal_zone), GFP_KERNEL);
>>>> + if (!pzone)
>>>> + return -ENOMEM;
>>>> +
>>>> + pzone->thsens_pdev = pdev;
>>>> +
>>>> + low_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_LOW");
>>>> + if (low_irq < 0) {
>>>> + pr_err("Get IRQ_HOTMON_LOW failed.\n");
>>>> + return low_irq;
>>>> + }
>>>> +
>>>> + ret = devm_request_threaded_irq(&pdev->dev, low_irq, NULL,
>>>> + prcmu_low_irq_handler,
>>>> + IRQF_NO_SUSPEND | IRQF_ONESHOT, "dbx500_temp_low", pzone);
>>>> + if (ret < 0) {
>>>> + pr_err("Failed to allocate temp low irq.\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + high_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_HIGH");
>>>> + if (high_irq < 0) {
>>>> + pr_err("Get IRQ_HOTMON_HIGH failed.\n");
>>>> + return high_irq;
>>>> + }
>>>> +
>>>> + ret = devm_request_threaded_irq(&pdev->dev, high_irq, NULL,
>>>> + prcmu_high_irq_handler,
>>>> + IRQF_NO_SUSPEND | IRQF_ONESHOT, "dbx500_temp_high", pzone);
>>>> + if (ret < 0) {
>>>> + pr_err("Failed to allocate temp high irq.\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + pzone->low_irq = low_irq;
>>>> + pzone->high_irq = high_irq;
>>>> +
>>>> + pzone->mode = THERMAL_DEVICE_DISABLED;
>>>> +
>>>> + mutex_init(&pzone->th_lock);
>>>> +
>>>> + INIT_WORK(&pzone->therm_work, db8500_thermal_work);
>>>> +
>>>> + ptrips = pdev->dev.platform_data;
>>>> + pzone->trip_tab = ptrips;
>>>> +
>>>> + pzone->therm_dev = thermal_zone_device_register("db8500_thermal_zone",
>>>> + ptrips->num_trips, 0, pzone, &thdev_ops, 0, 0);
>>>> +
>>>> + if (IS_ERR(pzone->therm_dev)) {
>>>> + pr_err("Failed to register thermal zone device\n");
>>>> + return PTR_ERR(pzone->therm_dev);
>>>> + }
>>>> +
>>>> + dft_low = PRCMU_DEFAULT_LOW_TEMP;
>>>> + dft_high = ptrips->trip_points[0].temp;
>>>> +
>>>> + prcmu_stop_temp_sense();
>>>> + prcmu_config_hotmon((u8)(dft_low/1000), (u8)(dft_high/1000));
>>>> + prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
>>>> +
>>>> + pzone->cur_index = 0;
>>>> + pzone->cur_temp_pseudo = (dft_low + dft_high)/2;
>>>> + pzone->trend = THERMAL_TREND_STABLE;
>>>
>>> All the stuff from prcmu_stop_temp_sense() up to this line can race with
>>> the irq handlers, I would suggest doing it before requesting the irqs.
>>>
>>>> + pzone->mode = THERMAL_DEVICE_ENABLED;
>>>
>>> Shouldn't this be protected by pzone->th_lock? Otherwise it should be
>>> set before the thermal zone is registered.
>>>
>>>> +
>>>> + platform_set_drvdata(pdev, pzone);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int __devexit db8500_thermal_remove(struct platform_device *pdev)
>>>> +{
>>>> + struct db8500_thermal_zone *pzone;
>>>> + pzone = platform_get_drvdata(pdev);
>>>> +
>>>> + cancel_work_sync(&pzone->therm_work);
>>>> +
>>>> + if (pzone->therm_dev)
>>>> + thermal_zone_device_unregister(pzone->therm_dev);
>>>> +
>>>> + return 0;
>>>> +}
>>>
>>> mutex_destroy() should be called on pzone->th_lock
>>>
>>>> +
>>>> +static int db8500_thermal_suspend(struct platform_device *pdev,
>>>> + pm_message_t state)
>>>> +{
>>>> + struct db8500_thermal_zone *pzone;
>>>> + pzone = platform_get_drvdata(pdev);
>>>> +
>>>> + flush_work_sync(&pzone->therm_work);
>>>> + prcmu_stop_temp_sense();
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int db8500_thermal_resume(struct platform_device *pdev)
>>>> +{
>>>> + struct db8500_thermal_zone *pzone;
>>>> + struct db8500_thsens_platform_data *ptrips;
>>>> + unsigned long dft_low, dft_high;
>>>> +
>>>> + pzone = platform_get_drvdata(pdev);
>>>> + ptrips = pzone->trip_tab;
>>>> + dft_low = PRCMU_DEFAULT_LOW_TEMP;
>>>> + dft_high = ptrips->trip_points[0].temp;
>>>> +
>>>> + prcmu_config_hotmon((u8)(dft_low/1000), (u8)(dft_high/1000));
>>>> + prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
>>>
>>> Shouldn't cur_index and cur_temp_pseudo be updated as well? You may want
>>> to define a helper function with all the code shared by irq handlers
>>> (both high and low), probe and resume.
>> No, they cannot be update because we don't know the actual current
>> temp[1] after short or long time suspend, everything goes as
>> beginning.
>
> That's what I wanted to say, if everything is reset to a default value,
> then cur_index and cur_temp should be reset too, as it's done in the
> probe function. Otherwise you may have a current pseudo-temp and a
> current index unrelated to how the hotmon is configured.
Yes, right.
>
>> If a helper function is introduced, it can be only used in probe and
>> resume I think, different and a bit complicated algorithm in irq
>> handlers.
>
> I was thinking about a function which takes the new index and the new
> low and high parameters, and updates cur_index and cur_pseudo_temp and
> does the prcmu stuff. It seems to me this is (or should be) common to
> all the 4 functions.
I misunderstood this helper will do all the updates, but algorithm of
cur_index is different everywhere.
If cur_index is calculated outside this new helper function, it works.
I will do that.
>
>> [1] due to lack of corresponding interface, search "TODO" in this file
>> to get more explanation.
>
> --
> Francesco
^ permalink raw reply
* Re: [PATCH 1/5] Thermal: do bind operation after thermal zone or cooling device register returns.
From: Hongbo Zhang @ 2012-10-24 2:37 UTC (permalink / raw)
To: Francesco Lavra
Cc: linaro-dev, linux-kernel, linux-pm, STEricsson_nomadik_linux,
kernel, linaro-kernel, hongbo.zhang, patches, amit.kachhap
In-Reply-To: <50871695.2040904@gmail.com>
On 24 October 2012 06:13, Francesco Lavra <francescolavra.fl@gmail.com> wrote:
> 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.
Hi Francesco,
When I found out this issue, I was hesitating to select the best
solution among several ideas.
It is clear now after talk with you, I will send patch for cpufreq
cooling layer.
Thank you.
>
> --
> Francesco
^ permalink raw reply
* RE: [PATCH] cpuidle: fix warning when compile
From: Song, Youquan @ 2012-10-24 2:14 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Wu, Fengguang, yuanhan.liu@linux.intel.com, Rik van Riel,
linux-pm@vger.kernel.org, Youquan Song, Song, Youquan
In-Reply-To: <4092469.vxd3uL7amA@vostro.rjw.lan>
>> int64_t thresh = LLONG_MAX; /* Discard outliers above this value. */
>Shouldn't this one be uint64_t too?
Thanks Rafael! Fenggung also asked me the similar question.
But I think, the compile only complains about do_div() at SH 32bit architecture, the above int64_t definition should be OK and I do not find any issue without changing it to uint64_t.
Thanks
-Youquan
-----Original Message-----
From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
Sent: Wednesday, October 24, 2012 8:28 AM
To: Song, Youquan
Cc: Wu, Fengguang; yuanhan.liu@linux.intel.com; Rik van Riel; linux-pm@vger.kernel.org; Youquan Song
Subject: Re: [PATCH] cpuidle: fix warning when compile
On Tuesday 23 of October 2012 22:45:28 Youquan Song wrote:
> Fix the warning complain when compile the kernel.
>
> drivers/cpuidle/governors/menu.c: In function 'get_typical_interval':
> drivers/cpuidle/governors/menu.c:268:2: warning: comparison of
> distinct pointer types lacks a cast [enabled by default]
> drivers/cpuidle/governors/menu.c:277:2: warning: comparison of
> distinct pointer types lacks a cast [enabled by default]
>
> Signed-off-by: Youquan Song <youquan.song@intel.com>
> ---
> drivers/cpuidle/governors/menu.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/cpuidle/governors/menu.c
> b/drivers/cpuidle/governors/menu.c
> index 26d5667..2efee27 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -248,7 +248,7 @@ static enum hrtimer_restart
> menu_hrtimer_notify(struct hrtimer *hrtimer) static u32
> get_typical_interval(struct menu_device *data) {
> int i = 0, divisor = 0;
> - int64_t max = 0, avg = 0, stddev = 0;
> + uint64_t max = 0, avg = 0, stddev = 0;
> int64_t thresh = LLONG_MAX; /* Discard outliers above this value. */
Shouldn't this one be uint64_t too?
> unsigned int ret = 0;
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: drivers/base/power/qos.c:330:5: sparse: symbol '__dev_pm_qos_update_request' was not declared. Should it be static?
From: Rafael J. Wysocki @ 2012-10-24 0:53 UTC (permalink / raw)
To: Fengguang Wu; +Cc: Rafael J. Wysocki, linux-pm
In-Reply-To: <50861b73.GgA1DPGYKvLmArF0%fengguang.wu@intel.com>
On Tuesday 23 of October 2012 12:22:11 Fengguang Wu wrote:
> Hi Rafael,
>
> FYI, there are new sparse warnings show up in
>
> tree: git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> head: 13f5e2d9a915373dd1573d8fe0214738bc69004f
> commit: a7a02d1a295d3ab9cd894af4f138e30a3000df3d PM / QoS: Make it possible to expose PM QoS device flags to user space
> date: 5 hours ago
>
> + drivers/base/power/qos.c:330:5: sparse: symbol '__dev_pm_qos_update_request' was not declared. Should it be static?
>
> vim +330 drivers/base/power/qos.c
>
> ae0fb4b7 Rafael J. Wysocki 2012-10-23 314 req->type = type;
> b66213cd Jean Pihet 2011-08-25 315 ret = apply_constraint(req, PM_QOS_ADD_REQ, value);
> ae0fb4b7 Rafael J. Wysocki 2012-10-23 316 }
> 91ff4cb8 Jean Pihet 2011-08-25 317
> 1a9a9152 Rafael J. Wysocki 2011-09-29 318 out:
> 91ff4cb8 Jean Pihet 2011-08-25 319 mutex_unlock(&dev_pm_qos_mtx);
> 1a9a9152 Rafael J. Wysocki 2011-09-29 320
> 91ff4cb8 Jean Pihet 2011-08-25 321 return ret;
> 91ff4cb8 Jean Pihet 2011-08-25 322 }
> 91ff4cb8 Jean Pihet 2011-08-25 323 EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
> 91ff4cb8 Jean Pihet 2011-08-25 324
> 91ff4cb8 Jean Pihet 2011-08-25 325 /**
> a7a02d1a Rafael J. Wysocki 2012-10-23 326 * __dev_pm_qos_update_request - Modify an existing device PM QoS request.
> a7a02d1a Rafael J. Wysocki 2012-10-23 327 * @req : PM QoS request to modify.
> a7a02d1a Rafael J. Wysocki 2012-10-23 328 * @new_value: New value to request.
> a7a02d1a Rafael J. Wysocki 2012-10-23 329 */
> a7a02d1a Rafael J. Wysocki 2012-10-23 @330 int __dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value)
> a7a02d1a Rafael J. Wysocki 2012-10-23 331 {
> a7a02d1a Rafael J. Wysocki 2012-10-23 332 s32 curr_value;
> a7a02d1a Rafael J. Wysocki 2012-10-23 333 int ret = 0;
> a7a02d1a Rafael J. Wysocki 2012-10-23 334
> a7a02d1a Rafael J. Wysocki 2012-10-23 335 if (!req->dev->power.qos)
> a7a02d1a Rafael J. Wysocki 2012-10-23 336 return -ENODEV;
> a7a02d1a Rafael J. Wysocki 2012-10-23 337
> a7a02d1a Rafael J. Wysocki 2012-10-23 338 switch(req->type) {
>
> Please consider folding the attached diff :-)
It should be fixed now.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: drivers/cpufreq/cpufreq_governor.c:46:53: sparse: incorrect type in argument 2 (different modifiers)
From: Rafael J. Wysocki @ 2012-10-24 0:33 UTC (permalink / raw)
To: Viresh Kumar; +Cc: Rafael J. Wysocki, Fengguang Wu, linux-pm, Lists linaro-dev
In-Reply-To: <CAKohpo=PF-MkG8KDRWSfoBXkK102r7WwToQ01UmfetvarRyK3w@mail.gmail.com>
On Tuesday 23 of October 2012 10:55:51 Viresh Kumar wrote:
> Hi Fengguang,
>
> Thanks for your mail. Few things i learnt from it:
> - Rafael has already applied my patches :)
Yes, I have. I didn't have the time to let all the people whose patches
were applied know individually, however.
> - I haven't used sparse in my life till this point. Now i know how to use it.
>
> On 23 October 2012 10:21, Fengguang Wu <fengguang.wu@intel.com> wrote:
> > Hi viresh,
> >
> > FYI, there are new sparse warnings show up in
>
> Actually these aren't new warnings, but old. Because some part is moved from
> one file to another, that's why your script shows them as new warnings.
>
> Anyway, i don't hesitate in fixing them.
> @Rafael: Most of these are due to mixed use of u64 and cputime64_t. Both of
> which are u64 if i am not wrong. Any specific reason that we used cputime64_t
> instead of u64? Or can i make everything u64 instead in governors?
Well, there might be a reason in the past, but I'm not sure it's still there. :-)
I _think_ you can use u64 everywhere. It would be more correct than the current
situation anyway.
Thanks,
Rafael
> > tree: git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> > head: 13f5e2d9a915373dd1573d8fe0214738bc69004f
> > commit: 83a73f712f2275033b2dc7f5c664988a1823ebc7 cpufreq: Move common part from governors to separate file, v2
> > date: 5 hours ago
> >
> > + drivers/cpufreq/cpufreq_governor.c:46:53: sparse: incorrect type in argument 2 (different modifiers)
> > drivers/cpufreq/cpufreq_governor.c:46:53: expected unsigned long long [usertype] *wall
> > drivers/cpufreq/cpufreq_governor.c:46:53: got unsigned long long [nocast] [usertype] *wall
> > + drivers/cpufreq/cpufreq_governor.c:46:53: sparse: implicit cast from nocast type
> > drivers/cpufreq/cpufreq_governor.c:48:58: sparse: incorrect type in argument 2 (different modifiers)
> > drivers/cpufreq/cpufreq_governor.c:48:58: expected unsigned long long [usertype] *last_update_time
> > drivers/cpufreq/cpufreq_governor.c:48:58: got unsigned long long [nocast] [usertype] *wall
> > drivers/cpufreq/cpufreq_governor.c:48:58: sparse: implicit cast from nocast type
> >
> > vim +46 drivers/cpufreq/cpufreq_governor.c
> >
> > 83a73f71 viresh kumar 2012-10-23 30 busy_time += kcpustat_cpu(cpu).cpustat[CPUTIME_SOFTIRQ];
> > 83a73f71 viresh kumar 2012-10-23 31 busy_time += kcpustat_cpu(cpu).cpustat[CPUTIME_STEAL];
> > 83a73f71 viresh kumar 2012-10-23 32 busy_time += kcpustat_cpu(cpu).cpustat[CPUTIME_NICE];
> > 83a73f71 viresh kumar 2012-10-23 33
> > 83a73f71 viresh kumar 2012-10-23 34 idle_time = cur_wall_time - busy_time;
> > 83a73f71 viresh kumar 2012-10-23 35 if (wall)
> > 83a73f71 viresh kumar 2012-10-23 36 *wall = jiffies_to_usecs(cur_wall_time);
> > 83a73f71 viresh kumar 2012-10-23 37
> > 83a73f71 viresh kumar 2012-10-23 38 return jiffies_to_usecs(idle_time);
> > 83a73f71 viresh kumar 2012-10-23 39 }
> > 83a73f71 viresh kumar 2012-10-23 40
> > 83a73f71 viresh kumar 2012-10-23 41 cputime64_t get_cpu_idle_time(unsigned int cpu, cputime64_t *wall)
> > 83a73f71 viresh kumar 2012-10-23 42 {
> > 83a73f71 viresh kumar 2012-10-23 43 u64 idle_time = get_cpu_idle_time_us(cpu, NULL);
> > 83a73f71 viresh kumar 2012-10-23 44
> > 83a73f71 viresh kumar 2012-10-23 45 if (idle_time == -1ULL)
> > 83a73f71 viresh kumar 2012-10-23 @46 return get_cpu_idle_time_jiffy(cpu, wall);
> > 83a73f71 viresh kumar 2012-10-23 47 else
> > 83a73f71 viresh kumar 2012-10-23 48 idle_time += get_cpu_iowait_time_us(cpu, wall);
> > 83a73f71 viresh kumar 2012-10-23 49
> > 83a73f71 viresh kumar 2012-10-23 50 return idle_time;
> > 83a73f71 viresh kumar 2012-10-23 51 }
> > 83a73f71 viresh kumar 2012-10-23 52 EXPORT_SYMBOL_GPL(get_cpu_idle_time);
> >
> > ---
> > 0-DAY kernel build testing backend Open Source Technology Center
> > Fengguang Wu, Yuanhan Liu Intel Corporation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] cpuidle: fix warning when compile
From: Rafael J. Wysocki @ 2012-10-24 0:28 UTC (permalink / raw)
To: Youquan Song
Cc: fengguang.wu, yuanhan.liu, Rik van Riel, linux-pm, Youquan Song
In-Reply-To: <1351046728-10965-1-git-send-email-youquan.song@intel.com>
On Tuesday 23 of October 2012 22:45:28 Youquan Song wrote:
> Fix the warning complain when compile the kernel.
>
> drivers/cpuidle/governors/menu.c: In function 'get_typical_interval':
> drivers/cpuidle/governors/menu.c:268:2: warning: comparison of distinct pointer types lacks a cast [enabled by default]
> drivers/cpuidle/governors/menu.c:277:2: warning: comparison of distinct pointer types lacks a cast [enabled by default]
>
> Signed-off-by: Youquan Song <youquan.song@intel.com>
> ---
> drivers/cpuidle/governors/menu.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index 26d5667..2efee27 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -248,7 +248,7 @@ static enum hrtimer_restart menu_hrtimer_notify(struct hrtimer *hrtimer)
> static u32 get_typical_interval(struct menu_device *data)
> {
> int i = 0, divisor = 0;
> - int64_t max = 0, avg = 0, stddev = 0;
> + uint64_t max = 0, avg = 0, stddev = 0;
> int64_t thresh = LLONG_MAX; /* Discard outliers above this value. */
Shouldn't this one be uint64_t too?
> unsigned int ret = 0;
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] rtpm-core: enable counters for PM state transitions
From: Rafael J. Wysocki @ 2012-10-23 23:37 UTC (permalink / raw)
To: Sundar Iyer; +Cc: vivek.thakkar, Linux PM list
In-Reply-To: <1351011142-6249-1-git-send-email-sundar.iyer@intel.com>
On Tuesday 23 of October 2012 22:22:22 Sundar Iyer wrote:
> Add usage counters for every runtime suspend/resume
> to enable runtime characteristics debug
>
> Change-Id: I174dbf3ffc4161eb9baf597ae545a1bc4108e516
> Signed-off-by: Sundar Iyer <sundar.iyer@intel.com>
First off, the linux-pm@lists.linux-foundation.org list is dead.
Please use linux-pm@vger.kernel.org instead (as documented in multiple places).
Second, what's the performance impact of this patch?
Rafael
> ---
> drivers/base/power/runtime.c | 6 +++++
> drivers/base/power/sysfs.c | 44 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm.h | 2 +
> 3 files changed, 52 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 1f339d5..5895bc2 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -52,6 +52,10 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
> update_pm_runtime_accounting(dev);
> trace_runtime_pm_status(dev, status);
> dev->power.runtime_status = status;
> + if (status == RPM_SUSPENDED)
> + atomic_inc(&dev->power.suspend_transition_count);
> + else if (status == RPM_ACTIVE)
> + atomic_inc(&dev->power.resume_transition_count);
> }
>
> /**
> @@ -1240,6 +1244,8 @@ void pm_runtime_init(struct device *dev)
>
> dev->power.disable_depth = 1;
> atomic_set(&dev->power.usage_count, 0);
> + atomic_set(&dev->power.suspend_transition_count, 0);
> + atomic_set(&dev->power.resume_transition_count, 0);
>
> dev->power.runtime_error = 0;
>
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index a9f5b89..32c372e 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -186,6 +186,46 @@ static ssize_t rtpm_status_show(struct device *dev,
>
> static DEVICE_ATTR(runtime_status, 0444, rtpm_status_show, NULL);
>
> +static ssize_t suspend_transition_count_store(struct device *dev,
> + struct device_attribute *attr, char *buf, size_t n)
> +{
> + atomic_set(&dev->power.suspend_transition_count, 0);
> +
> + return n;
> +}
> +
> +static ssize_t suspend_transition_count_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%d\n",
> + atomic_read(&dev->power.suspend_transition_count));
> +}
> +
> +static DEVICE_ATTR(runtime_suspend_transition, 0644,
> + suspend_transition_count_show,
> + suspend_transition_count_store);
> +
> +static ssize_t resume_transition_count_store(struct device *dev,
> + struct device_attribute *attr, char *buf, size_t n)
> +{
> + atomic_set(&dev->power.resume_transition_count, 0);
> +
> + return n;
> +}
> +
> +
> +static ssize_t resume_transition_count_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%d\n",
> + atomic_read(&dev->power.resume_transition_count));
> +}
> +
> +static DEVICE_ATTR(runtime_resume_transition, 0644,
> + resume_transition_count_show,
> + resume_transition_count_store);
> +
> +
> static ssize_t autosuspend_delay_ms_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> @@ -438,6 +478,8 @@ static struct attribute *power_attrs[] = {
> #endif
> #ifdef CONFIG_PM_RUNTIME
> &dev_attr_runtime_status.attr,
> + &dev_attr_runtime_suspend_transition.attr,
> + &dev_attr_runtime_resume_transition.attr,
> &dev_attr_runtime_usage.attr,
> &dev_attr_runtime_active_kids.attr,
> &dev_attr_runtime_enabled.attr,
> @@ -472,6 +514,8 @@ static struct attribute *runtime_attrs[] = {
> #ifdef CONFIG_PM_RUNTIME
> #ifndef CONFIG_PM_ADVANCED_DEBUG
> &dev_attr_runtime_status.attr,
> + &dev_attr_runtime_suspend_transition.attr,
> + &dev_attr_runtime_resume_transition.attr,
> #endif
> &dev_attr_control.attr,
> &dev_attr_runtime_suspended_time.attr,
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 411e4f4..4001ba2 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -442,6 +442,8 @@ struct dev_pm_info {
> wait_queue_head_t wait_queue;
> atomic_t usage_count;
> atomic_t child_count;
> + atomic_t suspend_transition_count;
> + atomic_t resume_transition_count;
> unsigned int disable_depth:3;
> unsigned int ignore_children:1;
> unsigned int idle_notification:1;
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH 1/5] Thermal: do bind operation after thermal zone or cooling device register returns.
From: Francesco Lavra @ 2012-10-23 22:13 UTC (permalink / raw)
To: Hongbo Zhang
Cc: linaro-dev, linux-kernel, linux-pm, STEricsson_nomadik_linux,
kernel, linaro-kernel, hongbo.zhang, patches, amit.kachhap
In-Reply-To: <CAJLyvQyrZ56_KhUTpuv=2fet=12QHSCUjgUKHF9n3cmaATke5w@mail.gmail.com>
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
^ permalink raw reply
* Re: [RFC PATCH v2 2/6] PM / Runtime: introduce pm_runtime_set_memalloc_noio()
From: Alan Stern @ 2012-10-23 18:16 UTC (permalink / raw)
To: Ming Lei
Cc: linux-kernel, Oliver Neukum, Minchan Kim, Greg Kroah-Hartman,
Rafael J. Wysocki, Jens Axboe, David S. Miller, Andrew Morton,
netdev, linux-usb, linux-pm, linux-mm
In-Reply-To: <CACVXFVN+=XH_f5BmRkXeagTNowz0o0-Pd7GcxCneO0FSq8xqEw@mail.gmail.com>
On Tue, 23 Oct 2012, Ming Lei wrote:
> With the problem of non-SMP-safe bitfields access, the power.lock should
> be held, but that is not enough to prevent children from being probed or
> disconnected. Looks another lock is still needed. I think a global lock
> is OK in the infrequent path.
Agreed.
> Got it, thanks for your detailed explanation.
>
> Looks the problem is worse than above, not only bitfields are affected, the
> adjacent fields might be involved too, see:
>
> http://lwn.net/Articles/478657/
Linus made it clear (in various emails at the time) that the kernel
requires the compiler not to do the sort of things discussed in that
article. But even the restrictions he wanted would not prevent
adjacent bitfields from interfering with each other.
Alan Stern
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH v2 2/6] PM / Runtime: introduce pm_runtime_set_memalloc_noio()
From: Ming Lei @ 2012-10-23 15:18 UTC (permalink / raw)
To: Alan Stern
Cc: linux-kernel, Oliver Neukum, Minchan Kim, Greg Kroah-Hartman,
Rafael J. Wysocki, Jens Axboe, David S. Miller, Andrew Morton,
netdev, linux-usb, linux-pm, linux-mm
In-Reply-To: <Pine.LNX.4.44L0.1210231022230.1635-100000@iolanthe.rowland.org>
On Tue, Oct 23, 2012 at 10:46 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 23 Oct 2012, Ming Lei wrote:
>
>> On Mon, Oct 22, 2012 at 10:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> >
>> > Tail recursion should be implemented as a loop, not as an explicit
>> > recursion. That is, the function should be:
>> >
>> > void pm_runtime_set_memalloc_noio(struct device *dev, bool enable)
>> > {
>> > do {
>> > dev->power.memalloc_noio_resume = enable;
>> >
>> > if (!enable) {
>> > /*
>> > * Don't clear the parent's flag if any of the
>> > * parent's children have their flag set.
>> > */
>> > if (device_for_each_child(dev->parent, NULL,
>> > dev_memalloc_noio))
>> > return;
>> > }
>> > dev = dev->parent;
>> > } while (dev);
>> > }
>>
>> OK, will take the non-recursion implementation for saving kernel
>> stack space.
>>
>> >
>> > except that you need to add locking, for two reasons:
>> >
>> > There's a race. What happens if another child sets the flag
>> > between the time device_for_each_child() runs and the next loop
>> > iteration?
>>
>> Yes, I know the race, and not adding a lock because the function
>> is mostly called in .probe() or .remove() callback and its parent's device
>> lock is held to avoid this race.
>>
>> Considered that it may be called in async probe() (scsi disk), one lock
>> is needed, the simplest way is to add a global lock. Any suggestion?
>
> No. Because of where you put the new flag, it must be protected by
> dev->power.lock. And this means the iterative implementation shown
> above can't be used as is. It will have to be more like this:
>
> void pm_runtime_set_memalloc_noio(struct device *dev, bool enable)
> {
> spin_lock_irq(&dev->power.lock);
> dev->power.memalloc_noio_resume = enable;
>
> while (dev->parent) {
> spin_unlock_irq(&dev->power.lock);
> dev = dev->parent;
>
> spin_lock_irq(&dev->power.lock);
> /*
> * Don't clear the parent's flag if any of the
> * parent's children have their flag set.
> */
> if (!enable && device_for_each_child(dev->parent, NULL,
s/dev->parent/dev
> dev_memalloc_noio))
> break;
> dev->power.memalloc_noio_resume = enable;
> }
> spin_unlock_irq(&dev->power.lock);
> }
With the problem of non-SMP-safe bitfields access, the power.lock should
be held, but that is not enough to prevent children from being probed or
disconnected. Looks another lock is still needed. I think a global lock
is OK in the infrequent path.
>
>> > Even without a race, access to bitfields is not SMP-safe
>> > without locking.
>>
>> You mean one ancestor device might not be in active when
>> one of its descendants is being probed or removed?
>
> No. Consider this example:
>
> struct foo {
> int a:1;
> int b:1;
> } x;
>
> Consider what happens if CPU 0 does "x.a = 1" at the same time as
> another CPU 1 does "x.b = 1". The compiler might produce object code
> looking like this for CPU 0:
>
> move x, reg1
> or 0x1, reg1
> move reg1, x
>
> and this for CPU 1:
>
> move x, reg2
> or 0x2, reg2
> move reg2, x
>
> With no locking, the two "or" instructions could execute
> simultaneously. What will the final value of x be?
>
> The two CPUs will interfere, even though they are touching different
> bitfields.
Got it, thanks for your detailed explanation.
Looks the problem is worse than above, not only bitfields are affected, the
adjacent fields might be involved too, see:
http://lwn.net/Articles/478657/
Thanks,
--
Ming Lei
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: USB port power off discussion
From: Lan Tianyu @ 2012-10-23 14:47 UTC (permalink / raw)
To: Alan Stern
Cc: Sarah Sharp, Rafael J. Wysocki, Lan Tianyu, Greg KH, Len Brown,
Arjan Van De Ven, Oliver Neukum, USB list, Linux-pm mailing list
In-Reply-To: <Pine.LNX.4.44L0.1210221422380.1724-100000@iolanthe.rowland.org>
于 2012/10/23 2:37, Alan Stern 写道:
> On Mon, 22 Oct 2012, Sarah Sharp wrote:
>
>> On Mon, Oct 15, 2012 at 08:42:47AM +0200, Rafael J. Wysocki wrote:
>>> Sorry for the delay, I have been distracted by a number of things.
>>
>> Me too. :)
>>
>>> On Monday 24 of September 2012 15:10:36 Sarah Sharp wrote:
>>>> On Tue, Sep 25, 2012 at 12:09:06AM +0200, Rafael J. Wysocki wrote:
>>>>> What about hubs connected to such ports? I don't think they're going to
>>>>> work when power is removed from it, are they?
>>>>
>>>> USB hubs would have remote wakeup enabled, so we would never power off
>>>> their port with the "auto" policy.
>>>
>>> Here's my idea how to arrange things.
>>>
>>> Why don't make runtime PM manage the USB port power on/off such that the
>>> port's .runtime_suspend() routine will remove power from it, if
>>> PM_QOS_FLAG_NO_POWER_OFF is not (effectively) set for it, and its
>>> .runtime_resume() will restore power to it (if removed previously).
>>
>> Ok, so you're basically proposing we power off suspended devices, except
>> when userspace (or perhaps the kernel) sets PM_QOS_FLAG_NO_POWER_OFF?
>
> Rafael is suggesting that the interface to control when ports get
> powered off should be associated with the usb_port device, rather than
> with the usb_device attached to the port.
That means we should add a driver for usb port device?
>
>> What happens if userspace clears the PM_QOS_FLAG_NO_POWER_OFF flag while
>> the port is suspended? Does the USB core then re-power it on? Should
>> it also resume the device?
>
> I don't know how the PM QOS flags interact with runtime PM. Rafael may
> still be working on that.
I think we can add a notifier event for PM Qos flags, just like the one for
PM Qos latency. Add notifier callback to check whether PM_QOS_FLAG_NO_POWER_OFF
is clear or set when flags was changed.
For both setting or clearing PM_QOS_FLAG_NO_POWER_OFF, invoking
pm_runtime_get_sync()
to resume port device and then pm_runtime_put_sync()(device will be suspended again
and power off or not depending on the setting).
>
> At any rate, runtime suspend for the usb_port will remove power.
> Runtime resume will turn power back on. But if there's a usb_device
> attached to the port, resuming the port will not necessarily resume the
> device. (However, we will be careful to make sure that the port cannot
> be runtime suspended unless the device is suspended first.)
>
>>> The USB core will then do pm_runtime_get_sync() on the port every time
>>> a device depending on it is added and pm_runtime_put() every time such
>>> a device is removed.
>>
>> Are you saying that every time a device is connected to an _external_
>> hub, the USB core would call pm_runtime_get_sync()? If you apply that
>> policy to roothubs, then you're basically disabling port power off when
>> a device is connected, which doesn't make sense in conjunction to your
>> last sentence, so now I'm just confused, sorry.
>
> He is saying that the core would call pm_runtime_get_sync(&port->dev),
> not pm_runtime_get_sync(&udev->dev). And this would only be for while
> the device was active, not for when the device is suspended (unless the
> device can't handle loss of power).
>
>> Wouldn't it make more sense to call pm_runtime_get_sync() when remote
>> wakeup is enabled for a device, and pm_runtime_put() when remote wakeup
>> is disabled? That way, when an external hub is attached to the port, it
>> always has remote wakeup enabled, so we will always increment the PM
>> counter. [1]
>>
>>> The initial setting of PM_QOS_FLAG_NO_POWER_OFF in the PM QoS request
>>> structure used by user space will depend on the type of the port (e.g.
>>> it will be unset for ports that aren't visible to the user and connectable).
>>>
>>> That should allow things to work automatically and it should allow user space
>>> to override the defaults in any case, either by disabling runtime PM for the
>>> ports altogether (by writing "on" to their device objects' "control" sysfs
>>> files), or by setting/unsetting PM_QOS_FLAG_NO_POWER_OFF in the PM QoS
>>> request controlled by it as desired.
>>
>> Are you then pushing the policy decision for whether ports should be
>> powered off completely into userspace then? I.e. you want userspace to
>> read the ACPI port connect type info and clear PM_QOS_FLAG_NO_POWER_OFF
>> in the userspace structure if the port is not user visible and not
>> connectable?
>>
>> Or are you thinking the kernel will set PM_QOS_FLAG_NO_POWER_OFF based
>> on the ACPI tables and userspace will have the option to override it?
>
> The latter.
>
>> We also probably need to allow USB interface drivers to set
>> PM_QOS_FLAG_NO_POWER_OFF as well, for cases like devices that have
>> firmware or other internal state. We probably need a USB core API to
>> add a request for PM_QOS_FLAG_NO_POWER_OFF to be set, and a way to clear
>> that flag if the driver is unbound or the driver decides it's safe to
>> power off the device.
>
> How about adding a no_power_off flag to the usb_interface structure,
> analogous to the needs_remote_wakeup flag?
I think we can add a new request for kernel space to set pm qos flags.
Now Rafael has added a request for user space. Because user choice should
be kept. So we should add one for kernel space. We can issue the request
where a device can not be power off in the driver or remote wakeup is
enabled/disabled.
usb interfaces also have pm qos flags in their struct device. So we can
reuse it. When usb device are being suspended, check all its interface
NO_POWER_OFF flag and issue request to set its NO_POWER_OFF flag.
>
> Alan Stern
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Best regards
Tianyu Lan
^ permalink raw reply
* Re: [RFC PATCH v2 2/6] PM / Runtime: introduce pm_runtime_set_memalloc_noio()
From: Alan Stern @ 2012-10-23 14:46 UTC (permalink / raw)
To: Ming Lei
Cc: linux-kernel, Oliver Neukum, Minchan Kim, Greg Kroah-Hartman,
Rafael J. Wysocki, Jens Axboe, David S. Miller, Andrew Morton,
netdev, linux-usb, linux-pm, linux-mm
In-Reply-To: <CACVXFVMmszZWHaeNS6LSG4nHR4wWBLwM_BvynRwUW8X=nO+JWA@mail.gmail.com>
On Tue, 23 Oct 2012, Ming Lei wrote:
> On Mon, Oct 22, 2012 at 10:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > Tail recursion should be implemented as a loop, not as an explicit
> > recursion. That is, the function should be:
> >
> > void pm_runtime_set_memalloc_noio(struct device *dev, bool enable)
> > {
> > do {
> > dev->power.memalloc_noio_resume = enable;
> >
> > if (!enable) {
> > /*
> > * Don't clear the parent's flag if any of the
> > * parent's children have their flag set.
> > */
> > if (device_for_each_child(dev->parent, NULL,
> > dev_memalloc_noio))
> > return;
> > }
> > dev = dev->parent;
> > } while (dev);
> > }
>
> OK, will take the non-recursion implementation for saving kernel
> stack space.
>
> >
> > except that you need to add locking, for two reasons:
> >
> > There's a race. What happens if another child sets the flag
> > between the time device_for_each_child() runs and the next loop
> > iteration?
>
> Yes, I know the race, and not adding a lock because the function
> is mostly called in .probe() or .remove() callback and its parent's device
> lock is held to avoid this race.
>
> Considered that it may be called in async probe() (scsi disk), one lock
> is needed, the simplest way is to add a global lock. Any suggestion?
No. Because of where you put the new flag, it must be protected by
dev->power.lock. And this means the iterative implementation shown
above can't be used as is. It will have to be more like this:
void pm_runtime_set_memalloc_noio(struct device *dev, bool enable)
{
spin_lock_irq(&dev->power.lock);
dev->power.memalloc_noio_resume = enable;
while (dev->parent) {
spin_unlock_irq(&dev->power.lock);
dev = dev->parent;
spin_lock_irq(&dev->power.lock);
/*
* Don't clear the parent's flag if any of the
* parent's children have their flag set.
*/
if (!enable && device_for_each_child(dev->parent, NULL,
dev_memalloc_noio))
break;
dev->power.memalloc_noio_resume = enable;
}
spin_unlock_irq(&dev->power.lock);
}
> > Even without a race, access to bitfields is not SMP-safe
> > without locking.
>
> You mean one ancestor device might not be in active when
> one of its descendants is being probed or removed?
No. Consider this example:
struct foo {
int a:1;
int b:1;
} x;
Consider what happens if CPU 0 does "x.a = 1" at the same time as
another CPU 1 does "x.b = 1". The compiler might produce object code
looking like this for CPU 0:
move x, reg1
or 0x1, reg1
move reg1, x
and this for CPU 1:
move x, reg2
or 0x2, reg2
move reg2, x
With no locking, the two "or" instructions could execute
simultaneously. What will the final value of x be?
The two CPUs will interfere, even though they are touching different
bitfields.
Alan Stern
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] cpuidle: fix warning when compile
From: Fengguang Wu @ 2012-10-23 14:42 UTC (permalink / raw)
To: Youquan Song; +Cc: yuanhan.liu, rjw, Rik van Riel, linux-pm, Youquan Song
In-Reply-To: <1351046728-10965-1-git-send-email-youquan.song@intel.com>
On Tue, Oct 23, 2012 at 10:45:28PM -0400, Youquan Song wrote:
> Fix the warning complain when compile the kernel.
>
> drivers/cpuidle/governors/menu.c: In function 'get_typical_interval':
> drivers/cpuidle/governors/menu.c:268:2: warning: comparison of distinct pointer types lacks a cast [enabled by default]
> drivers/cpuidle/governors/menu.c:277:2: warning: comparison of distinct pointer types lacks a cast [enabled by default]
>
> Signed-off-by: Youquan Song <youquan.song@intel.com>
Tested-by: Fengguang Wu <wfg@linux.intel.com>
> ---
> drivers/cpuidle/governors/menu.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index 26d5667..2efee27 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -248,7 +248,7 @@ static enum hrtimer_restart menu_hrtimer_notify(struct hrtimer *hrtimer)
> static u32 get_typical_interval(struct menu_device *data)
> {
> int i = 0, divisor = 0;
> - int64_t max = 0, avg = 0, stddev = 0;
> + uint64_t max = 0, avg = 0, stddev = 0;
> int64_t thresh = LLONG_MAX; /* Discard outliers above this value. */
> unsigned int ret = 0;
>
> --
> 1.7.7.4
^ permalink raw reply
* [PATCH] cpuidle: fix warning when compile
From: Youquan Song @ 2012-10-24 2:45 UTC (permalink / raw)
To: fengguang.wu, yuanhan.liu, rjw, Rik van Riel, linux-pm
Cc: Youquan Song, Youquan Song
Fix the warning complain when compile the kernel.
drivers/cpuidle/governors/menu.c: In function 'get_typical_interval':
drivers/cpuidle/governors/menu.c:268:2: warning: comparison of distinct pointer types lacks a cast [enabled by default]
drivers/cpuidle/governors/menu.c:277:2: warning: comparison of distinct pointer types lacks a cast [enabled by default]
Signed-off-by: Youquan Song <youquan.song@intel.com>
---
| 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
--git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 26d5667..2efee27 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -248,7 +248,7 @@ static enum hrtimer_restart menu_hrtimer_notify(struct hrtimer *hrtimer)
static u32 get_typical_interval(struct menu_device *data)
{
int i = 0, divisor = 0;
- int64_t max = 0, avg = 0, stddev = 0;
+ uint64_t max = 0, avg = 0, stddev = 0;
int64_t thresh = LLONG_MAX; /* Discard outliers above this value. */
unsigned int ret = 0;
--
1.7.7.4
^ permalink raw reply related
* Re: drivers/cpuidle/governors/menu.c:268:2: warning: comparison of distinct pointer types lacks a cast
From: Fengguang Wu @ 2012-10-23 12:44 UTC (permalink / raw)
To: Song, Youquan; +Cc: linux-pm@vger.kernel.org, Rik van Riel, Wysocki, Rafael J
In-Reply-To: <70A3B115BE79814398B8E4E7C4AE18EE138DCD@SHSMSX101.ccr.corp.intel.com>
Youquan,
On Tue, Oct 23, 2012 at 07:23:00PM +0800, Song, Youquan wrote:
> Hi Fengguang,
>
> Sorry. I built the kernel at 32bit OS RHEL6 i686 without any complain about it.
> Are you built it for sh architecture? by "make ARCH=sh sh7785lcr_32bit_defconfig", I do not know this architecture at all. Who can help me on it?
No worry. This is tested to remove the following warnings.
drivers/cpuidle/governors/menu.c: In function 'get_typical_interval':
drivers/cpuidle/governors/menu.c:268:2: warning: comparison of distinct pointer types lacks a cast [enabled by default]
drivers/cpuidle/governors/menu.c:277:2: warning: comparison of distinct pointer types lacks a cast [enabled by default]
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 26d5667..2efee27 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -248,7 +248,7 @@ static enum hrtimer_restart menu_hrtimer_notify(struct hrtimer *hrtimer)
static u32 get_typical_interval(struct menu_device *data)
{
int i = 0, divisor = 0;
- int64_t max = 0, avg = 0, stddev = 0;
+ uint64_t max = 0, avg = 0, stddev = 0;
int64_t thresh = LLONG_MAX; /* Discard outliers above this value. */
unsigned int ret = 0;
^ permalink raw reply related
* RE: drivers/cpuidle/governors/menu.c:268:2: warning: comparison of distinct pointer types lacks a cast
From: Song, Youquan @ 2012-10-23 11:23 UTC (permalink / raw)
To: Wu, Fengguang
Cc: linux-pm@vger.kernel.org, Rik van Riel, Wysocki, Rafael J,
Song, Youquan
In-Reply-To: <508610e6.lZq8SVXFvwZCcvw6%fengguang.wu@intel.com>
Hi Fengguang,
Sorry. I built the kernel at 32bit OS RHEL6 i686 without any complain about it.
Are you built it for sh architecture? by "make ARCH=sh sh7785lcr_32bit_defconfig", I do not know this architecture at all. Who can help me on it?
Thanks
-Youquan
-----Original Message-----
From: Wu, Fengguang
Sent: Tuesday, October 23, 2012 11:37 AM
To: Song, Youquan
Cc: linux-pm@vger.kernel.org; Rik van Riel; Wysocki, Rafael J
Subject: drivers/cpuidle/governors/menu.c:268:2: warning: comparison of distinct pointer types lacks a cast
Hi Youquan,
FYI, there are new compile warnings show up in
tree: git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
head: 13f5e2d9a915373dd1573d8fe0214738bc69004f
commit: 0c963c99eb65aa9991cec6b3ea3188862167c427 cpuidle: Get typical recent sleep interval
date: 4 hours ago
config: sh-sh7785lcr_32bit_defconfig # make ARCH=sh sh7785lcr_32bit_defconfig
All warnings:
drivers/cpuidle/governors/menu.c: In function 'get_typical_interval':
drivers/cpuidle/governors/menu.c:268:2: warning: comparison of distinct pointer types lacks a cast [enabled by default]
drivers/cpuidle/governors/menu.c:277:2: warning: comparison of distinct pointer types lacks a cast [enabled by default]
vim +268 drivers/cpuidle/governors/menu.c
0c963c99 Youquan Song 2012-10-23 252 int64_t thresh = LLONG_MAX; /* Discard outliers above this value. */
0c963c99 Youquan Song 2012-10-23 253 unsigned int ret = 0;
1f85f87d Arjan van de Ven 2010-05-24 254
0c963c99 Youquan Song 2012-10-23 255 again:
1f85f87d Arjan van de Ven 2010-05-24 256
0c963c99 Youquan Song 2012-10-23 257 /* first calculate average and standard deviation of the past */
0c963c99 Youquan Song 2012-10-23 258 max = avg = divisor = stddev = 0;
0c963c99 Youquan Song 2012-10-23 259 for (i = 0; i < INTERVALS; i++) {
0c963c99 Youquan Song 2012-10-23 260 int64_t value = data->intervals[i];
0c963c99 Youquan Song 2012-10-23 261 if (value <= thresh) {
0c963c99 Youquan Song 2012-10-23 262 avg += value;
0c963c99 Youquan Song 2012-10-23 263 divisor++;
0c963c99 Youquan Song 2012-10-23 264 if (value > max)
0c963c99 Youquan Song 2012-10-23 265 max = value;
0c963c99 Youquan Song 2012-10-23 266 }
0c963c99 Youquan Song 2012-10-23 267 }
0c963c99 Youquan Song 2012-10-23 @268 do_div(avg, divisor);
0c963c99 Youquan Song 2012-10-23 269
0c963c99 Youquan Song 2012-10-23 270 for (i = 0; i < INTERVALS; i++) {
0c963c99 Youquan Song 2012-10-23 271 int64_t value = data->intervals[i];
0c963c99 Youquan Song 2012-10-23 272 if (value <= thresh) {
0c963c99 Youquan Song 2012-10-23 273 int64_t diff = value - avg;
0c963c99 Youquan Song 2012-10-23 274 stddev += diff * diff;
0c963c99 Youquan Song 2012-10-23 275 }
0c963c99 Youquan Song 2012-10-23 276 }
---
0-DAY kernel build testing backend Open Source Technology Center
Fengguang Wu, Yuanhan Liu Intel Corporation
^ permalink raw reply
* Re: [RFC PATCH v2 2/6] PM / Runtime: introduce pm_runtime_set_memalloc_noio()
From: Ming Lei @ 2012-10-23 9:08 UTC (permalink / raw)
To: Alan Stern
Cc: linux-kernel, Oliver Neukum, Minchan Kim, Greg Kroah-Hartman,
Rafael J. Wysocki, Jens Axboe, David S. Miller, Andrew Morton,
netdev, linux-usb, linux-pm, linux-mm
In-Reply-To: <Pine.LNX.4.44L0.1210221023300.1724-100000@iolanthe.rowland.org>
On Mon, Oct 22, 2012 at 10:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> Tail recursion should be implemented as a loop, not as an explicit
> recursion. That is, the function should be:
>
> void pm_runtime_set_memalloc_noio(struct device *dev, bool enable)
> {
> do {
> dev->power.memalloc_noio_resume = enable;
>
> if (!enable) {
> /*
> * Don't clear the parent's flag if any of the
> * parent's children have their flag set.
> */
> if (device_for_each_child(dev->parent, NULL,
> dev_memalloc_noio))
> return;
> }
> dev = dev->parent;
> } while (dev);
> }
OK, will take the non-recursion implementation for saving kernel
stack space.
>
> except that you need to add locking, for two reasons:
>
> There's a race. What happens if another child sets the flag
> between the time device_for_each_child() runs and the next loop
> iteration?
Yes, I know the race, and not adding a lock because the function
is mostly called in .probe() or .remove() callback and its parent's device
lock is held to avoid this race.
Considered that it may be called in async probe() (scsi disk), one lock
is needed, the simplest way is to add a global lock. Any suggestion?
>
> Even without a race, access to bitfields is not SMP-safe
> without locking.
You mean one ancestor device might not be in active when
one of its descendants is being probed or removed?
Thanks,
--
Ming Lei
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH v2 6/6] USB: forbid memory allocation with I/O during bus reset
From: Ming Lei @ 2012-10-23 8:44 UTC (permalink / raw)
To: Alan Stern
Cc: linux-kernel, Oliver Neukum, Minchan Kim, Greg Kroah-Hartman,
Rafael J. Wysocki, Jens Axboe, David S. Miller, Andrew Morton,
netdev, linux-usb, linux-pm, linux-mm
In-Reply-To: <Pine.LNX.4.44L0.1210221035450.1724-100000@iolanthe.rowland.org>
On Mon, Oct 22, 2012 at 10:37 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 22 Oct 2012, Ming Lei wrote:
>>
>> + /*
>> + * Don't allocate memory with GFP_KERNEL in current
>> + * context to avoid possible deadlock if usb mass
>> + * storage interface or usbnet interface(iSCSI case)
>> + * is included in current configuration. The easiest
>> + * approach is to do it for all devices.
>> + */
>> + memalloc_noio_save(noio_flag);
>
> Why not check dev->power.memalloc_noio_resume here too?
Yes, we can use the flag here too even though it is introduced
for rutime_resume case.
Thanks,
--
Ming Lei
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH v2 4/6] net/core: apply pm_runtime_set_memalloc_noio on network devices
From: Ming Lei @ 2012-10-23 8:42 UTC (permalink / raw)
To: Alan Stern
Cc: linux-kernel, Oliver Neukum, Minchan Kim, Greg Kroah-Hartman,
Rafael J. Wysocki, Jens Axboe, David S. Miller, Andrew Morton,
netdev, linux-usb, linux-pm, linux-mm, Eric Dumazet,
David Decotigny, Tom Herbert, Ingo Molnar
In-Reply-To: <Pine.LNX.4.44L0.1210221516280.1724-100000@iolanthe.rowland.org>
On Tue, Oct 23, 2012 at 3:18 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 22 Oct 2012, Ming Lei wrote:
> Is this really needed? Even with iSCSI, doesn't register_disk() have
> to be called for the underlying block device? And given your 3/6
> patch, wouldn't that mark the network device?
The problem is that network device is not one ancestor of the
iSCSI disk device, which transfers data over tcp stack.
Thanks,
--
Ming Lei
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH 1/5] Thermal: do bind operation after thermal zone or cooling device register returns.
From: Hongbo Zhang @ 2012-10-23 8:23 UTC (permalink / raw)
To: Francesco Lavra
Cc: linaro-dev, linux-kernel, linux-pm, STEricsson_nomadik_linux,
kernel, linaro-kernel, hongbo.zhang, patches, amit.kachhap
In-Reply-To: <5083C8E4.7090007@gmail.com>
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.
On 21 October 2012 18:05, Francesco Lavra <francescolavra.fl@gmail.com> wrote:
> Hi,
>
> On 10/16/2012 01:44 PM, hongbo.zhang wrote:
>> From: "hongbo.zhang" <hongbo.zhang@linaro.com>
>>
>> In the previous bind function, cdev->get_max_state(cdev, &max_state) is called
>> before the registration function finishes, but at this moment, the parameter
>> cdev at thermal driver layer isn't ready--it will get ready only after its
>> registration, so the the get_max_state callback cannot tell the max_state
>> according to the cdev input.
>> This problem can be fixed by separating the bind operation out of registration
>> and doing it when registration completely finished.
>
> When thermal_cooling_device_register() is called, the thermal framework
> assumes the cooling device is "ready", i.e. all of its ops callbacks
> return meaningful results. If the cooling device is not ready at this
> point, then this is a bug in the code that registers it.
> Specifically, the faulty code in your case is in the cpufreq cooling
> implementation, where the cooling device is registered before being
> added to the internal list of cpufreq cooling devices. So, IMHO the fix
> is needed there.
>
> --
> Francesco
^ permalink raw reply
* Re: drivers/cpufreq/cpufreq_governor.c:46:53: sparse: incorrect type in argument 2 (different modifiers)
From: Fengguang Wu @ 2012-10-23 7:41 UTC (permalink / raw)
To: Viresh Kumar; +Cc: Rafael J. Wysocki, linux-pm, Lists linaro-dev
In-Reply-To: <CAKohpo=PF-MkG8KDRWSfoBXkK102r7WwToQ01UmfetvarRyK3w@mail.gmail.com>
Hi Viresh,
On Tue, Oct 23, 2012 at 10:55:51AM +0530, Viresh Kumar wrote:
> Hi Fengguang,
>
> Thanks for your mail. Few things i learnt from it:
> - Rafael has already applied my patches :)
> - I haven't used sparse in my life till this point. Now i know how to use it.
Good to know that! :-)
> On 23 October 2012 10:21, Fengguang Wu <fengguang.wu@intel.com> wrote:
> > Hi viresh,
> >
> > FYI, there are new sparse warnings show up in
>
> Actually these aren't new warnings, but old. Because some part is moved from
> one file to another, that's why your script shows them as new warnings.
You are right, the build system is actually catching new "error
messages" strictly speaking.
> Anyway, i don't hesitate in fixing them.
Thanks and appreciated!
Thanks,
Fengguang
^ permalink raw reply
* Re: drivers/cpufreq/cpufreq_governor.c:46:53: sparse: incorrect type in argument 2 (different modifiers)
From: Viresh Kumar @ 2012-10-23 5:25 UTC (permalink / raw)
To: Rafael J. Wysocki, Fengguang Wu; +Cc: linux-pm, Lists linaro-dev
In-Reply-To: <50862255.Foit2oXcLVE39lAk%fengguang.wu@intel.com>
Hi Fengguang,
Thanks for your mail. Few things i learnt from it:
- Rafael has already applied my patches :)
- I haven't used sparse in my life till this point. Now i know how to use it.
On 23 October 2012 10:21, Fengguang Wu <fengguang.wu@intel.com> wrote:
> Hi viresh,
>
> FYI, there are new sparse warnings show up in
Actually these aren't new warnings, but old. Because some part is moved from
one file to another, that's why your script shows them as new warnings.
Anyway, i don't hesitate in fixing them.
@Rafael: Most of these are due to mixed use of u64 and cputime64_t. Both of
which are u64 if i am not wrong. Any specific reason that we used cputime64_t
instead of u64? Or can i make everything u64 instead in governors?
--
viresh
> tree: git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> head: 13f5e2d9a915373dd1573d8fe0214738bc69004f
> commit: 83a73f712f2275033b2dc7f5c664988a1823ebc7 cpufreq: Move common part from governors to separate file, v2
> date: 5 hours ago
>
> + drivers/cpufreq/cpufreq_governor.c:46:53: sparse: incorrect type in argument 2 (different modifiers)
> drivers/cpufreq/cpufreq_governor.c:46:53: expected unsigned long long [usertype] *wall
> drivers/cpufreq/cpufreq_governor.c:46:53: got unsigned long long [nocast] [usertype] *wall
> + drivers/cpufreq/cpufreq_governor.c:46:53: sparse: implicit cast from nocast type
> drivers/cpufreq/cpufreq_governor.c:48:58: sparse: incorrect type in argument 2 (different modifiers)
> drivers/cpufreq/cpufreq_governor.c:48:58: expected unsigned long long [usertype] *last_update_time
> drivers/cpufreq/cpufreq_governor.c:48:58: got unsigned long long [nocast] [usertype] *wall
> drivers/cpufreq/cpufreq_governor.c:48:58: sparse: implicit cast from nocast type
>
> vim +46 drivers/cpufreq/cpufreq_governor.c
>
> 83a73f71 viresh kumar 2012-10-23 30 busy_time += kcpustat_cpu(cpu).cpustat[CPUTIME_SOFTIRQ];
> 83a73f71 viresh kumar 2012-10-23 31 busy_time += kcpustat_cpu(cpu).cpustat[CPUTIME_STEAL];
> 83a73f71 viresh kumar 2012-10-23 32 busy_time += kcpustat_cpu(cpu).cpustat[CPUTIME_NICE];
> 83a73f71 viresh kumar 2012-10-23 33
> 83a73f71 viresh kumar 2012-10-23 34 idle_time = cur_wall_time - busy_time;
> 83a73f71 viresh kumar 2012-10-23 35 if (wall)
> 83a73f71 viresh kumar 2012-10-23 36 *wall = jiffies_to_usecs(cur_wall_time);
> 83a73f71 viresh kumar 2012-10-23 37
> 83a73f71 viresh kumar 2012-10-23 38 return jiffies_to_usecs(idle_time);
> 83a73f71 viresh kumar 2012-10-23 39 }
> 83a73f71 viresh kumar 2012-10-23 40
> 83a73f71 viresh kumar 2012-10-23 41 cputime64_t get_cpu_idle_time(unsigned int cpu, cputime64_t *wall)
> 83a73f71 viresh kumar 2012-10-23 42 {
> 83a73f71 viresh kumar 2012-10-23 43 u64 idle_time = get_cpu_idle_time_us(cpu, NULL);
> 83a73f71 viresh kumar 2012-10-23 44
> 83a73f71 viresh kumar 2012-10-23 45 if (idle_time == -1ULL)
> 83a73f71 viresh kumar 2012-10-23 @46 return get_cpu_idle_time_jiffy(cpu, wall);
> 83a73f71 viresh kumar 2012-10-23 47 else
> 83a73f71 viresh kumar 2012-10-23 48 idle_time += get_cpu_iowait_time_us(cpu, wall);
> 83a73f71 viresh kumar 2012-10-23 49
> 83a73f71 viresh kumar 2012-10-23 50 return idle_time;
> 83a73f71 viresh kumar 2012-10-23 51 }
> 83a73f71 viresh kumar 2012-10-23 52 EXPORT_SYMBOL_GPL(get_cpu_idle_time);
>
> ---
> 0-DAY kernel build testing backend Open Source Technology Center
> Fengguang Wu, Yuanhan Liu Intel Corporation
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox