Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1321840 > unrolled thread
| Started by | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| First post | 2016-01-29 16:40 +0100 |
| Last post | 2016-02-09 21:20 +0100 |
| Articles | 7 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v12 2/4] PM / Domains: add setter for dev.pm_domain Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-29 16:40 +0100
Re: [PATCH v12 2/4] PM / Domains: add setter for dev.pm_domain Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-29 17:00 +0100
Re: [PATCH v12 2/4] PM / Domains: add setter for dev.pm_domain Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-29 17:40 +0100
Re: [PATCH v12 2/4] PM / Domains: add setter for dev.pm_domain "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-29 22:40 +0100
Re: [PATCH v12 2/4] PM / Domains: add setter for dev.pm_domain Valdis.Kletnieks@vt.edu - 2016-02-08 22:50 +0100
Re: [PATCH v12 2/4] PM / Domains: add setter for dev.pm_domain "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-08 23:00 +0100
Re: [PATCH v12 2/4] PM / Domains: add setter for dev.pm_domain Valdis.Kletnieks@vt.edu - 2016-02-09 21:20 +0100
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2016-01-29 16:40 +0100 |
| Subject | Re: [PATCH v12 2/4] PM / Domains: add setter for dev.pm_domain |
| Message-ID | <qWjjt-59f-23@gated-at.bofh.it> |
On Thu, Jan 7, 2016 at 5:46 PM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> Adds a function that sets the pointer to dev_pm_domain in struct device
> and that warns if the device has already finished probing. The reason
> why we want to enforce that is because in the general case that can
> cause problems and also that we can simplify code quite a bit if we can
> always assume that.
>
> This patch also changes all current code that directly sets the
> dev.pm_domain pointer.
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -875,13 +876,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>
> switch (action) {
> case BUS_NOTIFY_BIND_DRIVER:
> - pdev->dev.pm_domain = &acpi_lpss_pm_domain;
> + dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
> break;
> case BUS_NOTIFY_DRIVER_NOT_BOUND:
> case BUS_NOTIFY_UNBOUND_DRIVER:
> pdev->dev.pm_domain = NULL;
Missed?
> break;
> case BUS_NOTIFY_ADD_DEVICE:
> + dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
> if (pdata->dev_desc->flags & LPSS_LTR)
> return sysfs_create_group(&pdev->dev.kobj,
> &lpss_attr_group);
> @@ -889,6 +891,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
> case BUS_NOTIFY_DEL_DEVICE:
> if (pdata->dev_desc->flags & LPSS_LTR)
> sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
> + dev_pm_domain_set(&pdev->dev, NULL);
> break;
> default:
> break;
This looks wrong. I didn't test yet, but I have concerns here. Why did
you add those calls?
Have you been aware about the commit b5f88dd1d6ef ("Revert "ACPI /
LPSS: allow to use specific PM domain during ->probe()") ?
Sorry for being too late with this.
--
With Best Regards,
Andy Shevchenko
[toc] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2016-01-29 17:00 +0100 |
| Message-ID | <qWjCN-5kt-1@gated-at.bofh.it> |
| In reply to | #1321840 |
On Fri, Jan 29, 2016 at 5:38 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 7, 2016 at 5:46 PM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> Adds a function that sets the pointer to dev_pm_domain in struct device
>> and that warns if the device has already finished probing. The reason
>> why we want to enforce that is because in the general case that can
>> cause problems and also that we can simplify code quite a bit if we can
>> always assume that.
>>
>> This patch also changes all current code that directly sets the
>> dev.pm_domain pointer.
>
>
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>
>> @@ -875,13 +876,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>>
>> switch (action) {
>> case BUS_NOTIFY_BIND_DRIVER:
>> - pdev->dev.pm_domain = &acpi_lpss_pm_domain;
>> + dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
>> break;
>> case BUS_NOTIFY_DRIVER_NOT_BOUND:
>> case BUS_NOTIFY_UNBOUND_DRIVER:
>> pdev->dev.pm_domain = NULL;
>
> Missed?
>
>> break;
>> case BUS_NOTIFY_ADD_DEVICE:
>> + dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
>> if (pdata->dev_desc->flags & LPSS_LTR)
>> return sysfs_create_group(&pdev->dev.kobj,
>> &lpss_attr_group);
>> @@ -889,6 +891,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>> case BUS_NOTIFY_DEL_DEVICE:
>> if (pdata->dev_desc->flags & LPSS_LTR)
>> sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
>> + dev_pm_domain_set(&pdev->dev, NULL);
>> break;
>> default:
>> break;
>
> This looks wrong. I didn't test yet, but I have concerns here. Why did
> you add those calls?
Okay, for first glance it seems working on Intel Braswell. But can
yoyu point me out what was going on before this patch?
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2016-01-29 17:40 +0100 |
| Message-ID | <qWkfw-5Sw-15@gated-at.bofh.it> |
| In reply to | #1321846 |
On Fri, Jan 29, 2016 at 5:51 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jan 29, 2016 at 5:38 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Jan 7, 2016 at 5:46 PM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>> Adds a function that sets the pointer to dev_pm_domain in struct device
>>> and that warns if the device has already finished probing. The reason
>>> why we want to enforce that is because in the general case that can
>>> cause problems and also that we can simplify code quite a bit if we can
>>> always assume that.
>>>
>>> This patch also changes all current code that directly sets the
>>> dev.pm_domain pointer.
>>
>>
>>> --- a/drivers/acpi/acpi_lpss.c
>>> +++ b/drivers/acpi/acpi_lpss.c
>>
>>> @@ -875,13 +876,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>>>
>>> switch (action) {
>>> case BUS_NOTIFY_BIND_DRIVER:
>>> - pdev->dev.pm_domain = &acpi_lpss_pm_domain;
>>> + dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
>>> break;
>>> case BUS_NOTIFY_DRIVER_NOT_BOUND:
>>> case BUS_NOTIFY_UNBOUND_DRIVER:
>>> pdev->dev.pm_domain = NULL;
>>
>> Missed?
>>
>>> break;
>>> case BUS_NOTIFY_ADD_DEVICE:
>>> + dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
>>> if (pdata->dev_desc->flags & LPSS_LTR)
>>> return sysfs_create_group(&pdev->dev.kobj,
>>> &lpss_attr_group);
>>> @@ -889,6 +891,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>>> case BUS_NOTIFY_DEL_DEVICE:
>>> if (pdata->dev_desc->flags & LPSS_LTR)
>>> sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
>>> + dev_pm_domain_set(&pdev->dev, NULL);
>>> break;
>>> default:
>>> break;
>>
>> This looks wrong. I didn't test yet, but I have concerns here. Why did
>> you add those calls?
>
>
> Okay, for first glance it seems working on Intel Braswell.
One more test unveils this one
# modprobe -r sdhci-acpi
[ 1289.909441] ------------[ cut here ]------------
[ 1289.918205] WARNING: CPU: 1 PID: 4374 at
/home/andy/prj/linux-otc/drivers/base/power/common.c:150
dev_pm_domain_set+0x51/0x60()
[ 1289.934681] PM domains can only be changed for unbound devices
[ 1289.944843] Modules linked in: sdhci_acpi(-) sdhci mmc_core
led_class [last unloaded: dw_dmac_core]
[ 1289.958802] CPU: 1 PID: 4374 Comm: modprobe Not tainted 4.5.0-rc1+ #3
[ 1289.969736] ffffffff81c38330 ffff88007bb53d18 ffffffff8133162f
ffff88007bb53d60
[ 1289.981844] ffff88007bb53d50 ffffffff8105cd12 ffff88017a007410
0000000000000000
[ 1289.993996] 0000000000000001 0000000000000080 0000000000000000
ffff88007bb53db0
[ 1290.006123] Call Trace:
[ 1290.012600] [<ffffffff8133162f>] dump_stack+0x44/0x55
[ 1290.022052] [<ffffffff8105cd12>] warn_slowpath_common+0x82/0xc0
[ 1290.032462] [<ffffffff8105cd9c>] warn_slowpath_fmt+0x4c/0x50
[ 1290.042589] [<ffffffff814867c1>] dev_pm_domain_set+0x51/0x60
[ 1290.052695] [<ffffffff813a62a9>] acpi_dev_pm_detach+0x3f/0x84
[ 1290.062910] [<ffffffff81486747>] dev_pm_domain_detach+0x27/0x30
[ 1290.073294] [<ffffffff81480188>] platform_drv_remove+0x38/0x40
[ 1290.083585] [<ffffffff8147da51>] __device_release_driver+0xa1/0x160
[ 1290.094335] [<ffffffff8147e5a6>] driver_detach+0xa6/0xb0
[ 1290.104009] [<ffffffff8147d435>] bus_remove_driver+0x55/0xd0
[ 1290.114053] [<ffffffff8147eccc>] driver_unregister+0x2c/0x50
[ 1290.124076] [<ffffffff81480232>] platform_driver_unregister+0x12/0x20
[ 1290.134957] [<ffffffffa003681c>] sdhci_acpi_driver_exit+0x10/0x12
[sdhci_acpi]
[ 1290.146730] [<ffffffff810ce0e9>] SyS_delete_module+0x199/0x240
[ 1290.156907] [<ffffffff8100219b>] ? exit_to_usermode_loop+0x8b/0x90
[ 1290.167478] [<ffffffff81002b83>] do_syscall_32_irqs_off+0x53/0xa0
[ 1290.177949] [<ffffffff818b7f9a>] entry_INT80_compat+0x2a/0x40
[ 1290.187986] ---[ end trace a8351c0506ee239b ]---
[ 1290.213855] mmc0: card 0001 removed
Which function is guilty?
> But can
> yoyu point me out what was going on before this patch?
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-01-29 22:40 +0100 |
| Message-ID | <qWoVR-MS-15@gated-at.bofh.it> |
| In reply to | #1321877 |
On Friday, January 29, 2016 06:35:49 PM Andy Shevchenko wrote:
> On Fri, Jan 29, 2016 at 5:51 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 29, 2016 at 5:38 PM, Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Thu, Jan 7, 2016 at 5:46 PM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> >>> Adds a function that sets the pointer to dev_pm_domain in struct device
> >>> and that warns if the device has already finished probing. The reason
> >>> why we want to enforce that is because in the general case that can
> >>> cause problems and also that we can simplify code quite a bit if we can
> >>> always assume that.
> >>>
> >>> This patch also changes all current code that directly sets the
> >>> dev.pm_domain pointer.
> >>
> >>
> >>> --- a/drivers/acpi/acpi_lpss.c
> >>> +++ b/drivers/acpi/acpi_lpss.c
> >>
> >>> @@ -875,13 +876,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
> >>>
> >>> switch (action) {
> >>> case BUS_NOTIFY_BIND_DRIVER:
> >>> - pdev->dev.pm_domain = &acpi_lpss_pm_domain;
> >>> + dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
> >>> break;
> >>> case BUS_NOTIFY_DRIVER_NOT_BOUND:
> >>> case BUS_NOTIFY_UNBOUND_DRIVER:
> >>> pdev->dev.pm_domain = NULL;
> >>
> >> Missed?
> >>
> >>> break;
> >>> case BUS_NOTIFY_ADD_DEVICE:
> >>> + dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
> >>> if (pdata->dev_desc->flags & LPSS_LTR)
> >>> return sysfs_create_group(&pdev->dev.kobj,
> >>> &lpss_attr_group);
> >>> @@ -889,6 +891,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
> >>> case BUS_NOTIFY_DEL_DEVICE:
> >>> if (pdata->dev_desc->flags & LPSS_LTR)
> >>> sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
> >>> + dev_pm_domain_set(&pdev->dev, NULL);
> >>> break;
> >>> default:
> >>> break;
> >>
> >> This looks wrong. I didn't test yet, but I have concerns here. Why did
> >> you add those calls?
> >
> >
> > Okay, for first glance it seems working on Intel Braswell.
>
> One more test unveils this one
>
> # modprobe -r sdhci-acpi
> [ 1289.909441] ------------[ cut here ]------------
> [ 1289.918205] WARNING: CPU: 1 PID: 4374 at
> /home/andy/prj/linux-otc/drivers/base/power/common.c:150
> dev_pm_domain_set+0x51/0x60()
> [ 1289.934681] PM domains can only be changed for unbound devices
> [ 1289.944843] Modules linked in: sdhci_acpi(-) sdhci mmc_core
> led_class [last unloaded: dw_dmac_core]
> [ 1289.958802] CPU: 1 PID: 4374 Comm: modprobe Not tainted 4.5.0-rc1+ #3
> [ 1289.969736] ffffffff81c38330 ffff88007bb53d18 ffffffff8133162f
> ffff88007bb53d60
> [ 1289.981844] ffff88007bb53d50 ffffffff8105cd12 ffff88017a007410
> 0000000000000000
> [ 1289.993996] 0000000000000001 0000000000000080 0000000000000000
> ffff88007bb53db0
> [ 1290.006123] Call Trace:
> [ 1290.012600] [<ffffffff8133162f>] dump_stack+0x44/0x55
> [ 1290.022052] [<ffffffff8105cd12>] warn_slowpath_common+0x82/0xc0
> [ 1290.032462] [<ffffffff8105cd9c>] warn_slowpath_fmt+0x4c/0x50
> [ 1290.042589] [<ffffffff814867c1>] dev_pm_domain_set+0x51/0x60
> [ 1290.052695] [<ffffffff813a62a9>] acpi_dev_pm_detach+0x3f/0x84
> [ 1290.062910] [<ffffffff81486747>] dev_pm_domain_detach+0x27/0x30
> [ 1290.073294] [<ffffffff81480188>] platform_drv_remove+0x38/0x40
> [ 1290.083585] [<ffffffff8147da51>] __device_release_driver+0xa1/0x160
> [ 1290.094335] [<ffffffff8147e5a6>] driver_detach+0xa6/0xb0
> [ 1290.104009] [<ffffffff8147d435>] bus_remove_driver+0x55/0xd0
> [ 1290.114053] [<ffffffff8147eccc>] driver_unregister+0x2c/0x50
> [ 1290.124076] [<ffffffff81480232>] platform_driver_unregister+0x12/0x20
> [ 1290.134957] [<ffffffffa003681c>] sdhci_acpi_driver_exit+0x10/0x12
> [sdhci_acpi]
> [ 1290.146730] [<ffffffff810ce0e9>] SyS_delete_module+0x199/0x240
> [ 1290.156907] [<ffffffff8100219b>] ? exit_to_usermode_loop+0x8b/0x90
> [ 1290.167478] [<ffffffff81002b83>] do_syscall_32_irqs_off+0x53/0xa0
> [ 1290.177949] [<ffffffff818b7f9a>] entry_INT80_compat+0x2a/0x40
> [ 1290.187986] ---[ end trace a8351c0506ee239b ]---
> [ 1290.213855] mmc0: card 0001 removed
>
> Which function is guilty?
acpi_dev_pm_detach() calls dev_pm_domain_set() to clear the pm_domain pointer.
One might argue that this is happening too early.
Thanks,
Rafael
[toc] | [prev] | [next] | [standalone]
| From | Valdis.Kletnieks@vt.edu |
|---|---|
| Date | 2016-02-08 22:50 +0100 |
| Message-ID | <r01R1-2zw-23@gated-at.bofh.it> |
| In reply to | #1322113 |
On Fri, 29 Jan 2016 22:35:18 +0100, "Rafael J. Wysocki" said: > > One more test unveils this one > > > > # modprobe -r sdhci-acpi > > [ 1289.909441] ------------[ cut here ]------------ > > [ 1289.918205] WARNING: CPU: 1 PID: 4374 at > > /home/andy/prj/linux-otc/drivers/base/power/common.c:150 > > dev_pm_domain_set+0x51/0x60() > > [ 1289.934681] PM domains can only be changed for unbound devices > > [ 1289.944843] Modules linked in: sdhci_acpi(-) sdhci mmc_core > > led_class [last unloaded: dw_dmac_core] > > [ 1289.958802] CPU: 1 PID: 4374 Comm: modprobe Not tainted 4.5.0-rc1+ #3 > > [ 1289.969736] ffffffff81c38330 ffff88007bb53d18 ffffffff8133162f > > ffff88007bb53d60 > > [ 1289.981844] ffff88007bb53d50 ffffffff8105cd12 ffff88017a007410 > > 0000000000000000 > > [ 1289.993996] 0000000000000001 0000000000000080 0000000000000000 > > ffff88007bb53db0 > > [ 1290.006123] Call Trace: > > [ 1290.012600] [<ffffffff8133162f>] dump_stack+0x44/0x55 > > [ 1290.022052] [<ffffffff8105cd12>] warn_slowpath_common+0x82/0xc0 > > [ 1290.032462] [<ffffffff8105cd9c>] warn_slowpath_fmt+0x4c/0x50 > > [ 1290.042589] [<ffffffff814867c1>] dev_pm_domain_set+0x51/0x60 My Dell Latitude laptopi on next-20160201 is throwing a similar error at shutdown, except the traceback continues: mei_me_remove+0xbd/0xc0 pci_device_shutdown+0x32/0x50 device_shutdown+0xe2/0x210 (sorry, that's all I have, transcribing from one frame of video shot while the system was shutting down....)
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-02-08 23:00 +0100 |
| Message-ID | <r020F-2D0-5@gated-at.bofh.it> |
| In reply to | #1329601 |
On Mon, Feb 8, 2016 at 10:44 PM, <Valdis.Kletnieks@vt.edu> wrote: > On Fri, 29 Jan 2016 22:35:18 +0100, "Rafael J. Wysocki" said: >> > One more test unveils this one >> > >> > # modprobe -r sdhci-acpi >> > [ 1289.909441] ------------[ cut here ]------------ >> > [ 1289.918205] WARNING: CPU: 1 PID: 4374 at >> > /home/andy/prj/linux-otc/drivers/base/power/common.c:150 >> > dev_pm_domain_set+0x51/0x60() >> > [ 1289.934681] PM domains can only be changed for unbound devices >> > [ 1289.944843] Modules linked in: sdhci_acpi(-) sdhci mmc_core >> > led_class [last unloaded: dw_dmac_core] >> > [ 1289.958802] CPU: 1 PID: 4374 Comm: modprobe Not tainted 4.5.0-rc1+ #3 >> > [ 1289.969736] ffffffff81c38330 ffff88007bb53d18 ffffffff8133162f >> > ffff88007bb53d60 >> > [ 1289.981844] ffff88007bb53d50 ffffffff8105cd12 ffff88017a007410 >> > 0000000000000000 >> > [ 1289.993996] 0000000000000001 0000000000000080 0000000000000000 >> > ffff88007bb53db0 >> > [ 1290.006123] Call Trace: >> > [ 1290.012600] [<ffffffff8133162f>] dump_stack+0x44/0x55 >> > [ 1290.022052] [<ffffffff8105cd12>] warn_slowpath_common+0x82/0xc0 >> > [ 1290.032462] [<ffffffff8105cd9c>] warn_slowpath_fmt+0x4c/0x50 >> > [ 1290.042589] [<ffffffff814867c1>] dev_pm_domain_set+0x51/0x60 > > My Dell Latitude laptopi on next-20160201 is throwing a similar error > at shutdown, except the traceback continues: > > mei_me_remove+0xbd/0xc0 > pci_device_shutdown+0x32/0x50 > device_shutdown+0xe2/0x210 > > (sorry, that's all I have, transcribing from one frame of video shot while > the system was shutting down....) This should be fixed in 4.5-rc3, so please retest. Thanks, Rafael
[toc] | [prev] | [next] | [standalone]
| From | Valdis.Kletnieks@vt.edu |
|---|---|
| Date | 2016-02-09 21:20 +0100 |
| Message-ID | <r0mVr-wW-9@gated-at.bofh.it> |
| In reply to | #1329606 |
[Multipart message — attachments visible in raw view] — view raw
On Mon, 08 Feb 2016 22:50:39 +0100, "Rafael J. Wysocki" said: > On Mon, Feb 8, 2016 at 10:44 PM, <Valdis.Kletnieks@vt.edu> wrote: > > My Dell Latitude laptopi on next-20160201 is throwing a similar error > > at shutdown, except the traceback continues: > > > > mei_me_remove+0xbd/0xc0 > > pci_device_shutdown+0x32/0x50 > > device_shutdown+0xe2/0x210 > > > > (sorry, that's all I have, transcribing from one frame of video shot while > > the system was shutting down....) > > This should be fixed in 4.5-rc3, so please retest. Confirming that next-20160201 has the problem, but it is fixed as of next-20160208.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web