linux-kernel - Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

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:   Mon, 12 Jun 2017 19:02:09 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Grygorii Strashko <grygorii.strashko@...com>
Cc:     Sebastian Reichel <sre@...nel.org>, NeilBrown <neil@...wn.name>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        Marek Belisko <marek@...delico.com>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-pm@...r.kernel.org, letux-kernel@...nphoenux.org,
        notasas@...il.com, linux-omap@...r.kernel.org
Subject: Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

Hi Grygorii,

> Am 12.06.2017 um 18:24 schrieb Grygorii Strashko <grygorii.strashko@...com>:
> 
> 
> 
> On 06/09/2017 11:59 PM, H. Nikolaus Schaller wrote:
>> Hi Grygorii,
>> 
>>> Am 09.06.2017 um 18:25 schrieb Grygorii Strashko <grygorii.strashko@...com>:
>>> 
>>> 
>>>> 
>>>> So please advise how to proceed.
>>>> 
>>> 
>>> You should request irq as late as possible in probe - it's safest way to go always.
>>> You see crushes simply because request_irq enables IRQ and IRQ can trigger immediately, so
>>> IRQ handler will run and all required resources should be ready and initialized.
>>> 
>>> In this case:
>>> twl4030_bci_interrupt() -> twl4030_charger_update_current() -> ac_available() -> iio_read_channel_processed()
>>> OOOPS.
>>> 
>>> So, first thing to do is to reorder probe to ensure that sequence is safe.
>> 
>> That is exactly what I guessed when proposing the reordering patch.
>> 
>>> Then other stuff - devm, EPROBE_DEFER ...
>> 
>> IMHO, reordering alone doesn't make much sense as a single patch. Or does it?
>> 
> 
> The question is - is there bug in driver or not? As per current code seems yes.

If we all agree, do we need another confirmation?

> You can easily test it by directly calling twl4030_charger_interrupt() right after
> IRQ is requested. there is a bug if it will crush.

In the variant without EPROBE_DEFER you won't see it since ac_available either
has a valid iio channel or NULL.

The problem is only if we add an EPROBE_DEFER return if getting the iio channel
fails. This seems to make trouble if we don't take care of it. There are certainly
several options to work around but IMHO reordering is the best one (and even works
w/o devm for iio - which should be added in a separate step).

And there is a strong argument for reordering to have the most likely failure
first (iio fails more likely due to DEFER than allocating an irq). But only if
we process the iio failure at all.

So there are one a little doubtful argument for reordering (driver bug) and
a strong one.

> As for me it more reasonable to move forward using smaller steps.

Well, I wonder what it does help to fix a bug which does not even become visible
if we don't add EPROBE_DEFER?

So I would count two steps:
a) add EPROBE_DEFER and reorder code to make it work
b) convert to devm for iio

Ok?

BR and thanks,
Nikolaus