| Message ID | 20231129021219.20914-5-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: at803x: cleanup + split | expand |
On Wed, Nov 29, 2023 at 03:12:09AM +0100, Christian Marangi wrote: > +struct qca83xx_priv { > + u64 stats[ARRAY_SIZE(qca83xx_hw_stats)]; > +}; If QCA83xx is going to use an entirely separate private data structure, then it's clearly a separate driver, and it should be separated from this driver. Having two incompatible private data structures in phydev->priv in the same driver is a recipe for future errors, where functions that expect one private data structure may be called when the other private data structure is stored in phydev->priv. So, if we're going to do this, then the QCA83xx support needs to _first_ be split from this driver.
On Wed, Nov 29, 2023 at 09:29:24AM +0000, Russell King (Oracle) wrote: > On Wed, Nov 29, 2023 at 03:12:09AM +0100, Christian Marangi wrote: > > +struct qca83xx_priv { > > + u64 stats[ARRAY_SIZE(qca83xx_hw_stats)]; > > +}; > > If QCA83xx is going to use an entirely separate private data structure, > then it's clearly a separate driver, and it should be separated from > this driver. Having two incompatible private data structures in > phydev->priv in the same driver is a recipe for future errors, where > functions that expect one private data structure may be called when > the other private data structure is stored in phydev->priv. > > So, if we're going to do this, then the QCA83xx support needs to > _first_ be split from this driver. > As you notice later, it's really to make the split easier by first separating all the functions and then moving the function in the separate files. Idea was to only move them, ok to make the probe and this change when the PHY driver is detached but I feel it would make even more changes in that patch. (Instead of simply removing things from 803x.c)
On Wed, Nov 29, 2023 at 03:12:09AM +0100, Christian Marangi wrote: > Introduce a specific priv struct for qca83xx PHYs to store hw stats > data and a specific probe to allocate this alternative priv struct. > > This also have the benefits of reducing memory allocated for every other > at803x PHY since only qca83xx currently supports storing hw stats. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > drivers/net/phy/at803x.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index 4ff41d70fc47..3b7baa4bb637 100644 > --- a/drivers/net/phy/at803x.c > +++ b/drivers/net/phy/at803x.c > @@ -301,6 +301,10 @@ static struct at803x_hw_stat qca83xx_hw_stats[] = { > { "eee_wake_errors", 0x16, GENMASK(15, 0), MMD}, > }; > > +struct qca83xx_priv { > + u64 stats[ARRAY_SIZE(qca83xx_hw_stats)]; > +}; > + > struct at803x_priv { > int flags; > u16 clk_25m_reg; > @@ -311,7 +315,6 @@ struct at803x_priv { > bool is_1000basex; > struct regulator_dev *vddio_rdev; > struct regulator_dev *vddh_rdev; > - u64 stats[ARRAY_SIZE(qca83xx_hw_stats)]; > }; I agree with Russell here, this is the wrong way to go. Maybe keep at803x_priv for all the common private members which are shared by all variants. Add a qca83xx_priv which includes this: struct qca83xx_priv { struct at803x_priv at803_priv; u64 stats[ARRAY_SIZE(qca83xx_hw_stats)]; }; Andrew
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index 4ff41d70fc47..3b7baa4bb637 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -301,6 +301,10 @@ static struct at803x_hw_stat qca83xx_hw_stats[] = { { "eee_wake_errors", 0x16, GENMASK(15, 0), MMD}, }; +struct qca83xx_priv { + u64 stats[ARRAY_SIZE(qca83xx_hw_stats)]; +}; + struct at803x_priv { int flags; u16 clk_25m_reg; @@ -311,7 +315,6 @@ struct at803x_priv { bool is_1000basex; struct regulator_dev *vddio_rdev; struct regulator_dev *vddh_rdev; - u64 stats[ARRAY_SIZE(qca83xx_hw_stats)]; }; struct at803x_context { @@ -547,7 +550,7 @@ static void qca83xx_get_strings(struct phy_device *phydev, u8 *data) static u64 qca83xx_get_stat(struct phy_device *phydev, int i) { struct at803x_hw_stat stat = qca83xx_hw_stats[i]; - struct at803x_priv *priv = phydev->priv; + struct qca83xx_priv *priv = phydev->priv; int val; u64 ret; @@ -1591,6 +1594,20 @@ static int at803x_cable_test_start(struct phy_device *phydev) return 0; } +static int qca83xx_probe(struct phy_device *phydev) +{ + struct device *dev = &phydev->mdio.dev; + struct qca83xx_priv *priv; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + phydev->priv = priv; + + return 0; +} + static int qca83xx_config_init(struct phy_device *phydev) { u8 switch_revision; @@ -2164,7 +2181,7 @@ static struct phy_driver at803x_driver[] = { .name = "Qualcomm Atheros 8337 internal PHY", /* PHY_GBIT_FEATURES */ .link_change_notify = qca83xx_link_change_notify, - .probe = at803x_probe, + .probe = qca83xx_probe, .flags = PHY_IS_INTERNAL, .config_init = qca83xx_config_init, .soft_reset = genphy_soft_reset, @@ -2180,7 +2197,7 @@ static struct phy_driver at803x_driver[] = { .name = "Qualcomm Atheros 8327-A internal PHY", /* PHY_GBIT_FEATURES */ .link_change_notify = qca83xx_link_change_notify, - .probe = at803x_probe, + .probe = qca83xx_probe, .flags = PHY_IS_INTERNAL, .config_init = qca83xx_config_init, .soft_reset = genphy_soft_reset, @@ -2196,7 +2213,7 @@ static struct phy_driver at803x_driver[] = { .name = "Qualcomm Atheros 8327-B internal PHY", /* PHY_GBIT_FEATURES */ .link_change_notify = qca83xx_link_change_notify, - .probe = at803x_probe, + .probe = qca83xx_probe, .flags = PHY_IS_INTERNAL, .config_init = qca83xx_config_init, .soft_reset = genphy_soft_reset,
Introduce a specific priv struct for qca83xx PHYs to store hw stats data and a specific probe to allocate this alternative priv struct. This also have the benefits of reducing memory allocated for every other at803x PHY since only qca83xx currently supports storing hw stats. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/net/phy/at803x.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)