Re: [PATCH] nubus: Unconditionally register bus type

Re: [PATCH] nubus: Unconditionally register bus type

[Geert Uytterhoeven]

Hi Finn,

On Sun, May 6, 2018 at 3:47 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> Loading a NuBus driver module on a non-NuBus machine triggers the
> BUG_ON(!drv->bus->p) in driver_register() because the bus does not get
> registered unless MACH_IS_MAC(). Avoid this by registering the bus
> unconditionally using postcore_initcall().
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reported-by: Michael Schmitz <schmitzmic@gmail.com>
> Tested-by: Stan Johnson <userm57@yahoo.com>
> Fixes: 7f86c765a6a2 ("nubus: Add support for the driver model")
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> ---
>  drivers/nubus/bus.c   | 3 ++-
>  drivers/nubus/nubus.c | 5 -----
>  include/linux/nubus.h | 1 -
>  3 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/nubus/bus.c b/drivers/nubus/bus.c
> index d306c348c857..27ca9f1a281b 100644
> --- a/drivers/nubus/bus.c
> +++ b/drivers/nubus/bus.c
> @@ -63,7 +63,7 @@ static struct device nubus_parent = {
>         .init_name      = "nubus",
>  };
>
> -int __init nubus_bus_register(void)
> +static int __init nubus_bus_register(void)
>  {
>         int err;
>
> @@ -78,6 +78,7 @@ int __init nubus_bus_register(void)
>         device_unregister(&nubus_parent);
>         return err;
>  }
> +postcore_initcall(nubus_bus_register);

nubus_bus_register() two things:

  1. Register the NuBus parent device, which represents the bus host,
  2. Register the NuBus bus, which represents the bus type.

I think this should be split in two, and only the latter should be done
regardless of the presence of NuBus host hardware, to fix the crash.

>
>  static void nubus_device_release(struct device *dev)
>  {
> diff --git a/drivers/nubus/nubus.c b/drivers/nubus/nubus.c
> index 4621ff98138c..f6bab483f4cb 100644
> --- a/drivers/nubus/nubus.c
> +++ b/drivers/nubus/nubus.c
> @@ -869,15 +869,10 @@ static void __init nubus_scan_bus(void)
>
>  static int __init nubus_init(void)
>  {
> -       int err;
> -
>         if (!MACH_IS_MAC)
>                 return 0;
>
>         nubus_proc_init();
> -       err = nubus_bus_register();
> -       if (err)
> -               return err;

Hence you should still register the NuBus parent device here.

>         nubus_scan_bus();
>         return 0;
>  }


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] nubus: Unconditionally register bus type

[Finn Thain]

On Tue, 8 May 2018, Geert Uytterhoeven wrote:

> nubus_bus_register() two things:
> 
>   1. Register the NuBus parent device, which represents the bus host,
>   2. Register the NuBus bus, which represents the bus type.
> 
> I think this should be split in two, and only the latter should be done 
> regardless of the presence of NuBus host hardware, to fix the crash.

OK. Thanks.

-- 

[PATCH] nubus: Unconditionally register bus type

[Finn Thain]

Loading a NuBus driver module on a non-NuBus machine triggers the
BUG_ON(!drv->bus->p) in driver_register() because the bus does not get
registered unless MACH_IS_MAC(). Avoid this by registering the bus
unconditionally using postcore_initcall().

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reported-by: Michael Schmitz <schmitzmic@gmail.com>
Tested-by: Stan Johnson <userm57@yahoo.com>
Fixes: 7f86c765a6a2 ("nubus: Add support for the driver model")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/nubus/bus.c   | 3 ++-
 drivers/nubus/nubus.c | 5 -----
 include/linux/nubus.h | 1 -
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/nubus/bus.c b/drivers/nubus/bus.c
index d306c348c857..27ca9f1a281b 100644
--- a/drivers/nubus/bus.c
+++ b/drivers/nubus/bus.c
@@ -63,7 +63,7 @@ static struct device nubus_parent = {
 	.init_name	= "nubus",
 };
 
-int __init nubus_bus_register(void)
+static int __init nubus_bus_register(void)
 {
 	int err;
 
@@ -78,6 +78,7 @@ int __init nubus_bus_register(void)
 	device_unregister(&nubus_parent);
 	return err;
 }
