linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] Some cleanups after making bus_type::remove return void
@ 2021-07-27  8:08 Uwe Kleine-König
  2021-07-27  8:08 ` [PATCH 1/5] nubus: Simplify check in remove callback Uwe Kleine-König
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2021-07-27  8:08 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, kernel, Finn Thain, linux-m68k, David S. Miller,
	Jakub Kicinski, Zhang Qilong, Christophe JAILLET, netdev,
	Yoshinori Sato, Rich Felker, Samuel Iglesias Gonsálvez,
	Dmitry Torokhov, Chen-Yu Tsai, Pali Rohár, linux-sh,
	Geert Uytterhoeven

Hello,

while working on the patch set that made bus_type::remove return void I
noticed a few things that could be improved. This series addresses
these. Apart from a simple conflict between the two zorro patches there
are no interdependencies between these patches. I created them on top of
Greg's bus_remove_return_void-5.15 tag[1]. There might be further
(probably simple) conflicts if they are applied based on an earlier
commit.

So it should be easily possible to let these patches go in through their
usual maintainer trees. So please if you're a maintainer state if you
prefer to take the patches yourself or if you prefer that Greg takes
them together.

Best regards
Uwe

[1] available at

	git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git tags/bus_remove_return_void-5.15

    see https://lore.kernel.org/lkml/YPkwQwf0dUKnGA7L@kroah.com

Uwe Kleine-König (5):
  nubus: Simplify check in remove callback
  nubus: Make struct nubus_driver::remove return void
  sh: superhyway: Simplify check in remove callback
  zorro: Simplify remove callback
  zorro: Drop useless (and hardly used) .driver member in struct
    zorro_dev

 drivers/net/ethernet/8390/mac8390.c     |  3 +--
 drivers/net/ethernet/natsemi/macsonic.c |  4 +---
 drivers/nubus/bus.c                     |  2 +-
 drivers/sh/superhyway/superhyway.c      |  2 +-
 drivers/zorro/zorro-driver.c            | 13 ++++---------
 include/linux/nubus.h                   |  2 +-
 include/linux/zorro.h                   |  1 -
 7 files changed, 9 insertions(+), 18 deletions(-)


base-commit: fc7a6209d5710618eb4f72a77cd81b8d694ecf89
-- 
2.30.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/5] nubus: Simplify check in remove callback
  2021-07-27  8:08 [PATCH 0/5] Some cleanups after making bus_type::remove return void Uwe Kleine-König
@ 2021-07-27  8:08 ` Uwe Kleine-König
  2021-07-27  9:50   ` Finn Thain
  2021-07-27  8:08 ` [PATCH 2/5] nubus: Make struct nubus_driver::remove return void Uwe Kleine-König
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2021-07-27  8:08 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-kernel, kernel, Finn Thain, linux-m68k

The driver core only calls a remove callback when the device was
successfully bound (aka probed) before. So dev->driver is never NULL.

Apart from that, the compiler might already assume dev->driver being
non-NULL after to_nubus_driver(dev->driver) was called.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/nubus/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nubus/bus.c b/drivers/nubus/bus.c
index d9d04f27f89b..17fad660032c 100644
--- a/drivers/nubus/bus.c
+++ b/drivers/nubus/bus.c
@@ -33,7 +33,7 @@ static void nubus_device_remove(struct device *dev)
 {
 	struct nubus_driver *ndrv = to_nubus_driver(dev->driver);
 
-	if (dev->driver && ndrv->remove)
+	if (ndrv->remove)
 		ndrv->remove(to_nubus_board(dev));
 }
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/5] nubus: Make struct nubus_driver::remove return void
  2021-07-27  8:08 [PATCH 0/5] Some cleanups after making bus_type::remove return void Uwe Kleine-König
  2021-07-27  8:08 ` [PATCH 1/5] nubus: Simplify check in remove callback Uwe Kleine-König
@ 2021-07-27  8:08 ` Uwe Kleine-König
  2021-07-27  9:47   ` Finn Thain
  2021-07-27  8:08 ` [PATCH 3/5] sh: superhyway: Simplify check in remove callback Uwe Kleine-König
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2021-07-27  8:08 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, kernel, David S. Miller, Jakub Kicinski,
	Finn Thain, Zhang Qilong, Christophe JAILLET, netdev, linux-m68k

The nubus core ignores the return value of the remove callback (in
nubus_device_remove()) and all implementers return 0 anyway.

