diff mbox series

[net-next,v7,2/4] net: phy: extend PHY package API to support multiple global address

Message ID 20231214121026.4340-3-ansuelsmth@gmail.com
State New
Headers
Series net: phy: add PHY package base addr + mmd APIs |

Commit Message

Christian Marangi Dec. 14, 2023, 12:10 p.m. UTC 
  Current API for PHY package are limited to single address to configure
global settings for the PHY package.

It was found that some PHY package (for example the qca807x, a PHY
package that is shipped with a bundle of 5 PHY) requires multiple PHY
address to configure global settings. An example scenario is a PHY that
have a dedicated PHY for PSGMII/serdes calibrarion and have a specific
PHY in the package where the global PHY mode is set and affects every
other PHY in the package.

Change the API in the following way:
- Change phy_package_join() to take the base addr of the PHY package
  instead of the global PHY addr.
- Make __/phy_package_write/read() require an additional arg that
  select what global PHY address to use by passing the offset from the
  base addr passed on phy_package_join().

Each user of this API is updated to follow this new implementation
following a pattern where an enum is defined to declare the offset of the
addr.

We also drop the check if shared is defined as any user of the
phy_package_read/write is expected to use phy_package_join first. Misuse
of this will correctly trigger a kernel panic for NULL pointer
exception.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
Changes v7:
- Change addr to u8
Changes v2:
- Make kernel panic if shared is not init (bugged scenario)
- Fix some confusing comments

 drivers/net/phy/bcm54140.c       | 16 +++++++++----
 drivers/net/phy/mscc/mscc.h      |  5 ++++
 drivers/net/phy/mscc/mscc_main.c |  4 ++--
 drivers/net/phy/phy_device.c     | 35 ++++++++++++++-------------
 include/linux/phy.h              | 41 +++++++++++++++++++-------------
 5 files changed, 63 insertions(+), 38 deletions(-)

Comments