+postcore_initcall(nubus_bus_register);
 
 static void nubus_device_release(struct device *dev)
 {
diff --git a/drivers/nubus/nubus.c b/drivers/nubus/nubus.c
index 4621ff98138c..f6bab483f4cb 100644
--- a/drivers/nubus/nubus.c
+++ b/drivers/nubus/nubus.c
@@ -869,15 +869,10 @@ static void __init nubus_scan_bus(void)
 
 static int __init nubus_init(void)
 {
-	int err;
-
 	if (!MACH_IS_MAC)
 		return 0;
 
 	nubus_proc_init();
-	err = nubus_bus_register();
-	if (err)
-		return err;
 	nubus_scan_bus();
 	return 0;
 }
diff --git a/include/linux/nubus.h b/include/linux/nubus.h
index 6e8200215321..f01e7f4bcff8 100644
--- a/include/linux/nubus.h
+++ b/include/linux/nubus.h
@@ -163,7 +163,6 @@ void nubus_seq_write_rsrc_mem(struct seq_file *m,
 unsigned char *nubus_dirptr(const struct nubus_dirent *nd);
 
 /* Declarations relating to driver model objects */
-int nubus_bus_register(void);
 int nubus_device_register(struct nubus_board *board);
 int nubus_driver_register(struct nubus_driver *ndrv);
 void nubus_driver_unregister(struct nubus_driver *ndrv);
-- 
2.16.1

[PATCH] nubus: Unconditionally register bus type

[Finn Thain]

Loading a NuBus driver module on a non-NuBus machine triggers the
BUG_ON(!drv->bus->p) in driver_register() because the bus does not get
registered unless MACH_IS_MAC(). Avoid this by registering the bus
unconditionally using postcore_initcall().

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reported-by: Michael Schmitz <schmitzmic@gmail.com>
Tested-by: Stan Johnson <userm57@yahoo.com>
Fixes: 7f86c765a6a2 ("nubus: Add support for the driver model")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/nubus/bus.c   | 3 ++-
 drivers/nubus/nubus.c | 5 -----
 include/linux/nubus.h | 1 -
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/nubus/bus.c b/drivers/nubus/bus.c
index d306c348c857..27ca9f1a281b 100644
--- a/drivers/nubus/bus.c
+++ b/drivers/nubus/bus.c
@@ -63,7 +63,7 @@ static struct device nubus_parent = {
 	.init_name	= "nubus",
 };
 
-int __init nubus_bus_register(void)
+static int __init nubus_bus_register(void)
 {
 	int err;
 
@@ -78,6 +78,7 @@ int __init nubus_bus_register(void)
 	device_unregister(&nubus_parent);
 	return err;
 }
+postcore_initcall(nubus_bus_register);
 
 static void nubus_device_release(struct device *dev)
 {
diff --git a/drivers/nubus/nubus.c b/drivers/nubus/nubus.c
index 4621ff98138c..f6bab483f4cb 100644
--- a/drivers/nubus/nubus.c
+++ b/drivers/nubus/nubus.c
@@ -869,15 +869,10 @@ static void __init nubus_scan_bus(void)
 
 static int __init nubus_init(void)
 {
-	int err;
-
 	if (!MACH_IS_MAC)
 		return 0;
 
 	nubus_proc_init();
-	err = nubus_bus_register();
-	if (err)
-		return err;
 	nubus_scan_bus();
 	return 0;
 }
diff --git a/include/linux/nubus.h b/include/linux/nubus.h
index 6e8200215321..f01e7f4bcff8 100644
--- a/include/linux/nubus.h
+++ b/include/linux/nubus.h
@@ -163,7 +163,6 @@ void nubus_seq_write_rsrc_mem(struct seq_file *m,
 unsigned char *nubus_dirptr(const struct nubus_dirent *nd);
 
 /* Declarations relating to driver model objects */
-int nubus_bus_register(void);
 int nubus_device_register(struct nubus_board *board);
 int nubus_driver_register(struct nubus_driver *ndrv);
 void nubus_driver_unregister(struct nubus_driver *ndrv);
-- 
2.16.1

Re: [PATCH] nubus: Unconditionally register bus type

[Greg Kroah-Hartman]

On Sun, May 06, 2018 at 11:47:52AM +1000, Finn Thain wrote:
> Loading a NuBus driver module on a non-NuBus machine triggers the
> BUG_ON(!drv->bus->p) in driver_register() because the bus does not get
> registered unless MACH_IS_MAC(). Avoid this by registering the bus
> unconditionally using postcore_initcall().
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reported-by: Michael Schmitz <schmitzmic@gmail.com>
> Tested-by: Stan Johnson <userm57@yahoo.com>
> Fixes: 7f86c765a6a2 ("nubus: Add support for the driver model")
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> ---
>  drivers/nubus/bus.c   | 3 ++-
>  drivers/nubus/nubus.c | 5 -----
>  include/linux/nubus.h | 1 -
>  3 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/nubus/bus.c b/drivers/nubus/bus.c
> index d306c348c857..27ca9f1a281b 100644
> --- a/drivers/nubus/bus.c
> +++ b/drivers/nubus/bus.c
> @@ -63,7 +63,7 @@ static struct device nubus_parent = {
>  	.init_name	= "nubus",
>  };
>  
> -int __init nubus_bus_register(void)
> +static int __init nubus_bus_register(void)
>  {
>  	int err;
>  
> @@ -78,6 +78,7 @@ int __init nubus_bus_register(void)
>  	device_unregister(&nubus_parent);
>  	return err;
>  }
> +postcore_initcall(nubus_bus_register);

Why not just have an "bus is registered" flag in your driver register
function that refuses to let drivers register with the driver core if it
isn't set?  And then fix your linking error, the bus should come first
in link order, before your drivers :)

thanks,

greg k-h

Re: [PATCH] nubus: Unconditionally register bus type

[Finn Thain]

On Sat, 5 May 2018, Greg Kroah-Hartman wrote:

> On Sun, May 06, 2018 at 11:47:52AM +1000, Finn Thain wrote:
> > Loading a NuBus driver module on a non-NuBus machine triggers the
> > BUG_ON(!drv->bus->p) in driver_register() because the bus does not get
> > registered unless MACH_IS_MAC(). Avoid this by registering the bus
> > unconditionally using postcore_initcall().
> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Reported-by: Michael Schmitz <schmitzmic@gmail.com>
> > Tested-by: Stan Johnson <userm57@yahoo.com>
> > Fixes: 7f86c765a6a2 ("nubus: Add support for the driver model")
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > ---
> >  drivers/nubus/bus.c   | 3 ++-
> >  drivers/nubus/nubus.c | 5 -----
> >  include/linux/nubus.h | 1 -
> >  3 files changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/nubus/bus.c b/drivers/nubus/bus.c
> > index d306c348c857..27ca9f1a281b 100644
> > --- a/drivers/nubus/bus.c
> > +++ b/drivers/nubus/bus.c
> > @@ -63,7 +63,7 @@ static struct device nubus_parent = {
> >  	.init_name	= "nubus",
> >  };
> >  
> > -int __init nubus_bus_register(void)
> > +static int __init nubus_bus_register(void)
> >  {
> >  	int err;
> >  
> > @@ -78,6 +78,7 @@ int __init nubus_bus_register(void)
> >  	device_unregister(&nubus_parent);
> >  	return err;
> >  }
> > +postcore_initcall(nubus_bus_register);
> 
> Why not just have an "bus is registered" flag in your driver register
> function that refuses to let drivers register with the driver core if it
> isn't set? 

Perhaps that should happen in the core driver_register() function. BUG_ON 
is frowned upon, after all. Would that be acceptable?

I found a few drivers that set a flag the way you describe, which could 
then be simplified.

But that pattern is rare. Most buses use the postcore_initcall() pattern, 
and so my patch took the conventional approach.

> And then fix your linking error, the bus should come first in link 
> order, before your drivers :)
> 

I didn't encounter any errors. How shall I reproduce this?

Thanks.

-- 

> thanks,
> 
> greg k-h

Re: [PATCH] nubus: Unconditionally register bus type

[Greg Kroah-Hartman]

On Sun, May 06, 2018 at 04:00:15PM +1000, Finn Thain wrote:
> On Sat, 5 May 2018, Greg Kroah-Hartman wrote:
> 
> > On Sun, May 06, 2018 at 11:47:52AM +1000, Finn Thain wrote:
> > > Loading a NuBus driver module on a non-NuBus machine triggers the
> > > BUG_ON(!drv->bus->p) in driver_register() because the bus does not get
> > > registered unless MACH_IS_MAC(). Avoid this by registering the bus
> > > unconditionally using postcore_initcall().
> > > 
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Reported-by: Michael Schmitz <schmitzmic@gmail.com>
> > > Tested-by: Stan Johnson <userm57@yahoo.com>
> > > Fixes: 7f86c765a6a2 ("nubus: Add support for the driver model")
> > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > > ---
> > >  drivers/nubus/bus.c   | 3 ++-
> > >  drivers/nubus/nubus.c | 5 -----
> > >  include/linux/nubus.h | 1 -
> > >  3 files changed, 2 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/nubus/bus.c b/drivers/nubus/bus.c
> > > index d306c348c857..27ca9f1a281b 100644
> > > --- a/drivers/nubus/bus.c
> > > +++ b/drivers/nubus/bus.c
> > > @@ -63,7 +63,7 @@ static struct device nubus_parent = {
> > >  	.init_name	= "nubus",
> > >  };
> > >  
> > > -int __init nubus_bus_register(void)
> > > +static int __init nubus_bus_register(void)
> > >  {
> > >  	int err;
> > >  
> > > @@ -78,6 +78,7 @@ int __init nubus_bus_register(void)
> > >  	device_unregister(&nubus_parent);
> > >  	return err;
> > >  }
> > > +postcore_initcall(nubus_bus_register);
> > 
> > Why not just have an "bus is registered" flag in your driver register
> > function that refuses to let drivers register with the driver core if it
> > isn't set? 
> 
> Perhaps that should happen in the core driver_register() function. BUG_ON 
> is frowned upon, after all. Would that be acceptable?

I don't understand what you mean here, perhaps make a patch to show it?

> I found a few drivers that set a flag the way you describe, which could 
> then be simplified.
> 
> But that pattern is rare. Most buses use the postcore_initcall() pattern, 
> and so my patch took the conventional approach.

It all depends on link order, not necessarily the postcore stuff.

> > And then fix your linking error, the bus should come first in link 
> > order, before your drivers :)
> > 
> 
> I didn't encounter any errors. How shall I reproduce this?

If you have not seen this error, then why change the code at all if it
is working properly?  Most busses do not need this as they have their
link order set up correctly, no need to mess with stuff that is not
broken :)

