[PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Sasha Levin]

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 <sasha.levin@oracle.com>
---
 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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[David Cohen]

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 <sasha.levin@oracle.com>
> ---
>  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 <david.a.cohen@linux.intel.com>

>  	drv->driver.bus = &ulpi_bus;
>  
>  	return driver_register(&drv->driver);
> -- 
> 1.7.10.4
> 

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Lu, Baolu]

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 <sasha.levin@oracle.com>
>> ---
>>   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 <david.a.cohen@linux.intel.com>

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Heikki Krogerus]

> >>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 <sasha.levin@oracle.com>
> >>---
> >>  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 <david.a.cohen@linux.intel.com>
> 
> 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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Heikki Krogerus]

> > >>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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Heikki Krogerus]

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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Sasha Levin]

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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Greg KH]

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 <sasha.levin@oracle.com>
> ---
>  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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Sudip Mukherjee]

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 <sasha.levin@oracle.com>
> > ---
> >  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Tal Shorer]

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
<sudipm.mukherjee@gmail.com> 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 <sasha.levin@oracle.com>
>> > ---
>> >  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 majordomo@vger.kernel.org
>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Heikki Krogerus]

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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Greg KH]

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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Bjørn Mork]

Greg KH <gregkh@linuxfoundation.org> 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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[David Cohen]

Hi,

On Mon, May 25, 2015 at 07:00:13PM +0200, Bjørn Mork wrote:
> Greg KH <gregkh@linuxfoundation.org> 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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Greg KH]

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 <gregkh@linuxfoundation.org> 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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Sudip Mukherjee]

On Tue, May 26, 2015 at 07:41:18PM -0700, Greg KH wrote:
> 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 <gregkh@linuxfoundation.org> writes:
> > > 
> 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.
Being a newbie I had a newbie kind of doubt that if a module is builtin
and the init fails then what happens to the functions exported by it.
And to test that I created a module:
int abcd(void)
{
	pr_err("test: in abcd\n");
	return 0;
}
EXPORT_SYMBOL(abcd);

static int __init test_init(void)
{
	return -ENOMEM;
}
module_init(test_init);

static void __exit test_exit(void)
{
}
module_exit(test_exit);

Compiled it as builtin, and created another module which calls abcd();
and as expected abcd() executed.

So same thing can happen here also:
if bus_register() in ulpi_init() fails then also ulpi_unregister_driver()
can be executed as the symbol has been exported. you are saying bus->p is
private and not to use that but you are also saying that if we use another
variable to keep the status of bus registration then the design is wrong.
Then what should be the correct way?

regards
sudip

> 
> thanks,
> 
> greg k-h

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[David Cohen]

Hi Greg,

On Tue, May 26, 2015 at 07:41:18PM -0700, Greg KH wrote:
> 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 <gregkh@linuxfoundation.org> 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.

I get that, I agree it's ugly and I'm not trying to push it further. If
you follow the thread, you'll see I reviewed and commented a macro would
make more sense than checking the private data directly for the same
reaon you mentioned. But since with Linux development the source code is
part of the documentation, we need a clear state about what's the
correct way to handle it before go telling people "do not do what kernel
is already doing because it's wrong".

Br, David

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Greg KH]

On Mon, May 25, 2015 at 07:00:13PM +0200, Bjørn Mork wrote:
> Greg KH <gregkh@linuxfoundation.org> 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

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Heikki Krogerus]

Hi Greg,

> > 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 :)

This problem is not ulpi specific. We have the same issue with every
single bus. With a bus like PCI it's just really unlikely to hit it
because PCI bus driver uses postcore_initcall. But if there was
a PCI driver that used postcore_initcall itself, maybe a gpio
controller driver for example, exactly the same panic would happen
that we see happening when a driver tries to register itself with ulpi
bus before ulpi bus has been registered.

I can appreciate now that fixing the core problem like I2C did is
wrong, but I still feel that the driver core should provide something
like the helper for checking if the bus was registered already or not.
Otherwise all the bus drivers should really have a variable like Sudip
suggested for checking it, but that would be boilerplate, no?


Thanks,

-- 
heikki

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Sudip Mukherjee]

On Wed, May 27, 2015 at 11:39:33AM +0300, Heikki Krogerus wrote:
> Hi Greg,
> 
> I can appreciate now that fixing the core problem like I2C did is
> wrong, but I still feel that the driver core should provide something
> like the helper for checking if the bus was registered already or not.
> Otherwise all the bus drivers should really have a variable like Sudip
> suggested for checking it, but that would be boilerplate, no?
There might have been another way. There was a function called find_bus()
but it is under #if 0 , it used to find for a registered bus with its
name and NULL if not registered.

