| Message ID | E1rVxXt-002YqY-9G@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-next] net: phy: constify phydev->drv | expand |
On Fri, Feb 02, 2024 at 05:41:45PM +0000, Russell King (Oracle) wrote: > Device driver structures are shared between all devices that they > match, and thus nothing should never write to the device driver nothing should never ??? I guess the never should be ever? > diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c > index 7fd9fe6a602b..7b1bc5fcef9b 100644 > --- a/drivers/net/phy/xilinx_gmii2rgmii.c > +++ b/drivers/net/phy/xilinx_gmii2rgmii.c > @@ -22,7 +22,7 @@ > > struct gmii2rgmii { > struct phy_device *phy_dev; > - struct phy_driver *phy_drv; > + const struct phy_driver *phy_drv; > struct phy_driver conv_phy_drv; > struct mdio_device *mdio; > }; Did you build testing include xilinx_gmii2rgmii.c ? It does funky things with phy_driver structures. Thanks Andrew
On Sat, Feb 03, 2024 at 05:56:19PM +0100, Andrew Lunn wrote: > On Fri, Feb 02, 2024 at 05:41:45PM +0000, Russell King (Oracle) wrote: > > Device driver structures are shared between all devices that they > > match, and thus nothing should never write to the device driver > > nothing should never ??? > > I guess the never should be ever? > > > diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c > > index 7fd9fe6a602b..7b1bc5fcef9b 100644 > > --- a/drivers/net/phy/xilinx_gmii2rgmii.c > > +++ b/drivers/net/phy/xilinx_gmii2rgmii.c > > @@ -22,7 +22,7 @@ > > > > struct gmii2rgmii { > > struct phy_device *phy_dev; > > - struct phy_driver *phy_drv; > > + const struct phy_driver *phy_drv; > > struct phy_driver conv_phy_drv; > > struct mdio_device *mdio; > > }; > > Did you build testing include xilinx_gmii2rgmii.c ? It does funky > things with phy_driver structures. > Looking at the probe function it seems they only swap phy_drv with conv_phy_drv but it doesn't seems they touch stuff in the phy_dev struct. Looks like the thing while hackish, seems clean enough to follow the rule of not touching the OPs and causing side effects.
On Sat, Feb 03, 2024 at 05:56:19PM +0100, Andrew Lunn wrote: > On Fri, Feb 02, 2024 at 05:41:45PM +0000, Russell King (Oracle) wrote: > > Device driver structures are shared between all devices that they > > match, and thus nothing should never write to the device driver > > nothing should never ??? > > I guess the never should be ever? Yes, thanks for spotting that. > > struct gmii2rgmii { > > struct phy_device *phy_dev; > > - struct phy_driver *phy_drv; > > + const struct phy_driver *phy_drv; > > struct phy_driver conv_phy_drv; > > struct mdio_device *mdio; > > }; > > Did you build testing include xilinx_gmii2rgmii.c ? It does funky > things with phy_driver structures. CONFIG_XILINX_GMII2RGMII=m So yes.
Hello: This patch was applied to netdev/net-next.git (main) by Paolo Abeni <pabeni@redhat.com>: On Fri, 02 Feb 2024 17:41:45 +0000 you wrote: > Device driver structures are shared between all devices that they > match, and thus nothing should never write to the device driver > structure through the phydev->drv pointer. Let's make this pointer > const to catch code that attempts to do so. > > Suggested-by: Christian Marangi <ansuelsmth@gmail.com> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > [...] Here is the summary with links: - [net-next] net: phy: constify phydev->drv https://git.kernel.org/netdev/net-next/c/0bd199fd9c19 You are awesome, thank you!
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 3b9531143be1..14224e06d69f 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -1290,7 +1290,6 @@ int phy_disable_interrupts(struct phy_device *phydev) static irqreturn_t phy_interrupt(int irq, void *phy_dat) { struct phy_device *phydev = phy_dat; - struct phy_driver *drv = phydev->drv; irqreturn_t ret; /* Wakeup interrupts may occur during a system sleep transition. @@ -1316,7 +1315,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat) } mutex_lock(&phydev->lock); - ret = drv->handle_interrupt(phydev); + ret = phydev->drv->handle_interrupt(phydev); mutex_unlock(&phydev->lock); return ret; diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 52828d1c64f7..2eed8f03621d 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1413,7 +1413,7 @@ int phy_sfp_probe(struct phy_device *phydev, } EXPORT_SYMBOL(phy_sfp_probe); -static bool phy_drv_supports_irq(struct phy_driver *phydrv) +static bool phy_drv_supports_irq(const struct phy_driver *phydrv) { return phydrv->config_intr && phydrv->handle_interrupt; } @@ -1867,7 +1867,7 @@ int phy_suspend(struct phy_device *phydev) { struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL }; struct net_device *netdev = phydev->attached_dev; - struct phy_driver *phydrv = phydev->drv; + const struct phy_driver *phydrv = phydev->drv; int ret; if (phydev->suspended) @@ -1892,7 +1892,7 @@ EXPORT_SYMBOL(phy_suspend); int __phy_resume(struct phy_device *phydev) { - struct phy_driver *phydrv = phydev->drv; + const struct phy_driver *phydrv = phydev->drv; int ret; lockdep_assert_held(&phydev->lock); diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c index 7fd9fe6a602b..7b1bc5fcef9b 100644 --- a/drivers/net/phy/xilinx_gmii2rgmii.c +++ b/drivers/net/phy/xilinx_gmii2rgmii.c @@ -22,7 +22,7 @@ struct gmii2rgmii { struct phy_device *phy_dev; - struct phy_driver *phy_drv; + const struct phy_driver *phy_drv; struct phy_driver conv_phy_drv; struct mdio_device *mdio; }; diff --git a/include/linux/phy.h b/include/linux/phy.h index a66f07d3f5f4..ad93f8b1b128 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -638,7 +638,7 @@ struct phy_device { /* Information about the PHY type */ /* And management functions */ - struct phy_driver *drv; + const struct phy_driver *drv; struct device_link *devlink;
Device driver structures are shared between all devices that they match, and thus nothing should never write to the device driver structure through the phydev->drv pointer. Let's make this pointer const to catch code that attempts to do so. Suggested-by: Christian Marangi <ansuelsmth@gmail.com> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phy.c | 3 +-- drivers/net/phy/phy_device.c | 6 +++--- drivers/net/phy/xilinx_gmii2rgmii.c | 2 +- include/linux/phy.h | 2 +- 4 files changed, 6 insertions(+), 7 deletions(-)