So make it impossible for future drivers to return an unused error code
by changing the remove prototype to return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/8390/mac8390.c     | 3 +--
 drivers/net/ethernet/natsemi/macsonic.c | 4 +---
 include/linux/nubus.h                   | 2 +-
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/8390/mac8390.c b/drivers/net/ethernet/8390/mac8390.c
index 9aac7119d382..91b04abfd687 100644
--- a/drivers/net/ethernet/8390/mac8390.c
+++ b/drivers/net/ethernet/8390/mac8390.c
@@ -428,13 +428,12 @@ static int mac8390_device_probe(struct nubus_board *board)
 	return err;
 }
 
-static int mac8390_device_remove(struct nubus_board *board)
+static void mac8390_device_remove(struct nubus_board *board)
 {
 	struct net_device *dev = nubus_get_drvdata(board);
 
 	unregister_netdev(dev);
 	free_netdev(dev);
-	return 0;
 }
 
 static struct nubus_driver mac8390_driver = {
diff --git a/drivers/net/ethernet/natsemi/macsonic.c b/drivers/net/ethernet/natsemi/macsonic.c
index 2289e1fe3741..8709d700e15a 100644
--- a/drivers/net/ethernet/natsemi/macsonic.c
+++ b/drivers/net/ethernet/natsemi/macsonic.c
@@ -603,7 +603,7 @@ static int mac_sonic_nubus_probe(struct nubus_board *board)
 	return err;
 }
 
-static int mac_sonic_nubus_remove(struct nubus_board *board)
+static void mac_sonic_nubus_remove(struct nubus_board *board)
 {
 	struct net_device *ndev = nubus_get_drvdata(board);
 	struct sonic_local *lp = netdev_priv(ndev);
@@ -613,8 +613,6 @@ static int mac_sonic_nubus_remove(struct nubus_board *board)
 			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
 			  lp->descriptors, lp->descriptors_laddr);
 	free_netdev(ndev);
-
-	return 0;
 }
 
 static struct nubus_driver mac_sonic_nubus_driver = {
diff --git a/include/linux/nubus.h b/include/linux/nubus.h
index eba50b057f6f..392fc6c53e96 100644
--- a/include/linux/nubus.h
+++ b/include/linux/nubus.h
@@ -86,7 +86,7 @@ extern struct list_head nubus_func_rsrcs;
 struct nubus_driver {
 	struct device_driver driver;
 	int (*probe)(struct nubus_board *board);
-	int (*remove)(struct nubus_board *board);
+	void (*remove)(struct nubus_board *board);
 };
 
 extern struct bus_type nubus_bus_type;
-- 
2.30.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/5] sh: superhyway: Simplify check in remove callback
  2021-07-27  8:08 [PATCH 0/5] Some cleanups after making bus_type::remove return void Uwe Kleine-König
  2021-07-27  8:08 ` [PATCH 1/5] nubus: Simplify check in remove callback Uwe Kleine-König
  2021-07-27  8:08 ` [PATCH 2/5] nubus: Make struct nubus_driver::remove return void Uwe Kleine-König
