| Message ID | 20201118220357.22292-7-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: make ksz8795 driver more dynamic | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 7 this patch: 7 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 81 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 7 this patch: 7 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
> case BR_STATE_DISABLED: > data |= PORT_LEARN_DISABLE; > - if (port < SWITCH_PORT_NUM) > + if (port < dev->phy_port_cnt) > member = 0; > break; So this, unlike all the other patches so far, is not obviously correct. What exactly does phy_port_cnt mean? Can there be ports without PHYs? What if the PHYs are external? You still need to be able to change the STP state of such ports. Andrew
On 11/18/2020 2:03 PM, Michael Grzeschik wrote: > Since the driver can be used on more switches it needs > to use flexible port count where ever possible. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On Thu, Nov 19, 2020 at 01:29:15AM +0100, Andrew Lunn wrote: >> case BR_STATE_DISABLED: >> data |= PORT_LEARN_DISABLE; >> - if (port < SWITCH_PORT_NUM) >> + if (port < dev->phy_port_cnt) >> member = 0; >> break; > >So this, unlike all the other patches so far, is not obviously >correct. What exactly does phy_port_cnt mean? Can there be ports >without PHYs? What if the PHYs are external? You still need to be able >to change the STP state of such ports. The variable phy_port_cnt is referring to all external physical available ports, that are not connected internally to the cpu. That means that the previous code path was already broken, when stp handling should include the cpu_port. Michael
On Thu, Nov 19, 2020 at 09:40:05AM +0100, Michael Grzeschik wrote: > On Thu, Nov 19, 2020 at 01:29:15AM +0100, Andrew Lunn wrote: > > > case BR_STATE_DISABLED: > > > data |= PORT_LEARN_DISABLE; > > > - if (port < SWITCH_PORT_NUM) > > > + if (port < dev->phy_port_cnt) > > > member = 0; > > > break; > > > > So this, unlike all the other patches so far, is not obviously > > correct. What exactly does phy_port_cnt mean? Can there be ports > > without PHYs? What if the PHYs are external? You still need to be able > > to change the STP state of such ports. > > The variable phy_port_cnt is referring to all external physical > available ports, that are not connected internally to the cpu. > > That means that the previous code path was already broken, when stp > handling should include the cpu_port. So using DSA names, it is the number of user ports. And the assumption is, the last port is the CPU port. Please add to the commit message that this patch fixes the code as well. That sort of comment helps the reviewer understand why it is not just an obvious mechanical change. Andrew
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index 6ddba2de2d3026e..7114902495a0ebb 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -418,8 +418,8 @@ static void ksz8795_r_vlan_entries(struct ksz_device *dev, u16 addr) int i; ksz8795_r_table(dev, TABLE_VLAN, addr, &data); - addr *= 4; - for (i = 0; i < 4; i++) { + addr *= dev->phy_port_cnt; + for (i = 0; i < dev->phy_port_cnt; i++) { dev->vlan_cache[addr + i].table[0] = (u16)data; data >>= VLAN_TABLE_S; } @@ -433,7 +433,7 @@ static void ksz8795_r_vlan_table(struct ksz_device *dev, u16 vid, u16 *vlan) u64 buf; data = (u16 *)&buf; - addr = vid / 4; + addr = vid / dev->phy_port_cnt; index = vid & 3; ksz8795_r_table(dev, TABLE_VLAN, addr, &buf); *vlan = data[index]; @@ -447,7 +447,7 @@ static void ksz8795_w_vlan_table(struct ksz_device *dev, u16 vid, u16 vlan) u64 buf; data = (u16 *)&buf; - addr = vid / 4; + addr = vid / dev->phy_port_cnt; index = vid & 3; ksz8795_r_table(dev, TABLE_VLAN, addr, &buf); data[index] = vlan; @@ -691,12 +691,12 @@ static void ksz8795_port_stp_state_set(struct dsa_switch *ds, int port, switch (state) { case BR_STATE_DISABLED: data |= PORT_LEARN_DISABLE; - if (port < SWITCH_PORT_NUM) + if (port < dev->phy_port_cnt) member = 0; break; case BR_STATE_LISTENING: data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE); - if (port < SWITCH_PORT_NUM && + if (port < dev->phy_port_cnt && p->stp_state == BR_STATE_DISABLED) member = dev->host_mask | p->vid_member; break; @@ -720,7 +720,7 @@ static void ksz8795_port_stp_state_set(struct dsa_switch *ds, int port, break; case BR_STATE_BLOCKING: data |= PORT_LEARN_DISABLE; - if (port < SWITCH_PORT_NUM && + if (port < dev->phy_port_cnt && p->stp_state == BR_STATE_DISABLED) member = dev->host_mask | p->vid_member; break; @@ -1005,7 +1005,7 @@ static void ksz8795_config_cpu_port(struct dsa_switch *ds) ksz8795_port_setup(dev, dev->cpu_port, true); dev->member = dev->host_mask; - for (i = 0; i < SWITCH_PORT_NUM; i++) { + for (i = 0; i < dev->phy_port_cnt; i++) { p = &dev->ports[i]; /* Initialize to non-zero so that ksz_cfg_port_member() will @@ -1016,7 +1016,7 @@ static void ksz8795_config_cpu_port(struct dsa_switch *ds) ksz8795_port_stp_state_set(ds, i, BR_STATE_DISABLED); /* Last port may be disabled. */ - if (i == dev->port_cnt) + if (i == dev->phy_port_cnt) break; p->on = 1; p->phy = 1; @@ -1239,7 +1239,7 @@ static int ksz8795_switch_init(struct ksz_device *dev) dev->mib_cnt = ARRAY_SIZE(mib_names); dev->mib_port_cnt = TOTAL_PORT_NUM; - dev->phy_port_cnt = SWITCH_PORT_NUM; + dev->phy_port_cnt = dev->port_cnt; dev->cpu_port = dev->mib_port_cnt - 1; dev->host_mask = BIT(dev->cpu_port); diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h index c131224850135bd..6377165a236fdf3 100644 --- a/drivers/net/dsa/microchip/ksz8795_reg.h +++ b/drivers/net/dsa/microchip/ksz8795_reg.h @@ -848,9 +848,6 @@ #define TOTAL_PORT_NUM 5 -/* Host port can only be last of them. */ -#define SWITCH_PORT_NUM (TOTAL_PORT_NUM - 1) - #define KSZ8795_COUNTER_NUM 0x20 /* Common names used by other drivers */
Since the driver can be used on more switches it needs to use flexible port count where ever possible. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v1: - based on "[PATCH v4 04/11] net: dsa: microchip: ksz8795: use port_cnt where possible" - lore: https://lore.kernel.org/netdev/20200803054442.20089-5-m.grzeschik@pengutronix.de/ --- drivers/net/dsa/microchip/ksz8795.c | 20 ++++++++++---------- drivers/net/dsa/microchip/ksz8795_reg.h | 3 --- 2 files changed, 10 insertions(+), 13 deletions(-)