From: Grygorii Strashko <grygorii.strashko@ti.com> To: "H. Nikolaus Schaller" <hns@goldelico.com> Cc: Sebastian Reichel <sre@kernel.org>, NeilBrown <neil@brown.name>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Russell King <linux@armlinux.org.uk>, Marek Belisko <marek@goldelico.com>, <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>, <linux-pm@vger.kernel.org>, <letux-kernel@openphoenux.org>, <notasas@gmail.com>, <linux-omap@vger.kernel.org> Subject: Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio Date: Mon, 12 Jun 2017 13:42:48 -0500 [thread overview] Message-ID: <a91d2ad7-5985-789e-e4ad-04057dfdd8a3@ti.com> (raw) In-Reply-To: <D12A5BD7-65A1-49E6-A25C-B2A2162FCA05@goldelico.com> 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@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? 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
WARNING: multiple messages have this Message-ID (diff)
From: Grygorii Strashko <grygorii.strashko@ti.com> To: "H. Nikolaus Schaller" <hns@goldelico.com> Cc: Sebastian Reichel <sre@kernel.org>, NeilBrown <neil@brown.name>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Russell King <linux@armlinux.org.uk>, Marek Belisko <marek@goldelico.com>, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-pm@vger.kernel.org, letux-kernel@openphoenux.org, notasas@gmail.com, linux-omap@vger.kernel.org Subject: Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio Date: Mon, 12 Jun 2017 13:42:48 -0500 [thread overview] Message-ID: <a91d2ad7-5985-789e-e4ad-04057dfdd8a3@ti.com> (raw) In-Reply-To: <D12A5BD7-65A1-49E6-A25C-B2A2162FCA05@goldelico.com> 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@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? 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
next prev parent reply other threads:[~2017-06-12 18:44 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-05-21 10:38 [PATCH v5 0/3] More fixes for twl4030 charger H. Nikolaus Schaller 2017-05-21 10:38 ` H. Nikolaus Schaller 2017-05-21 10:38 ` [PATCH v5 1/3] drivers:power:twl4030-charger: remove nonstandard max_current sysfs attribute H. Nikolaus Schaller 2017-06-07 20:23 ` Sebastian Reichel 2017-05-21 10:38 ` [PATCH v5 2/3] ARM: dts: twl4030: Add missing madc reference for bci subnode H. Nikolaus Schaller 2017-06-06 7:23 ` Tony Lindgren 2017-06-07 9:50 ` H. Nikolaus Schaller 2017-05-21 10:38 ` [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio H. Nikolaus Schaller 2017-05-21 10:38 ` H. Nikolaus Schaller 2017-06-07 20:44 ` Sebastian Reichel 2017-06-07 20:44 ` Sebastian Reichel 2017-06-09 6:05 ` H. Nikolaus Schaller 2017-06-09 6:05 ` H. Nikolaus Schaller 2017-06-09 16:25 ` Grygorii Strashko 2017-06-09 16:25 ` Grygorii Strashko 2017-06-10 4:59 ` H. Nikolaus Schaller 2017-06-12 16:24 ` Grygorii Strashko 2017-06-12 16:24 ` Grygorii Strashko 2017-06-12 17:02 ` H. Nikolaus Schaller 2017-06-12 18:42 ` Grygorii Strashko [this message] 2017-06-12 18:42 ` Grygorii Strashko 2017-06-12 20:38 ` H. Nikolaus Schaller 2017-06-12 20:38 ` H. Nikolaus Schaller 2017-06-03 6:01 ` [PATCH v5 0/3] More fixes for twl4030 charger H. Nikolaus Schaller
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=a91d2ad7-5985-789e-e4ad-04057dfdd8a3@ti.com \ --to=grygorii.strashko@ti.com \ --cc=devicetree@vger.kernel.org \ --cc=hns@goldelico.com \ --cc=letux-kernel@openphoenux.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=marek@goldelico.com \ --cc=mark.rutland@arm.com \ --cc=neil@brown.name \ --cc=notasas@gmail.com \ --cc=robh+dt@kernel.org \ --cc=sre@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.