<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Sebastian,<div class=""><br class=""><blockquote type="cite" class="">Am 15.06.2017 um 13:47 schrieb Sebastian Reichel <<a href="mailto:sebastian.reichel@collabora.co.uk" class="">sebastian.reichel@collabora.co.uk</a>>:<br class=""><br class="">Hi,<br class=""><br class="">On Wed, Jun 14, 2017 at 11:25:55AM +0200, H. Nikolaus Schaller wrote:<br class=""><blockquote type="cite" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">This avoids a potential race if irqs are enabled and triggered too early<br class="">before the worker is properly set up.<br class=""><br class="">Suggested-by: Grygorii Strashko <<a href="mailto:grygorii.strashko@ti.com" class="">grygorii.strashko@ti.com</a>><br class="">Signed-off-by: H. Nikolaus Schaller <<a href="mailto:hns@goldelico.com" class="">hns@goldelico.com</a>><br class="">---<br class="">drivers/power/supply/twl4030_charger.c | 28 ++++++++++++++--------------<br class="">1 file changed, 14 insertions(+), 14 deletions(-)<br class=""><br class="">diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c<br class="">index 1fbbe0cc216a..3bebeecb4a1f 100644<br class="">--- a/drivers/power/supply/twl4030_charger.c<br class="">+++ b/drivers/power/supply/twl4030_charger.c<br class="">@@ -984,6 +984,16 @@ static int twl4030_bci_probe(struct platform_device *pdev)<br class=""><br class=""><span class="Apple-tab-span" style="white-space:pre"> </span>platform_set_drvdata(pdev, bci);<br class=""><br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>if (bci->dev->of_node) {<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>struct device_node *phynode;<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>phynode = of_find_compatible_node(bci->dev->of_node->parent,<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> NULL, "ti,twl4030-usb");<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>if (phynode)<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>bci->transceiver = devm_usb_get_phy_by_node(<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>bci->dev, phynode, &bci->usb_nb);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>}<br class="">+<br class=""></blockquote><br class="">The notifier is not that much different from an irq. I see<br class="">the call can access at least the iio channel (so iio channel<br class="">should be registered first).</blockquote><div class=""><br class=""></div>Hm. I only see that it can schedule the worker (which means the worker should be</div><div class="">initialized first) but does not access iio by itself.</div><div class=""><br class=""></div><div class="">Can the worker run before probe() succeeds?</div><div class=""><br class=""><blockquote type="cite" class=""> I suspect worker and power-supply<br class="">should also be initialized/registered first. Best location seems<br class="">to be directly before requesting the irqs.<br class=""></blockquote><div class=""><br class=""></div>I think they should be initialized even before devm_usb_get_phy_by_node().</div><div class=""><br class=""><blockquote type="cite" class=""><br class=""><blockquote type="cite" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span class="Apple-tab-span" style="white-space:pre"> </span>bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");<br class=""><span class="Apple-tab-span" style="white-space:pre"> </span>if (IS_ERR(bci->channel_vac)) {<br class=""><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>bci->channel_vac = NULL;<br class="">@@ -1006,6 +1016,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)<br class=""><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>return ret;<br class=""><span class="Apple-tab-span" style="white-space:pre"> </span>}<br class=""><br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>INIT_WORK(&bci->work, twl4030_bci_usb_work);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;<br class=""></blockquote><br class="">You should configure the notifier block *before* registering it.<br class=""></blockquote><div class=""><br class=""></div>Isn't the notifier block already configured by devm_usb_get_phy_by_node() - except for the pointer to the notifier_call?</div><div class=""><br class=""></div><div class="">So the only harmful thing I can see is that a notifier arrives with usb_nb.notifier_call still being uninitialized as NULL.</div><div class=""><br class=""></div><div class="">Is this harmful or not? I don't know.</div><div class=""><br class=""></div><div class="">I can only guess that it could equally well be set right after devm_usb_get_phy_by_node()</div><div class="">and I have reworked it that way.</div><div class=""><br class=""></div><div class="">It still works on GTA04 and OpenPandora with neither observing a regression nor an improvement.</div><div class=""><br class=""></div><div class="">So I will send a v7 with what I think should be most robust.</div><div class=""><br class=""></div><div class="">BR and thanks,</div><div class="">Nikolaus</div><div class=""><br class=""></div></body></html>