[PATCH] drivers: core: clear wake irq in device_unbind_cleanup

[PATCH] drivers: core: clear wake irq in device_unbind_cleanup

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

With dev_pm_clear_wake_irq in device_unbind_cleanup, there is no need
to invoke dev_pm_clear_wake_irq in driver remove hook explicitly.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/base/dd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f0e4b4aba885..ea3a871bdd11 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -26,6 +26,7 @@
 #include <linux/wait.h>
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/pinctrl/devinfo.h>
 #include <linux/slab.h>
 
@@ -556,6 +557,7 @@ static void device_unbind_cleanup(struct device *dev)
 		dev->pm_domain->dismiss(dev);
 	pm_runtime_reinit(dev);
 	dev_pm_set_driver_flags(dev, 0);
+	dev_pm_clear_wake_irq(dev);
 }
 
 static void device_remove(struct device *dev)
-- 
2.37.1

Re: [PATCH] drivers: core: clear wake irq in device_unbind_cleanup

[Greg Kroah-Hartman]

On Mon, Nov 11, 2024 at 05:21:30PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> With dev_pm_clear_wake_irq in device_unbind_cleanup, there is no need
> to invoke dev_pm_clear_wake_irq in driver remove hook explicitly.
> 
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/base/dd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index f0e4b4aba885..ea3a871bdd11 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -26,6 +26,7 @@
>  #include <linux/wait.h>
>  #include <linux/async.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_wakeirq.h>
>  #include <linux/pinctrl/devinfo.h>
>  #include <linux/slab.h>
>  
> @@ -556,6 +557,7 @@ static void device_unbind_cleanup(struct device *dev)
>  		dev->pm_domain->dismiss(dev);
>  	pm_runtime_reinit(dev);
>  	dev_pm_set_driver_flags(dev, 0);
> +	dev_pm_clear_wake_irq(dev);

I don't understand, you say you don't need to invoke it, yet you are
calling it here.

What commit id does this fix?  And what bug is this resolving?  What
drivers are broken without this?

thanks,

greg k-h

RE: [PATCH] drivers: core: clear wake irq in device_unbind_cleanup

[Peng Fan]

Hi Greg

> Subject: Re: [PATCH] drivers: core: clear wake irq in
> device_unbind_cleanup
> 
> On Mon, Nov 11, 2024 at 05:21:30PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > With dev_pm_clear_wake_irq in device_unbind_cleanup, there is no
> need
> > to invoke dev_pm_clear_wake_irq in driver remove hook explicitly.
> >
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/base/dd.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > f0e4b4aba885..ea3a871bdd11 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/wait.h>
> >  #include <linux/async.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/pm_wakeirq.h>
> >  #include <linux/pinctrl/devinfo.h>
> >  #include <linux/slab.h>
> >
> > @@ -556,6 +557,7 @@ static void device_unbind_cleanup(struct
> device *dev)
> >  		dev->pm_domain->dismiss(dev);
> >  	pm_runtime_reinit(dev);
> >  	dev_pm_set_driver_flags(dev, 0);
> > +	dev_pm_clear_wake_irq(dev);
> 
> I don't understand, you say you don't need to invoke it, yet you are
> calling it here.

I mean not need to invoke it in driver.remove hook. With this patch, we
could remove
https://elixir.bootlin.com/linux/v6.11.7/source/drivers/input/touchscreen/ti_am335x_tsc.c#L498
and same to other drivers.

> 
> What commit id does this fix? 

I am thinking to take this as a improvement, with core code
has this, the various drivers no need explicitly invoke it
in their own driver remove hook.

 And what bug is this resolving?  What
> drivers are broken without this?

See here:
https://lore.kernel.org/all/ZymxvLMkkktRoCXZ@google.com/

Thanks,
Peng.

> 
> thanks,
> 
> greg k-h

Re: [PATCH] drivers: core: clear wake irq in device_unbind_cleanup

[Greg Kroah-Hartman]

On Tue, Nov 12, 2024 at 01:09:19PM +0000, Peng Fan wrote:
> Hi Greg
> 
> > Subject: Re: [PATCH] drivers: core: clear wake irq in
> > device_unbind_cleanup
> > 
> > On Mon, Nov 11, 2024 at 05:21:30PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > With dev_pm_clear_wake_irq in device_unbind_cleanup, there is no
> > need
> > > to invoke dev_pm_clear_wake_irq in driver remove hook explicitly.
> > >
> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Cc: Rob Herring <robh@kernel.org>
> > > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/base/dd.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > > f0e4b4aba885..ea3a871bdd11 100644
> > > --- a/drivers/base/dd.c
> > > +++ b/drivers/base/dd.c
> > > @@ -26,6 +26,7 @@
> > >  #include <linux/wait.h>
> > >  #include <linux/async.h>
> > >  #include <linux/pm_runtime.h>
> > > +#include <linux/pm_wakeirq.h>
> > >  #include <linux/pinctrl/devinfo.h>
> > >  #include <linux/slab.h>
> > >
> > > @@ -556,6 +557,7 @@ static void device_unbind_cleanup(struct
> > device *dev)
> > >  		dev->pm_domain->dismiss(dev);
> > >  	pm_runtime_reinit(dev);
> > >  	dev_pm_set_driver_flags(dev, 0);
> > > +	dev_pm_clear_wake_irq(dev);
> > 
> > I don't understand, you say you don't need to invoke it, yet you are
> > calling it here.
> 
> I mean not need to invoke it in driver.remove hook. With this patch, we
> could remove
> https://elixir.bootlin.com/linux/v6.11.7/source/drivers/input/touchscreen/ti_am335x_tsc.c#L498
> and same to other drivers.

