diff mbox series

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

Message ID E1rVxXt-002YqY-9G@rmk-PC.armlinux.org.uk (mailing list archive)
State Accepted
Commit 0bd199fd9c19aa545f677fd0a99f2be101cb6309
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: phy: constify phydev->drv | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1497 this patch: 1497
netdev/build_tools success Errors and warnings before: 1 this patch: 0
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1079 this patch: 1079
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1523 this patch: 1523
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 55 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-04--21-00 (tests: 721)

Commit Message

Russell King (Oracle) Feb. 2, 2024, 5:41 p.m. UTC
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(-)

Comments

Andrew Lunn Feb. 3, 2024, 4:56 p.m. UTC | #1
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
Christian Marangi Feb. 3, 2024, 5:04 p.m. UTC | #2
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.
Russell King (Oracle) Feb. 3, 2024, 5:23 p.m. UTC | #3
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.
patchwork-bot+netdevbpf@kernel.org Feb. 6, 2024, 12:10 p.m. UTC | #4
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 mbox series

Patch

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;