[PATCH net-next] net: phy: constify phydev->drv

[PATCH net-next] net: phy: constify phydev->drv

[Russell King (Oracle)]

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(-)

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;
 
-- 
2.30.2

Re: [PATCH net-next] net: phy: constify phydev->drv

[Andrew Lunn]

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

Re: [PATCH net-next] net: phy: constify phydev->drv

[Christian Marangi]

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.

-- 
	Ansuel

Re: [PATCH net-next] net: phy: constify phydev->drv

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phy: constify phydev->drv

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html