But you did not say that, and you would need to make this as part of a
series.

Also, are you sure that ll drivers want to clear this irq flag?  What is
wrong with just doing it explicitly in the drivers that need it?

> > 
> > What commit id does this fix? 
> 
> I am thinking to take this as a improvement, with core code
> has this, the various drivers no need explicitly invoke it
> in their own driver remove hook.
> 
>  And what bug is this resolving?  What
> > drivers are broken without this?
> 
> See here:
> https://lore.kernel.org/all/ZymxvLMkkktRoCXZ@google.com/

Again, this seems to be a per-driver thing.  What do you break if you
attempt to do this for all drivers?  What about drivers that share irqs?

How was this tested?  On what platfoms?

thanks,

greg k-h

RE: [PATCH] drivers: core: clear wake irq in device_unbind_cleanup

[Peng Fan]

> Subject: Re: [PATCH] drivers: core: clear wake irq in
> device_unbind_cleanup
> 
> On Tue, Nov 12, 2024 at 01:09:19PM +0000, Peng Fan wrote:
> > Hi Greg
> >
> > > Subject: Re: [PATCH] drivers: core: clear wake irq in
> > > device_unbind_cleanup
> > >
> > > On Mon, Nov 11, 2024 at 05:21:30PM +0800, Peng Fan (OSS)
> wrote:
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > With dev_pm_clear_wake_irq in device_unbind_cleanup, there is
> no
> > > need
> > > > to invoke dev_pm_clear_wake_irq in driver remove hook
> explicitly.
> > > >
> > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > Cc: Rob Herring <robh@kernel.org>
> > > > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >  drivers/base/dd.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > > > f0e4b4aba885..ea3a871bdd11 100644
> > > > --- a/drivers/base/dd.c
> > > > +++ b/drivers/base/dd.c
> > > > @@ -26,6 +26,7 @@
> > > >  #include <linux/wait.h>
> > > >  #include <linux/async.h>
> > > >  #include <linux/pm_runtime.h>
> > > > +#include <linux/pm_wakeirq.h>
> > > >  #include <linux/pinctrl/devinfo.h>  #include <linux/slab.h>
> > > >
> > > > @@ -556,6 +557,7 @@ static void device_unbind_cleanup(struct
> > > device *dev)
> > > >  		dev->pm_domain->dismiss(dev);
> > > >  	pm_runtime_reinit(dev);
> > > >  	dev_pm_set_driver_flags(dev, 0);
> > > > +	dev_pm_clear_wake_irq(dev);
> > >
> > > I don't understand, you say you don't need to invoke it, yet you are
> > > calling it here.
> >
> > I mean not need to invoke it in driver.remove hook. With this patch,
> > we could remove
> >https://elixir.bootlin.com/linux/v6.11.7/source/drivers/input/touchscreen/ti_am335x_tsc.c#L498
> > and same to other drivers.
> 
> But you did not say that, and you would need to make this as part of a
> series.

Sorry if I not describe clearly in commit log.

There are many drivers are invoking clear wake irq in their remove hook.
I was thinking that if this patch got accepted, I could write a patch
to clean up the various drivers.

> 
> Also, are you sure that ll drivers want to clear this irq flag?

Without clear the flag, module test will report a kernel that
wake irq already initialized.

  What is
> wrong with just doing it explicitly in the drivers that need it?

Nothing wrong.
Dmitry commented on my patch to see whether the clear wake
irq could be done in driver core or not. So I did this patch.

> 
> > >
> > > What commit id does this fix?
> >
> > I am thinking to take this as a improvement, with core code has this,
> > the various drivers no need explicitly invoke it in their own driver
> > remove hook.
> >
> >  And what bug is this resolving?  What
> > > drivers are broken without this?
> >
> > See here:
> >https://lore.kernel.org/all/ZymxvLMkkktRoCXZ@google.com/
> 
> Again, this seems to be a per-driver thing. 

If driver not set wake irq, clear wake irq does no harm.
If driver set wake irq, clear wake irq in driver core could avoid
various drivers doing this work.

 What do you break if you
> attempt to do this for all drivers?  

I could not guarantee on this. Sorry.

> What about drivers that share irqs?

From my understanding, wake irq here is per device, not
related to shared irq.

Rafael, Ulf may help comment.

> 
> How was this tested?  On what platfoms?

Test on NXP i.MX9 platform.

Anyway I put this patch as an optimization, if you
think it is improper, I will go back to do a fix
in the 
drivers/input/misc/nxp-bbnsm-pwrkey.c

Thanks,
Peng.

> 
> thanks,
> 
> greg k-h