thanks,

greg k-h

Re: [PATCH] nubus: Unconditionally register bus type

[Michael Schmitz]

Hi Greg,

the BUG() was triggered by loading a Mac Nubus network card module on
a multiplatform kernel running on an Amiga machine. Up to Finn's Nubus
core rewrite (this February), we've seen no errors. Since then, Nubus
drivers fail to register because the Nubus bus is only registered on
Macs.

Can't see link order involved here at all.

Safeguarding against this bug could be done by checking a
bus-is-registered flag, or checking what machine model the kernel runs
on. Simply registering the Nubus bus driver regardless of machine
model seemed the easiest way.

Cheers,

  Michael


On Mon, May 7, 2018 at 8:20 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, May 06, 2018 at 04:00:15PM +1000, Finn Thain wrote:
>> On Sat, 5 May 2018, Greg Kroah-Hartman wrote:
>>
>> > On Sun, May 06, 2018 at 11:47:52AM +1000, Finn Thain wrote:
>> > > Loading a NuBus driver module on a non-NuBus machine triggers the
>> > > BUG_ON(!drv->bus->p) in driver_register() because the bus does not get
>> > > registered unless MACH_IS_MAC(). Avoid this by registering the bus
>> > > unconditionally using postcore_initcall().
>> > >
>> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > > Reported-by: Michael Schmitz <schmitzmic@gmail.com>
>> > > Tested-by: Stan Johnson <userm57@yahoo.com>
>> > > Fixes: 7f86c765a6a2 ("nubus: Add support for the driver model")
>> > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>> > > ---
>> > >  drivers/nubus/bus.c   | 3 ++-
>> > >  drivers/nubus/nubus.c | 5 -----
>> > >  include/linux/nubus.h | 1 -
>> > >  3 files changed, 2 insertions(+), 7 deletions(-)
>> > >
>> > > diff --git a/drivers/nubus/bus.c b/drivers/nubus/bus.c
>> > > index d306c348c857..27ca9f1a281b 100644
>> > > --- a/drivers/nubus/bus.c
>> > > +++ b/drivers/nubus/bus.c
>> > > @@ -63,7 +63,7 @@ static struct device nubus_parent = {
>> > >   .init_name      = "nubus",
>> > >  };
>> > >
>> > > -int __init nubus_bus_register(void)
>> > > +static int __init nubus_bus_register(void)
>> > >  {
>> > >   int err;
>> > >
>> > > @@ -78,6 +78,7 @@ int __init nubus_bus_register(void)
>> > >   device_unregister(&nubus_parent);
>> > >   return err;
>> > >  }
>> > > +postcore_initcall(nubus_bus_register);
>> >
>> > Why not just have an "bus is registered" flag in your driver register
>> > function that refuses to let drivers register with the driver core if it
>> > isn't set?
>>
>> Perhaps that should happen in the core driver_register() function. BUG_ON
>> is frowned upon, after all. Would that be acceptable?
>
> I don't understand what you mean here, perhaps make a patch to show it?
>
>> I found a few drivers that set a flag the way you describe, which could
>> then be simplified.
>>
>> But that pattern is rare. Most buses use the postcore_initcall() pattern,
>> and so my patch took the conventional approach.
>
> It all depends on link order, not necessarily the postcore stuff.
>
>> > And then fix your linking error, the bus should come first in link
>> > order, before your drivers :)
>> >
>>
>> I didn't encounter any errors. How shall I reproduce this?
>
> If you have not seen this error, then why change the code at all if it
> is working properly?  Most busses do not need this as they have their
> link order set up correctly, no need to mess with stuff that is not
> broken :)
>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nubus: Unconditionally register bus type

