| Message ID | 1448551153-84719-3-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 11/26/2015 05:19 PM, Andy Shevchenko wrote: > This is an amendment to previously pushed commit 01ac170ba29a (ACPI / LPSS: > allow to use specific PM domain during ->probe()). We can't assign anything to > the platform device on ADD_DEVICE stage since it might be changed during > unbound / bind cycle. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/acpi/acpi_lpss.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c > index f9e0d09..e840229 100644 > --- a/drivers/acpi/acpi_lpss.c > +++ b/drivers/acpi/acpi_lpss.c > @@ -705,8 +705,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb, > } > > switch (action) { > - case BUS_NOTIFY_ADD_DEVICE: > + case BUS_NOTIFY_BIND_DRIVER: > pdev->dev.pm_domain = &acpi_lpss_pm_domain; > + break; > + case BUS_NOTIFY_BIND_DRIVER_ERROR: > + case BUS_NOTIFY_UNBOUND_DRIVER: > + pdev->dev.pm_domain = NULL; > + break; > + case BUS_NOTIFY_ADD_DEVICE: This won't fix like revert of original commit does. Primary problem here is that there is no explicit power on at all during LPSS device probe because dev->pm_domain is set before probing. driver_probe_device platform_drv_prove dev_pm_domain_attach acpi_dev_pm_attach returns instantly because of dev->pm_domain is set Problem got unnoticed because devices are on during boot but rmmod/modprobe cycle will hit it. With this patch situation remains the same but works if you assign the domain after probing when BUS_NOTIFY_BOUND_DRIVER notification comes (s/BUS_NOTIFY_BIND_DRIVER/BUS_NOTIFY_BOUND_DRIVER/ above).
On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote: > On 11/26/2015 05:19 PM, Andy Shevchenko wrote: > > This is an amendment to previously pushed commit 01ac170ba29a (ACPI > > / LPSS: > > allow to use specific PM domain during ->probe()). We can't assign > > anything to > > the platform device on ADD_DEVICE stage since it might be changed > > during > > unbound / bind cycle. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/acpi/acpi_lpss.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c > > index f9e0d09..e840229 100644 > > --- a/drivers/acpi/acpi_lpss.c > > +++ b/drivers/acpi/acpi_lpss.c > > @@ -705,8 +705,14 @@ static int acpi_lpss_platform_notify(struct > > notifier_block *nb, > > } > > > > switch (action) { > > - case BUS_NOTIFY_ADD_DEVICE: > > + case BUS_NOTIFY_BIND_DRIVER: > > pdev->dev.pm_domain = &acpi_lpss_pm_domain; > > + break; > > + case BUS_NOTIFY_BIND_DRIVER_ERROR: > > + case BUS_NOTIFY_UNBOUND_DRIVER: > > + pdev->dev.pm_domain = NULL; > > + break; > > + case BUS_NOTIFY_ADD_DEVICE: > > This won't fix like revert of original commit does. Primary problem > here > is that there is no explicit power on at all during LPSS device probe > because dev->pm_domain is set before probing. And we can't do this as in very original code of acpi_lpss.c since DMA has to be sure it's powered on while probing. We could guarantee this only in case when PM domain is assigned already and we do our quirk for it. From my point of view we have to fix hang first since it's most painful case for users and their experience. Though I'm open to any better solution if you have any in mind. > > driver_probe_device > platform_drv_prove > dev_pm_domain_attach > acpi_dev_pm_attach > returns instantly because of dev->pm_domain is set > > Problem got unnoticed because devices are on during boot but > rmmod/modprobe cycle will hit it. > > With this patch situation remains the same but works if you assign > the > domain after probing when BUS_NOTIFY_BOUND_DRIVER notification comes > (s/BUS_NOTIFY_BIND_DRIVER/BUS_NOTIFY_BOUND_DRIVER/ above). >
On Thursday, November 26, 2015 06:45:17 PM Andy Shevchenko wrote: > On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote: > > On 11/26/2015 05:19 PM, Andy Shevchenko wrote: > > > This is an amendment to previously pushed commit 01ac170ba29a (ACPI > > > / LPSS: > > > allow to use specific PM domain during ->probe()). We can't assign > > > anything to > > > the platform device on ADD_DEVICE stage since it might be changed > > > during > > > unbound / bind cycle. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > drivers/acpi/acpi_lpss.c | 9 +++++++-- > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c > > > index f9e0d09..e840229 100644 > > > --- a/drivers/acpi/acpi_lpss.c > > > +++ b/drivers/acpi/acpi_lpss.c > > > @@ -705,8 +705,14 @@ static int acpi_lpss_platform_notify(struct > > > notifier_block *nb, > > > } > > > > > > switch (action) { > > > - case BUS_NOTIFY_ADD_DEVICE: > > > + case BUS_NOTIFY_BIND_DRIVER: > > > pdev->dev.pm_domain = &acpi_lpss_pm_domain; > > > + break; > > > + case BUS_NOTIFY_BIND_DRIVER_ERROR: > > > + case BUS_NOTIFY_UNBOUND_DRIVER: > > > + pdev->dev.pm_domain = NULL; > > > + break; > > > + case BUS_NOTIFY_ADD_DEVICE: > > > > This won't fix like revert of original commit does. Which commit exactly are you talking about, for reference? > > Primary problem here > > is that there is no explicit power on at all during LPSS device probe > > because dev->pm_domain is set before probing. > > And we can't do this as in very original code of acpi_lpss.c since DMA > has to be sure it's powered on while probing. We could guarantee this > only in case when PM domain is assigned already and we do our quirk for > it. > > From my point of view we have to fix hang first since it's most painful > case for users and their experience. Though I'm open to any better > solution if you have any in mind. > > > > > driver_probe_device > > platform_drv_prove > > dev_pm_domain_attach > > acpi_dev_pm_attach > > returns instantly because of dev->pm_domain is set This looks like a candidate for the new PM domain callbacks, ->activate and ->dismiss. ->activate() is called before the probe, so it may power up things. ->dismiss() in turn is called in the failed probe case, so it can do the cleanup. Have you considered using these? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/26/2015 06:45 PM, Andy Shevchenko wrote: > On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote: >> On 11/26/2015 05:19 PM, Andy Shevchenko wrote: >>> This is an amendment to previously pushed commit 01ac170ba29a (ACPI >>> / LPSS: >>> allow to use specific PM domain during ->probe()). We can't assign >>> anything to >>> the platform device on ADD_DEVICE stage since it might be changed >>> during >>> unbound / bind cycle. >>> >>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>> --- >>> drivers/acpi/acpi_lpss.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c >>> index f9e0d09..e840229 100644 >>> --- a/drivers/acpi/acpi_lpss.c >>> +++ b/drivers/acpi/acpi_lpss.c >>> @@ -705,8 +705,14 @@ static int acpi_lpss_platform_notify(struct >>> notifier_block *nb, >>> } >>> >>> switch (action) { >>> - case BUS_NOTIFY_ADD_DEVICE: >>> + case BUS_NOTIFY_BIND_DRIVER: >>> pdev->dev.pm_domain = &acpi_lpss_pm_domain; >>> + break; >>> + case BUS_NOTIFY_BIND_DRIVER_ERROR: >>> + case BUS_NOTIFY_UNBOUND_DRIVER: >>> + pdev->dev.pm_domain = NULL; >>> + break; >>> + case BUS_NOTIFY_ADD_DEVICE: >> >> This won't fix like revert of original commit does. Primary problem >> here >> is that there is no explicit power on at all during LPSS device probe >> because dev->pm_domain is set before probing. > > And we can't do this as in very original code of acpi_lpss.c since DMA > has to be sure it's powered on while probing. We could guarantee this > only in case when PM domain is assigned already and we do our quirk for > it. > I'm not sure do I follow here. Is the power on chain different for LPSS DMA because setting the domain at BIND stage prevents the call to acpi_dev_pm_full_power() before driver probe? See below. driver_probe_device really_probe driver_sysfs_add -> BUS_NOTIFY_BIND_DRIVER -> platform_drv_prove dev_pm_domain_attach acpi_dev_pm_attach if (dev->pm_domain) return -EEXIST; ... if (power_on) { acpi_dev_pm_full_power(adev);... } -> probe driver_bound -> BUS_NOTIFY_BOUND_DRIVER > From my point of view we have to fix hang first since it's most painful > case for users and their experience. Though I'm open to any better > solution if you have any in mind. > Sure.
On Fri, 2015-11-27 at 00:15 +0100, Rafael J. Wysocki wrote: > On Thursday, November 26, 2015 06:45:17 PM Andy Shevchenko wrote: > > On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote: > > > On 11/26/2015 05:19 PM, Andy Shevchenko wrote: > > > > This is an amendment to previously pushed commit 01ac170ba29a > > > > (ACPI > > > > / LPSS: > > > > allow to use specific PM domain during ->probe()). We can't > > > > assign > > > > anything to > > > > the platform device on ADD_DEVICE stage since it might be > > > > changed > > > > during > > > > unbound / bind cycle. > > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.c > > > > om> > > > > --- > > > > drivers/acpi/acpi_lpss.c | 9 +++++++-- > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/acpi/acpi_lpss.c > > > > b/drivers/acpi/acpi_lpss.c > > > > index f9e0d09..e840229 100644 > > > > --- a/drivers/acpi/acpi_lpss.c > > > > +++ b/drivers/acpi/acpi_lpss.c > > > > @@ -705,8 +705,14 @@ static int > > > > acpi_lpss_platform_notify(struct > > > > notifier_block *nb, > > > > } > > > > > > > > switch (action) { > > > > - case BUS_NOTIFY_ADD_DEVICE: > > > > + case BUS_NOTIFY_BIND_DRIVER: > > > > pdev->dev.pm_domain = &acpi_lpss_pm_domain; > > > > + break; > > > > + case BUS_NOTIFY_BIND_DRIVER_ERROR: > > > > + case BUS_NOTIFY_UNBOUND_DRIVER: > > > > + pdev->dev.pm_domain = NULL; > > > > + break; > > > > + case BUS_NOTIFY_ADD_DEVICE: > > > > > > This won't fix like revert of original commit does. > > Which commit exactly are you talking about, for reference? commit 01ac170ba29a9903ee590e1ef2d8e6b27b49a16c Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: Wed Nov 5 18:34:46 2014 +0200 ACPI / LPSS: allow to use specific PM domain during ->probe() > > > Primary problem here > > > is that there is no explicit power on at all during LPSS device > > > probe > > > because dev->pm_domain is set before probing. > > > > And we can't do this as in very original code of acpi_lpss.c since > > DMA > > has to be sure it's powered on while probing. We could guarantee > > this > > only in case when PM domain is assigned already and we do our quirk > > for > > it. > > > > From my point of view we have to fix hang first since it's most > > painful > > case for users and their experience. Though I'm open to any better > > solution if you have any in mind. > > > > > > > > driver_probe_device > > > platform_drv_prove > > > dev_pm_domain_attach > > > acpi_dev_pm_attach > > > returns instantly because of dev->pm_domain is set > > This looks like a candidate for the new PM domain callbacks, > ->activate and > ->dismiss. > > ->activate() is called before the probe, so it may power up things. > > ->dismiss() in turn is called in the failed probe case, so it can do > the > cleanup. > > Have you considered using these? Thanks for the hint. We will check this.
On Fri, 2015-11-27 at 09:05 +0200, Jarkko Nikula wrote: > On 11/26/2015 06:45 PM, Andy Shevchenko wrote: > > On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote: > > > On 11/26/2015 05:19 PM, Andy Shevchenko wrote: > > > > This is an amendment to previously pushed commit 01ac170ba29a > > > > (ACPI > > > > / LPSS: > > > > allow to use specific PM domain during ->probe()). We can't > > > > assign > > > > anything to > > > > the platform device on ADD_DEVICE stage since it might be > > > > changed > > > > during > > > > unbound / bind cycle. > > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.c > > > > om> > > > > --- > > > > drivers/acpi/acpi_lpss.c | 9 +++++++-- > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/acpi/acpi_lpss.c > > > > b/drivers/acpi/acpi_lpss.c > > > > index f9e0d09..e840229 100644 > > > > --- a/drivers/acpi/acpi_lpss.c > > > > +++ b/drivers/acpi/acpi_lpss.c > > > > @@ -705,8 +705,14 @@ static int > > > > acpi_lpss_platform_notify(struct > > > > notifier_block *nb, > > > > } > > > > > > > > switch (action) { > > > > - case BUS_NOTIFY_ADD_DEVICE: > > > > + case BUS_NOTIFY_BIND_DRIVER: > > > > pdev->dev.pm_domain = &acpi_lpss_pm_domain; > > > > + break; > > > > + case BUS_NOTIFY_BIND_DRIVER_ERROR: > > > > + case BUS_NOTIFY_UNBOUND_DRIVER: > > > > + pdev->dev.pm_domain = NULL; > > > > + break; > > > > + case BUS_NOTIFY_ADD_DEVICE: > > > > > > This won't fix like revert of original commit does. Primary > > > problem > > > here > > > is that there is no explicit power on at all during LPSS device > > > probe > > > because dev->pm_domain is set before probing. > > > > And we can't do this as in very original code of acpi_lpss.c since > > DMA > > has to be sure it's powered on while probing. We could guarantee > > this > > only in case when PM domain is assigned already and we do our quirk > > for > > it. > > > I'm not sure do I follow here. Is the power on chain different for > LPSS > DMA because setting the domain at BIND stage prevents the call to > acpi_dev_pm_full_power() before driver probe? See below. > > driver_probe_device > really_probe > driver_sysfs_add -> BUS_NOTIFY_BIND_DRIVER > -> platform_drv_prove > dev_pm_domain_attach > acpi_dev_pm_attach > if (dev->pm_domain) return -EEXIST; > ... > if (power_on) { acpi_dev_pm_full_power(adev);... } And how exactly does it enable a power on DMA controller that doesn't have _PS0 / _PS3 / _PSC methods? Maybe I missed something obvious. > -> probe > driver_bound -> BUS_NOTIFY_BOUND_DRIVER
On Fri, 2015-11-27 at 11:56 +0200, Andy Shevchenko wrote: > > > > > > On Fri, 2015-11-27 at 00:15 +0100, Rafael J. Wysocki wrote: > > On Thursday, November 26, 2015 06:45:17 PM Andy Shevchenko wrote: > > > On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote: > > > > On 11/26/2015 05:19 PM, Andy Shevchenko wrote: > > > > This won't fix like revert of original commit does. Jarkko, I will split this one to the revert (with Fixes tag) and new patch to target DMA issue. > > > > Primary problem here > > > > is that there is no explicit power on at all during LPSS device > > > > probe > > > > because dev->pm_domain is set before probing. > > > > > > And we can't do this as in very original code of acpi_lpss.c > > > since > > > DMA > > > has to be sure it's powered on while probing. We could guarantee > > > this > > > only in case when PM domain is assigned already and we do our > > > quirk > > > for > > > it. > > > > > > From my point of view we have to fix hang first since it's most > > > painful > > > case for users and their experience. Though I'm open to any > > > better > > > solution if you have any in mind. > > > > > > > > > > > driver_probe_device > > > > platform_drv_prove > > > > dev_pm_domain_attach > > > > acpi_dev_pm_attach > > > > returns instantly because of dev->pm_domain is set > > > > This looks like a candidate for the new PM domain callbacks, > > ->activate and > > ->dismiss. > > > > ->activate() is called before the probe, so it may power up things. > > > > ->dismiss() in turn is called in the failed probe case, so it can > > do > > the > > cleanup. > > > > Have you considered using these? > > Thanks for the hint. We will check this. I briefly checked this for DMA issue. It will not help anyhow, so we *have to* move a power domain assignment to the BIND stage. For I2C and rest LPSS devices this might help (though didn't look deeply). My understanding that we assign those callbacks in the LPSS custom PM domain and call them explicitly in acpi_lpss.c. The code will be the same as we are using now to bring device from runtime suspend resume. This means whenever we call probe for e.g. I2C we end up in a sequence similar to: pm_runtime_resume(I2C); ->probe(I2C); pm_runtime_suspend(I2C); I will try to mock up this and check if it will work, though have no idea what to do if I2C during probe calls pm_runtime_forbid(). Jarkko, what do you think? -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On 12/03/2015 09:29 PM, Shevchenko, Andriy wrote: > I briefly checked this for DMA issue. It will not help anyhow, so we > *have to* move a power domain assignment to the BIND stage. > > For I2C and rest LPSS devices this might help (though didn't look > deeply). My understanding that we assign those callbacks in the LPSS > custom PM domain and call them explicitly in acpi_lpss.c. > > The code will be the same as we are using now to bring device from > runtime suspend resume. This means whenever we call probe for e.g. I2C > we end up in a sequence similar to: > pm_runtime_resume(I2C); > ->probe(I2C); > pm_runtime_suspend(I2C); > > I will try to mock up this and check if it will work, though have no > idea what to do if I2C during probe calls pm_runtime_forbid(). > > Jarkko, what do you think? > I suppose device core will handle it. If the runtime PM is forbidden or not initialized at all the device shouldn't idle.
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index f9e0d09..e840229 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -705,8 +705,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb, } switch (action) { - case BUS_NOTIFY_ADD_DEVICE: + case BUS_NOTIFY_BIND_DRIVER: pdev->dev.pm_domain = &acpi_lpss_pm_domain; + break; + case BUS_NOTIFY_BIND_DRIVER_ERROR: + case BUS_NOTIFY_UNBOUND_DRIVER: + pdev->dev.pm_domain = NULL; + break; + case BUS_NOTIFY_ADD_DEVICE: if (pdata->dev_desc->flags & LPSS_LTR) return sysfs_create_group(&pdev->dev.kobj, &lpss_attr_group); @@ -714,7 +720,6 @@ 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); - pdev->dev.pm_domain = NULL; break; default: break;
This is an amendment to previously pushed commit 01ac170ba29a (ACPI / LPSS: allow to use specific PM domain during ->probe()). We can't assign anything to the platform device on ADD_DEVICE stage since it might be changed during unbound / bind cycle. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/acpi/acpi_lpss.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)