@ 2021-07-27  8:08 ` Uwe Kleine-König
  2021-07-27  8:08 ` [PATCH 4/5] zorro: Simplify " Uwe Kleine-König
  2021-07-27  8:08 ` [PATCH 5/5] zorro: Drop useless (and hardly used) .driver member in struct zorro_dev Uwe Kleine-König
  4 siblings, 0 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2021-07-27  8:08 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, kernel, Yoshinori Sato, Rich Felker,
	Samuel Iglesias Gonsálvez, Dmitry Torokhov, Chen-Yu Tsai,
	Pali Rohár, linux-sh

The driver core only calls a remove callback when the device was
successfully bound (aka probed) before. So dev->driver is never NULL.

(And even if it was NULL, to_superhyway_driver(NULL) isn't ...)

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/sh/superhyway/superhyway.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/sh/superhyway/superhyway.c b/drivers/sh/superhyway/superhyway.c
index c0ab904c76ec..44324abe21da 100644
--- a/drivers/sh/superhyway/superhyway.c
+++ b/drivers/sh/superhyway/superhyway.c
@@ -155,7 +155,7 @@ static void superhyway_device_remove(struct device *dev)
 	struct superhyway_device *shyway_dev = to_superhyway_device(dev);
 	struct superhyway_driver *shyway_drv = to_superhyway_driver(dev->driver);
 
-	if (shyway_drv && shyway_drv->remove)
+	if (shyway_drv->remove)
 		shyway_drv->remove(shyway_dev);
 }
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 4/5] zorro: Simplify remove callback
  2021-07-27  8:08 [PATCH 0/5] Some cleanups after making bus_type::remove return void Uwe Kleine-König
                   ` (2 preceding siblings ...)
  2021-07-27  8:08 ` [PATCH 3/5] sh: superhyway: Simplify check in remove callback Uwe Kleine-König
@ 2021-07-27  8:08 ` Uwe Kleine-König
  2021-07-27  8:08 ` [PATCH 5/5] zorro: Drop useless (and hardly used) .driver member in struct zorro_dev Uwe Kleine-König
  4 siblings, 0 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2021-07-27  8:08 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-kernel, kernel, Geert Uytterhoeven, linux-m68k

The driver core only calls a remove callback when the device was
successfully bound (aka probed) before. So dev->driver is never NULL.

(And even if it was NULL, to_zorro_driver(NULL) isn't ...)

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/zorro/zorro-driver.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/zorro/zorro-driver.c b/drivers/zorro/zorro-driver.c
index c18524bb8b2a..ab06c9ce2c78 100644
--- a/drivers/zorro/zorro-driver.c
+++ b/drivers/zorro/zorro-driver.c
@@ -67,11 +67,9 @@ static void zorro_device_remove(struct device *dev)
 	struct zorro_dev *z = to_zorro_dev(dev);
 	struct zorro_driver *drv = to_zorro_driver(dev->driver);
 
-	if (drv) {
-		if (drv->remove)
-			drv->remove(z);
-		z->driver = NULL;
-	}
+	if (drv->remove)
+		drv->remove(z);
+	z->driver = NULL;
 }
 
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 5/5] zorro: Drop useless (and hardly used) .driver member in struct zorro_dev
  2021-07-27  8:08 [PATCH 0/5] Some cleanups after making bus_type::remove return void Uwe Kleine-König
                   ` (3 preceding siblings ...)
  2021-07-27  8:08 ` [PATCH 4/5] zorro: Simplify " Uwe Kleine-König
@ 2021-07-27  8:08 ` Uwe Kleine-König
  4 siblings, 0 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2021-07-27  8:08 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-kernel, kernel, Geert Uytterhoeven, linux-m68k

The only actual use is to check in zorro_device_probe() that the device
isn't already bound. The driver core already ensures this however so the
check can go away which allows to drop the then assigned-only member
from struct zorro_dev.

If the value was indeed needed somewhere it can always be calculated by

	to_zorro_driver(z->dev.driver)

.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/zorro/zorro-driver.c | 7 ++-----
 include/linux/zorro.h        | 1 -
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/zorro/zorro-driver.c b/drivers/zorro/zorro-driver.c
index ab06c9ce2c78..96f068830549 100644
--- a/drivers/zorro/zorro-driver.c
+++ b/drivers/zorro/zorro-driver.c
@@ -47,16 +47,14 @@ static int zorro_device_probe(struct device *dev)
 	struct zorro_driver *drv = to_zorro_driver(dev->driver);
 	struct zorro_dev *z = to_zorro_dev(dev);
 
-	if (!z->driver && drv->probe) {
+	if (drv->probe) {
 		const struct zorro_device_id *id;
 
 		id = zorro_match_device(drv->id_table, z);
 		if (id)
 			error = drv->probe(z, id);
-		if (error >= 0) {
-			z->driver = drv;
+		if (error >= 0)
 			error = 0;
-		}
 	}
 	return error;
 }
@@ -69,7 +67,6 @@ static void zorro_device_remove(struct device *dev)
 
 	if (drv->remove)
 		drv->remove(z);
-	z->driver = NULL;
 }
 
 
diff --git a/include/linux/zorro.h b/include/linux/zorro.h
index e2e4de188d84..db7416ed6057 100644
--- a/include/linux/zorro.h
+++ b/include/linux/zorro.h
@@ -29,7 +29,6 @@
 struct zorro_dev {
     struct ExpansionRom rom;
     zorro_id id;
-    struct zorro_driver *driver;	/* which driver has allocated this device */
     struct device dev;			/* Generic device interface */
     u16 slotaddr;
     u16 slotsize;
-- 
2.30.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/5] nubus: Make struct nubus_driver::remove return void
  2021-07-27  8:08 ` [PATCH 2/5] nubus: Make struct nubus_driver::remove return void Uwe Kleine-König
@ 2021-07-27  9:47   ` Finn Thain
  0 siblings, 0 replies; 13+ messages in thread