[Greg Kroah-Hartman]

On Mon, May 07, 2018 at 09:51:12AM +1200, Michael Schmitz wrote:
> Hi Greg,
> 
> the BUG() was triggered by loading a Mac Nubus network card module on
> a multiplatform kernel running on an Amiga machine. Up to Finn's Nubus
> core rewrite (this February), we've seen no errors. Since then, Nubus
> drivers fail to register because the Nubus bus is only registered on
> Macs.
> 
> Can't see link order involved here at all.

The link order is totally involved here :)

Link order determines the order in which init calls are run, so you need
to ensure that your bus code comes before any drivers that use that bus
code in link order.  That way, at init time, your bus is created first,
preventing this type of error to happen.

> Safeguarding against this bug could be done by checking a
> bus-is-registered flag, or checking what machine model the kernel runs
> on. Simply registering the Nubus bus driver regardless of machine
> model seemed the easiest way.

If you really have this problem due to link ordering issues not being
able to be worked around (due to different directories and the like),
then put a "bus is registered" flag in your bus code.  That will solve
the initial BUG_ON() problem, and you should return an error that allows
the driver code to probe your driver later on after your bus is enabled.

Hope this helps,

greg k-h

Re: [PATCH] nubus: Unconditionally register bus type

[Geert Uytterhoeven]

Hi Greg,

On Tue, May 8, 2018 at 9:00 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, May 07, 2018 at 09:51:12AM +1200, Michael Schmitz wrote:
>> the BUG() was triggered by loading a Mac Nubus network card module on
>> a multiplatform kernel running on an Amiga machine. Up to Finn's Nubus
>> core rewrite (this February), we've seen no errors. Since then, Nubus
>> drivers fail to register because the Nubus bus is only registered on
>> Macs.
>>
>> Can't see link order involved here at all.
>
> The link order is totally involved here :)
>
> Link order determines the order in which init calls are run, so you need
> to ensure that your bus code comes before any drivers that use that bus
> code in link order.  That way, at init time, your bus is created first,
> preventing this type of error to happen.

The issue here is not due to link ordering, but due to the bus not being
registered on a system that doesn't have that particular bus.

Akin to booting a kernel on an old PC without PCI, and loading a driver
module for a PCI network card. I guess that doesn't crash (because no one
has a PC without PCI anymore? ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] nubus: Unconditionally register bus type

[Greg Kroah-Hartman]

On Tue, May 08, 2018 at 09:07:27AM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Tue, May 8, 2018 at 9:00 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, May 07, 2018 at 09:51:12AM +1200, Michael Schmitz wrote:
> >> the BUG() was triggered by loading a Mac Nubus network card module on
> >> a multiplatform kernel running on an Amiga machine. Up to Finn's Nubus
> >> core rewrite (this February), we've seen no errors. Since then, Nubus
> >> drivers fail to register because the Nubus bus is only registered on
> >> Macs.
> >>
> >> Can't see link order involved here at all.
> >
> > The link order is totally involved here :)
> >
> > Link order determines the order in which init calls are run, so you need
> > to ensure that your bus code comes before any drivers that use that bus
> > code in link order.  That way, at init time, your bus is created first,
> > preventing this type of error to happen.
> 
> The issue here is not due to link ordering, but due to the bus not being
> registered on a system that doesn't have that particular bus.

But how can that happen if the bus code is not present in the system at
that point in time?  Hardware doesn't matter at all here.

> Akin to booting a kernel on an old PC without PCI, and loading a driver
> module for a PCI network card. I guess that doesn't crash (because no one
> has a PC without PCI anymore? ;-)

No, it should work just fine, try it!  :)

The driver will not bind to anything, but the bus code should work
properly, as long as it is initialized before the driver tries to
register with that specific bus type.

thanks,

greg k-h

Re: [PATCH] nubus: Unconditionally register bus type

[Geert Uytterhoeven]

Hi Greg,

On Tue, May 8, 2018 at 9:25 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, May 08, 2018 at 09:07:27AM +0200, Geert Uytterhoeven wrote:
>> On Tue, May 8, 2018 at 9:00 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Mon, May 07, 2018 at 09:51:12AM +1200, Michael Schmitz wrote:
>> >> the BUG() was triggered by loading a Mac Nubus network card module on
>> >> a multiplatform kernel running on an Amiga machine. Up to Finn's Nubus
>> >> core rewrite (this February), we've seen no errors. Since then, Nubus
>> >> drivers fail to register because the Nubus bus is only registered on
>> >> Macs.
>> >>
>> >> Can't see link order involved here at all.
>> >
>> > The link order is totally involved here :)
>> >
>> > Link order determines the order in which init calls are run, so you need
>> > to ensure that your bus code comes before any drivers that use that bus
>> > code in link order.  That way, at init time, your bus is created first,
>> > preventing this type of error to happen.
>>
>> The issue here is not due to link ordering, but due to the bus not being
>> registered on a system that doesn't have that particular bus.
>
> But how can that happen if the bus code is not present in the system at
> that point in time?  Hardware doesn't matter at all here.

