Re: [PATCH] pnp: Bypass the calling to pnp_stop_dev at suspend when there is a protocol suspend

2014-01-05 Thread Rafael J. Wysocki 
On Monday, January 06, 2014 09:20:51 AM Yanmin Zhang wrote:
> On 二, 2013-12-24 at 09:35 +0800, shuox@intel.com wrote:
> > From: Zhang Yanmin 
> > 
> > pnp pnp_bus_suspend/_resume have an issue.
> > pnp_bus_suspend calls pnp_stop_dev to disable the device. With ACPI,
> > pnp_stop_dev turns off the dev usually. Then,
> > pnp_bus_suspend=>pnp_dev->protocol->suspend accesses the device and
> > suspend it again.
> > 
> > pnp_bus_resume has the similar issue.
> > 
> > Another issue is firmware might just provide _DIS, but no_STS method.
> > 
> > The patch fixes it by adding a checking. If there is
> > pnp_dev->protocol->suspend, pnp_bus_suspend doesn't call pnp_stop_dev.
> > Do the similar thing for _resume.
> Rafael,
> 
> What's your idea about this patch?

I haven't had the time to look deeper into that, sorry.  I'll let you know
when I do that.

Thanks!

> We hit the issue when enabling Android on a latest tablet. After
> suspend-to-ram wakeup, serial console doesn't work.
> This serial port is bound by pnpcore driver.
> At suspending, 
> static int __pnp_bus_suspend(struct device *dev, pm_message_t state)
> {
>   ...
> 
> if (pnp_can_disable(pnp_dev)) {
> error = pnp_stop_dev(pnp_dev);
> if (error)
> return error;
> }
> 
> if (pnp_dev->protocol->suspend)
> pnp_dev->protocol->suspend(pnp_dev, state);
> return 0;
> }
> 
> pnp_stop_dev calls dev->protocol->disable.
> As for ACPI device, that disable callback calls _DIS. Based
> on ACPI spec, driver need turn off the device before disabling it
> by _DIS. That means, after pnp_stop_dev returns, the device is at OFF
> state.
> 
> Then, __pnp_bus_suspend calls pnp_dev->protocol->suspend, which
> continues to access the device while the device is at OFF.
> 
> Our firmware just provides _DIS for the device. There is no _STS
> method. Then, after wakeup, the device doesn't work.
> 
> But just like what the patch points out, pnp_dev->protocol->suspend
> continues to access the device while the device is at OFF. It's not safe.
> 
> The patch looks like a workaround. Another possible fix is to just
> delete the calling of pnp_stop_dev in function __pnp_bus_suspend, as
> suspend is not equal to _disable_.
> The deletion might be a little intrusive. That's why we sent a workaround
> patch to LKML.
> 
> Which one is better?
> 
> Thanks,
> Yanmin
> 
> > 
> > Signed-off-by: Zhang Yanmin 
> > Signed-off-by: Liu ShuoX 
> > ---
> >  drivers/pnp/driver.c | 6 ++
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
> > index f748cc8..2512e47 100644
> > --- a/drivers/pnp/driver.c
> > +++ b/drivers/pnp/driver.c
> > @@ -176,7 +176,7 @@ static int __pnp_bus_suspend(struct device *dev, 
> > pm_message_t state)
> > return error;
> > }
> >  
> > -   if (pnp_can_disable(pnp_dev)) {
> > +   if (pnp_can_disable(pnp_dev) && !pnp_dev->protocol->suspend) {
> > error = pnp_stop_dev(pnp_dev);
> > if (error)
> > return error;
> > @@ -215,9 +215,7 @@ static int pnp_bus_resume(struct device *dev)
> > error = pnp_dev->protocol->resume(pnp_dev);
> > if (error)
> > return error;
> > -   }
> > -
> > -   if (pnp_can_write(pnp_dev)) {
> > +   } else if (pnp_can_write(pnp_dev)) {
> > error = pnp_start_dev(pnp_dev);
> > if (error)
> > return error;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] pnp: Bypass the calling to pnp_stop_dev at suspend when there is a protocol suspend

2014-01-05 Thread Yanmin Zhang 
On 二, 2013-12-24 at 09:35 +0800, shuox@intel.com wrote:
> From: Zhang Yanmin 
> 
> pnp pnp_bus_suspend/_resume have an issue.
> pnp_bus_suspend calls pnp_stop_dev to disable the device. With ACPI,
> pnp_stop_dev turns off the dev usually. Then,
> pnp_bus_suspend=>pnp_dev->protocol->suspend accesses the device and
> suspend it again.
> 
> pnp_bus_resume has the similar issue.
> 
> Another issue is firmware might just provide _DIS, but no_STS method.
> 
> The patch fixes it by adding a checking. If there is
> pnp_dev->protocol->suspend, pnp_bus_suspend doesn't call pnp_stop_dev.
> Do the similar thing for _resume.
Rafael,

What's your idea about this patch?

We hit the issue when enabling Android on a latest tablet. After
suspend-to-ram wakeup, serial console doesn't work.
This serial port is bound by pnpcore driver.
At suspending, 
static int __pnp_bus_suspend(struct device *dev, pm_message_t state)
{
...

if (pnp_can_disable(pnp_dev)) {
error = pnp_stop_dev(pnp_dev);
if (error)
return error;
}

if (pnp_dev->protocol->suspend)
pnp_dev->protocol->suspend(pnp_dev, state);
return 0;
}

pnp_stop_dev calls dev->protocol->disable.
As for ACPI device, that disable callback calls _DIS. Based
on ACPI spec, driver need turn off the device before disabling it
by _DIS. That means, after pnp_stop_dev returns, the device is at OFF
state.

Then, __pnp_bus_suspend calls pnp_dev->protocol->suspend, which
continues to access the device while the device is at OFF.

Our firmware just provides _DIS for the device. There is no _STS
method. Then, after wakeup, the device doesn't work.

But just like what the patch points out, pnp_dev->protocol->suspend
continues to access the device while the device is at OFF. It's not safe.

The patch looks like a workaround. Another possible fix is to just
delete the calling of pnp_stop_dev in function __pnp_bus_suspend, as
suspend is not equal to _disable_.
The deletion might be a little intrusive. That's why we sent a workaround
patch to LKML.

Which one is better?

Thanks,
Yanmin

> 
> Signed-off-by: Zhang Yanmin 
> Signed-off-by: Liu ShuoX 
> ---
>  drivers/pnp/driver.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
> index f748cc8..2512e47 100644
> --- a/drivers/pnp/driver.c
> +++ b/drivers/pnp/driver.c
> @@ -176,7 +176,7 @@ static int __pnp_bus_suspend(struct device *dev, 
> pm_message_t state)
>   return error;
>   }
>  
> - if (pnp_can_disable(pnp_dev)) {
> + if (pnp_can_disable(pnp_dev) && !pnp_dev->protocol->suspend) {
>   error = pnp_stop_dev(pnp_dev);
>   if (error)
>   return error;
> @@ -215,9 +215,7 @@ static int pnp_bus_resume(struct device *dev)
>   error = pnp_dev->protocol->resume(pnp_dev);
>   if (error)
>   return error;
> - }
> -
> - if (pnp_can_write(pnp_dev)) {
> + } else if (pnp_can_write(pnp_dev)) {
>   error = pnp_start_dev(pnp_dev);
>   if (error)
>   return error;


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

Re: [PATCH] pnp: Bypass the calling to pnp_stop_dev at suspend when there is a protocol suspend

2014-01-05 Thread Yanmin Zhang 
On 二, 2013-12-24 at 09:35 +0800, shuox@intel.com wrote:
 From: Zhang Yanmin yanmin.zh...@intel.com
 
 pnp pnp_bus_suspend/_resume have an issue.
 pnp_bus_suspend calls pnp_stop_dev to disable the device. With ACPI,
 pnp_stop_dev turns off the dev usually. Then,
 pnp_bus_suspend=pnp_dev-protocol-suspend accesses the device and
 suspend it again.
 
 pnp_bus_resume has the similar issue.
 
 Another issue is firmware might just provide _DIS, but no_STS method.
 
 The patch fixes it by adding a checking. If there is
 pnp_dev-protocol-suspend, pnp_bus_suspend doesn't call pnp_stop_dev.
 Do the similar thing for _resume.
Rafael,

What's your idea about this patch?

We hit the issue when enabling Android on a latest tablet. After
suspend-to-ram wakeup, serial console doesn't work.
This serial port is bound by pnpcore driver.
At suspending, 
static int __pnp_bus_suspend(struct device *dev, pm_message_t state)
{
...

if (pnp_can_disable(pnp_dev)) {
error = pnp_stop_dev(pnp_dev);
if (error)
return error;
}

if (pnp_dev-protocol-suspend)
pnp_dev-protocol-suspend(pnp_dev, state);
return 0;
}

pnp_stop_dev calls dev-protocol-disable.
As for ACPI device, that disable callback calls _DIS. Based
on ACPI spec, driver need turn off the device before disabling it
by _DIS. That means, after pnp_stop_dev returns, the device is at OFF
state.

Then, __pnp_bus_suspend calls pnp_dev-protocol-suspend, which
continues to access the device while the device is at OFF.

Our firmware just provides _DIS for the device. There is no _STS
method. Then, after wakeup, the device doesn't work.

But just like what the patch points out, pnp_dev-protocol-suspend
continues to access the device while the device is at OFF. It's not safe.

The patch looks like a workaround. Another possible fix is to just
delete the calling of pnp_stop_dev in function __pnp_bus_suspend, as
suspend is not equal to _disable_.
The deletion might be a little intrusive. That's why we sent a workaround
patch to LKML.

Which one is better?

Thanks,
Yanmin

 
 Signed-off-by: Zhang Yanmin yanmin.zh...@intel.com
 Signed-off-by: Liu ShuoX shuox@intel.com
 ---
  drivers/pnp/driver.c | 6 ++
  1 file changed, 2 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
 index f748cc8..2512e47 100644
 --- a/drivers/pnp/driver.c
 +++ b/drivers/pnp/driver.c
 @@ -176,7 +176,7 @@ static int __pnp_bus_suspend(struct device *dev, 
 pm_message_t state)
   return error;
   }
  
 - if (pnp_can_disable(pnp_dev)) {
 + if (pnp_can_disable(pnp_dev)  !pnp_dev-protocol-suspend) {
   error = pnp_stop_dev(pnp_dev);
   if (error)
   return error;
 @@ -215,9 +215,7 @@ static int pnp_bus_resume(struct device *dev)
   error = pnp_dev-protocol-resume(pnp_dev);
   if (error)
   return error;
 - }
 -
 - if (pnp_can_write(pnp_dev)) {
 + } else if (pnp_can_write(pnp_dev)) {
   error = pnp_start_dev(pnp_dev);
   if (error)
   return error;


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

Re: [PATCH] pnp: Bypass the calling to pnp_stop_dev at suspend when there is a protocol suspend

2014-01-05 Thread Rafael J. Wysocki 
On Monday, January 06, 2014 09:20:51 AM Yanmin Zhang wrote:
 On 二, 2013-12-24 at 09:35 +0800, shuox@intel.com wrote:
  From: Zhang Yanmin yanmin.zh...@intel.com
  
  pnp pnp_bus_suspend/_resume have an issue.
  pnp_bus_suspend calls pnp_stop_dev to disable the device. With ACPI,
  pnp_stop_dev turns off the dev usually. Then,
  pnp_bus_suspend=pnp_dev-protocol-suspend accesses the device and
  suspend it again.
  
  pnp_bus_resume has the similar issue.
  
  Another issue is firmware might just provide _DIS, but no_STS method.
  
  The patch fixes it by adding a checking. If there is
  pnp_dev-protocol-suspend, pnp_bus_suspend doesn't call pnp_stop_dev.
  Do the similar thing for _resume.
 Rafael,
 
 What's your idea about this patch?

I haven't had the time to look deeper into that, sorry.  I'll let you know
when I do that.

Thanks!

 We hit the issue when enabling Android on a latest tablet. After
 suspend-to-ram wakeup, serial console doesn't work.
 This serial port is bound by pnpcore driver.
 At suspending, 
 static int __pnp_bus_suspend(struct device *dev, pm_message_t state)
 {
   ...
 
 if (pnp_can_disable(pnp_dev)) {
 error = pnp_stop_dev(pnp_dev);
 if (error)
 return error;
 }
 
 if (pnp_dev-protocol-suspend)
 pnp_dev-protocol-suspend(pnp_dev, state);
 return 0;
 }
 
 pnp_stop_dev calls dev-protocol-disable.
 As for ACPI device, that disable callback calls _DIS. Based
 on ACPI spec, driver need turn off the device before disabling it
 by _DIS. That means, after pnp_stop_dev returns, the device is at OFF
 state.
 
 Then, __pnp_bus_suspend calls pnp_dev-protocol-suspend, which
 continues to access the device while the device is at OFF.
 
 Our firmware just provides _DIS for the device. There is no _STS
 method. Then, after wakeup, the device doesn't work.
 
 But just like what the patch points out, pnp_dev-protocol-suspend
 continues to access the device while the device is at OFF. It's not safe.
 
 The patch looks like a workaround. Another possible fix is to just
 delete the calling of pnp_stop_dev in function __pnp_bus_suspend, as
 suspend is not equal to _disable_.
 The deletion might be a little intrusive. That's why we sent a workaround
 patch to LKML.
 
 Which one is better?
 
 Thanks,
 Yanmin
 
  
  Signed-off-by: Zhang Yanmin yanmin.zh...@intel.com
  Signed-off-by: Liu ShuoX shuox@intel.com
  ---
   drivers/pnp/driver.c | 6 ++
   1 file changed, 2 insertions(+), 4 deletions(-)
  
  diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
  index f748cc8..2512e47 100644
  --- a/drivers/pnp/driver.c
  +++ b/drivers/pnp/driver.c
  @@ -176,7 +176,7 @@ static int __pnp_bus_suspend(struct device *dev, 
  pm_message_t state)
  return error;
  }
   
  -   if (pnp_can_disable(pnp_dev)) {
  +   if (pnp_can_disable(pnp_dev)  !pnp_dev-protocol-suspend) {
  error = pnp_stop_dev(pnp_dev);
  if (error)
  return error;
  @@ -215,9 +215,7 @@ static int pnp_bus_resume(struct device *dev)
  error = pnp_dev-protocol-resume(pnp_dev);
  if (error)
  return error;
  -   }
  -
  -   if (pnp_can_write(pnp_dev)) {
  +   } else if (pnp_can_write(pnp_dev)) {
  error = pnp_start_dev(pnp_dev);
  if (error)
  return error;
 
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] pnp: Bypass the calling to pnp_stop_dev at suspend when there is a protocol suspend

2013-12-23 Thread shuox . liu 
From: Zhang Yanmin 

pnp pnp_bus_suspend/_resume have an issue.
pnp_bus_suspend calls pnp_stop_dev to disable the device. With ACPI,
pnp_stop_dev turns off the dev usually. Then,
pnp_bus_suspend=>pnp_dev->protocol->suspend accesses the device and
suspend it again.

pnp_bus_resume has the similar issue.

Another issue is firmware might just provide _DIS, but no_STS method.

The patch fixes it by adding a checking. If there is
pnp_dev->protocol->suspend, pnp_bus_suspend doesn't call pnp_stop_dev.
Do the similar thing for _resume.

Signed-off-by: Zhang Yanmin 
Signed-off-by: Liu ShuoX 
---
 drivers/pnp/driver.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index f748cc8..2512e47 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -176,7 +176,7 @@ static int __pnp_bus_suspend(struct device *dev, 
pm_message_t state)
return error;
}
 
-   if (pnp_can_disable(pnp_dev)) {
+   if (pnp_can_disable(pnp_dev) && !pnp_dev->protocol->suspend) {
error = pnp_stop_dev(pnp_dev);
if (error)
return error;
@@ -215,9 +215,7 @@ static int pnp_bus_resume(struct device *dev)
error = pnp_dev->protocol->resume(pnp_dev);
if (error)
return error;
-   }
-
-   if (pnp_can_write(pnp_dev)) {
+   } else if (pnp_can_write(pnp_dev)) {
error = pnp_start_dev(pnp_dev);
if (error)
return error;
-- 
1.8.3

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

[PATCH] pnp: Bypass the calling to pnp_stop_dev at suspend when there is a protocol suspend

2013-12-23 Thread shuox . liu 
From: Zhang Yanmin yanmin.zh...@intel.com

pnp pnp_bus_suspend/_resume have an issue.
pnp_bus_suspend calls pnp_stop_dev to disable the device. With ACPI,
pnp_stop_dev turns off the dev usually. Then,
pnp_bus_suspend=pnp_dev-protocol-suspend accesses the device and
suspend it again.

pnp_bus_resume has the similar issue.

Another issue is firmware might just provide _DIS, but no_STS method.

The patch fixes it by adding a checking. If there is
pnp_dev-protocol-suspend, pnp_bus_suspend doesn't call pnp_stop_dev.
Do the similar thing for _resume.

Signed-off-by: Zhang Yanmin yanmin.zh...@intel.com
Signed-off-by: Liu ShuoX shuox@intel.com
---
 drivers/pnp/driver.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index f748cc8..2512e47 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -176,7 +176,7 @@ static int __pnp_bus_suspend(struct device *dev, 
pm_message_t state)
return error;
}
 
-   if (pnp_can_disable(pnp_dev)) {
+   if (pnp_can_disable(pnp_dev)  !pnp_dev-protocol-suspend) {
error = pnp_stop_dev(pnp_dev);
if (error)
return error;
@@ -215,9 +215,7 @@ static int pnp_bus_resume(struct device *dev)
error = pnp_dev-protocol-resume(pnp_dev);
if (error)
return error;
-   }
-
-   if (pnp_can_write(pnp_dev)) {
+   } else if (pnp_can_write(pnp_dev)) {
error = pnp_start_dev(pnp_dev);
if (error)
return error;
-- 
1.8.3

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