Christian Marangi Dec. 14, 2023, 4:54 p.m. UTC | #1 
On Thu, Dec 14, 2023 at 05:05:39PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 14, 2023 at 01:10:24PM +0100, Christian Marangi wrote:
> > @@ -1998,46 +1999,54 @@ int __phy_hwtstamp_set(struct phy_device *phydev,
> >  		       struct kernel_hwtstamp_config *config,
> >  		       struct netlink_ext_ack *extack);
> >  
> > -static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
> > +static inline int phy_package_read(struct phy_device *phydev,
> > +				   unsigned int addr_offset, u32 regnum)
> >  {
> >  	struct phy_package_shared *shared = phydev->shared;
> > +	u8 addr = shared->base_addr + addr_offset;
> >  
> > -	if (!shared)
> > +	if (addr >= PHY_MAX_ADDR)
> >  		return -EIO;
> 
> I did notice that you're using u8 in patch 1 as well - and while it's
> fine in patch 1 (because we validate the range of the value we will
> assign to that variable) that is not the case here.
> 
> Yes, shared->base_addr is a u8, but addr_offset is an unsigned int,
> and this is implicitly cast-down to a u8 in the calculation of addr,
> chopping off the bits above bit 7.
> 
> How about this approach:
> 
> static int phy_package_address(struct phy_device *phydev,
> 			       unsigned int addr_offset)
> {
> 	struct phy_package_shared *shared = phydev->shared;
> 	unsigned int addr = shared->addr + addr_offset;
> 
> 	/* detect wrap */
> 	if (addr < addr_offset)
> 		return -EIO;
> 
> 	/* detect invalid address */
> 	if (addr >= PHY_ADDR_MAX)
> 		return -EIO;
> 
> 	/* we know that addr will be in the range 0..31 and thus the
> 	 * implicit cast to a signed int is not a problem.
> 	 */
> 	return addr;
> }
> 
> and then these functions all become:
> 
> 	int addr = phy_package_address(phydev, addr_offset);
> 
> 	if (addr < 0)
> 		return addr;
> 
> I'll give you that this is belt and braces, but it avoids problems
> should a negative errno value be passed in as addr_offset (which will
> be cast to a very large positive integer.)

I also feel an helper is needed (since as you pointed out in the mmd
function we would have duplicated logic)

What I don't like is the wrap check.

But I wonder... Isn't it easier to have 

unsigned int addr = shared->base_addr + addr_offset;

and check if >= PHY_MAC_ADDR?

Everything is unsigned (so no negative case) and wrap is not possible as
nothing is downcasted.

After the check value is O.K. and can be trated as an int in
mdiobus_read (as we are sure it's in the limits and positive)

If this is correct, and the thing is a simple condition I think the
helper is not needed (or should we use it anyway for consistency in each
function?)

> 
> Andrew, any opinions on how far this should be taken?
>
Russell King (Oracle) Dec. 14, 2023, 5:05 p.m. UTC | #2 
On Thu, Dec 14, 2023 at 01:10:24PM +0100, Christian Marangi wrote:
> @@ -1998,46 +1999,54 @@ int __phy_hwtstamp_set(struct phy_device *phydev,
>  		       struct kernel_hwtstamp_config *config,
>  		       struct netlink_ext_ack *extack);
>  
> -static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
> +static inline int phy_package_read(struct phy_device *phydev,
> +				   unsigned int addr_offset, u32 regnum)
>  {
>  	struct phy_package_shared *shared = phydev->shared;
> +	u8 addr = shared->base_addr + addr_offset;
>  
> -	if (!shared)
> +	if (addr >= PHY_MAX_ADDR)
>  		return -EIO;

I did notice that you're using u8 in patch 1 as well - and while it's
fine in patch 1 (because we validate the range of the value we will
assign to that variable) that is not the case here.

Yes, shared->base_addr is a u8, but addr_offset is an unsigned int,
and this is implicitly cast-down to a u8 in the calculation of addr,
chopping off the bits above bit 7.

How about this approach:

static int phy_package_address(struct phy_device *phydev,
			       unsigned int addr_offset)
{
	struct phy_package_shared *shared = phydev->shared;
	unsigned int addr = shared->addr + addr_offset;

	/* detect wrap */
	if (addr < addr_offset)
		return -EIO;

	/* detect invalid address */
	if (addr >= PHY_ADDR_MAX)
		return -EIO;

	/* we know that addr will be in the range 0..31 and thus the
	 * implicit cast to a signed int is not a problem.
	 */
	return addr;
}

and then these functions all become:

	int addr = phy_package_address(phydev, addr_offset);

	if (addr < 0)
		return addr;

I'll give you that this is belt and braces, but it avoids problems
should a negative errno value be passed in as addr_offset (which will
be cast to a very large positive integer.)

Andrew, any opinions on how far this should be taken?
Christian Marangi Dec. 14, 2023, 5:29 p.m. UTC | #3 
On Thu, Dec 14, 2023 at 11:54:26PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 14, 2023 at 05:54:51PM +0100, Christian Marangi wrote:
> > What I don't like is the wrap check.
> > 
> > But I wonder... Isn't it easier to have 
> > 
> > unsigned int addr = shared->base_addr + addr_offset;
> > 
> > and check if >= PHY_MAC_ADDR?
> > 
> > Everything is unsigned (so no negative case) and wrap is not possible as
> > nothing is downcasted.
> 
> I'm afraid that I LOL'd at "wrap is not possible" ! Of course it's
> possible. Here's an example:
>

Yes I just think about it and I'm also LOLing at the "not possible"...

> 	shared->base_addr is 20
> 	addr_offset is ~0 (or -1 casted to an unsigned int)
> 	addr becomes 19
> 
> How about:
> 
> 	if (addr_offset >= PHY_MAX_ADDR)
> 		return -EIO;
> 
> 	addr = shared->base_addr + addr_offset;
> 	if (addr >= PHY_MAX_ADDR)
> 		return -EIO;
> 
> and then we could keep 'addr' as u8.

Ok just to make sure

static int phy_package_address(struct phy_device *phydev,
                               unsigned int addr_offset)
{
        struct phy_package_shared *shared = phydev->shared;
        unsigned int addr;

        if (addr_offset >= PHY_MAX_ADDR)
                return -EIO;

        addr = shared->base_addr + addr_offset;
        if (addr >= PHY_ADDR_MAX)
                return -EIO;

        /* we know that addr will be in the range 0..31 and thus the
         * implicit cast to a signed int is not a problem.
         */
        return addr;
}

And call u8 addr = phy_package_address(phydev, addr_offset);

Maybe one if can be skipped with the following fun thing?

static int phy_package_address(struct phy_device *phydev,
                               unsigned int addr_offset)
{
        struct phy_package_shared *shared = phydev->shared;
        u8 base_addr = shared->base_addr;

        if (addr_offset >= PHY_MAX_ADDR - base_addr)
                return -EIO;

        /* we know that addr will be in the range 0..31 and thus the
         * implicit cast to a signed int is not a problem.
         */
        return base_addr + addr_offset;
}

(don't hate me it's late here and my brain is half working ahahha)

> 
> Honestly, I have wondered why the mdio bus address is a signed int, but
> never decided to do anything about it.
> 

Maybe because direct usage of mdiobus_ is discouraged and phy_write will
use an addr that is already validated.
Russell King (Oracle) Dec. 14, 2023, 11:54 p.m. UTC | #4 
On Thu, Dec 14, 2023 at 05:54:51PM +0100, Christian Marangi wrote:
> What I don't like is the wrap check.
> 
> But I wonder... Isn't it easier to have 
> 
> unsigned int addr = shared->base_addr + addr_offset;
> 
> and check if >= PHY_MAC_ADDR?
> 
> Everything is unsigned (so no negative case) and wrap is not possible as
> nothing is downcasted.

I'm afraid that I LOL'd at "wrap is not possible" ! Of course it's
possible. Here's an example:

	shared->base_addr is 20
	addr_offset is ~0 (or -1 casted to an unsigned int)
	addr becomes 19

How about:

	if (addr_offset >= PHY_MAX_ADDR)
		return -EIO;

	addr = shared->base_addr + addr_offset;
	if (addr >= PHY_MAX_ADDR)
		return -EIO;

and then we could keep 'addr' as u8.

Honestly, I have wondered why the mdio bus address is a signed int, but
never decided to do anything about it.
Russell King (Oracle) Dec. 15, 2023, 9:16 a.m. UTC | #5 
On Thu, Dec 14, 2023 at 06:29:28PM +0100, Christian Marangi wrote:
> On Thu, Dec 14, 2023 at 11:54:26PM +0000, Russell King (Oracle) wrote:
> > On Thu, Dec 14, 2023 at 05:54:51PM +0100, Christian Marangi wrote:
> > > What I don't like is the wrap check.
> > > 
> > > But I wonder... Isn't it easier to have 
> > > 
> > > unsigned int addr = shared->base_addr + addr_offset;
> > > 
> > > and check if >= PHY_MAC_ADDR?
> > > 
> > > Everything is unsigned (so no negative case) and wrap is not possible as
> > > nothing is downcasted.
> > 
> > I'm afraid that I LOL'd at "wrap is not possible" ! Of course it's
> > possible. Here's an example:
> >
> 
> Yes I just think about it and I'm also LOLing at the "not possible"...
> 
> > 	shared->base_addr is 20
> > 	addr_offset is ~0 (or -1 casted to an unsigned int)
> > 	addr becomes 19
> > 
> > How about:
> > 
> > 	if (addr_offset >= PHY_MAX_ADDR)
> > 		return -EIO;
> > 
> > 	addr = shared->base_addr + addr_offset;
> > 	if (addr >= PHY_MAX_ADDR)
> > 		return -EIO;
> > 
> > and then we could keep 'addr' as u8.
> 
> Ok just to make sure
> 
> static int phy_package_address(struct phy_device *phydev,
>                                unsigned int addr_offset)
> {
>         struct phy_package_shared *shared = phydev->shared;
>         unsigned int addr;
> 
>         if (addr_offset >= PHY_MAX_ADDR)
>                 return -EIO;
> 
>         addr = shared->base_addr + addr_offset;
>         if (addr >= PHY_ADDR_MAX)
>                 return -EIO;
> 
>         /* we know that addr will be in the range 0..31 and thus the
>          * implicit cast to a signed int is not a problem.
>          */
>         return addr;
> }

Yep.

> And call u8 addr = phy_package_address(phydev, addr_offset);

This has to be int to cater for -EIO

> Maybe one if can be skipped with the following fun thing?
> 
> static int phy_package_address(struct phy_device *phydev,
>                                unsigned int addr_offset)
> {
>         struct phy_package_shared *shared = phydev->shared;
>         u8 base_addr = shared->base_addr;
> 
>         if (addr_offset >= PHY_MAX_ADDR - base_addr)
>                 return -EIO;

That also works.
> 
>         /* we know that addr will be in the range 0..31 and thus the
>          * implicit cast to a signed int is not a problem.
>          */
>         return base_addr + addr_offset;
> }
> 
> (don't hate me it's late here and my brain is half working ahahha)
> 
> > 
> > Honestly, I have wondered why the mdio bus address is a signed int, but
> > never decided to do anything about it.
> > 
> 
> Maybe because direct usage of mdiobus_ is discouraged and phy_write will
> use an addr that is already validated.

It's discourged in PHY drivers where one has the phy_device.
diff mbox series

Patch

diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index d43076592f81..2eea3d09b1e6 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -128,6 +128,10 @@ 
 #define BCM54140_DEFAULT_DOWNSHIFT 5
 #define BCM54140_MAX_DOWNSHIFT 9
 
+enum bcm54140_global_phy {
+	BCM54140_BASE_ADDR = 0,
+};
+
 struct bcm54140_priv {
 	int port;
 	int base_addr;
@@ -429,11 +433,13 @@  static int bcm54140_base_read_rdb(struct phy_device *phydev, u16 rdb)
 	int ret;
 
 	phy_lock_mdio_bus(phydev);
-	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_ADDR, rdb);
+	ret = __phy_package_write(phydev, BCM54140_BASE_ADDR,
+				  MII_BCM54XX_RDB_ADDR, rdb);
 	if (ret < 0)
 		goto out;
 
-	ret = __phy_package_read(phydev, MII_BCM54XX_RDB_DATA);
+	ret = __phy_package_read(phydev, BCM54140_BASE_ADDR,
+				 MII_BCM54XX_RDB_DATA);
 
 out:
 	phy_unlock_mdio_bus(phydev);
@@ -446,11 +452,13 @@  static int bcm54140_base_write_rdb(struct phy_device *phydev,
 	int ret;
 
 	phy_lock_mdio_bus(phydev);
-	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_ADDR, rdb);
+	ret = __phy_package_write(phydev, BCM54140_BASE_ADDR,
+				  MII_BCM54XX_RDB_ADDR, rdb);
 	if (ret < 0)
 		goto out;
 
-	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_DATA, val);
+	ret = __phy_package_write(phydev, BCM54140_BASE_ADDR,
+				  MII_BCM54XX_RDB_DATA, val);
 
 out:
 	phy_unlock_mdio_bus(phydev);
diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index 7a962050a4d4..6a3d8a754eb8 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -416,6 +416,11 @@  struct vsc8531_private {
  * gpio_lock: used for PHC operations. Common for all PHYs as the load/save GPIO
  * is shared.
  */
+
+enum vsc85xx_global_phy {
+	VSC88XX_BASE_ADDR = 0,
+};
+
 struct vsc85xx_shared_private {
 	struct mutex gpio_lock;
 };
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 4171f01d34e5..6f74ce0ab1aa 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -711,7 +711,7 @@  int phy_base_write(struct phy_device *phydev, u32 regnum, u16 val)
 		dump_stack();
 	}
 
-	return __phy_package_write(phydev, regnum, val);
+	return __phy_package_write(phydev, VSC88XX_BASE_ADDR, regnum, val);
 }
 
 /* phydev->bus->mdio_lock should be locked when using this function */
@@ -722,7 +722,7 @@  int phy_base_read(struct phy_device *phydev, u32 regnum)
 		dump_stack();
 	}
 