The bus code is present in the system.
The bus is just not registered by the NuBus bus driver if the hardware
doesn't have a NuBus host.

>> Akin to booting a kernel on an old PC without PCI, and loading a driver
>> module for a PCI network card. I guess that doesn't crash (because no one
>> has a PC without PCI anymore? ;-)
>
> No, it should work just fine, try it!  :)
>
> The driver will not bind to anything, but the bus code should work
> properly, as long as it is initialized before the driver tries to
> register with that specific bus type.

Hence the NuBus bus code should register the bus irregardless of the
presence of the NuBus host hardware.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] nubus: Unconditionally register bus type

[Greg Kroah-Hartman]

On Tue, May 08, 2018 at 09:35:10AM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Tue, May 8, 2018 at 9:25 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Tue, May 08, 2018 at 09:07:27AM +0200, Geert Uytterhoeven wrote:
> >> On Tue, May 8, 2018 at 9:00 AM, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > On Mon, May 07, 2018 at 09:51:12AM +1200, Michael Schmitz wrote:
> >> >> the BUG() was triggered by loading a Mac Nubus network card module on
> >> >> a multiplatform kernel running on an Amiga machine. Up to Finn's Nubus
> >> >> core rewrite (this February), we've seen no errors. Since then, Nubus
> >> >> drivers fail to register because the Nubus bus is only registered on
> >> >> Macs.
> >> >>
> >> >> Can't see link order involved here at all.
> >> >
> >> > The link order is totally involved here :)
> >> >
> >> > Link order determines the order in which init calls are run, so you need
> >> > to ensure that your bus code comes before any drivers that use that bus
> >> > code in link order.  That way, at init time, your bus is created first,
> >> > preventing this type of error to happen.
> >>
> >> The issue here is not due to link ordering, but due to the bus not being
> >> registered on a system that doesn't have that particular bus.
> >
> > But how can that happen if the bus code is not present in the system at
> > that point in time?  Hardware doesn't matter at all here.
> 
> The bus code is present in the system.
> The bus is just not registered by the NuBus bus driver if the hardware
> doesn't have a NuBus host.

Ah, that sounds like some "tight" coupling that other bus cores do not
have.  That's the confusion here that I have, sorry.

> >> Akin to booting a kernel on an old PC without PCI, and loading a driver
> >> module for a PCI network card. I guess that doesn't crash (because no one
> >> has a PC without PCI anymore? ;-)
> >
> > No, it should work just fine, try it!  :)
> >
> > The driver will not bind to anything, but the bus code should work
> > properly, as long as it is initialized before the driver tries to
> > register with that specific bus type.
> 
> Hence the NuBus bus code should register the bus irregardless of the
> presence of the NuBus host hardware.

Yes it should.  Shouldn't cause any problems as long as you don't have
any hardware specific code in your bus code.  Because of this, I don't
think the original patch would help at all here, as the error would
still be the same.

So add a "is bus registered" flag to your bus and all should be fine.

thanks,

greg k-h

Re: [PATCH] nubus: Unconditionally register bus type

[Michael Schmitz]

Hi Greg,

Am 08.05.2018 um 19:25 schrieb Greg Kroah-Hartman:
> On Tue, May 08, 2018 at 09:07:27AM +0200, Geert Uytterhoeven wrote:
>> Hi Greg,
>>
>> On Tue, May 8, 2018 at 9:00 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>> On Mon, May 07, 2018 at 09:51:12AM +1200, Michael Schmitz wrote:
>>>> the BUG() was triggered by loading a Mac Nubus network card module on
>>>> a multiplatform kernel running on an Amiga machine. Up to Finn's Nubus
>>>> core rewrite (this February), we've seen no errors. Since then, Nubus
>>>> drivers fail to register because the Nubus bus is only registered on
>>>> Macs.
>>>>
>>>> Can't see link order involved here at all.
>>>
>>> The link order is totally involved here :)
>>>
>>> Link order determines the order in which init calls are run, so you need
>>> to ensure that your bus code comes before any drivers that use that bus
>>> code in link order.  That way, at init time, your bus is created first,
>>> preventing this type of error to happen.
>>
>> The issue here is not due to link ordering, but due to the bus not being
>> registered on a system that doesn't have that particular bus.
> 
> But how can that happen if the bus code is not present in the system at
> that point in time?  Hardware doesn't matter at all here.
> 
>> Akin to booting a kernel on an old PC without PCI, and loading a driver
>> module for a PCI network card. I guess that doesn't crash (because no one
>> has a PC without PCI anymore? ;-)
> 
> No, it should work just fine, try it!  :)
> 
> The driver will not bind to anything, but the bus code should work
> properly, as long as it is initialized before the driver tries to
> register with that specific bus type.

Before Finn's patch, the Nubus init call used to do this:

static int __init nubus_init(void)
{
        int err;

        if (!MACH_IS_MAC)
                return 0;

        nubus_proc_init();
        err = nubus_bus_register();
        if (err)
                return err;
        nubus_scan_bus();
        return 0;
}

subsys_initcall(nubus_init);