regards
sudip


> 
> Thanks,
> 
> -- 
> heikki

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Alan Stern]

On Wed, 27 May 2015, Heikki Krogerus wrote:

> This problem is not ulpi specific. We have the same issue with every
> single bus. With a bus like PCI it's just really unlikely to hit it
> because PCI bus driver uses postcore_initcall. But if there was
> a PCI driver that used postcore_initcall itself, maybe a gpio
> controller driver for example, exactly the same panic would happen
> that we see happening when a driver tries to register itself with ulpi
> bus before ulpi bus has been registered.
> 
> I can appreciate now that fixing the core problem like I2C did is
> wrong, but I still feel that the driver core should provide something
> like the helper for checking if the bus was registered already or not.
> Otherwise all the bus drivers should really have a variable like Sudip
> suggested for checking it, but that would be boilerplate, no?

This sounds like a tricky problem.

What is a driver supposed to do if it wants to register itself with a 
bus that hasn't been registered yet?  It can't just sit around 
and wait for the bus to be registered.  But the only alternative is to 
let its initialization fail, which means the driver will never be used.

The only solution I can think of is for the driver core to keep a list
of pending drivers in struct bus_type, for before the bus has been
registered.  When the bus finally does get registered, the core should
then register all the drivers waiting on that list.

Alan Stern

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Greg KH]

On Wed, May 27, 2015 at 11:16:34AM -0400, Alan Stern wrote:
> On Wed, 27 May 2015, Heikki Krogerus wrote:
> 
> > This problem is not ulpi specific. We have the same issue with every
> > single bus. With a bus like PCI it's just really unlikely to hit it
> > because PCI bus driver uses postcore_initcall. But if there was
> > a PCI driver that used postcore_initcall itself, maybe a gpio
> > controller driver for example, exactly the same panic would happen
> > that we see happening when a driver tries to register itself with ulpi
> > bus before ulpi bus has been registered.
> > 
> > I can appreciate now that fixing the core problem like I2C did is
> > wrong, but I still feel that the driver core should provide something
> > like the helper for checking if the bus was registered already or not.
> > Otherwise all the bus drivers should really have a variable like Sudip
> > suggested for checking it, but that would be boilerplate, no?
> 
> This sounds like a tricky problem.
> 
> What is a driver supposed to do if it wants to register itself with a 
> bus that hasn't been registered yet?  It can't just sit around 
> and wait for the bus to be registered.  But the only alternative is to 
> let its initialization fail, which means the driver will never be used.
> 
> The only solution I can think of is for the driver core to keep a list
> of pending drivers in struct bus_type, for before the bus has been
> registered.  When the bus finally does get registered, the core should
> then register all the drivers waiting on that list.

Ugh, no, that's a mess.  The bus needs to be registered first, if it
wasn't then something "bad" happened in the design, or the registration
failed.  If registration failed, then the driver should also fail when
it tries to register the bus.

Maybe we need to test for this in the driver core, not allowing drivers
for busses that are not registered, that might solve the main problem
here.  I'll try to look at it tonight.

thanks,

greg k-h

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Sudip Mukherjee]

On Wed, May 27, 2015 at 08:21:16AM -0700, Greg KH wrote:
> On Wed, May 27, 2015 at 11:16:34AM -0400, Alan Stern wrote:
> > On Wed, 27 May 2015, Heikki Krogerus wrote:
> 
> Maybe we need to test for this in the driver core, not allowing drivers
> for busses that are not registered, that might solve the main problem
> here.  I'll try to look at it tonight.
may i suggest something like this ?
buildtest with allmodconfig and allyesconfig on x86_64.
built and booted on x86.


diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 5005924..95cefa0 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -943,6 +943,7 @@ int bus_register(struct bus_type *bus)
 	if (retval)
 		goto bus_groups_fail;
 
+	bus->registered = true;
 	pr_debug("bus: '%s': registered\n", bus->name);
 	return 0;
 
diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 4eabfe2..1acae5b 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -150,6 +150,11 @@ int driver_register(struct device_driver *drv)
 	int ret;
 	struct device_driver *other;
 
