linux-kernel - RE: [PATCH] drivers: core: clear wake irq in device_unbind

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <PAXPR04MB8459FFCD910ED7C261FC672688592@PAXPR04MB8459.eurprd04.prod.outlook.com>
Date: Tue, 12 Nov 2024 15:05:38 +0000
From: Peng Fan <peng.fan@....com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC: "Peng Fan (OSS)" <peng.fan@....nxp.com>, "Rafael J. Wysocki"
	<rafael@...nel.org>, open list <linux-kernel@...r.kernel.org>, Dmitry
 Torokhov <dmitry.torokhov@...il.com>, Rob Herring <robh@...nel.org>, Ulf
 Hansson <ulf.hansson@...aro.org>
Subject: RE: [PATCH] drivers: core: clear wake irq in device_unbind_cleanup

> 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@....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@...il.com>
> > > > Cc: Rob Herring <robh@...nel.org>
> > > > Cc: Ulf Hansson <ulf.hansson@...aro.org>
> > > > Signed-off-by: Peng Fan <peng.fan@....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