Re: [PATCH RFC 08/28] net: dsa: Keep the mii bus and address in the private structure
Hi Andrew, Andrew Lunnwrites: > Rather than looking up the mii bus and address every time, do it once > and setup, and keep it in the private structure. > > Signed-off-by: Andrew Lunn > --- > drivers/net/dsa/mv88e6060.c | 23 +-- > drivers/net/dsa/mv88e6060.h | 11 +++ > drivers/net/dsa/mv88e6xxx.c | 16 ++-- > drivers/net/dsa/mv88e6xxx.h | 6 ++ > 4 files changed, 36 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c > index 34bc374882c7..d48708dfd963 100644 > --- a/drivers/net/dsa/mv88e6060.c > +++ b/drivers/net/dsa/mv88e6060.c > @@ -19,12 +19,9 @@ > > static int reg_read(struct dsa_switch *ds, int addr, int reg) > { > - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); > + struct mv88e6060_priv *priv = ds_to_priv(ds); > > - if (bus == NULL) > - return -EINVAL; > - > - return mdiobus_read_nested(bus, ds->pd->sw_addr + addr, reg); > + return mdiobus_read_nested(priv->bus, priv->sw_addr + addr, reg); > } > > #define REG_READ(addr, reg) \ > @@ -40,12 +37,9 @@ static int reg_read(struct dsa_switch *ds, int addr, int > reg) > > static int reg_write(struct dsa_switch *ds, int addr, int reg, u16 val) > { > - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); > - > - if (bus == NULL) > - return -EINVAL; > + struct mv88e6060_priv *priv = ds_to_priv(ds); > > - return mdiobus_write_nested(bus, ds->pd->sw_addr + addr, reg, val); > + return mdiobus_write_nested(priv->bus, priv->sw_addr + addr, reg, val); > } > > #define REG_WRITE(addr, reg, val)\ > @@ -176,6 +170,15 @@ static int mv88e6060_setup(struct dsa_switch *ds, struct > device *dev) > { > int i; > int ret; > + struct mv88e6060_priv *priv; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + ds->priv = priv; > + priv->bus = dsa_host_dev_to_mii_bus(ds->master_dev); This patch drops the checks for !bus, and thus mdiobus_write_nested can segfault later. Maybe restore the check here? if (!priv->bus) return -EINVAL; > + priv->sw_addr = ds->pd->sw_addr; > > ret = mv88e6060_switch_reset(ds); > if (ret < 0) > diff --git a/drivers/net/dsa/mv88e6060.h b/drivers/net/dsa/mv88e6060.h > index cc9b2ed4aff4..10249bd16292 100644 > --- a/drivers/net/dsa/mv88e6060.h > +++ b/drivers/net/dsa/mv88e6060.h > @@ -108,4 +108,15 @@ > #define GLOBAL_ATU_MAC_230x0e > #define GLOBAL_ATU_MAC_450x0f > > +struct mv88e6060_priv { > + /* MDIO bus and address on bus to use. When in single chip > + * mode, address is 0, and the switch uses multiple addresses > + * on the bus. When in multi-chip mode, the switch uses a > + * single address which contains two registers used for > + * indirect access to more registers. > + */ > + struct mii_bus *bus; > + int sw_addr; > +}; > + > #endif > diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c > index 772adc7f9397..170b98f3acbe 100644 > --- a/drivers/net/dsa/mv88e6xxx.c > +++ b/drivers/net/dsa/mv88e6xxx.c > @@ -94,15 +94,12 @@ static int __mv88e6xxx_reg_read(struct mii_bus *bus, int > sw_addr, int addr, > > static int _mv88e6xxx_reg_read(struct dsa_switch *ds, int addr, int reg) > { > - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); > + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds); > int ret; > > assert_smi_lock(ds); > > - if (bus == NULL) > - return -EINVAL; > - > - ret = __mv88e6xxx_reg_read(bus, ds->pd->sw_addr, addr, reg); > + ret = __mv88e6xxx_reg_read(ps->bus, ps->sw_addr, addr, reg); > if (ret < 0) > return ret; > > @@ -159,17 +156,14 @@ static int __mv88e6xxx_reg_write(struct mii_bus *bus, > int sw_addr, int addr, > static int _mv88e6xxx_reg_write(struct dsa_switch *ds, int addr, int reg, > u16 val) > { > - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); > + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds); > > assert_smi_lock(ds); > > - if (bus == NULL) > - return -EINVAL; > - > dev_dbg(ds->master_dev, "-> addr: 0x%.2x reg: 0x%.2x val: 0x%.4x\n", > addr, reg, val); > > - return __mv88e6xxx_reg_write(bus, ds->pd->sw_addr, addr, reg, val); > + return __mv88e6xxx_reg_write(ps->bus, ps->sw_addr, addr, reg, val); > } > > int mv88e6xxx_reg_write(struct dsa_switch *ds, int addr, int reg, u16 val) > @@ -2197,6 +2191,8 @@ int mv88e6xxx_setup_common(struct dsa_switch *ds, > struct device *dev) > > ds->priv = ps; > ps->ds = ds; > + ps->bus = dsa_host_dev_to_mii_bus(ds->master_dev); Same comment here. > +
Re: [PATCH RFC 08/28] net: dsa: Keep the mii bus and address in the private structure
> > @@ -176,6 +170,15 @@ static int mv88e6060_setup(struct dsa_switch *ds, > > struct device *dev) > > { > > int i; > > int ret; > > + struct mv88e6060_priv *priv; > > + > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + ds->priv = priv; > > + priv->bus = dsa_host_dev_to_mii_bus(ds->master_dev); > > This patch drops the checks for !bus, and thus mdiobus_write_nested can > segfault later. Maybe restore the check here? Agreed. I will post a refresh of these patches in a few days. I made a few changes to integrate with the mdio device patchset which has recently been merged. Andrew
Re: [PATCH RFC 08/28] net: dsa: Keep the mii bus and address in the private structure
Le 23/12/2015 04:56, Andrew Lunn a écrit : > Rather than looking up the mii bus and address every time, do it once > and setup, and keep it in the private structure. > > Signed-off-by: Andrew LunnAcked-by: Florian Fainelli -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC 08/28] net: dsa: Keep the mii bus and address in the private structure
Rather than looking up the mii bus and address every time, do it once and setup, and keep it in the private structure. Signed-off-by: Andrew Lunn--- drivers/net/dsa/mv88e6060.c | 23 +-- drivers/net/dsa/mv88e6060.h | 11 +++ drivers/net/dsa/mv88e6xxx.c | 16 ++-- drivers/net/dsa/mv88e6xxx.h | 6 ++ 4 files changed, 36 insertions(+), 20 deletions(-) diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c index 34bc374882c7..d48708dfd963 100644 --- a/drivers/net/dsa/mv88e6060.c +++ b/drivers/net/dsa/mv88e6060.c @@ -19,12 +19,9 @@ static int reg_read(struct dsa_switch *ds, int addr, int reg) { - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); + struct mv88e6060_priv *priv = ds_to_priv(ds); - if (bus == NULL) - return -EINVAL; - - return mdiobus_read_nested(bus, ds->pd->sw_addr + addr, reg); + return mdiobus_read_nested(priv->bus, priv->sw_addr + addr, reg); } #define REG_READ(addr, reg)\ @@ -40,12 +37,9 @@ static int reg_read(struct dsa_switch *ds, int addr, int reg) static int reg_write(struct dsa_switch *ds, int addr, int reg, u16 val) { - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); - - if (bus == NULL) - return -EINVAL; + struct mv88e6060_priv *priv = ds_to_priv(ds); - return mdiobus_write_nested(bus, ds->pd->sw_addr + addr, reg, val); + return mdiobus_write_nested(priv->bus, priv->sw_addr + addr, reg, val); } #define REG_WRITE(addr, reg, val) \ @@ -176,6 +170,15 @@ static int mv88e6060_setup(struct dsa_switch *ds, struct device *dev) { int i; int ret; + struct mv88e6060_priv *priv; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + ds->priv = priv; + priv->bus = dsa_host_dev_to_mii_bus(ds->master_dev); + priv->sw_addr = ds->pd->sw_addr; ret = mv88e6060_switch_reset(ds); if (ret < 0) diff --git a/drivers/net/dsa/mv88e6060.h b/drivers/net/dsa/mv88e6060.h index cc9b2ed4aff4..10249bd16292 100644 --- a/drivers/net/dsa/mv88e6060.h +++ b/drivers/net/dsa/mv88e6060.h @@ -108,4 +108,15 @@ #define GLOBAL_ATU_MAC_23 0x0e #define GLOBAL_ATU_MAC_45 0x0f +struct mv88e6060_priv { + /* MDIO bus and address on bus to use. When in single chip +* mode, address is 0, and the switch uses multiple addresses +* on the bus. When in multi-chip mode, the switch uses a +* single address which contains two registers used for +* indirect access to more registers. +*/ + struct mii_bus *bus; + int sw_addr; +}; + #endif diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c index 772adc7f9397..170b98f3acbe 100644 --- a/drivers/net/dsa/mv88e6xxx.c +++ b/drivers/net/dsa/mv88e6xxx.c @@ -94,15 +94,12 @@ static int __mv88e6xxx_reg_read(struct mii_bus *bus, int sw_addr, int addr, static int _mv88e6xxx_reg_read(struct dsa_switch *ds, int addr, int reg) { - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds); int ret; assert_smi_lock(ds); - if (bus == NULL) - return -EINVAL; - - ret = __mv88e6xxx_reg_read(bus, ds->pd->sw_addr, addr, reg); + ret = __mv88e6xxx_reg_read(ps->bus, ps->sw_addr, addr, reg); if (ret < 0) return ret; @@ -159,17 +156,14 @@ static int __mv88e6xxx_reg_write(struct mii_bus *bus, int sw_addr, int addr, static int _mv88e6xxx_reg_write(struct dsa_switch *ds, int addr, int reg, u16 val) { - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds); assert_smi_lock(ds); - if (bus == NULL) - return -EINVAL; - dev_dbg(ds->master_dev, "-> addr: 0x%.2x reg: 0x%.2x val: 0x%.4x\n", addr, reg, val); - return __mv88e6xxx_reg_write(bus, ds->pd->sw_addr, addr, reg, val); + return __mv88e6xxx_reg_write(ps->bus, ps->sw_addr, addr, reg, val); } int mv88e6xxx_reg_write(struct dsa_switch *ds, int addr, int reg, u16 val) @@ -2197,6 +2191,8 @@ int mv88e6xxx_setup_common(struct dsa_switch *ds, struct device *dev) ds->priv = ps; ps->ds = ds; + ps->bus = dsa_host_dev_to_mii_bus(ds->master_dev); + ps->sw_addr = ds->pd->sw_addr; mutex_init(>smi_mutex); diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h index 72f7dbbce0e2..62069b30f6e5 100644 --- a/drivers/net/dsa/mv88e6xxx.h +++ b/drivers/net/dsa/mv88e6xxx.h @@ -388,6 +388,12 @@ struct mv88e6xxx_priv_state { */ struct mutexsmi_mutex; + /* The MII bus and the address on the bus that is