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

Date:   Mon, 12 Jun 2017 22:38:48 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Grygorii Strashko <grygorii.strashko@...com>,
        Sebastian Reichel <sre@...nel.org>
Cc:     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>,
        LKML <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux PM mailing list <linux-pm@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>,
        Grazvydas Ignotas <notasas@...il.com>,
        linux-omap <linux-omap@...r.kernel.org>
Subject: Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

Hi,
>> 
>>> 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?
> 
> Sry, but have you tried test I've proposed (I can't try it by myself now, sry)? 

No because I can't easily try it at the moment.

And I think it does not show what you expect.

Usually the iio_channel_get() fails and we have bci->channel_vac = NULL
which does not make problems in the irq handler and is catched in ac_available().

To make iio_channel_get() not fail we have to implement deferred probe
handling.

> There is not only iio channel can be a problem - for example "current_worker"
> initialized after IRQ request, but can be scheduled from IRQ handler.

Ah, yet another bug.

Looks as if there are some race conditions and we are just lucky that it
rarely happens.

> 
>> 
>> So I would count two steps:
>> a) add EPROBE_DEFER and reorder code to make it work
>> b) convert to devm for iio
>> 
> 
> Few words regarding devm_request_irq() - it's useful, from one side, and
> dangerous form another :(, as below init sequence is proved to be unsafe
> and so it has to be used very carefully:
> 
> probe() {
> <do some initialization>
> <create some object:> objX = create_objX() -- no devm
> devm_request_irq(IrqY) -- IrqY handler using objX
> 
> <init step failed>
> 	goto err;
> ...
> 
> err:
> [a]
> release_objX() - note IRQ is still enabled here
> }
> dd_core if err:
> devres_release_all() - IRQ disabled and freed only here
> 
> remove() {
> [b]
> release_objX() -- note IRQ is still enabled here
> }
> dd_core:
> devres_release_all() - IRQ disabled and freed only here
> 
> IRQ has to be explicitly disabled at points [a] & [b] 

Or everything done devm so that irqs are released first.

> 
> Sequence to move forward can be up to you in general,
> personally I'd try to add devm_iio and move  devm_request_irq()
> down right before "/* Enable interrupts now. */" line first.

Yes, that is what I almost proposed as well.

Except that you propose to move lines from INIT_WORK() up to

if (bci->dev->of_node) {
...
}

as well.

Looks good to me.

So if Sebastian or others have no more comments, I will prepare a patch v6 asap.

BR and thanks,
Nikolaus