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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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));
> }
>
>
Hi Finn,
On Tue, Jul 27, 2021 at 11:50 AM Finn Thain <[email protected]> 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 -- [email protected]
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
On Tue, 27 Jul 2021, Geert Uytterhoeven wrote:
> On Tue, Jul 27, 2021 at 11:50 AM Finn Thain <[email protected]> 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.
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/ |
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).
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/ |