MACH_IS_MAC returns false if run on Amiga, Atari, ... anything but Mac.
The Nubus bus driver is only registered on Mac hardware (working on the
assumption it won't be useful anywhere else so should not be needed.
That was a little naive, as we've seen now.).

The initcalls for Nubus hardware drivers attempt to register the driver
with the Nubus bus driver, regardless of what hardware we're running on,
i.e. regardless of whether or not the Nubus bus ever registered with the
core.

Finn's patch sidesteps the issue by registering the Nubus bus with the
core unconditionally (PCI probably does the same). Drivers can then
register successfully but will never see their probe code called because
the Nubus driver only scans the bus for devices if running on a Mac. So
no harm done either way.

An alternative might be to only allow devices to register if the Nubus
bus has successfully registered with the core (which it wouldn't do
unless running on Mac hardware). That would solve link order and wrong
hardware issues at the same time.
(Link order is Nubus before network drivers as far as I could see from
drivers/built-in.a)

Cheers,

	Michael

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH] nubus: Unconditionally register bus type

[Finn Thain]

On Sun, 6 May 2018, Greg Kroah-Hartman wrote:

> > > Why not just have an "bus is registered" flag in your driver 
> > > register function that refuses to let drivers register with the 
> > > driver core if it isn't set?
> > 
> > Perhaps that should happen in the core driver_register() function. 
> > BUG_ON is frowned upon, after all. Would that be acceptable?
> 
> I don't understand what you mean here, perhaps make a patch to show it?
> 

As an alternative to your suggestion (add flag to avoid the BUG_ON):

--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -148,7 +148,10 @@ int driver_register(struct device_driver *drv)
 	int ret;
 	struct device_driver *other;
 
-	BUG_ON(!drv->bus->p);
+	if (!drv->bus->p) {
+		WARN_ONCE(1, "Cannot register driver with invalid bus\n");
+		return -EPROBE_DEFER;
+	}
 
 	if ((drv->bus->probe && drv->probe) ||
 	    (drv->bus->remove && drv->remove) ||

I'm not actually proposing this change; just responding to your question.

For the bug at hand, I still prefer the patch at the beginning of this 
thread, because it seems to follow the conventional pattern.

> > I found a few drivers that set a flag the way you describe, which 
> > could then be simplified.
> > 
> > But that pattern is rare. Most buses use the postcore_initcall() 
> > pattern, and so my patch took the conventional approach.
> 
> It all depends on link order, not necessarily the postcore stuff.
> 
> > > And then fix your linking error, the bus should come first in link 
> > > order, before your drivers :)
> > > 
> > 
> > I didn't encounter any errors. How shall I reproduce this?
> 
> If you have not seen this error, then why change the code at all if it 
> is working properly?

I never saw the link error you mentioned.

Please see this thread for one example of how to hit the BUG_ON.
https://marc.info/?l=linux-m68k&m=152522162801182&w=2

Another way to trigger the BUG_ON is to set,
CONFIG_ATARI=y
CONFIG_MAC=y
CONFIG_NUBUS=y
CONFIG_MAC8390=y
and try to boot the result on aranym.

-- 

> Most busses do not need this as they have their link order set up 
> correctly, no need to mess with stuff that is not broken :)
> 
> thanks,
> 
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] nubus: Unconditionally register bus type

[Geert Uytterhoeven]

Hi Finn,

(responding immediately to patches doing non-kosher things ;-)

On Mon, May 7, 2018 at 1:57 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> On Sun, 6 May 2018, Greg Kroah-Hartman wrote:
>> > > Why not just have an "bus is registered" flag in your driver
>> > > register function that refuses to let drivers register with the
>> > > driver core if it isn't set?
>> >
>> > Perhaps that should happen in the core driver_register() function.
>> > BUG_ON is frowned upon, after all. Would that be acceptable?
>>
>> I don't understand what you mean here, perhaps make a patch to show it?
>>
>
> As an alternative to your suggestion (add flag to avoid the BUG_ON):
>
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -148,7 +148,10 @@ int driver_register(struct device_driver *drv)
>         int ret;
>         struct device_driver *other;
>
> -       BUG_ON(!drv->bus->p);
> +       if (!drv->bus->p) {
> +               WARN_ONCE(1, "Cannot register driver with invalid bus\n");
> +               return -EPROBE_DEFER;
> +       }
>
>         if ((drv->bus->probe && drv->probe) ||
>             (drv->bus->remove && drv->remove) ||
>
> I'm not actually proposing this change; just responding to your question.

The bus_type.p field may be unused by some bus drivers, hence this
would prevent their drivers from being registered.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] nubus: Unconditionally register bus type

[Greg Kroah-Hartman]

