| Message ID | 20231117162323.626979-5-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce PHY listing and link_topology tracking | expand |
> +const char *sfp_get_name(struct sfp_bus *bus) > +{ > + if (bus->sfp_dev) > + return dev_name(bus->sfp_dev); > + > + return NULL; > +} Locking? Do you assume rtnl? Does this function need to take rtnl? Andrew
On Tue, Nov 21, 2023 at 02:00:58AM +0100, Andrew Lunn wrote: > > +const char *sfp_get_name(struct sfp_bus *bus) > > +{ > > + if (bus->sfp_dev) > > + return dev_name(bus->sfp_dev); > > + > > + return NULL; > > +} > > Locking? Do you assume rtnl? Does this function need to take rtnl? Yes, rtnl needs to be held to safely access bus->sfp_dev, and that either needs to happen in this function, or be documented as being requried (and ASSERT_RTNL() added here.) The reason is that sfp_dev is the SFP socket device which can be unbound via sfp_unregister_socket(), which will set bus->sfp_dev to NULL. This could race with the above.
Hi Andrew, Russell, On Tue, 21 Nov 2023 10:20:43 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Tue, Nov 21, 2023 at 02:00:58AM +0100, Andrew Lunn wrote: > > > +const char *sfp_get_name(struct sfp_bus *bus) > > > +{ > > > + if (bus->sfp_dev) > > > + return dev_name(bus->sfp_dev); > > > + > > > + return NULL; > > > +} > > > > Locking? Do you assume rtnl? Does this function need to take rtnl? > > Yes, rtnl needs to be held to safely access bus->sfp_dev, and that > either needs to happen in this function, or be documented as being > requried (and ASSERT_RTNL() added here.) > > The reason is that sfp_dev is the SFP socket device which can be > unbound via sfp_unregister_socket(), which will set bus->sfp_dev to > NULL. This could race with the above. > That's right, I'll add an assert and document it, thanks for spotting this. Maxime
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c index f42e9a082935..835fb2271b12 100644 --- a/drivers/net/phy/sfp-bus.c +++ b/drivers/net/phy/sfp-bus.c @@ -859,3 +859,12 @@ void sfp_unregister_socket(struct sfp_bus *bus) sfp_bus_put(bus); } EXPORT_SYMBOL_GPL(sfp_unregister_socket); + +const char *sfp_get_name(struct sfp_bus *bus) +{ + if (bus->sfp_dev) + return dev_name(bus->sfp_dev); + + return NULL; +} +EXPORT_SYMBOL_GPL(sfp_get_name); diff --git a/include/linux/sfp.h b/include/linux/sfp.h index 0573e53b0c11..96f7644908bd 100644 --- a/include/linux/sfp.h +++ b/include/linux/sfp.h @@ -570,6 +570,7 @@ struct sfp_bus *sfp_bus_find_fwnode(const struct fwnode_handle *fwnode); int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream, const struct sfp_upstream_ops *ops); void sfp_bus_del_upstream(struct sfp_bus *bus); +const char *sfp_get_name(struct sfp_bus *bus); #else static inline int sfp_parse_port(struct sfp_bus *bus, const struct sfp_eeprom_id *id, @@ -648,6 +649,11 @@ static inline int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream, static inline void sfp_bus_del_upstream(struct sfp_bus *bus) { } + +static const char *sfp_get_name(struct sfp_bus *bus) +{ + return NULL; +} #endif #endif
Knowing the bus name is helpful when we want to expose the link topology to userspace, add a helper to return the SFP bus name. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- drivers/net/phy/sfp-bus.c | 9 +++++++++ include/linux/sfp.h | 6 ++++++ 2 files changed, 15 insertions(+)