-	return __phy_package_read(phydev, regnum);
+	return __phy_package_read(phydev, VSC88XX_BASE_ADDR, regnum);
 }
 
 u32 vsc85xx_csr_read(struct phy_device *phydev,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 478126f6b5bc..424cbb13de13 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1648,20 +1648,22 @@  EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g);
 /**
  * phy_package_join - join a common PHY group
  * @phydev: target phy_device struct
- * @addr: cookie and PHY address for global register access
+ * @base_addr: cookie and base PHY address of PHY package for offset
+ *   calculation of global register access
  * @priv_size: if non-zero allocate this amount of bytes for private data
  *
  * This joins a PHY group and provides a shared storage for all phydevs in
  * this group. This is intended to be used for packages which contain
  * more than one PHY, for example a quad PHY transceiver.
  *
- * The addr parameter serves as a cookie which has to have the same value
- * for all members of one group and as a PHY address to access generic
- * registers of a PHY package. Usually, one of the PHY addresses of the
- * different PHYs in the package provides access to these global registers.
+ * The base_addr parameter serves as cookie which has to have the same values
+ * for all members of one group and as the base PHY address of the PHY package
+ * for offset calculation to access generic registers of a PHY package.
+ * Usually, one of the PHY addresses of the different PHYs in the package
+ * provides access to these global registers.
  * The address which is given here, will be used in the phy_package_read()
- * and phy_package_write() convenience functions. If your PHY doesn't have
- * global registers you can just pick any of the PHY addresses.
+ * and phy_package_write() convenience functions as base and added to the
+ * passed offset in those functions.
  *
  * This will set the shared pointer of the phydev to the shared storage.
  * If this is the first call for a this cookie the shared storage will be
@@ -1671,17 +1673,17 @@  EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g);
  * Returns < 1 on error, 0 on success. Esp. calling phy_package_join()
  * with the same cookie but a different priv_size is an error.
  */
