| Message ID | 20221216161721.23863-3-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/5] net: dsa: qca8k: fix wrong length value for mgmt eth packet | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | warning | Series does not have a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| 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: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 133 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Dec 16, 2022 at 05:17:19PM +0100, Christian Marangi wrote: > This reverts commit 2481d206fae7884cd07014fd1318e63af35e99eb. > > The Documentation is very confusing about the topic. > The cache logic for hi and lo is wrong and actually miss some regs to be > actually written. > > What the Docuemntation actually intended was that it's possible to skip Just notice that I forgot to fix a typo here! Hope it's not a problem if this can be fixed when merged. If it is I will happly send v2 if the rest of the changes are OK. > writing hi OR lo if half of the reg is not needed to be written or read. > > Revert the change in favor of a better and correct implementation. > > Reported-by: Ronald Wahl <ronald.wahl@raritan.com> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > Cc: stable@vger.kernel.org # v5.18+ > --- > drivers/net/dsa/qca/qca8k-8xxx.c | 61 +++++++------------------------- > drivers/net/dsa/qca/qca8k.h | 5 --- > 2 files changed, 12 insertions(+), 54 deletions(-) > > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c > index 46151320b2a8..fbcd5c2b13ae 100644 > --- a/drivers/net/dsa/qca/qca8k-8xxx.c > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c > @@ -36,44 +36,6 @@ qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page) > *page = regaddr & 0x3ff; > } > > -static int > -qca8k_set_lo(struct qca8k_priv *priv, int phy_id, u32 regnum, u16 lo) > -{ > - u16 *cached_lo = &priv->mdio_cache.lo; > - struct mii_bus *bus = priv->bus; > - int ret; > - > - if (lo == *cached_lo) > - return 0; > - > - ret = bus->write(bus, phy_id, regnum, lo); > - if (ret < 0) > - dev_err_ratelimited(&bus->dev, > - "failed to write qca8k 32bit lo register\n"); > - > - *cached_lo = lo; > - return 0; > -} > - > -static int > -qca8k_set_hi(struct qca8k_priv *priv, int phy_id, u32 regnum, u16 hi) > -{ > - u16 *cached_hi = &priv->mdio_cache.hi; > - struct mii_bus *bus = priv->bus; > - int ret; > - > - if (hi == *cached_hi) > - return 0; > - > - ret = bus->write(bus, phy_id, regnum, hi); > - if (ret < 0) > - dev_err_ratelimited(&bus->dev, > - "failed to write qca8k 32bit hi register\n"); > - > - *cached_hi = hi; > - return 0; > -} > - > static int > qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val) > { > @@ -97,7 +59,7 @@ qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val) > } > > static void > -qca8k_mii_write32(struct qca8k_priv *priv, int phy_id, u32 regnum, u32 val) > +qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val) > { > u16 lo, hi; > int ret; > @@ -105,9 +67,12 @@ qca8k_mii_write32(struct qca8k_priv *priv, int phy_id, u32 regnum, u32 val) > lo = val & 0xffff; > hi = (u16)(val >> 16); > > - ret = qca8k_set_lo(priv, phy_id, regnum, lo); > + ret = bus->write(bus, phy_id, regnum, lo); > if (ret >= 0) > - ret = qca8k_set_hi(priv, phy_id, regnum + 1, hi); > + ret = bus->write(bus, phy_id, regnum + 1, hi); > + if (ret < 0) > + dev_err_ratelimited(&bus->dev, > + "failed to write qca8k 32bit register\n"); > } > > static int > @@ -442,7 +407,7 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val) > if (ret < 0) > goto exit; > > - qca8k_mii_write32(priv, 0x10 | r2, r1, val); > + qca8k_mii_write32(bus, 0x10 | r2, r1, val); > > exit: > mutex_unlock(&bus->mdio_lock); > @@ -475,7 +440,7 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_ > > val &= ~mask; > val |= write_val; > - qca8k_mii_write32(priv, 0x10 | r2, r1, val); > + qca8k_mii_write32(bus, 0x10 | r2, r1, val); > > exit: > mutex_unlock(&bus->mdio_lock); > @@ -750,14 +715,14 @@ qca8k_mdio_write(struct qca8k_priv *priv, int phy, int regnum, u16 data) > if (ret) > goto exit; > > - qca8k_mii_write32(priv, 0x10 | r2, r1, val); > + qca8k_mii_write32(bus, 0x10 | r2, r1, val); > > ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL, > QCA8K_MDIO_MASTER_BUSY); > > exit: > /* even if the busy_wait timeouts try to clear the MASTER_EN */ > - qca8k_mii_write32(priv, 0x10 | r2, r1, 0); > + qca8k_mii_write32(bus, 0x10 | r2, r1, 0); > > mutex_unlock(&bus->mdio_lock); > > @@ -787,7 +752,7 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum) > if (ret) > goto exit; > > - qca8k_mii_write32(priv, 0x10 | r2, r1, val); > + qca8k_mii_write32(bus, 0x10 | r2, r1, val); > > ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL, > QCA8K_MDIO_MASTER_BUSY); > @@ -798,7 +763,7 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum) > > exit: > /* even if the busy_wait timeouts try to clear the MASTER_EN */ > - qca8k_mii_write32(priv, 0x10 | r2, r1, 0); > + qca8k_mii_write32(bus, 0x10 | r2, r1, 0); > > mutex_unlock(&bus->mdio_lock); > > @@ -1968,8 +1933,6 @@ qca8k_sw_probe(struct mdio_device *mdiodev) > } > > priv->mdio_cache.page = 0xffff; > - priv->mdio_cache.lo = 0xffff; > - priv->mdio_cache.hi = 0xffff; > > /* Check the detected switch id */ > ret = qca8k_read_switch_id(priv); > diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h > index 0b7a5cb12321..03514f7a20be 100644 > --- a/drivers/net/dsa/qca/qca8k.h > +++ b/drivers/net/dsa/qca/qca8k.h > @@ -375,11 +375,6 @@ struct qca8k_mdio_cache { > * mdio writes > */ > u16 page; > -/* lo and hi can also be cached and from Documentation we can skip one > - * extra mdio write if lo or hi is didn't change. > - */ > - u16 lo; > - u16 hi; > }; > > struct qca8k_pcs { > -- > 2.37.2 >
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c index 46151320b2a8..fbcd5c2b13ae 100644 --- a/drivers/net/dsa/qca/qca8k-8xxx.c +++ b/drivers/net/dsa/qca/qca8k-8xxx.c @@ -36,44 +36,6 @@ qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page) *page = regaddr & 0x3ff; } -static int -qca8k_set_lo(struct qca8k_priv *priv, int phy_id, u32 regnum, u16 lo) -{ - u16 *cached_lo = &priv->mdio_cache.lo; - struct mii_bus *bus = priv->bus; - int ret; - - if (lo == *cached_lo) - return 0; - - ret = bus->write(bus, phy_id, regnum, lo); - if (ret < 0) - dev_err_ratelimited(&bus->dev, - "failed to write qca8k 32bit lo register\n"); - - *cached_lo = lo; - return 0; -} - -static int -qca8k_set_hi(struct qca8k_priv *priv, int phy_id, u32 regnum, u16 hi) -{ - u16 *cached_hi = &priv->mdio_cache.hi; - struct mii_bus *bus = priv->bus; - int ret; - - if (hi == *cached_hi) - return 0; - - ret = bus->write(bus, phy_id, regnum, hi); - if (ret < 0) - dev_err_ratelimited(&bus->dev, - "failed to write qca8k 32bit hi register\n"); - - *cached_hi = hi; - return 0; -} - static int qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val) { @@ -97,7 +59,7 @@ qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val) } static void -qca8k_mii_write32(struct qca8k_priv *priv, int phy_id, u32 regnum, u32 val) +qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val) { u16 lo, hi; int ret; @@ -105,9 +67,12 @@ qca8k_mii_write32(struct qca8k_priv *priv, int phy_id, u32 regnum, u32 val) lo = val & 0xffff; hi = (u16)(val >> 16); - ret = qca8k_set_lo(priv, phy_id, regnum, lo); + ret = bus->write(bus, phy_id, regnum, lo); if (ret >= 0) - ret = qca8k_set_hi(priv, phy_id, regnum + 1, hi); + ret = bus->write(bus, phy_id, regnum + 1, hi); + if (ret < 0) + dev_err_ratelimited(&bus->dev, + "failed to write qca8k 32bit register\n"); } static int @@ -442,7 +407,7 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val) if (ret < 0) goto exit; - qca8k_mii_write32(priv, 0x10 | r2, r1, val); + qca8k_mii_write32(bus, 0x10 | r2, r1, val); exit: mutex_unlock(&bus->mdio_lock); @@ -475,7 +440,7 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_ val &= ~mask; val |= write_val; - qca8k_mii_write32(priv, 0x10 | r2, r1, val); + qca8k_mii_write32(bus, 0x10 | r2, r1, val); exit: mutex_unlock(&bus->mdio_lock); @@ -750,14 +715,14 @@ qca8k_mdio_write(struct qca8k_priv *priv, int phy, int regnum, u16 data) if (ret) goto exit; - qca8k_mii_write32(priv, 0x10 | r2, r1, val); + qca8k_mii_write32(bus, 0x10 | r2, r1, val); ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL, QCA8K_MDIO_MASTER_BUSY); exit: /* even if the busy_wait timeouts try to clear the MASTER_EN */ - qca8k_mii_write32(priv, 0x10 | r2, r1, 0); + qca8k_mii_write32(bus, 0x10 | r2, r1, 0); mutex_unlock(&bus->mdio_lock); @@ -787,7 +752,7 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum) if (ret) goto exit; - qca8k_mii_write32(priv, 0x10 | r2, r1, val); + qca8k_mii_write32(bus, 0x10 | r2, r1, val); ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL, QCA8K_MDIO_MASTER_BUSY); @@ -798,7 +763,7 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum) exit: /* even if the busy_wait timeouts try to clear the MASTER_EN */ - qca8k_mii_write32(priv, 0x10 | r2, r1, 0); + qca8k_mii_write32(bus, 0x10 | r2, r1, 0); mutex_unlock(&bus->mdio_lock); @@ -1968,8 +1933,6 @@ qca8k_sw_probe(struct mdio_device *mdiodev) } priv->mdio_cache.page = 0xffff; - priv->mdio_cache.lo = 0xffff; - priv->mdio_cache.hi = 0xffff; /* Check the detected switch id */ ret = qca8k_read_switch_id(priv); diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h index 0b7a5cb12321..03514f7a20be 100644 --- a/drivers/net/dsa/qca/qca8k.h +++ b/drivers/net/dsa/qca/qca8k.h @@ -375,11 +375,6 @@ struct qca8k_mdio_cache { * mdio writes */ u16 page; -/* lo and hi can also be cached and from Documentation we can skip one - * extra mdio write if lo or hi is didn't change. - */ - u16 lo; - u16 hi; }; struct qca8k_pcs {
This reverts commit 2481d206fae7884cd07014fd1318e63af35e99eb. The Documentation is very confusing about the topic. The cache logic for hi and lo is wrong and actually miss some regs to be actually written. What the Docuemntation actually intended was that it's possible to skip writing hi OR lo if half of the reg is not needed to be written or read. Revert the change in favor of a better and correct implementation. Reported-by: Ronald Wahl <ronald.wahl@raritan.com> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> Cc: stable@vger.kernel.org # v5.18+ --- drivers/net/dsa/qca/qca8k-8xxx.c | 61 +++++++------------------------- drivers/net/dsa/qca/qca8k.h | 5 --- 2 files changed, 12 insertions(+), 54 deletions(-)