From: Finn Thain @ 2021-07-27  9:47 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Greg Kroah-Hartman, linux-kernel, kernel, David S. Miller,
	Jakub Kicinski, Zhang Qilong, Christophe JAILLET, netdev,
	linux-m68k

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

On Tue, 27 Jul 2021, Uwe Kleine-König wrote:

> The nubus core ignores the return value of the remove callback (in
> nubus_device_remove()) and all implementers return 0 anyway.
> 
> So make it impossible for future drivers to return an unused error code
> by changing the remove prototype to return void.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Acked-by: Finn Thain <fthain@linux-m68k.org>

> ---
>  drivers/net/ethernet/8390/mac8390.c     | 3 +--
>  drivers/net/ethernet/natsemi/macsonic.c | 4 +---
>  include/linux/nubus.h                   | 2 +-
>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/8390/mac8390.c b/drivers/net/ethernet/8390/mac8390.c
> index 9aac7119d382..91b04abfd687 100644
> --- a/drivers/net/ethernet/8390/mac8390.c
> +++ b/drivers/net/ethernet/8390/mac8390.c
> @@ -428,13 +428,12 @@ static int mac8390_device_probe(struct nubus_board *board)
>  	return err;
>  }
>  
> -static int mac8390_device_remove(struct nubus_board *board)
> +static void mac8390_device_remove(struct nubus_board *board)
>  {
>  	struct net_device *dev = nubus_get_drvdata(board);
>  
>  	unregister_netdev(dev);
>  	free_netdev(dev);
> -	return 0;
>  }
>  
>  static struct nubus_driver mac8390_driver = {
> diff --git a/drivers/net/ethernet/natsemi/macsonic.c b/drivers/net/ethernet/natsemi/macsonic.c
> index 2289e1fe3741..8709d700e15a 100644
> --- a/drivers/net/ethernet/natsemi/macsonic.c
> +++ b/drivers/net/ethernet/natsemi/macsonic.c
> @@ -603,7 +603,7 @@ static int mac_sonic_nubus_probe(struct nubus_board *board)
>  	return err;
>  }
>  
> -static int mac_sonic_nubus_remove(struct nubus_board *board)
> +static void mac_sonic_nubus_remove(struct nubus_board *board)
>  {
>  	struct net_device *ndev = nubus_get_drvdata(board);
>  	struct sonic_local *lp = netdev_priv(ndev);
> @@ -613,8 +613,6 @@ static int mac_sonic_nubus_remove(struct nubus_board *board)
>  			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
>  			  lp->descriptors, lp->descriptors_laddr);
>  	free_netdev(ndev);
> -
> -	return 0;
>  }
>  
>  static struct nubus_driver mac_sonic_nubus_driver = {
> diff --git a/include/linux/nubus.h b/include/linux/nubus.h
> index eba50b057f6f..392fc6c53e96 100644
> --- a/include/linux/nubus.h
> +++ b/include/linux/nubus.h
> @@ -86,7 +86,7 @@ extern struct list_head nubus_func_rsrcs;
>  struct nubus_driver {
>  	struct device_driver driver;
>  	int (*probe)(struct nubus_board *board);
> -	int (*remove)(struct nubus_board *board);
> +	void (*remove)(struct nubus_board *board);
>  };
>  
>  extern struct bus_type nubus_bus_type;
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] nubus: Simplify check in remove callback
  2021-07-27  8:08 ` [PATCH 1/5] nubus: Simplify check in remove callback Uwe Kleine-König
@ 2021-07-27  9:50   ` Finn Thain
  2021-07-27 10:07     ` Geert Uytterhoeven
  2021-07-27 11:42     ` Uwe Kleine-König
  0 siblings, 2 replies; 13+ messages in thread
From: Finn Thain @ 2021-07-27  9:50 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Greg Kroah-Hartman, linux-kernel, kernel, linux-m68k

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

On Tue, 27 Jul 2021, Uwe Kleine-König wrote:

> The driver core only calls a remove callback when the device was
> successfully bound (aka probed) before. So dev->driver is never NULL.
> 

Are you sure dev->driver is non-NULL for the lifetime of the device?
A quick glance at device_reprobe() makes me wonder about that.

> Apart from that, the compiler might already assume dev->driver being
> non-NULL after to_nubus_driver(dev->driver) was called.
> 

