lkml.org 
[lkml]   [2017]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio
From
Date
Hi Grygorii,

> Am 12.06.2017 um 18:24 schrieb Grygorii Strashko <grygorii.strashko@ti.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@ti.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


\
 
 \ /
  Last update: 2017-06-12 20:55    [W:0.059 / U:0.652 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site