| Message ID | 1353506793-8354-4-git-send-email-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 21, 2012 at 03:06:29PM +0100, Michael Grzeschik wrote: > From: Marc Kleine-Budde <mkl@pengutronix.de> > > This fixes a potential race condition where the ci13xxx_imx glue code > could be fast enough to call one of the usbmisc_ops before he got a > valid value on the static usbmisc pointer. To fix that we first set > usbmisc, then call usbmisc_set_ops(). usbmisc is subsys_initcall, and cil13xxx_imx is module_init. Any potential situation that the ci13xxx_imx's probe is ran before the usbmisc's probe is completed? Besides, there is usbmisc_ops value check at the beginning of cil13xxx_imx's probe. > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > Changes since v1: > * split previous patch into two seperate. > > drivers/usb/chipidea/usbmisc_imx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c > index 552c63f..9145e04 100644 > --- a/drivers/usb/chipidea/usbmisc_imx.c > +++ b/drivers/usb/chipidea/usbmisc_imx.c > @@ -116,14 +116,14 @@ static int __devinit usbmisc_imx_probe(struct platform_device *pdev) > return ret; > } > > + usbmisc = data; > ret = usbmisc_set_ops(&imx6q_usbmisc_ops); > if (ret) { > + usbmisc = NULL; > clk_disable_unprepare(data->clk); > return ret; > } > > - usbmisc = data; > - > return 0; > } > > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Fri, Nov 23, 2012 at 01:36:36PM +0800, Peter Chen wrote: > On Wed, Nov 21, 2012 at 03:06:29PM +0100, Michael Grzeschik wrote: > > From: Marc Kleine-Budde <mkl@pengutronix.de> > > > > This fixes a potential race condition where the ci13xxx_imx glue code > > could be fast enough to call one of the usbmisc_ops before he got a > > valid value on the static usbmisc pointer. To fix that we first set > > usbmisc, then call usbmisc_set_ops(). > > usbmisc is subsys_initcall, and cil13xxx_imx is module_init. Any > potential situation that the ci13xxx_imx's probe is ran before the > usbmisc's probe is completed? Not having looked at the code you are referring to at all I just want to say that: drivers can be modules (don't know if that's true for chipidea) and sooner or later we'll probably get devicetree overlays, so the devicetree nodes might just appear during runtime. Depending on initcall order is generally not a good idea. Sascha
Sascha Hauer <s.hauer@pengutronix.de> writes: > On Fri, Nov 23, 2012 at 01:36:36PM +0800, Peter Chen wrote: >> On Wed, Nov 21, 2012 at 03:06:29PM +0100, Michael Grzeschik wrote: >> > From: Marc Kleine-Budde <mkl@pengutronix.de> >> > >> > This fixes a potential race condition where the ci13xxx_imx glue code >> > could be fast enough to call one of the usbmisc_ops before he got a >> > valid value on the static usbmisc pointer. To fix that we first set >> > usbmisc, then call usbmisc_set_ops(). >> >> usbmisc is subsys_initcall, and cil13xxx_imx is module_init. Any >> potential situation that the ci13xxx_imx's probe is ran before the >> usbmisc's probe is completed? > > Not having looked at the code you are referring to at all I just want > to say that: drivers can be modules (don't know if that's true for > chipidea) and sooner or later we'll probably get devicetree overlays, ChipIdea can be not even one, but two modules (ci_hdrc, the actual controller driver, always a platform_driver) and platform bindings like ci13xxx_imx, ci13xxx_pci, ci13xxx_msm, which can be platform or pci or whatever else drivers. > so the devicetree nodes might just appear during runtime. Depending on > initcall order is generally not a good idea. That's very true. Regards, -- Alex
On 11/23/2012 06:36 AM, Peter Chen wrote: > On Wed, Nov 21, 2012 at 03:06:29PM +0100, Michael Grzeschik wrote: >> From: Marc Kleine-Budde <mkl@pengutronix.de> >> >> This fixes a potential race condition where the ci13xxx_imx glue code >> could be fast enough to call one of the usbmisc_ops before he got a >> valid value on the static usbmisc pointer. To fix that we first set >> usbmisc, then call usbmisc_set_ops(). > > usbmisc is subsys_initcall, and cil13xxx_imx is module_init. Any > potential situation that the ci13xxx_imx's probe is ran before the > usbmisc's probe is completed? Besides, there is usbmisc_ops value > check at the beginning of cil13xxx_imx's probe. It's bad practice to rely your code on some external ordering mechanism, even more if the correct solution is so simple. And as Sascha pointed out everything might be build as modules and loaded individually. Marc
diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c index 552c63f..9145e04 100644 --- a/drivers/usb/chipidea/usbmisc_imx.c +++ b/drivers/usb/chipidea/usbmisc_imx.c @@ -116,14 +116,14 @@ static int __devinit usbmisc_imx_probe(struct platform_device *pdev) return ret; } + usbmisc = data; ret = usbmisc_set_ops(&imx6q_usbmisc_ops); if (ret) { + usbmisc = NULL; clk_disable_unprepare(data->clk); return ret; } - usbmisc = data; - return 0; }