From: Andrew Lunn <andrew@lunn.ch>
To: Michael Grzeschik <mgr@pengutronix.de>
Cc: netdev@vger.kernel.org, f.fainelli@gmail.com,
davem@davemloft.net, kernel@pengutronix.de,
matthias.schiffer@ew.tq-group.com, woojung.huh@microchip.com,
UNGLinuxDriver@microchip.com
Subject: Re: [PATCH 06/11] net: dsa: microchip: ksz8795: use phy_port_cnt where possible
Date: Thu, 19 Nov 2020 14:59:09 +0100 [thread overview]
Message-ID: <20201119135909.GU1804098@lunn.ch> (raw)
In-Reply-To: <20201119084005.GC6507@pengutronix.de>
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
next prev parent reply other threads:[~2020-11-19 13:59 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-18 22:03 [PATCH 00/11] net: dsa: microchip: make ksz8795 driver more dynamic Michael Grzeschik
2020-11-18 22:03 ` [PATCH 01/11] net: dsa: microchip: ksz8795: remove unused last_port variable Michael Grzeschik
2020-11-19 0:05 ` Andrew Lunn
2020-11-19 3:06 ` Florian Fainelli
2020-11-18 22:03 ` [PATCH 02/11] net: dsa: microchip: ksz8795: remove superfluous port_cnt assignment Michael Grzeschik
2020-11-19 0:10 ` Andrew Lunn
2020-11-19 3:07 ` Florian Fainelli
2020-11-18 22:03 ` [PATCH 03/11] net: dsa: microchip: ksz8795: move variable assignments from detect to init Michael Grzeschik
2020-11-19 0:14 ` Andrew Lunn
2020-11-19 3:07 ` Florian Fainelli
2020-11-18 22:03 ` [PATCH 04/11] net: dsa: microchip: ksz8795: use reg_mib_cnt where possible Michael Grzeschik
2020-11-19 0:16 ` Andrew Lunn
2020-11-19 3:08 ` Florian Fainelli
2020-11-18 22:03 ` [PATCH 05/11] net: dsa: microchip: ksz8795: use mib_cnt " Michael Grzeschik
2020-11-19 0:20 ` Andrew Lunn
2020-11-19 7:36 ` Michael Grzeschik
2020-11-19 3:09 ` Florian Fainelli
2020-11-19 7:01 ` kernel test robot
2020-11-18 22:03 ` [PATCH 06/11] net: dsa: microchip: ksz8795: use phy_port_cnt " Michael Grzeschik
2020-11-19 0:29 ` Andrew Lunn
2020-11-19 8:40 ` Michael Grzeschik
2020-11-19 13:59 ` Andrew Lunn [this message]
2020-11-19 3:13 ` Florian Fainelli
2020-11-18 22:03 ` [PATCH 07/11] net: dsa: microchip: remove superfluous num_ports asignment Michael Grzeschik
2020-11-19 0:32 ` Andrew Lunn
2020-11-19 3:10 ` Florian Fainelli
2020-11-18 22:03 ` [PATCH 08/11] net: dsa: microchip: ksz8795: align port_cnt usage with other microchip drivers Michael Grzeschik
2020-11-19 0:35 ` Andrew Lunn
2020-11-19 18:06 ` Tristram.Ha
2020-11-19 3:13 ` Florian Fainelli
2020-11-18 22:03 ` [PATCH 09/11] net: dsa: microchip: remove usage of mib_port_count Michael Grzeschik
2020-11-19 1:13 ` Andrew Lunn
2020-11-19 3:12 ` Florian Fainelli
2020-11-18 22:03 ` [PATCH 10/11] net: dsa: microchip: ksz8795: dynamic allocate memory for flush_dyn_mac_table Michael Grzeschik
2020-11-19 1:21 ` Andrew Lunn
2020-11-19 3:12 ` Florian Fainelli
2020-11-18 22:03 ` [PATCH 11/11] net: dsa: microchip: ksz8795: use num_vlans where possible Michael Grzeschik
2020-11-19 1:22 ` Andrew Lunn
2020-11-19 3:11 ` Florian Fainelli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201119135909.GU1804098@lunn.ch \
--to=andrew@lunn.ch \
--cc=UNGLinuxDriver@microchip.com \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=kernel@pengutronix.de \
--cc=matthias.schiffer@ew.tq-group.com \
--cc=mgr@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=woojung.huh@microchip.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).