+	if (!drv->bus->registered) {
+		pr_err("Driver %s registration failed. bus not yet registered\n",
+		       drv->name);
+		return -ENODEV;
+	}
 	BUG_ON(!drv->bus->p);
 
 	if ((drv->bus->probe && drv->probe) ||
diff --git a/include/linux/device.h b/include/linux/device.h
index 00ac57c..8fe4745 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -126,6 +126,7 @@ struct bus_type {
 	const struct dev_pm_ops *pm;
 
 	const struct iommu_ops *iommu_ops;
+	bool registered;	/* DON'T TOUCH THIS */
 
 	struct subsys_private *p;
 	struct lock_class_key lock_key;


regards
sudip
> 
> thanks,
> 
> greg k-h

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

On Thu, May 28, 2015 at 11:09:38AM +0530, Sudip Mukherjee wrote:
> On Wed, May 27, 2015 at 08:21:16AM -0700, Greg KH wrote:
> > On Wed, May 27, 2015 at 11:16:34AM -0400, Alan Stern wrote:
> > > On Wed, 27 May 2015, Heikki Krogerus wrote:
> > 
> > Maybe we need to test for this in the driver core, not allowing drivers
> > for busses that are not registered, that might solve the main problem
> > here.  I'll try to look at it tonight.
> may i suggest something like this ?
> buildtest with allmodconfig and allyesconfig on x86_64.
> built and booted on x86.
> 
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 5005924..95cefa0 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -943,6 +943,7 @@ int bus_register(struct bus_type *bus)
>  	if (retval)
>  		goto bus_groups_fail;
>  
> +	bus->registered = true;

once set, it's never cleared.

>  	pr_debug("bus: '%s': registered\n", bus->name);
>  	return 0;
>  
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 4eabfe2..1acae5b 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -150,6 +150,11 @@ int driver_register(struct device_driver *drv)
>  	int ret;
>  	struct device_driver *other;
>  
> +	if (!drv->bus->registered) {
> +		pr_err("Driver %s registration failed. bus not yet registered\n",
> +		       drv->name);
> +		return -ENODEV;
> +	}
>  	BUG_ON(!drv->bus->p);
>  
>  	if ((drv->bus->probe && drv->probe) ||
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 00ac57c..8fe4745 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -126,6 +126,7 @@ struct bus_type {
>  	const struct dev_pm_ops *pm;
>  
>  	const struct iommu_ops *iommu_ops;
> +	bool registered;	/* DON'T TOUCH THIS */

I would rather use an atomic_t

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Sudip Mukherjee]

On Thu, May 28, 2015 at 12:54:59AM -0500, Felipe Balbi wrote:
> On Thu, May 28, 2015 at 11:09:38AM +0530, Sudip Mukherjee wrote:
> > On Wed, May 27, 2015 at 08:21:16AM -0700, Greg KH wrote:
> > > On Wed, May 27, 2015 at 11:16:34AM -0400, Alan Stern wrote:
> > > > On Wed, 27 May 2015, Heikki Krogerus wrote:
> > > 
> > > Maybe we need to test for this in the driver core, not allowing drivers
> > > for busses that are not registered, that might solve the main problem
> > > here.  I'll try to look at it tonight.
> > may i suggest something like this ?
> > buildtest with allmodconfig and allyesconfig on x86_64.
> > built and booted on x86.
> > 
> > 
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 5005924..95cefa0 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -943,6 +943,7 @@ int bus_register(struct bus_type *bus)
> >  	if (retval)
> >  		goto bus_groups_fail;
> >  
> > +	bus->registered = true;
> 
> once set, it's never cleared.
It should be cleared when we go for bus_unregister.
> 
> >  	pr_debug("bus: '%s': registered\n", bus->name);
> >  	return 0;
> >  
> > diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> > index 4eabfe2..1acae5b 100644
> > --- a/drivers/base/driver.c
> > +++ b/drivers/base/driver.c
> > @@ -150,6 +150,11 @@ int driver_register(struct device_driver *drv)
> >  	int ret;
> >  	struct device_driver *other;
> >  
> > +	if (!drv->bus->registered) {
> > +		pr_err("Driver %s registration failed. bus not yet registered\n",
> > +		       drv->name);
> > +		return -ENODEV;
> > +	}
> >  	BUG_ON(!drv->bus->p);
> >  
> >  	if ((drv->bus->probe && drv->probe) ||
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 00ac57c..8fe4745 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -126,6 +126,7 @@ struct bus_type {
> >  	const struct dev_pm_ops *pm;
> >  
> >  	const struct iommu_ops *iommu_ops;
> > +	bool registered;	/* DON'T TOUCH THIS */
> 
> I would rather use an atomic_t
ok.
This was just an idea, if Greg and you all are okay with it then I
can submit a formal patch.
BTW, the original comment that I thought was:
/* don't use else Greg will scold */

regards
sudip
> 
> -- 
> balbi

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Sudip Mukherjee]

On Thu, May 28, 2015 at 12:12:56PM +0530, Sudip Mukherjee wrote:
> On Thu, May 28, 2015 at 12:54:59AM -0500, Felipe Balbi wrote:
> > On Thu, May 28, 2015 at 11:09:38AM +0530, Sudip Mukherjee wrote:
> > > On Wed, May 27, 2015 at 08:21:16AM -0700, Greg KH wrote:
> > > > On Wed, May 27, 2015 at 11:16:34AM -0400, Alan Stern wrote:
> > > > > On Wed, 27 May 2015, Heikki Krogerus wrote:
> > > > 
> > > > Maybe we need to test for this in the driver core, not allowing drivers
> > > > for busses that are not registered, that might solve the main problem
> > > > here.  I'll try to look at it tonight.
> > > may i suggest something like this ?
> > > buildtest with allmodconfig and allyesconfig on x86_64.
> > > built and booted on x86.
> > > 
> > > 
> > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > > index 5005924..95cefa0 100644
> > > --- a/drivers/base/bus.c
> > > +++ b/drivers/base/bus.c
> > > @@ -943,6 +943,7 @@ int bus_register(struct bus_type *bus)
> > >  	if (retval)
> > >  		goto bus_groups_fail;
> > >  
> > > +	bus->registered = true;
> > 
> > once set, it's never cleared.
> It should be cleared when we go for bus_unregister.
apart from bus_register we need the check in some other places also.
like bus_create_file().

regards
sudip

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Alan Stern]

On Thu, 28 May 2015, Felipe Balbi wrote:

> On Thu, May 28, 2015 at 11:09:38AM +0530, Sudip Mukherjee wrote:
> > On Wed, May 27, 2015 at 08:21:16AM -0700, Greg KH wrote:
> > > On Wed, May 27, 2015 at 11:16:34AM -0400, Alan Stern wrote:
> > > > On Wed, 27 May 2015, Heikki Krogerus wrote:
> > > 
> > > Maybe we need to test for this in the driver core, not allowing drivers
> > > for busses that are not registered, that might solve the main problem
> > > here.  I'll try to look at it tonight.
> > may i suggest something like this ?
> > buildtest with allmodconfig and allyesconfig on x86_64.
> > built and booted on x86.
> > 
> > 
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 5005924..95cefa0 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -943,6 +943,7 @@ int bus_register(struct bus_type *bus)
> >  	if (retval)
> >  		goto bus_groups_fail;
> >  
> > +	bus->registered = true;
> 
> once set, it's never cleared.

It's worse than that...

> >  	pr_debug("bus: '%s': registered\n", bus->name);
> >  	return 0;
> >  
> > diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> > index 4eabfe2..1acae5b 100644
> > --- a/drivers/base/driver.c
> > +++ b/drivers/base/driver.c
> > @@ -150,6 +150,11 @@ int driver_register(struct device_driver *drv)
> >  	int ret;
> >  	struct device_driver *other;
> >  
> > +	if (!drv->bus->registered) {
> > +		pr_err("Driver %s registration failed. bus not yet registered\n",
> > +		       drv->name);
> > +		return -ENODEV;
> > +	}
> >  	BUG_ON(!drv->bus->p);
> >  
> >  	if ((drv->bus->probe && drv->probe) ||
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 00ac57c..8fe4745 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -126,6 +126,7 @@ struct bus_type {
> >  	const struct dev_pm_ops *pm;
> >  
> >  	const struct iommu_ops *iommu_ops;
> > +	bool registered;	/* DON'T TOUCH THIS */
> 
> I would rather use an atomic_t

What reason is there to use an atomic_t?  The value is never going to 
be changed by two threads at the same time.

More importantly, clearing the flag races with checking it.  If 
somebody tries to register a driver at the same time as the bus is 
unregistered, the result is undefined.

Of course, the same problem exists when a device is added to a bus at 
the same time as the bus is unregistered.

Alan Stern

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Sasha Levin]

On 05/28/2015 01:39 AM, Sudip Mukherjee wrote:
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 4eabfe2..1acae5b 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -150,6 +150,11 @@ int driver_register(struct device_driver *drv)
>  	int ret;
>  	struct device_driver *other;
>  
> +	if (!drv->bus->registered) {
> +		pr_err("Driver %s registration failed. bus not yet registered\n",
> +		       drv->name);
> +		return -ENODEV;
> +	}
>  	BUG_ON(!drv->bus->p);

This is a design issue with the code in the layer above, there's no reason
driver_register() should be called with a bus that wasn't registered to
begin with.

This is why there's a BUG_ON there to catch these issues - it's a bug, not
a desired behaviour.


Thanks,
Sasha

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Heikki Krogerus]

On Thu, May 28, 2015 at 08:36:46AM -0400, Sasha Levin wrote:
> On 05/28/2015 01:39 AM, Sudip Mukherjee wrote:
> > diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> > index 4eabfe2..1acae5b 100644
> > --- a/drivers/base/driver.c
> > +++ b/drivers/base/driver.c
> > @@ -150,6 +150,11 @@ int driver_register(struct device_driver *drv)
> >  	int ret;
> >  	struct device_driver *other;
> >  
> > +	if (!drv->bus->registered) {
> > +		pr_err("Driver %s registration failed. bus not yet registered\n",
> > +		       drv->name);
> > +		return -ENODEV;
> > +	}
> >  	BUG_ON(!drv->bus->p);
> 
> This is a design issue with the code in the layer above, there's no reason
> driver_register() should be called with a bus that wasn't registered to
> begin with.
> 
> This is why there's a BUG_ON there to catch these issues - it's a bug, not
> a desired behaviour.

Unfortunately problems with the design are not the only cases why we
could end up here before the bus has been registered. If the bus has
failed to register, we definitely should not trigger a BUG here. The
bus management driver has in that case already made the decision to
not BUG. Or if the user is allowed to disable a bus somehow, for
example with something like nousb parameter, but we still manage do
get here, we should again not trigger BUG().

I don't think BUG_ON here is ever the correct thing to do. This
function can see that the bus doesn't exist or possibly that something
has gone wrong by checking the "p", but it does not know any details,
nor should it. This function should trigger a warning in those cases
and return failure, and not make any extra decisions.


Thanks,

-- 
heikki

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Greg KH]

On Thu, May 28, 2015 at 11:09:38AM +0530, Sudip Mukherjee wrote:
> On Wed, May 27, 2015 at 08:21:16AM -0700, Greg KH wrote:
> > On Wed, May 27, 2015 at 11:16:34AM -0400, Alan Stern wrote:
> > > On Wed, 27 May 2015, Heikki Krogerus wrote:
> > 
> > Maybe we need to test for this in the driver core, not allowing drivers
> > for busses that are not registered, that might solve the main problem
> > here.  I'll try to look at it tonight.
> may i suggest something like this ?
> buildtest with allmodconfig and allyesconfig on x86_64.
> built and booted on x86.
> 
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 5005924..95cefa0 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -943,6 +943,7 @@ int bus_register(struct bus_type *bus)
>  	if (retval)
>  		goto bus_groups_fail;
>  
> +	bus->registered = true;
>  	pr_debug("bus: '%s': registered\n", bus->name);
>  	return 0;
>  
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 4eabfe2..1acae5b 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -150,6 +150,11 @@ int driver_register(struct device_driver *drv)
>  	int ret;
>  	struct device_driver *other;
>  
> +	if (!drv->bus->registered) {
> +		pr_err("Driver %s registration failed. bus not yet registered\n",
> +		       drv->name);
> +		return -ENODEV;
> +	}
>  	BUG_ON(!drv->bus->p);
>  
>  	if ((drv->bus->probe && drv->probe) ||
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 00ac57c..8fe4745 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -126,6 +126,7 @@ struct bus_type {
>  	const struct dev_pm_ops *pm;
>  
>  	const struct iommu_ops *iommu_ops;
> +	bool registered;	/* DON'T TOUCH THIS */

As if anyone ever looks at comments and follows them :)

Anyway, let me look at this some more, I think this needs to go into the
individual buses, not the driver core.  There's a lot more to initialize
than just the driver core for most busses, they should be the ones to
allow a driver to be registered or not, not the driver core, which
shouldn't care.

I have some time on Saturday (very long plane ride), where I'll take a
look at this.

thanks,

greg k-h