linux-kernel - Re: [PATCH 2/2] chipidea: Use devm_request

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 31 Jul 2013 05:54:11 -0400
From:	Tejun Heo <htejun@...il.com>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
Cc:	Mark Brown <broonie@...nel.org>,
	Peter Chen <peter.chen@...escale.com>,
	Fabio Estevam <festevam@...il.com>,
	alexander.shishkin@...ux.intel.com, kernel@...gutronix.de,
	linux-usb@...r.kernel.org,
	Fabio Estevam <fabio.estevam@...escale.com>,
	Jeff Garzik <jeff@...zik.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org,
	Mike Turquette <mturquette@...aro.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] chipidea: Use devm_request_irq()

Hello,

On Wed, Jul 31, 2013 at 11:44:34AM +0200, Uwe Kleine-König wrote:
> > > OK, so the possible problem is that remove is called while the irq is
> > > still active. That means you have to assert that all resources the irq

If your driver destruction path is running while your irq handler is
still running, it's a crappy / broken driver.  You need a deactivation
step whether you're using devm or not.  IRQs can be shared and the
device should be in a quiesced state before the driver detaches
itself.  Note that you can queue deactivation routine using devm.  For
an example, please take a look at
drivers/ata/libata-core.c::ata_host_start().

> > > handler is using (e.g. ioremap, clk_prepare_enable) are only freed
> > > *after* the irq is done. For ioremap that means it must be done using
> > > devm_ioremap_resource. For a clock it's not that easy because the irq
> > > handler has to assert that a used clk is kept prepared which can only be
> > > done using clk_prepare which in turn is not allowed in an irq handler.
> > 
> > > Hmm. So the only possible fixes are
> > > 	- devm* can be told to also care about clk_disable_unprepare
> > > 	- after disabling irqs in the remove callback wait for all
> > > 	  active irqs to be done. (i.e. call synchronize_irq(irq))
> > > 	- don't use devm_request_irq

Again, the right thing to do is having a proper deactivation step.
This is nothing devm can do automatically.  There's no way for it to
find out that the device is actually quiesced.  Let's say it waits for
the current instance of irq handler to finish.  How would it know that
it won't start again between the flushing of the current instance and
irq deregistration.  Add an explicit deactivation step using
devres_alloc().

> > I'm not sure that devm_ guarantees any ordering in the cleanups it does
> > so I'd not like to rely on the first option either, if there were some
> > guarantee of that it'd help.  The nice thing about explicitly freeing
> > the IRQ is that you can tell that all this stuff is safe by inspection.
> devm_* at least uses list_for_each_entry_reverse
> (drivers/base/devres.c:release_nodes()). Without this guarantee devm_
> would not make much sense IMHO.

devm guarantees that the destruction callbacks are called in the
reverse order of registration.

Thanks.

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