-int phy_package_join(struct phy_device *phydev, int addr, size_t priv_size)
+int phy_package_join(struct phy_device *phydev, int base_addr, size_t priv_size)
 {
 	struct mii_bus *bus = phydev->mdio.bus;
 	struct phy_package_shared *shared;
 	int ret;
 
-	if (addr < 0 || addr >= PHY_MAX_ADDR)
+	if (base_addr < 0 || base_addr >= PHY_MAX_ADDR)
 		return -EINVAL;
 
 	mutex_lock(&bus->shared_lock);
-	shared = bus->shared[addr];
+	shared = bus->shared[base_addr];
 	if (!shared) {
 		ret = -ENOMEM;
 		shared = kzalloc(sizeof(*shared), GFP_KERNEL);
@@ -1693,9 +1695,9 @@  int phy_package_join(struct phy_device *phydev, int addr, size_t priv_size)
 				goto err_free;
 			shared->priv_size = priv_size;
 		}
-		shared->addr = addr;
+		shared->base_addr = base_addr;
 		refcount_set(&shared->refcnt, 1);
-		bus->shared[addr] = shared;
+		bus->shared[base_addr] = shared;
 	} else {
 		ret = -EINVAL;
 		if (priv_size && priv_size != shared->priv_size)
@@ -1733,7 +1735,7 @@  void phy_package_leave(struct phy_device *phydev)
 		return;
 
 	if (refcount_dec_and_mutex_lock(&shared->refcnt, &bus->shared_lock)) {
-		bus->shared[shared->addr] = NULL;
+		bus->shared[shared->base_addr] = NULL;
 		mutex_unlock(&bus->shared_lock);
 		kfree(shared->priv);
 		kfree(shared);
@@ -1752,7 +1754,8 @@  static void devm_phy_package_leave(struct device *dev, void *res)
  * devm_phy_package_join - resource managed phy_package_join()
  * @dev: device that is registering this PHY package
  * @phydev: target phy_device struct
- * @addr: cookie and PHY address for global register access
+ * @base_addr: cookie and base PHY address of PHY package for offset
+ *   calculation of global register access
  * @priv_size: if non-zero allocate this amount of bytes for private data
  *
  * Managed phy_package_join(). Shared storage fetched by this function,
@@ -1760,7 +1763,7 @@  static void devm_phy_package_leave(struct device *dev, void *res)
  * phy_package_join() for more information.
  */
 int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
-			  int addr, size_t priv_size)
+			  int base_addr, size_t priv_size)
 {
 	struct phy_device **ptr;
 	int ret;
@@ -1770,7 +1773,7 @@  int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
 	if (!ptr)
 		return -ENOMEM;
 
-	ret = phy_package_join(phydev, addr, priv_size);
+	ret = phy_package_join(phydev, base_addr, priv_size);
 
 	if (!ret) {
 		*ptr = phydev;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e5e29fca4d17..aaec34389f48 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -327,7 +327,8 @@  struct mdio_bus_stats {
 
 /**
  * struct phy_package_shared - Shared information in PHY packages
- * @addr: Common PHY address used to combine PHYs in one package
+ * @base_addr: Base PHY address of PHY package used to combine PHYs
+ *   in one package and for offset calculation of phy_package_read/write
  * @refcnt: Number of PHYs connected to this shared data
  * @flags: Initialization of PHY package
  * @priv_size: Size of the shared private data @priv
@@ -338,7 +339,7 @@  struct mdio_bus_stats {
  * phy_package_leave().
  */
 struct phy_package_shared {
-	u8 addr;
+	u8 base_addr;
 	refcount_t refcnt;
 	unsigned long flags;
 	size_t priv_size;
@@ -1972,10 +1973,10 @@  int phy_ethtool_get_link_ksettings(struct net_device *ndev,
 int phy_ethtool_set_link_ksettings(struct net_device *ndev,
 				   const struct ethtool_link_ksettings *cmd);
 int phy_ethtool_nway_reset(struct net_device *ndev);
-int phy_package_join(struct phy_device *phydev, int addr, size_t priv_size);
+int phy_package_join(struct phy_device *phydev, int base_addr, size_t priv_size);
 void phy_package_leave(struct phy_device *phydev);
 int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
-			  int addr, size_t priv_size);
+			  int base_addr, size_t priv_size);
 
 int __init mdio_bus_init(void);
 void mdio_bus_exit(void);
@@ -1998,46 +1999,54 @@  int __phy_hwtstamp_set(struct phy_device *phydev,
 		       struct kernel_hwtstamp_config *config,
 		       struct netlink_ext_ack *extack);
 
-static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
+static inline int phy_package_read(struct phy_device *phydev,
+				   unsigned int addr_offset, u32 regnum)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	u8 addr = shared->base_addr + addr_offset;
 
-	if (!shared)
+	if (addr >= PHY_MAX_ADDR)
 		return -EIO;
 
-	return mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
+	return mdiobus_read(phydev->mdio.bus, addr, regnum);
 }
 
-static inline int __phy_package_read(struct phy_device *phydev, u32 regnum)
+static inline int __phy_package_read(struct phy_device *phydev,
+				     unsigned int addr_offset, u32 regnum)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	u8 addr = shared->base_addr + addr_offset;
 
-	if (!shared)
+	if (addr >= PHY_MAX_ADDR)
 		return -EIO;
 
-	return __mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
+	return __mdiobus_read(phydev->mdio.bus, addr, regnum);
 }
 
 static inline int phy_package_write(struct phy_device *phydev,
-				    u32 regnum, u16 val)
+				    unsigned int addr_offset, u32 regnum,
+				    u16 val)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	u8 addr = shared->base_addr + addr_offset;
 
-	if (!shared)
+	if (addr >= PHY_MAX_ADDR)
 		return -EIO;
 
-	return mdiobus_write(phydev->mdio.bus, shared->addr, regnum, val);
+	return mdiobus_write(phydev->mdio.bus, addr, regnum, val);
 }
 
 static inline int __phy_package_write(struct phy_device *phydev,
-				      u32 regnum, u16 val)
+				      unsigned int addr_offset, u32 regnum,
+				      u16 val)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	u8 addr = shared->base_addr + addr_offset;
 
-	if (!shared)
+	if (addr >= PHY_MAX_ADDR)
 		return -EIO;
 
-	return __mdiobus_write(phydev->mdio.bus, shared->addr, regnum, val);
+	return __mdiobus_write(phydev->mdio.bus, addr, regnum, val);
 }
 
 static inline bool __phy_package_set_once(struct phy_device *phydev,