On Mon, May 07, 2018 at 02:53:13PM +0200, Geert Uytterhoeven wrote:
> Hi Finn,
> 
> (responding immediately to patches doing non-kosher things ;-)
> 
> On Mon, May 7, 2018 at 1:57 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> > On Sun, 6 May 2018, Greg Kroah-Hartman wrote:
> >> > > Why not just have an "bus is registered" flag in your driver
> >> > > register function that refuses to let drivers register with the
> >> > > driver core if it isn't set?
> >> >
> >> > Perhaps that should happen in the core driver_register() function.
> >> > BUG_ON is frowned upon, after all. Would that be acceptable?
> >>
> >> I don't understand what you mean here, perhaps make a patch to show it?
> >>
> >
> > As an alternative to your suggestion (add flag to avoid the BUG_ON):
> >
> > --- a/drivers/base/driver.c
> > +++ b/drivers/base/driver.c
> > @@ -148,7 +148,10 @@ int driver_register(struct device_driver *drv)
> >         int ret;
> >         struct device_driver *other;
> >
> > -       BUG_ON(!drv->bus->p);
> > +       if (!drv->bus->p) {
> > +               WARN_ONCE(1, "Cannot register driver with invalid bus\n");
> > +               return -EPROBE_DEFER;
> > +       }
> >
> >         if ((drv->bus->probe && drv->probe) ||
> >             (drv->bus->remove && drv->remove) ||
> >
> > I'm not actually proposing this change; just responding to your question.
> 
> The bus_type.p field may be unused by some bus drivers, hence this
> would prevent their drivers from being registered.

bus_type.p is an internal-to-the-driver-core structure, no bus driver
should ever be touching it.  This is a catch to see if someone is using
the driver core incorrectly.

thanks,

greg k-h

Re: [PATCH] nubus: Unconditionally register bus type

[Geert Uytterhoeven]

Hi Greg,

On Mon, May 7, 2018 at 4:45 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, May 07, 2018 at 02:53:13PM +0200, Geert Uytterhoeven wrote:
>> On Mon, May 7, 2018 at 1:57 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
>> > On Sun, 6 May 2018, Greg Kroah-Hartman wrote:
>> >> > > Why not just have an "bus is registered" flag in your driver
>> >> > > register function that refuses to let drivers register with the
>> >> > > driver core if it isn't set?
>> >> >
>> >> > Perhaps that should happen in the core driver_register() function.
>> >> > BUG_ON is frowned upon, after all. Would that be acceptable?
>> >>
>> >> I don't understand what you mean here, perhaps make a patch to show it?
>> >>
>> >
>> > As an alternative to your suggestion (add flag to avoid the BUG_ON):
>> >
>> > --- a/drivers/base/driver.c
>> > +++ b/drivers/base/driver.c
>> > @@ -148,7 +148,10 @@ int driver_register(struct device_driver *drv)
>> >         int ret;
>> >         struct device_driver *other;
>> >
>> > -       BUG_ON(!drv->bus->p);
>> > +       if (!drv->bus->p) {
>> > +               WARN_ONCE(1, "Cannot register driver with invalid bus\n");
>> > +               return -EPROBE_DEFER;
>> > +       }
>> >
>> >         if ((drv->bus->probe && drv->probe) ||
>> >             (drv->bus->remove && drv->remove) ||
>> >
>> > I'm not actually proposing this change; just responding to your question.
>>
>> The bus_type.p field may be unused by some bus drivers, hence this
>> would prevent their drivers from being registered.
>
> bus_type.p is an internal-to-the-driver-core structure, no bus driver
> should ever be touching it.  This is a catch to see if someone is using
> the driver core incorrectly.

Thanks, I stand corrected.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] nubus: Unconditionally register bus type

[Finn Thain]

On Mon, 7 May 2018, I wrote:

> On Sun, 6 May 2018, Greg Kroah-Hartman wrote:
> 
> > > > Why not just have an "bus is registered" flag in your driver 
> > > > register function that refuses to let drivers register with the 
> > > > driver core if it isn't set?
> > > 
> > > Perhaps that should happen in the core driver_register() function. 
> > > BUG_ON is frowned upon, after all. Would that be acceptable?
> > 
> > I don't understand what you mean here, perhaps make a patch to show it?
> > 
> 
> As an alternative to your suggestion (add flag to avoid the BUG_ON):
> 
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -148,7 +148,10 @@ int driver_register(struct device_driver *drv)
>  	int ret;
>  	struct device_driver *other;
>  
> -	BUG_ON(!drv->bus->p);
> +	if (!drv->bus->p) {
> +		WARN_ONCE(1, "Cannot register driver with invalid bus\n");
> +		return -EPROBE_DEFER;
> +	}
>  
>  	if ((drv->bus->probe && drv->probe) ||
>  	    (drv->bus->remove && drv->remove) ||
> 

That rushed example I gave above seems to be confusing the issue. Sorry 
about that. (See sioux-core.c for the code that motivated it.)

This example is the sort of flag removal that I had in mind --

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index ba912558a510..4ee22fb3db92 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -148,7 +148,8 @@ int driver_register(struct device_driver *drv)
 	int ret;
 	struct device_driver *other;
 
-	BUG_ON(!drv->bus->p);
+	if (!drv->bus->p)
+		return -ENODEV;
 
 	if ((drv->bus->probe && drv->probe) ||
 	    (drv->bus->remove && drv->remove) ||
diff --git a/drivers/visorbus/visorbus_main.c b/drivers/visorbus/visorbus_main.c
index 0b2434cc4ecd..73ff294fd449 100644
--- a/drivers/visorbus/visorbus_main.c
+++ b/drivers/visorbus/visorbus_main.c
@@ -19,8 +19,6 @@ static const guid_t visor_vbus_channel_guid = VISOR_VBUS_CHANNEL_GUID;
 #define LINESIZE 99
 #define POLLJIFFIES_NORMALCHANNEL 10
 
-/* stores whether bus_registration was successful */
-static bool initialized;
 static struct dentry *visorbus_debugfs_dir;
 
 /*
@@ -965,9 +963,6 @@ static int visordriver_probe_device(struct device *xdev)
  */
 int visorbus_register_visor_driver(struct visor_driver *drv)
 {
-	/* can't register on a nonexistent bus */
-	if (!initialized)
-		return -ENODEV;
 	if (!drv->probe)
 		return -EINVAL;
 	if (!drv->remove)
@@ -1212,7 +1207,6 @@ int visorbus_init(void)
 	err = bus_register(&visorbus_type);
 	if (err < 0)
 		return err;
-	initialized = true;
 	bus_device_info_init(&chipset_driverinfo, "chipset", "visorchipset");
 	return 0;
 }
@@ -1229,6 +1223,5 @@ void visorbus_exit(void)
 		visorbus_remove_instance(dev);
 	}
 	bus_unregister(&visorbus_type);
-	initialized = false;
 	debugfs_remove_recursive(visorbus_debugfs_dir);
 }

It's very hard to find examples of this pattern, where a flag is used to 
inhibit driver_register() calls. As a method of avoiding the BUG_ON this 
pattern is not popular.

Hence, the opportunities for flag removal that I mentioned are similarly 
rare. So this kind of change is not something I would propose.

Instead I would prefer either of the two solution I've previously sent, 
though any one of the 3 would actually resolve the bug reported by 
Michael.

In anycase, I'm happy to work on a new solution if it would be more 
acceptable. Would you please explain how changing the link order would 
avoid the modprobe crash? I still can't figure it out.

-- 

Re: [PATCH] nubus: Unconditionally register bus type

[Geert Uytterhoeven]

Hi Finn,

On Tue, May 8, 2018 at 1:44 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> On Mon, 7 May 2018, I wrote:
>> On Sun, 6 May 2018, Greg Kroah-Hartman wrote:
>> > > > Why not just have an "bus is registered" flag in your driver
>> > > > register function that refuses to let drivers register with the
>> > > > driver core if it isn't set?
>> > >
>> > > Perhaps that should happen in the core driver_register() function.
>> > > BUG_ON is frowned upon, after all. Would that be acceptable?
>> >
>> > I don't understand what you mean here, perhaps make a patch to show it?
>> >
>>
>> As an alternative to your suggestion (add flag to avoid the BUG_ON):
>>
>> --- a/drivers/base/driver.c
>> +++ b/drivers/base/driver.c
>> @@ -148,7 +148,10 @@ int driver_register(struct device_driver *drv)
>>       int ret;
>>       struct device_driver *other;
>>
>> -     BUG_ON(!drv->bus->p);
>> +     if (!drv->bus->p) {
>> +             WARN_ONCE(1, "Cannot register driver with invalid bus\n");
>> +             return -EPROBE_DEFER;
>> +     }
>>
>>       if ((drv->bus->probe && drv->probe) ||
>>           (drv->bus->remove && drv->remove) ||
>>
>
> That rushed example I gave above seems to be confusing the issue. Sorry
> about that. (See sioux-core.c for the code that motivated it.)
>
> This example is the sort of flag removal that I had in mind --
>
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index ba912558a510..4ee22fb3db92 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -148,7 +148,8 @@ int driver_register(struct device_driver *drv)
>         int ret;
>         struct device_driver *other;
>
> -       BUG_ON(!drv->bus->p);
> +       if (!drv->bus->p)
> +               return -ENODEV;

If this is meant to handle the case where the device driver is registered
before the bus is registered, while the latter can still happen later,
then you want to return -EPROBE_DEFER.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] nubus: Unconditionally register bus type

[Finn Thain]

On Tue, 8 May 2018, Geert Uytterhoeven wrote:

> > This example is the sort of flag removal that I had in mind --
> >
> > diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> > index ba912558a510..4ee22fb3db92 100644
> > --- a/drivers/base/driver.c
> > +++ b/drivers/base/driver.c
> > @@ -148,7 +148,8 @@ int driver_register(struct device_driver *drv)
> >         int ret;
> >         struct device_driver *other;
> >
> > -       BUG_ON(!drv->bus->p);
> > +       if (!drv->bus->p)
> > +               return -ENODEV;
> 
> If this is meant to handle the case where the device driver is registered
> before the bus is registered, while the latter can still happen later,
> then you want to return -EPROBE_DEFER.
> 

Returning -EAGAIN might be appropriate if driver_register() could 
reasonably expect the bus to come into existence.

However, a separation of concerns would seem to imply that the driver core 
has no way of knowing whether that might happen.

Anyway, this discussion is academic. The patch was only meant to 
illustrate a way to remove code instead of adding it, while achieving the 
same goal.

I'm not proposing this patch. I don't claim to understand all of the 
considerations that apply here. I'm certain there are situations where 
BUG_ON is appropriate. This may be one of those situations.

-- 

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 

Re: [PATCH] nubus: Unconditionally register bus type

[Greg Kroah-Hartman]

On Mon, May 07, 2018 at 09:57:22AM +1000, Finn Thain wrote:
> On Sun, 6 May 2018, Greg Kroah-Hartman wrote:
> 
> > > > Why not just have an "bus is registered" flag in your driver 
> > > > register function that refuses to let drivers register with the 
> > > > driver core if it isn't set?
> > > 
> > > Perhaps that should happen in the core driver_register() function. 
> > > BUG_ON is frowned upon, after all. Would that be acceptable?
> > 
> > I don't understand what you mean here, perhaps make a patch to show it?
> > 
> 
> As an alternative to your suggestion (add flag to avoid the BUG_ON):
> 
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -148,7 +148,10 @@ int driver_register(struct device_driver *drv)
>  	int ret;
>  	struct device_driver *other;
>  
> -	BUG_ON(!drv->bus->p);
> +	if (!drv->bus->p) {
> +		WARN_ONCE(1, "Cannot register driver with invalid bus\n");
> +		return -EPROBE_DEFER;
> +	}
>  
>  	if ((drv->bus->probe && drv->probe) ||
>  	    (drv->bus->remove && drv->remove) ||
> 
> I'm not actually proposing this change; just responding to your question.
> 
> For the bug at hand, I still prefer the patch at the beginning of this 
> thread, because it seems to follow the conventional pattern.
> 
> > > I found a few drivers that set a flag the way you describe, which 
> > > could then be simplified.
> > > 
> > > But that pattern is rare. Most buses use the postcore_initcall() 
> > > pattern, and so my patch took the conventional approach.
> > 
> > It all depends on link order, not necessarily the postcore stuff.
> > 
> > > > And then fix your linking error, the bus should come first in link 
> > > > order, before your drivers :)
> > > > 
> > > 
> > > I didn't encounter any errors. How shall I reproduce this?
> > 
> > If you have not seen this error, then why change the code at all if it 
> > is working properly?
> 
> I never saw the link error you mentioned.
> 
> Please see this thread for one example of how to hit the BUG_ON.
> https://marc.info/?l=linux-m68k&m=152522162801182&w=2
> 
> Another way to trigger the BUG_ON is to set,
> CONFIG_ATARI=y
> CONFIG_MAC=y
> CONFIG_NUBUS=y
> CONFIG_MAC8390=y
> and try to boot the result on aranym.

See my other response in this thread describing the link order problem.
But if you can't resolve this that way, then yes, your original patch
should be fine.

thanks,

greg k-h