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

[Grygorii Strashko]

On 06/12/2017 12:02 PM, H. Nikolaus Schaller wrote:
> Hi Grygorii,
> 
>> Am 12.06.2017 um 18:24 schrieb Grygorii Strashko <grygorii.strashko@xxxxxx>:
>>
>>
>>
>> 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@xxxxxx>:
>>>>
>>>>
>>>>>
>>>>> 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?

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

> 
> 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] 

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.

-- 
regards,
-grygorii