[net-next,04/14] net: phy: at803x: move qca83xx stats out of generic at803x_priv struct
Commit Message
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(-)
Comments
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
@@ -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,