I don't understand how a compiler can make that assumption. But then, I 
don't know why compilers do a lot of the things they do...

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/nubus/bus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nubus/bus.c b/drivers/nubus/bus.c
> index d9d04f27f89b..17fad660032c 100644
> --- a/drivers/nubus/bus.c
> +++ b/drivers/nubus/bus.c
> @@ -33,7 +33,7 @@ static void nubus_device_remove(struct device *dev)
>  {
>  	struct nubus_driver *ndrv = to_nubus_driver(dev->driver);
>  
> -	if (dev->driver && ndrv->remove)
> +	if (ndrv->remove)
>  		ndrv->remove(to_nubus_board(dev));
>  }
>  
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] nubus: Simplify check in remove callback
  2021-07-27  9:50   ` Finn Thain
@ 2021-07-27 10:07     ` Geert Uytterhoeven
  2021-07-27 10:19       ` Finn Thain
  2021-07-27 11:42     ` Uwe Kleine-König
  1 sibling, 1 reply; 13+ messages in thread
From: Geert Uytterhoeven @ 2021-07-27 10:07 UTC (permalink / raw)
  To: Finn Thain
  Cc: Uwe Kleine-König, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Sascha Hauer, linux-m68k

Hi Finn,

On Tue, Jul 27, 2021 at 11:50 AM Finn Thain <fthain@linux-m68k.org> wrote:
> On Tue, 27 Jul 2021, Uwe Kleine-König wrote:
> > Apart from that, the compiler might already assume dev->driver being
> > non-NULL after to_nubus_driver(dev->driver) was called.
>
> I don't understand how a compiler can make that assumption. But then, I
> don't know why compilers do a lot of the things they do...

It is one of those recent optimizations people have been complaining
about.  Once you have dereferenced a pointer, compilers may remove
all further NULL-checks, assuming they can't happen, as the code
would have crashed anyway before due to the dereference.
Good luck running on bare metal with RAM at zero ;-)

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] nubus: Simplify check in remove callback
  2021-07-27 10:07     ` Geert Uytterhoeven
@ 2021-07-27 10:19       ` Finn Thain
  0 siblings, 0 replies; 13+ messages in thread
From: Finn Thain @ 2021-07-27 10:19 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Uwe Kleine-König, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Sascha Hauer, linux-m68k

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

On Tue, 27 Jul 2021, Geert Uytterhoeven wrote:

> On Tue, Jul 27, 2021 at 11:50 AM Finn Thain <fthain@linux-m68k.org> wrote:
> > On Tue, 27 Jul 2021, Uwe Kleine-König wrote:
> > > Apart from that, the compiler might already assume dev->driver being 
> > > non-NULL after to_nubus_driver(dev->driver) was called.
> >
> > I don't understand how a compiler can make that assumption. But then, 
> > I don't know why compilers do a lot of the things they do...
> 
> It is one of those recent optimizations people have been complaining 
> about.  Once you have dereferenced a pointer, compilers may remove all 
> further NULL-checks, assuming they can't happen, as the code would have 
> crashed anyway before due to the dereference.

But that's the point -- there is no dereference, just pointer arithmetic.

> Good luck running on bare metal with RAM at zero ;-)

Quite.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] nubus: Simplify check in remove callback
  2021-07-27  9:50   ` Finn Thain
  2021-07-27 10:07     ` Geert Uytterhoeven
@ 2021-07-27 11:42     ` Uwe Kleine-König
  2021-07-31 10:32       ` Finn Thain
  1 sibling, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2021-07-27 11:42 UTC (permalink / raw)
  To: Finn Thain; +Cc: Greg Kroah-Hartman, linux-m68k, linux-kernel, kernel

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

On Tue, Jul 27, 2021 at 07:50:39PM +1000, Finn Thain wrote:
> On Tue, 27 Jul 2021, Uwe Kleine-König wrote:
> 
> > The driver core only calls a remove callback when the device was
> > successfully bound (aka probed) before. So dev->driver is never NULL.
> 
> Are you sure dev->driver is non-NULL for the lifetime of the device?
> A quick glance at device_reprobe() makes me wonder about that.

Not for the lifetime of the device, but as long as a driver is bound to
the device. device_reprobe() calls device_driver_detach() after which
the driver is considered unbound and dev->driver is assigned NULL. (via
device_driver_detach -> device_release_driver_internal ->
__device_release_driver)

> > Apart from that, the compiler might already assume dev->driver being
> > non-NULL after to_nubus_driver(dev->driver) was called.
> 
> I don't understand how a compiler can make that assumption. But then, I 
> don't know why compilers do a lot of the things they do...

