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? 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
