Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1278288 > unrolled thread

[PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain during ->probe()

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.

Contents

  [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain during ->probe() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2015-11-26 16:30 +0100
    Re: [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain  during ->probe() Jarkko Nikula <jarkko.nikula@linux.intel.com> - 2015-11-26 17:40 +0100
      Re: [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain  during ->probe() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2015-11-26 17:50 +0100
        Re: [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain during ->probe() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-11-26 23:50 +0100
          Re: [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain  during ->probe() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2015-11-27 11:00 +0100
        Re: [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain  during ->probe() Jarkko Nikula <jarkko.nikula@linux.intel.com> - 2015-11-27 08:10 +0100
          Re: [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain  during ->probe() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2015-11-27 11:10 +0100

#1278288 — [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain during ->probe()

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:
 		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;
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1278333 — Re: [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain during ->probe()

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).

-- 
Jarkko

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278344 — Re: [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain during ->probe()

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).
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278477

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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278688 — Re: [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain during ->probe()

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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278569 — Re: [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain during ->probe()

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.

-- 
Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278689 — Re: [PATCH v2 2/7] ACPI / LPSS: allow to use specific PM domain during ->probe()

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


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web