linux-kernel - Re: [PATCH 02/10] mfd: cros_ec: IRQs for cros

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 for Android: free password hash cracker in your pocket

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

Date:	Wed, 18 Jun 2014 09:23:29 -0700
From:	Doug Anderson <dianders@...omium.org>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	Andrew Bresticker <abrestic@...omium.org>,
	Stephen Warren <swarren@...dotorg.org>,
	Olof Johansson <olof@...om.net>,
	Sonny Rao <sonnyrao@...omium.org>,
	linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	Bill Richardson <wfrichar@...omium.org>,
	Simon Glass <sjg@...omium.org>,
	Wolfram Sang <wsa@...-dreams.de>,
	"broonie@...nel.org" <broonie@...nel.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 02/10] mfd: cros_ec: IRQs for cros_ec should be optional

Lee,

On Wed, Jun 18, 2014 at 12:55 AM, Lee Jones <lee.jones@...aro.org> wrote:
> On Mon, 16 Jun 2014, Doug Anderson wrote:
>
>> From: Bill Richardson <wfrichar@...omium.org>
>>
>> Preparing the way for the LPC device, which is just a plaform_device without
>> interrupts.
>>
>> Signed-off-by: Bill Richardson <wfrichar@...omium.org>
>> Signed-off-by: Doug Anderson <dianders@...omium.org>
>> ---
>>  drivers/mfd/cros_ec.c | 26 +++++++++++++-------------
>>  1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>> index 38fe9bf..bd6f936 100644
>> --- a/drivers/mfd/cros_ec.c
>> +++ b/drivers/mfd/cros_ec.c
>> @@ -119,17 +119,15 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>>                       return -ENOMEM;
>>       }
>>
>> -     if (!ec_dev->irq) {
>> -             dev_dbg(dev, "no valid IRQ: %d\n", ec_dev->irq);
>> -             return err;
>> -     }
>> -
>> -     err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
>> -                                IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> -                                "chromeos-ec", ec_dev);
>> -     if (err) {
>> -             dev_err(dev, "request irq %d: error %d\n", ec_dev->irq, err);
>> -             return err;
>> +     if (ec_dev->irq) {
>> +             err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
>> +                                     IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> +                                     "chromeos-ec", ec_dev);
>
> Is there anything stopping you using the devm_* version?

I'm generally quite hesitant about using the devm_ IRQ functions.
Requesting an IRQ enables the IRQ at the time of request and freeing
it disables it, right?  Leaving it up to the the devm subsystem to
disable your IRQ tends to be a race waiting to happen if an IRQ
happens after you've freed all your memory / cleaned up all your
state.

Looking at cros_ec in particular though:

* Right now the last thing done in cros_ec_remove() (which is the last
thing in both cros_ec_i2c_remove and cros_ec_spi_remove) is to free
the IRQ.  That means that you're right: we could switch to devm_ in
this case and it wouldn't introduce any new bugs.

* ...but I'm not convinced that the location of the free_irq() today
is quite right.  I couldn't actually get it to crash or hang, but
there is a time period where we've called mfd_remove_devices() and the
IRQ is still active.  That doesn't seem like a super great situation
to be in.  I'll add a move of the irq_free to the patch series.

-Doug
--
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/