Ah, you're right, there is no dereference in container_of, so I might
also be wrong here. I will send a v2 without this last sentence in the
commit log.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] nubus: Simplify check in remove callback
  2021-07-27 11:42     ` Uwe Kleine-König
@ 2021-07-31 10:32       ` Finn Thain
  2021-07-31 12:05         ` Uwe Kleine-König
  0 siblings, 1 reply; 13+ messages in thread
From: Finn Thain @ 2021-07-31 10:32 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Greg Kroah-Hartman, linux-m68k, linux-kernel, kernel

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

On Tue, 27 Jul 2021, Uwe Kleine-König wrote:

> On Tue, Jul 27, 2021 at 07:50:39PM +1000, Finn Thain wrote:
> > On Tue, 27 Jul 2021, Uwe Kleine-König wrote:
> > 
> > > The driver core only calls a remove callback when the device was 
> > > successfully bound (aka probed) before. So dev->driver is never 
> > > NULL.
> > 
> > Are you sure dev->driver is non-NULL for the lifetime of the device? A 
> > quick glance at device_reprobe() makes me wonder about that.
> 
> Not for the lifetime of the device, but as long as a driver is bound to 
> the device. device_reprobe() calls device_driver_detach() after which 
> the driver is considered unbound and dev->driver is assigned NULL. (via 
> device_driver_detach -> device_release_driver_internal -> 
> __device_release_driver)
> 

Are you saying that this situation does not presently apply to nubus, 
zorro or superhyway? Or are you saying that the remove callback will never 
be called in this situation?

Perhaps the device_reprobe() API should be documented accordingly, so that 
a missing NULL check can be added if need be. Perhaps it is better to just 
leave this pattern as is (i.e. keep the NULL check).

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] nubus: Simplify check in remove callback
  2021-07-31 10:32       ` Finn Thain
@ 2021-07-31 12:05         ` Uwe Kleine-König
  0 siblings, 0 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2021-07-31 12:05 UTC (permalink / raw)
  To: Finn Thain; +Cc: Greg Kroah-Hartman, linux-m68k, linux-kernel, kernel

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

Hello Finn,

On Sat, Jul 31, 2021 at 08:32:38PM +1000, Finn Thain wrote:
> On Tue, 27 Jul 2021, Uwe Kleine-König wrote:
> > On Tue, Jul 27, 2021 at 07:50:39PM +1000, Finn Thain wrote:
> > > On Tue, 27 Jul 2021, Uwe Kleine-König wrote:
> > > 
> > > > The driver core only calls a remove callback when the device was 
> > > > successfully bound (aka probed) before. So dev->driver is never 
> > > > NULL.
> > > 
> > > Are you sure dev->driver is non-NULL for the lifetime of the device? A 
> > > quick glance at device_reprobe() makes me wonder about that.
> > 
> > Not for the lifetime of the device, but as long as a driver is bound to 
> > the device. device_reprobe() calls device_driver_detach() after which 
> > the driver is considered unbound and dev->driver is assigned NULL. (via 
> > device_driver_detach -> device_release_driver_internal -> 
> > __device_release_driver)
> 
> Are you saying that this situation does not presently apply to nubus, 
> zorro or superhyway? Or are you saying that the remove callback will never 
> be called in this situation?

I'm saying that if you call device_reprobe() the remove callback is
called before dev->driver becomes NULL. So in the remove callback it's
safe to assume that dev->driver is non-NULL.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-07-31 12:05 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27  8:08 [PATCH 0/5] Some cleanups after making bus_type::remove return void Uwe Kleine-König
2021-07-27  8:08 ` [PATCH 1/5] nubus: Simplify check in remove callback Uwe Kleine-König
2021-07-27  9:50   ` Finn Thain
2021-07-27 10:07     ` Geert Uytterhoeven
2021-07-27 10:19       ` Finn Thain
2021-07-27 11:42     ` Uwe Kleine-König
2021-07-31 10:32       ` Finn Thain
2021-07-31 12:05         ` Uwe Kleine-König
2021-07-27  8:08 ` [PATCH 2/5] nubus: Make struct nubus_driver::remove return void Uwe Kleine-König
2021-07-27  9:47   ` Finn Thain
2021-07-27  8:08 ` [PATCH 3/5] sh: superhyway: Simplify check in remove callback Uwe Kleine-König
2021-07-27  8:08 ` [PATCH 4/5] zorro: Simplify " Uwe Kleine-König
2021-07-27  8:08 ` [PATCH 5/5] zorro: Drop useless (and hardly used) .driver member in struct zorro_dev Uwe Kleine-König

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).