ULPI registers it's bus at module_init so if the bus fails to register, the
module will fail to load and all will be well in the world.
However, if the ULPI code is built-in rather than a module, the bus
initialization may fail but we'd still try to register drivers later onto
a non-existant bus, which will panic the kernel.
Fix that by checking that the bus was indeed initialized before trying to
register drivers on top of it.
Signed-off-by: Sasha Levin <[email protected]>
---
drivers/usb/common/ulpi.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index 0e6f968..0b0a5e7 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
if (!drv->probe)
return -EINVAL;
+ /* Was the bus registered successfully? */
+ if (!ulpi_bus.p)
+ return -ENODEV;
+
drv->driver.bus = &ulpi_bus;
return driver_register(&drv->driver);
--
1.7.10.4
Hi,
On Wed, May 20, 2015 at 03:33:26PM -0400, Sasha Levin wrote:
> ULPI registers it's bus at module_init so if the bus fails to register, the
A minor comment: s/it's/its/
> module will fail to load and all will be well in the world.
>
> However, if the ULPI code is built-in rather than a module, the bus
> initialization may fail but we'd still try to register drivers later onto
> a non-existant bus, which will panic the kernel.
>
> Fix that by checking that the bus was indeed initialized before trying to
> register drivers on top of it.
>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
> drivers/usb/common/ulpi.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index 0e6f968..0b0a5e7 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
> if (!drv->probe)
> return -EINVAL;
>
> + /* Was the bus registered successfully? */
> + if (!ulpi_bus.p)
> + return -ENODEV;
> +
Good catch. Otherwise it may trigger BUG() on driver_register().
I wonder if it would be nice to have a macro for that checking :)
Anyway,
Reviewed-by: David Cohen <[email protected]>
> drv->driver.bus = &ulpi_bus;
>
> return driver_register(&drv->driver);
> --
> 1.7.10.4
>
On 05/21/2015 05:22 AM, David Cohen wrote:
> Hi,
>
> On Wed, May 20, 2015 at 03:33:26PM -0400, Sasha Levin wrote:
>> ULPI registers it's bus at module_init so if the bus fails to register, the
> A minor comment: s/it's/its/
>
>> module will fail to load and all will be well in the world.
>>
>> However, if the ULPI code is built-in rather than a module, the bus
>> initialization may fail but we'd still try to register drivers later onto
>> a non-existant bus, which will panic the kernel.
>>
>> Fix that by checking that the bus was indeed initialized before trying to
>> register drivers on top of it.
>>
>> Signed-off-by: Sasha Levin <[email protected]>
>> ---
>> drivers/usb/common/ulpi.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
>> index 0e6f968..0b0a5e7 100644
>> --- a/drivers/usb/common/ulpi.c
>> +++ b/drivers/usb/common/ulpi.c
>> @@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
>> if (!drv->probe)
>> return -EINVAL;
>>
>> + /* Was the bus registered successfully? */
>> + if (!ulpi_bus.p)
>> + return -ENODEV;
>> +
> Good catch. Otherwise it may trigger BUG() on driver_register().
> I wonder if it would be nice to have a macro for that checking :)
>
> Anyway,
>
> Reviewed-by: David Cohen <[email protected]>
Well, I was also encountering panic issue when running it on
Intel Bay Trail tablets. In my case, it's due to the execution
sequence. When ulpi bus is built-in, driver or device registered
before ulpi bus registration.
Thanks,
Baolu
>
>> drv->driver.bus = &ulpi_bus;
>>
>> return driver_register(&drv->driver);
>> --
>> 1.7.10.4
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> >>ULPI registers it's bus at module_init so if the bus fails to register, the
> >A minor comment: s/it's/its/
> >
> >>module will fail to load and all will be well in the world.
> >>
> >>However, if the ULPI code is built-in rather than a module, the bus
> >>initialization may fail but we'd still try to register drivers later onto
> >>a non-existant bus, which will panic the kernel.
> >>
> >>Fix that by checking that the bus was indeed initialized before trying to
> >>register drivers on top of it.
> >>
> >>Signed-off-by: Sasha Levin <[email protected]>
> >>---
> >> drivers/usb/common/ulpi.c | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >>diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> >>index 0e6f968..0b0a5e7 100644
> >>--- a/drivers/usb/common/ulpi.c
> >>+++ b/drivers/usb/common/ulpi.c
> >>@@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
> >> if (!drv->probe)
> >> return -EINVAL;
> >>+ /* Was the bus registered successfully? */
> >>+ if (!ulpi_bus.p)
> >>+ return -ENODEV;
I think we need to warn in this case. How about:
if (unlikely(WARN_ON(!ulpi_bus.p)))
return -ENODEV;
> >Good catch. Otherwise it may trigger BUG() on driver_register().
> >I wonder if it would be nice to have a macro for that checking :)
> >
> >Anyway,
> >
> >Reviewed-by: David Cohen <[email protected]>
>
> Well, I was also encountering panic issue when running it on
> Intel Bay Trail tablets. In my case, it's due to the execution
> sequence. When ulpi bus is built-in, driver or device registered
> before ulpi bus registration.
This patch will fix the panic you saw as well, but of course we still
want the ulpi phy drivers to load successfully even if they and the
bus are build-in, so we need your patch as well.
Thanks,
--
heikki
> > >>diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > >>index 0e6f968..0b0a5e7 100644
> > >>--- a/drivers/usb/common/ulpi.c
> > >>+++ b/drivers/usb/common/ulpi.c
> > >>@@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
> > >> if (!drv->probe)
> > >> return -EINVAL;
> > >>+ /* Was the bus registered successfully? */
> > >>+ if (!ulpi_bus.p)
> > >>+ return -ENODEV;
>
> I think we need to warn in this case. How about:
>
> if (unlikely(WARN_ON(!ulpi_bus.p)))
> return -ENODEV;
I think we should also return -EAGAIN here.
Thanks,
--
heikki
On Fri, May 22, 2015 at 01:16:38PM +0300, Heikki Krogerus wrote:
> > > >>diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > >>index 0e6f968..0b0a5e7 100644
> > > >>--- a/drivers/usb/common/ulpi.c
> > > >>+++ b/drivers/usb/common/ulpi.c
> > > >>@@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
> > > >> if (!drv->probe)
> > > >> return -EINVAL;
> > > >>+ /* Was the bus registered successfully? */
> > > >>+ if (!ulpi_bus.p)
> > > >>+ return -ENODEV;
> >
> > I think we need to warn in this case. How about:
> >
> > if (unlikely(WARN_ON(!ulpi_bus.p)))
> > return -ENODEV;
>
> I think we should also return -EAGAIN here.
The same check needs to be added to ulpi_register_interface() as well.
Cheers,
--
heikki
On 05/22/2015 06:16 AM, Heikki Krogerus wrote:
>>>>> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
>>>>> > > >>index 0e6f968..0b0a5e7 100644
>>>>> > > >>--- a/drivers/usb/common/ulpi.c
>>>>> > > >>+++ b/drivers/usb/common/ulpi.c
>>>>> > > >>@@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
>>>>> > > >> if (!drv->probe)
>>>>> > > >> return -EINVAL;
>>>>> > > >>+ /* Was the bus registered successfully? */
>>>>> > > >>+ if (!ulpi_bus.p)
>>>>> > > >>+ return -ENODEV;
>> >
>> > I think we need to warn in this case. How about:
>> >
>> > if (unlikely(WARN_ON(!ulpi_bus.p)))
No, please, the bus just doesn't exist - there's nothing wrong with that and there's
no reason to trigger an alarm for the user. Nothing out of the ordinary happened
here and the return value should be enough to tell the user what's up.
This will cause a perma-WARN for folks who have that bus built in but don't actually
have it on their system.
>> > return -ENODEV;
> I think we should also return -EAGAIN here.
EAGAIN? For when a bus doesn't exist? How would a user retrying fix the issue?
Thanks,
Sasha
On Wed, May 20, 2015 at 03:33:26PM -0400, Sasha Levin wrote:
> ULPI registers it's bus at module_init so if the bus fails to register, the
> module will fail to load and all will be well in the world.
>
> However, if the ULPI code is built-in rather than a module, the bus
> initialization may fail but we'd still try to register drivers later onto
> a non-existant bus, which will panic the kernel.
>
> Fix that by checking that the bus was indeed initialized before trying to
> register drivers on top of it.
>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
> drivers/usb/common/ulpi.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index 0e6f968..0b0a5e7 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
> if (!drv->probe)
> return -EINVAL;
>
> + /* Was the bus registered successfully? */
> + if (!ulpi_bus.p)
> + return -ENODEV;
Ick, no, don't go mucking around in the bus internals like this, that's
not ok. You should either "know" the bus is registered, or something is
really wrong with the design here.
greg k-h
On Sun, May 24, 2015 at 12:19:48AM -0700, Greg KH wrote:
> On Wed, May 20, 2015 at 03:33:26PM -0400, Sasha Levin wrote:
> > ULPI registers it's bus at module_init so if the bus fails to register, the
> > module will fail to load and all will be well in the world.
> >
> > However, if the ULPI code is built-in rather than a module, the bus
> > initialization may fail but we'd still try to register drivers later onto
> > a non-existant bus, which will panic the kernel.
> >
> > Fix that by checking that the bus was indeed initialized before trying to
> > register drivers on top of it.
> >
> > Signed-off-by: Sasha Levin <[email protected]>
> > ---
> > drivers/usb/common/ulpi.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > index 0e6f968..0b0a5e7 100644
> > --- a/drivers/usb/common/ulpi.c
> > +++ b/drivers/usb/common/ulpi.c
> > @@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
> > if (!drv->probe)
> > return -EINVAL;
> >
> > + /* Was the bus registered successfully? */
> > + if (!ulpi_bus.p)
> > + return -ENODEV;
>
> Ick, no, don't go mucking around in the bus internals like this, that's
> not ok. You should either "know" the bus is registered, or something is
> really wrong with the design here.
can't we use a variable which can be initialized to 1 in ulpi_init() if
the bus registers successfully and later check that?
regards
sudip
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Why do we even need that? If you take patch that makes ulpi_init a
subsys_initcall you won't have this problem, and no additional weird
hacks and errors will be needed
On Sun, May 24, 2015 at 11:09 AM, Sudip Mukherjee
<[email protected]> wrote:
> On Sun, May 24, 2015 at 12:19:48AM -0700, Greg KH wrote:
>> On Wed, May 20, 2015 at 03:33:26PM -0400, Sasha Levin wrote:
>> > ULPI registers it's bus at module_init so if the bus fails to register, the
>> > module will fail to load and all will be well in the world.
>> >
>> > However, if the ULPI code is built-in rather than a module, the bus
>> > initialization may fail but we'd still try to register drivers later onto
>> > a non-existant bus, which will panic the kernel.
>> >
>> > Fix that by checking that the bus was indeed initialized before trying to
>> > register drivers on top of it.
>> >
>> > Signed-off-by: Sasha Levin <[email protected]>
>> > ---
>> > drivers/usb/common/ulpi.c | 4 ++++
>> > 1 file changed, 4 insertions(+)
>> >
>> > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
>> > index 0e6f968..0b0a5e7 100644
>> > --- a/drivers/usb/common/ulpi.c
>> > +++ b/drivers/usb/common/ulpi.c
>> > @@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
>> > if (!drv->probe)
>> > return -EINVAL;
>> >
>> > + /* Was the bus registered successfully? */
>> > + if (!ulpi_bus.p)
>> > + return -ENODEV;
>>
>> Ick, no, don't go mucking around in the bus internals like this, that's
>> not ok. You should either "know" the bus is registered, or something is
>> really wrong with the design here.
> can't we use a variable which can be initialized to 1 in ulpi_init() if
> the bus registers successfully and later check that?
>
> regards
> sudip
>>
>> greg k-h
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
> Why do we even need that? If you take patch that makes ulpi_init a
> subsys_initcall you won't have this problem, and no additional weird
> hacks and errors will be needed
Using subsys_initcall will work around the problem for now, but I
would not make the assumption that there will never be ulpi phy
drivers and ulpi interface drivers that don't use subsys_initcall
themselves. By checking the old phy drivers at drivers/usb/phy/, at
least so far it hasn't been uncommon for them to use subsys_initcall.
I would much prefer to have a proper fix for this issue instead of
just working around it, but we need to use subsys_initcall in any
case.
> >> > + /* Was the bus registered successfully? */
> >> > + if (!ulpi_bus.p)
> >> > + return -ENODEV;
> >>
> >> Ick, no, don't go mucking around in the bus internals like this, that's
> >> not ok. You should either "know" the bus is registered, or something is
> >> really wrong with the design here.
> > can't we use a variable which can be initialized to 1 in ulpi_init() if
> > the bus registers successfully and later check that?
Just a note. We should also be aware if the bus registration failed or
if it just hasn't been loaded yet.
If we used a variable like that, I guess it could initially have the
value -EAGAIN. If bus_register returns error, we store -ENODEV to it.
If bus_register succeeds we store 0 to it. I don't know if we can just
store the return value from bus_register to it.
In ulpi_register_driver and ulpi_register_interface we can then just
return it if it has a value other then 0.
But couldn't we add a helper function to drivers/base/bus.c that the
bus drivers can use to at least check was the bus already loaded or
not? It looks like there are a couple of bus drivers that use the
struct bus member "p" to check that.
Greg, what do you think?
Thanks,
--
heikki
On Mon, May 25, 2015 at 02:40:10PM +0300, Heikki Krogerus wrote:
> Hi,
>
> > Why do we even need that? If you take patch that makes ulpi_init a
> > subsys_initcall you won't have this problem, and no additional weird
> > hacks and errors will be needed
>
> Using subsys_initcall will work around the problem for now, but I
> would not make the assumption that there will never be ulpi phy
> drivers and ulpi interface drivers that don't use subsys_initcall
> themselves. By checking the old phy drivers at drivers/usb/phy/, at
> least so far it hasn't been uncommon for them to use subsys_initcall.
>
> I would much prefer to have a proper fix for this issue instead of
> just working around it, but we need to use subsys_initcall in any
> case.
>
> > >> > + /* Was the bus registered successfully? */
> > >> > + if (!ulpi_bus.p)
> > >> > + return -ENODEV;
> > >>
> > >> Ick, no, don't go mucking around in the bus internals like this, that's
> > >> not ok. You should either "know" the bus is registered, or something is
> > >> really wrong with the design here.
> > > can't we use a variable which can be initialized to 1 in ulpi_init() if
> > > the bus registers successfully and later check that?
>
> Just a note. We should also be aware if the bus registration failed or
> if it just hasn't been loaded yet.
>
> If we used a variable like that, I guess it could initially have the
> value -EAGAIN. If bus_register returns error, we store -ENODEV to it.
> If bus_register succeeds we store 0 to it. I don't know if we can just
> store the return value from bus_register to it.
>
> In ulpi_register_driver and ulpi_register_interface we can then just
> return it if it has a value other then 0.
>
> But couldn't we add a helper function to drivers/base/bus.c that the
> bus drivers can use to at least check was the bus already loaded or
> not? It looks like there are a couple of bus drivers that use the
> struct bus member "p" to check that.
>
> Greg, what do you think?
I think your design is wrong if you need to worry about this :)
If there are other bus drivers that do this, I'll go fix them up,
pointers to those files would be appreciated.
thanks,
greg k-h
Greg KH <[email protected]> writes:
> If there are other bus drivers that do this, I'll go fix them up,
> pointers to those files would be appreciated.
git grep -E 'if .*\.p\W' found a couple of interesting candidates you
might want to check out:
drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p))) {
drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p)))
drivers/spmi/spmi.c: if (WARN_ON(!spmi_bus_type.p))
And this looks somewhat suspicious too, but could very well be OK for
all I know (very close to nothing :)
drivers/gpio/gpiolib-sysfs.c: if (!gpio_class.p) {
drivers/gpio/gpiolib-sysfs.c: if (!gpio_class.p)
Bjørn
Hi,
On Mon, May 25, 2015 at 07:00:13PM +0200, Bj?rn Mork wrote:
> Greg KH <[email protected]> writes:
>
> > If there are other bus drivers that do this, I'll go fix them up,
> > pointers to those files would be appreciated.
>
> git grep -E 'if .*\.p\W' found a couple of interesting candidates you
> might want to check out:
>
> drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p))) {
> drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p)))
> drivers/spmi/spmi.c: if (WARN_ON(!spmi_bus_type.p))
>
> And this looks somewhat suspicious too, but could very well be OK for
> all I know (very close to nothing :)
>
> drivers/gpio/gpiolib-sysfs.c: if (!gpio_class.p) {
> drivers/gpio/gpiolib-sysfs.c: if (!gpio_class.p)
I think we need a clear statement on how to proceed on this case. With
Linux, the code is the documentation most of times. I gave a reviewed-by
feedback based on seeing other examples following the same approach.
What would be the correct way?
Thanks,
David
On Mon, May 25, 2015 at 07:00:13PM +0200, Bj?rn Mork wrote:
> Greg KH <[email protected]> writes:
>
> > If there are other bus drivers that do this, I'll go fix them up,
> > pointers to those files would be appreciated.
>
> git grep -E 'if .*\.p\W' found a couple of interesting candidates you
> might want to check out:
>
> drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p))) {
> drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p)))
> drivers/spmi/spmi.c: if (WARN_ON(!spmi_bus_type.p))
>
> And this looks somewhat suspicious too, but could very well be OK for
> all I know (very close to nothing :)
>
> drivers/gpio/gpiolib-sysfs.c: if (!gpio_class.p) {
> drivers/gpio/gpiolib-sysfs.c: if (!gpio_class.p)
Thanks, when I get a chance I'll look into this...
greg k-h
On Tue, May 26, 2015 at 10:54:01AM -0700, David Cohen wrote:
> Hi,
>
> On Mon, May 25, 2015 at 07:00:13PM +0200, Bj?rn Mork wrote:
> > Greg KH <[email protected]> writes:
> >
> > > If there are other bus drivers that do this, I'll go fix them up,
> > > pointers to those files would be appreciated.
> >
> > git grep -E 'if .*\.p\W' found a couple of interesting candidates you
> > might want to check out:
> >
> > drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p))) {
> > drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p)))
> > drivers/spmi/spmi.c: if (WARN_ON(!spmi_bus_type.p))
> >
> > And this looks somewhat suspicious too, but could very well be OK for
> > all I know (very close to nothing :)
> >
> > drivers/gpio/gpiolib-sysfs.c: if (!gpio_class.p) {
> > drivers/gpio/gpiolib-sysfs.c: if (!gpio_class.p)
>
> I think we need a clear statement on how to proceed on this case.
Don't mess with bus->p. I can rename it to
"do_not_touch_this_isnt_for_you" if people think that would make it more
obvious that a private data structure shouldn't be messed with in any
way. Outside of the driver core, you have no knowledge that even if it
is a pointer, what that means with regards to anything.
thanks,
greg k-h