The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
switch. This switch traps frames to the CPU port set on the CPU_PORT bits,
regardless of the affinity of the user port which the frames are received
from.
When multiple CPU ports are being used, the trapped frames won't be
received when the DSA conduit interface, which the frames are supposed to
be trapped to, is down because it's not affine to any user port. This
requires the DSA conduit interface to be manually set up for the trapped
frames to be received.
To fix this, implement ds->ops->master_state_change() on this subdriver and
set the CPU_PORT bits to the CPU port which the DSA conduit interface its
affine to is up. Introduce the active_cpu_ports field to store the
information of the active CPU ports. Correct the macros, CPU_PORT is bits 4
through 6 of the register.
Add comments to explain frame trapping for this switch.
Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Suggested-by: Vladimir Oltean <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 32 ++++++++++++++++++++++++++++----
drivers/net/dsa/mt7530.h | 6 ++++--
2 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 8ab4718abb06..da75f9b312bc 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1006,10 +1006,6 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
mt7530_set(priv, MT7530_MFC, BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) |
UNU_FFP(BIT(port)));
- /* Set CPU port number */
- if (priv->id == ID_MT7621)
- mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
-
/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
* the MT7988 SoC. Any frames set for trapping to CPU port will be
* trapped to the CPU port the user port, which the frames are received
@@ -3063,6 +3059,33 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
return 0;
}
+static void
+mt753x_master_state_change(struct dsa_switch *ds,
+ const struct net_device *master,
+ bool operational)
+{
+ struct mt7530_priv *priv = ds->priv;
+ struct dsa_port *cpu_dp = master->dsa_ptr;
+
+ /* Set the CPU port to trap frames to for MT7530. There can be only one
+ * CPU port due to CPU_PORT having only 3 bits. Any frames received from
+ * a user port which are set for trapping to CPU port will be trapped to
+ * the numerically smallest CPU port which is affine to the DSA conduit
+ * interface that is up.
+ */
+ if (priv->id != ID_MT7621)
+ return;
+
+ if (operational)
+ priv->active_cpu_ports |= BIT(cpu_dp->index);
+ else
+ priv->active_cpu_ports &= ~BIT(cpu_dp->index);
+
+ if (priv->active_cpu_ports)
+ mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, CPU_EN |
+ CPU_PORT(__ffs(priv->active_cpu_ports)));
+}
+
static int mt7988_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
{
return 0;
@@ -3117,6 +3140,7 @@ const struct dsa_switch_ops mt7530_switch_ops = {
.phylink_mac_link_up = mt753x_phylink_mac_link_up,
.get_mac_eee = mt753x_get_mac_eee,
.set_mac_eee = mt753x_set_mac_eee,
+ .master_state_change = mt753x_master_state_change,
};
EXPORT_SYMBOL_GPL(mt7530_switch_ops);
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index e590cf43f3ae..28dbd131a535 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -41,8 +41,8 @@ enum mt753x_id {
#define UNU_FFP(x) (((x) & 0xff) << 8)
#define UNU_FFP_MASK UNU_FFP(~0)
#define CPU_EN BIT(7)
-#define CPU_PORT(x) ((x) << 4)
-#define CPU_MASK (0xf << 4)
+#define CPU_PORT_MASK GENMASK(6, 4)
+#define CPU_PORT(x) FIELD_PREP(CPU_PORT_MASK, x)
#define MIRROR_EN BIT(3)
#define MIRROR_PORT(x) ((x) & 0x7)
#define MIRROR_MASK 0x7
@@ -753,6 +753,7 @@ struct mt753x_info {
* @irq_domain: IRQ domain of the switch irq_chip
* @irq_enable: IRQ enable bits, synced to SYS_INT_EN
* @create_sgmii: Pointer to function creating SGMII PCS instance(s)
+ * @active_cpu_ports: Holding the active CPU ports
*/
struct mt7530_priv {
struct device *dev;
@@ -779,6 +780,7 @@ struct mt7530_priv {
struct irq_domain *irq_domain;
u32 irq_enable;
int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
+ unsigned long active_cpu_ports;
};
struct mt7530_hw_vlan_entry {
--
2.39.2
On Sun, Jun 11, 2023 at 11:15:42AM +0300, Arınç ÜNAL wrote:
> The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
> switch. This switch traps frames to the CPU port set on the CPU_PORT bits,
> regardless of the affinity of the user port which the frames are received
> from.
>
> When multiple CPU ports are being used, the trapped frames won't be
> received when the DSA conduit interface, which the frames are supposed to
> be trapped to, is down because it's not affine to any user port. This
> requires the DSA conduit interface to be manually set up for the trapped
> frames to be received.
>
> To fix this, implement ds->ops->master_state_change() on this subdriver and
> set the CPU_PORT bits to the CPU port which the DSA conduit interface its
> affine to is up. Introduce the active_cpu_ports field to store the
> information of the active CPU ports. Correct the macros, CPU_PORT is bits 4
> through 6 of the register.
>
> Add comments to explain frame trapping for this switch.
>
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Suggested-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
My only concern with this patch is that it depends upon functionality
that was introduced in kernel v5.18 - commit 295ab96f478d ("net: dsa:
provide switch operations for tracking the master state"). But otherwise
it is correct, does not require subsequent net-next rework, and
relatively clean, at least I think it's cleaner than checking which of
the multiple CPU ports is the active CPU port - the other will have no
user port dp->cpu_dp pointing to it. But strictly, the master_state_change()
logic is not needed when you can't change the CPU port assignment.
It might also be that your patch "net: dsa: introduce
preferred_default_local_cpu_port and use on MT7530" gets backported
to stable kernels that this patch doesn't get backported to, and then,
we have a problem, because that will cause even more breakage.
I wonder if there's a way to specify a dependency from this to that
other patch, to ensure that at least that does not happen?
On 13.06.2023 18:08, Vladimir Oltean wrote:
> On Sun, Jun 11, 2023 at 11:15:42AM +0300, Arınç ÜNAL wrote:
>> The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
>> switch. This switch traps frames to the CPU port set on the CPU_PORT bits,
>> regardless of the affinity of the user port which the frames are received
>> from.
>>
>> When multiple CPU ports are being used, the trapped frames won't be
>> received when the DSA conduit interface, which the frames are supposed to
>> be trapped to, is down because it's not affine to any user port. This
>> requires the DSA conduit interface to be manually set up for the trapped
>> frames to be received.
>>
>> To fix this, implement ds->ops->master_state_change() on this subdriver and
>> set the CPU_PORT bits to the CPU port which the DSA conduit interface its
>> affine to is up. Introduce the active_cpu_ports field to store the
>> information of the active CPU ports. Correct the macros, CPU_PORT is bits 4
>> through 6 of the register.
>>
>> Add comments to explain frame trapping for this switch.
>>
>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>> Suggested-by: Vladimir Oltean <[email protected]>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> ---
>
> My only concern with this patch is that it depends upon functionality
> that was introduced in kernel v5.18 - commit 295ab96f478d ("net: dsa:
> provide switch operations for tracking the master state"). But otherwise
> it is correct, does not require subsequent net-next rework, and
> relatively clean, at least I think it's cleaner than checking which of
> the multiple CPU ports is the active CPU port - the other will have no
> user port dp->cpu_dp pointing to it. But strictly, the master_state_change()
> logic is not needed when you can't change the CPU port assignment.
>
> It might also be that your patch "net: dsa: introduce
> preferred_default_local_cpu_port and use on MT7530" gets backported
> to stable kernels that this patch doesn't get backported to, and then,
> we have a problem, because that will cause even more breakage.
>
> I wonder if there's a way to specify a dependency from this to that
> other patch, to ensure that at least that does not happen?
Actually, having only "net: dsa: introduce
preferred_default_local_cpu_port and use on MT7530" backported is an
enough solution for the current stable kernels.
When multiple CPU ports are defined on the devicetree, the CPU_PORT bits
will be set to port 6. The active CPU port will also be port 6.
This would only become an issue with the changing the DSA conduit
support. But that's never going to happen as this patch will always be
on the kernels that support changing the DSA conduit.
Arınç
On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
> Actually, having only "net: dsa: introduce preferred_default_local_cpu_port
> and use on MT7530" backported is an enough solution for the current stable
> kernels.
>
> When multiple CPU ports are defined on the devicetree, the CPU_PORT bits
> will be set to port 6. The active CPU port will also be port 6.
>
> This would only become an issue with the changing the DSA conduit support.
> But that's never going to happen as this patch will always be on the kernels
> that support changing the DSA conduit.
Aha, ok. I thought that device trees with CPU port 5 exclusively defined
also exist in the wild. If not, and this patch fixes a theoretical only
issue, then it is net-next material.
On Tue, Jun 13, 2023 at 08:18:58PM +0300, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
> > Actually, having only "net: dsa: introduce preferred_default_local_cpu_port
> > and use on MT7530" backported is an enough solution for the current stable
> > kernels.
> >
> > When multiple CPU ports are defined on the devicetree, the CPU_PORT bits
> > will be set to port 6. The active CPU port will also be port 6.
> >
> > This would only become an issue with the changing the DSA conduit support.
> > But that's never going to happen as this patch will always be on the kernels
> > that support changing the DSA conduit.
>
> Aha, ok. I thought that device trees with CPU port 5 exclusively defined
> also exist in the wild. If not, and this patch fixes a theoretical only
> issue, then it is net-next material.
On second thought, compatibility with future device trees is the reason
for this patch set, so that should equally be a reason for keeping this
patch in a "net" series.
If I understand you correctly, port 5 should have worked since commit
c8b8a3c601f2 ("net: dsa: mt7530: permit port 5 to work without port 6 on
MT7621 SoC"), and it did, except for trapping, right?
So how about settling on that as a more modest Fixes: tag, and
explaining clearly in the commit message what's affected?
On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
> That fixes port 5 on certain variants of the MT7530 switch, as it was
> already working on the other variants, which, in conclusion, fixes port 5 on
> all MT7530 variants.
Ok. I didn't pay enough attention to the commit message.
> And no, trapping works. Having only CPU port 5 defined on the devicetree
> will cause the CPU_PORT bits to be set to port 5. There's only a problem
> when multiple CPU ports are defined.
Got it. Then this is really not a problem, and the commit message frames
it incorrectly.
> > So how about settling on that as a more modest Fixes: tag, and
> > explaining clearly in the commit message what's affected?
>
> I don't see anything to change in the patch log except addressing Russell's
> comments.
Ok. I see Russell has commented on v4, though I don't see that he particularly
pointed out that this fixes a problem which isn't yet a problem. I got lost in
all the versions. v2 and v3 are out of my inbox now :)
Hi
> Gesendet: Dienstag, 13. Juni 2023 um 19:18 Uhr
> Von: "Vladimir Oltean" <[email protected]>
> On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
> > Actually, having only "net: dsa: introduce preferred_default_local_cpu_port
> > and use on MT7530" backported is an enough solution for the current stable
> > kernels.
> >
> > When multiple CPU ports are defined on the devicetree, the CPU_PORT bits
> > will be set to port 6. The active CPU port will also be port 6.
> >
> > This would only become an issue with the changing the DSA conduit support.
> > But that's never going to happen as this patch will always be on the kernels
> > that support changing the DSA conduit.
>
> Aha, ok. I thought that device trees with CPU port 5 exclusively defined
> also exist in the wild. If not, and this patch fixes a theoretical only
> issue, then it is net-next material.
BananaPi R2Pro (Rockchip rk3568 arm64) has currently port5 only. And there only port5
is connected to the SoC (so port6 is not added there).
Most boards using port6 at the moment.
regards Frank
On 13.06.2023 20:24, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:18:58PM +0300, Vladimir Oltean wrote:
>> On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
>>> Actually, having only "net: dsa: introduce preferred_default_local_cpu_port
>>> and use on MT7530" backported is an enough solution for the current stable
>>> kernels.
>>>
>>> When multiple CPU ports are defined on the devicetree, the CPU_PORT bits
>>> will be set to port 6. The active CPU port will also be port 6.
>>>
>>> This would only become an issue with the changing the DSA conduit support.
>>> But that's never going to happen as this patch will always be on the kernels
>>> that support changing the DSA conduit.
>>
>> Aha, ok. I thought that device trees with CPU port 5 exclusively defined
>> also exist in the wild. If not, and this patch fixes a theoretical only
>> issue, then it is net-next material.
>
> On second thought, compatibility with future device trees is the reason
> for this patch set, so that should equally be a reason for keeping this
> patch in a "net" series.
>
> If I understand you correctly, port 5 should have worked since commit
> c8b8a3c601f2 ("net: dsa: mt7530: permit port 5 to work without port 6 on
> MT7621 SoC"), and it did, except for trapping, right?
That fixes port 5 on certain variants of the MT7530 switch, as it was
already working on the other variants, which, in conclusion, fixes port
5 on all MT7530 variants.
And no, trapping works. Having only CPU port 5 defined on the devicetree
will cause the CPU_PORT bits to be set to port 5. There's only a problem
when multiple CPU ports are defined.
>
> So how about settling on that as a more modest Fixes: tag, and
> explaining clearly in the commit message what's affected?
I don't see anything to change in the patch log except addressing
Russell's comments.
Arınç
On 13.06.2023 20:39, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
>> That fixes port 5 on certain variants of the MT7530 switch, as it was
>> already working on the other variants, which, in conclusion, fixes port 5 on
>> all MT7530 variants.
>
> Ok. I didn't pay enough attention to the commit message.
>
>> And no, trapping works. Having only CPU port 5 defined on the devicetree
>> will cause the CPU_PORT bits to be set to port 5. There's only a problem
>> when multiple CPU ports are defined.
>
> Got it. Then this is really not a problem, and the commit message frames
> it incorrectly.
Actually this patch fixes the issue it describes. At the state of this
patch, when multiple CPU ports are defined, port 5 is the active CPU
port, CPU_PORT bits are set to port 6.
Once "the patch that prefers port 6, I could easily find the exact name
but your mail snipping makes it hard" is applied, this issue becomes
redundant.
>
>>> So how about settling on that as a more modest Fixes: tag, and
>>> explaining clearly in the commit message what's affected?
>>
>> I don't see anything to change in the patch log except addressing Russell's
>> comments.
>
> Ok. I see Russell has commented on v4, though I don't see that he particularly
> pointed out that this fixes a problem which isn't yet a problem. I got lost in
> all the versions. v2 and v3 are out of my inbox now :)
All good, I had to quickly roll v3 as v2 had wrong author information
and I couldn't risk getting v2 applied.
Arınç
On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
> On 13.06.2023 20:39, Vladimir Oltean wrote:
> > On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
> > > That fixes port 5 on certain variants of the MT7530 switch, as it was
> > > already working on the other variants, which, in conclusion, fixes port 5 on
> > > all MT7530 variants.
> >
> > Ok. I didn't pay enough attention to the commit message.
> >
> > > And no, trapping works. Having only CPU port 5 defined on the devicetree
> > > will cause the CPU_PORT bits to be set to port 5. There's only a problem
> > > when multiple CPU ports are defined.
> >
> > Got it. Then this is really not a problem, and the commit message frames
> > it incorrectly.
>
> Actually this patch fixes the issue it describes. At the state of this
> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
> CPU_PORT bits are set to port 6.
Maybe it's just me being dumb, but I keep finding things difficult to
understand, such as the above paragraph.
It sounds like you're saying that _before_ this patch, port 5 is the
active CPU port, but the CPU_PORT *FIELD* NOT BITS are set such that
port 6 is the active CPU port. Therefore, things are broken, and this
patch fixes it.
Or are you saying that after this patch is applied, port 5 is the
active CPU port, but the CPU_PORT *FIELD* is set to port 6. If that's
true, then I've no idea what the hell is going on here because it
seems to be senseless.
I think at this point I just give up trying to understand what the
hell these patches are trying to do - in my opinion, the commit
messages are worded attrociously and incomprehensively.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Jun 13, 2023 at 08:39:08PM +0300, Vladimir Oltean wrote:
> Ok. I see Russell has commented on v4, though I don't see that he particularly
> pointed out that this fixes a problem which isn't yet a problem. I got lost in
> all the versions. v2 and v3 are out of my inbox now :)
I didn't really get to that level of detail (which is why I haven't
given any r-b's) - I was more focused on trying to understand what
each commit was doing, and trying to get easier to parse commit
messages.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, 13 Jun 2023 20:58:33 +0300 Arınç ÜNAL wrote:
> > Ok. I see Russell has commented on v4, though I don't see that he particularly
> > pointed out that this fixes a problem which isn't yet a problem. I got lost in
> > all the versions. v2 and v3 are out of my inbox now :)
>
> All good, I had to quickly roll v3 as v2 had wrong author information
> and I couldn't risk getting v2 applied.
FWIW you can reply with pw-bot: changes-requested to your own patches
and the bot should discard them from patchwork.
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#updating-patch-status
It's a new capability that nobody has used, yet, so YMMV :)
On 13.06.2023 22:03, Arınç ÜNAL wrote:
> On 13.06.2023 21:12, Jakub Kicinski wrote:
>> On Tue, 13 Jun 2023 20:58:33 +0300 Arınç ÜNAL wrote:
>>>> Ok. I see Russell has commented on v4, though I don't see that he
>>>> particularly
>>>> pointed out that this fixes a problem which isn't yet a problem. I
>>>> got lost in
>>>> all the versions. v2 and v3 are out of my inbox now :)
>>>
>>> All good, I had to quickly roll v3 as v2 had wrong author information
>>> and I couldn't risk getting v2 applied.
>>
>> FWIW you can reply with pw-bot: changes-requested to your own patches
>> and the bot should discard them from patchwork.
>>
>> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#updating-patch-status
>>
>> It's a new capability that nobody has used, yet, so YMMV :)
>
> Interesting, I've got some questions regarding this.
>
>> For example to mark a series as Changes Requested one needs to send
>> the following line anywhere in the email thread:
>>
>> pw-bot: changes-requested
>
> I suppose a reply to the cover letter or under the cover letter thread
> applies?
>
>> The use of the bot is restricted to authors of the patches (the From:
>> header on patch submission and command must match!)
>
> So, for example, if this patch series was new, Vladimir would reply to
> the patch they're the author of with 'pw-bot: changes-requested', and
> the patchworks would mark the whole patch series as changes requested?
Also, replying with 'pw-bot: changes-requested' on an already accepted
patch series as the author won't change the state from accepted to
changes requested, correct?
Arınç
On 13.06.2023 21:12, Jakub Kicinski wrote:
> On Tue, 13 Jun 2023 20:58:33 +0300 Arınç ÜNAL wrote:
>>> Ok. I see Russell has commented on v4, though I don't see that he particularly
>>> pointed out that this fixes a problem which isn't yet a problem. I got lost in
>>> all the versions. v2 and v3 are out of my inbox now :)
>>
>> All good, I had to quickly roll v3 as v2 had wrong author information
>> and I couldn't risk getting v2 applied.
>
> FWIW you can reply with pw-bot: changes-requested to your own patches
> and the bot should discard them from patchwork.
>
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#updating-patch-status
>
> It's a new capability that nobody has used, yet, so YMMV :)
Interesting, I've got some questions regarding this.
> For example to mark a series as Changes Requested one needs to send the following line anywhere in the email thread:
>
> pw-bot: changes-requested
I suppose a reply to the cover letter or under the cover letter thread
applies?
> The use of the bot is restricted to authors of the patches (the From: header on patch submission and command must match!)
So, for example, if this patch series was new, Vladimir would reply to
the patch they're the author of with 'pw-bot: changes-requested', and
the patchworks would mark the whole patch series as changes requested?
Arınç
On 13.06.2023 21:29, Russell King (Oracle) wrote:
> On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
>> On 13.06.2023 20:39, Vladimir Oltean wrote:
>>> On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
>>>> That fixes port 5 on certain variants of the MT7530 switch, as it was
>>>> already working on the other variants, which, in conclusion, fixes port 5 on
>>>> all MT7530 variants.
>>>
>>> Ok. I didn't pay enough attention to the commit message.
>>>
>>>> And no, trapping works. Having only CPU port 5 defined on the devicetree
>>>> will cause the CPU_PORT bits to be set to port 5. There's only a problem
>>>> when multiple CPU ports are defined.
>>>
>>> Got it. Then this is really not a problem, and the commit message frames
>>> it incorrectly.
>>
>> Actually this patch fixes the issue it describes. At the state of this
>> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
>> CPU_PORT bits are set to port 6.
>
> Maybe it's just me being dumb, but I keep finding things difficult to
> understand, such as the above paragraph.
>
> It sounds like you're saying that _before_ this patch, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* NOT BITS are set such that
> port 6 is the active CPU port. Therefore, things are broken, and this
> patch fixes it.
Yes, CPU_PORT field, and yes this patch fixes the issue by setting the
field to 5 when multiple CPU ports are used. Because before this patch,
the active CPU port is 5. The CPU_PORT field must match this.
>
> Or are you saying that after this patch is applied, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* is set to port 6. If that's
> true, then I've no idea what the hell is going on here because it
> seems to be senseless.
No, when the "prefer port 6" patch is applied, the active CPU port will
be port 6.
The CPU_PORT field will always be set to 6, regardless of "net: dsa:
mt7530: fix trapping frames with multiple CPU ports on MT7530".
Therefore, the "prefer port 6" patch makes "net: dsa: mt7530: fix
trapping frames with multiple CPU ports on MT7530" redundant.
"net: dsa: mt7530: fix trapping frames with multiple CPU ports on
MT7530" becomes important after the changing the DSA conduit support is
introduced.
>
> I think at this point I just give up trying to understand what the
> hell these patches are trying to do - in my opinion, the commit
> messages are worded attrociously and incomprehensively.
If that's how you feel, you can tune in to v5 where I will address the
patch logs.
Arınç
On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
> On 13.06.2023 20:39, Vladimir Oltean wrote:
> > Got it. Then this is really not a problem, and the commit message frames
> > it incorrectly.
>
> Actually this patch fixes the issue it describes. At the state of this
> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
> CPU_PORT bits are set to port 6.
>
> Once "the patch that prefers port 6, I could easily find the exact name but
> your mail snipping makes it hard" is applied, this issue becomes redundant.
Ok. Well, you don't get bonus points for fixing a problem in 2 different
ways, once is enough :)
On 13.06.2023 23:18, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
>> On 13.06.2023 20:39, Vladimir Oltean wrote:
>>> Got it. Then this is really not a problem, and the commit message frames
>>> it incorrectly.
>>
>> Actually this patch fixes the issue it describes. At the state of this
>> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
>> CPU_PORT bits are set to port 6.
>>
>> Once "the patch that prefers port 6, I could easily find the exact name but
>> your mail snipping makes it hard" is applied, this issue becomes redundant.
>
> Ok. Well, you don't get bonus points for fixing a problem in 2 different
> ways, once is enough :)
This is not the case here though.
This patch fixes an issue that can be stumbled upon in two ways. This is
for when multiple CPU ports are defined on the devicetree.
As I explained to Russell, the first is the CPU_PORT field not matching
the active CPU port.
The second is when port 5 becomes the only active CPU port. This can
only happen with the changing the DSA conduit support.
The "prefer port 6" patch only prevents the first way from happening.
The latter still can happen. But this feature doesn't exist yet. Hence
why I think we should apply this series as-is (after some patch log
changes) and backport it without this patch on kernels older than 5.18.
Arınç
On Tue, 13 Jun 2023 22:09:02 +0300 Arınç ÜNAL wrote:
> >> It's a new capability that nobody has used, yet, so YMMV :)
> >
> > Interesting, I've got some questions regarding this.
> >
> >> For example to mark a series as Changes Requested one needs to send
> >> the following line anywhere in the email thread:
> >>
> >> pw-bot: changes-requested
> >
> > I suppose a reply to the cover letter or under the cover letter thread
> > applies?
Anywhere in the thread, the bot should walk references.
> >> The use of the bot is restricted to authors of the patches (the From:
> >> header on patch submission and command must match!)
> >
> > So, for example, if this patch series was new, Vladimir would reply to
> > the patch they're the author of with 'pw-bot: changes-requested', and
> > the patchworks would mark the whole patch series as changes requested?
Yes.
> Also, replying with 'pw-bot: changes-requested' on an already accepted
> patch series as the author won't change the state from accepted to
> changes requested, correct?
Yes, authors can only changes status from active to inactive. Accepted
is an inactive state.
On 13.06.2023 23:59, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 11:35:08PM +0300, Arınç ÜNAL wrote:
>> On 13.06.2023 23:18, Vladimir Oltean wrote:
>>> On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
>>>> On 13.06.2023 20:39, Vladimir Oltean wrote:
>>>>> Got it. Then this is really not a problem, and the commit message frames
>>>>> it incorrectly.
>>>>
>>>> Actually this patch fixes the issue it describes. At the state of this
>>>> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
>>>> CPU_PORT bits are set to port 6.
>>>>
>>>> Once "the patch that prefers port 6, I could easily find the exact name but
>>>> your mail snipping makes it hard" is applied, this issue becomes redundant.
>>>
>>> Ok. Well, you don't get bonus points for fixing a problem in 2 different
>>> ways, once is enough :)
>>
>> This is not the case here though.
>>
>> This patch fixes an issue that can be stumbled upon in two ways. This is for
>> when multiple CPU ports are defined on the devicetree.
>>
>> As I explained to Russell, the first is the CPU_PORT field not matching the
>> active CPU port.
>>
>> The second is when port 5 becomes the only active CPU port. This can only
>> happen with the changing the DSA conduit support.
>>
>> The "prefer port 6" patch only prevents the first way from happening. The
>> latter still can happen. But this feature doesn't exist yet. Hence why I
>> think we should apply this series as-is (after some patch log changes) and
>> backport it without this patch on kernels older than 5.18.
>>
>> Arınç
>
> I was following you until the last phrase. Why should we apply this series
> as-is [ to net.git ], if this patch fixes a problem (the *only* problem in
> lack of .port_change_master() support, aka for stable kernels) that is
> already masked by a different patch targeted to net.git?
Because I don't see the latter patch as a fix. It treats the symptom,
not the cause.
Anyway, I'm fine with taking this patch from this series and put it on
my series for net-next instead.
Arınç
On Tue, Jun 13, 2023 at 07:29:18PM +0100, Russell King (Oracle) wrote:
> Maybe it's just me being dumb, but I keep finding things difficult to
> understand, such as the above paragraph.
>
> It sounds like you're saying that _before_ this patch, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* NOT BITS are set such that
> port 6 is the active CPU port. Therefore, things are broken, and this
> patch fixes it.
>
> Or are you saying that after this patch is applied, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* is set to port 6. If that's
> true, then I've no idea what the hell is going on here because it
> seems to be senseless.
There are 2 distinct patches at play. I'll be showing some tables below.
Their legend is that patch (1) affects only the second column and patch
(2) affects only the third column.
Patch (1): net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530
----------------------------------------------------------------------------------
What you need to know looking at the current code in net-next is that
mt753x_cpu_port_enable() always overwrites the CPU_MASK field of MT7530_MFC,
because that contains a single port. So when operating on a device tree
with multiple CPU ports defined, only the last CPU port will be recorded
in that register, and this will affect the destination port for
trapped-to-CPU frames.
However, DSA, when operating on a DT with multiple CPU ports, makes the
dp->cpu_dp pointer of all user ports equal to the first CPU port. That
affects which DSA master is automatically brought up when user ports are
brought up. Minor issue, TBH, because it is sufficient for the user to
manually bring up the DSA master corresponding to the second CPU port,
and then trapped frames would be processed just fine. Prior to ~2021/v5.12,
that facility wasn't even a thing - the user always had to bring that
interface up manually.
That means that without patch (1) we have:
CPU ports in the Trapping CPU port Default CPU port
device tree (MT7530_MFC:CPU_MASK) (all dp->cpu_dp point to it)
-------------------------------------------------------------------------
5 5 5
6 6 6
5 and 6 6 5
The semi-problem is that the DSA master of the trapping port (6) is not
automatically brought up by the dsa_slave_open() logic, because no slave
has port 6 (the trapping port) as a master.
All that this patch is doing is that it moves around the trapping CPU
port towards one of the DSA masters that is up, so that the user doesn't
really need to care. The table becomes:
CPU ports in the Trapping CPU port Default CPU port
device tree (MT7530_MFC:CPU_MASK) (all dp->cpu_dp point to it)
--------------------------------------------------------------------------
5 5 5
6 6 6
5 and 6 5 5
Patch (2) net: dsa: introduce preferred_default_local_cpu_port and use on MT7530
--------------------------------------------------------------------------------
This patch influences the choice that DSA makes when it comes to the
dp->cpu_dp assignments of user ports, when fed a device tree with
multiple CPU ports.
The preference of the mt7530 driver is: if port 6 is defined in the DT
as a CPU port, choose that. Otherwise don't care (which implicitly means:
let DSA pick the first CPU port that is defined there, be it 5 or 6).
The effect of this patch taken in isolation is (showing only "after",
because "before" is the same as the "before" of patch 1):
CPU ports in the Trapping CPU port Default CPU port
device tree (MT7530_MFC:CPU_MASK) (all dp->cpu_dp point to it)
-------------------------------------------------------------------------
5 5 5
6 6 6
5 and 6 6 6
As you can see, patch (2) resolves the same semi-problem even in
isolation because it makes the trapping port coincide with the default
CPU port in a different way.
>
> I think at this point I just give up trying to understand what the
> hell these patches are trying to do - in my opinion, the commit
> messages are worded attrociously and incomprehensively.
+1, could have been better..
On Tue, Jun 13, 2023 at 11:35:08PM +0300, Arınç ÜNAL wrote:
> On 13.06.2023 23:18, Vladimir Oltean wrote:
> > On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
> > > On 13.06.2023 20:39, Vladimir Oltean wrote:
> > > > Got it. Then this is really not a problem, and the commit message frames
> > > > it incorrectly.
> > >
> > > Actually this patch fixes the issue it describes. At the state of this
> > > patch, when multiple CPU ports are defined, port 5 is the active CPU port,
> > > CPU_PORT bits are set to port 6.
> > >
> > > Once "the patch that prefers port 6, I could easily find the exact name but
> > > your mail snipping makes it hard" is applied, this issue becomes redundant.
> >
> > Ok. Well, you don't get bonus points for fixing a problem in 2 different
> > ways, once is enough :)
>
> This is not the case here though.
>
> This patch fixes an issue that can be stumbled upon in two ways. This is for
> when multiple CPU ports are defined on the devicetree.
>
> As I explained to Russell, the first is the CPU_PORT field not matching the
> active CPU port.
>
> The second is when port 5 becomes the only active CPU port. This can only
> happen with the changing the DSA conduit support.
>
> The "prefer port 6" patch only prevents the first way from happening. The
> latter still can happen. But this feature doesn't exist yet. Hence why I
> think we should apply this series as-is (after some patch log changes) and
> backport it without this patch on kernels older than 5.18.
>
> Arınç
I was following you until the last phrase. Why should we apply this series
as-is [ to net.git ], if this patch fixes a problem (the *only* problem in
lack of .port_change_master() support, aka for stable kernels) that is
already masked by a different patch targeted to net.git?
On Wed, Jun 14, 2023 at 12:04:10AM +0300, Arınç ÜNAL wrote:
> Because I don't see the latter patch as a fix. It treats the symptom, not
> the cause.
>
> Anyway, I'm fine with taking this patch from this series and put it on my
> series for net-next instead.
Right, but what seems to have been the case during the net.git
(and linux-stable.git) triage so far is that user impact matters.
A configuration that works by coincidence and not by intention, but
otherwise works reliably, still works, at the end of the day.
If you read the weekly net.git pull requests sent to Linus Torvalds,
you'll see that maintainers try to make a summary of what had to be
changed and why. There isn't really a strong reason why this patch *has*
to be in those pull requests. That's kind of the mindset of what makes
"stable" "stable".
On 14.06.2023 00:14, Vladimir Oltean wrote:
> On Wed, Jun 14, 2023 at 12:04:10AM +0300, Arınç ÜNAL wrote:
>> Because I don't see the latter patch as a fix. It treats the symptom, not
>> the cause.
>>
>> Anyway, I'm fine with taking this patch from this series and put it on my
>> series for net-next instead.
>
> Right, but what seems to have been the case during the net.git
> (and linux-stable.git) triage so far is that user impact matters.
> A configuration that works by coincidence and not by intention, but
> otherwise works reliably, still works, at the end of the day.
>
> If you read the weekly net.git pull requests sent to Linus Torvalds,
> you'll see that maintainers try to make a summary of what had to be
> changed and why. There isn't really a strong reason why this patch *has*
> to be in those pull requests. That's kind of the mindset of what makes
> "stable" "stable".
Makes sense. I have prepared v5 that addresses everything so far, should
I send it today now that Russell has reviewed v4?
Arınç
On Wed, Jun 14, 2023 at 10:03:04AM +0300, Arınç ÜNAL wrote:
> Makes sense. I have prepared v5 that addresses everything so far, should I
> send it today now that Russell has reviewed v4?
>
> Arınç
Let's wait for Russell to ack that all discussions on v2-v4 are closed
and that there aren't any follow-up questions there.