[RFC PATCH net-next 0/5] net: dsa: add support for RTL8365MB-VC

[RFC PATCH net-next 0/5] net: dsa: add support for RTL8365MB-VC

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

This series adds support for Realtek's RTL8365MB-VC, a 4+1 port
10/100/1000M Ethernet switch. The driver - rtl8365mb - was developed by
Michael Ramussen and myself.

Summary of patches:

  - The first patch in the series is a bugfix in realtek-smi which I
    found when writing the new rtl8365mb subdriver and building
    realtek-smi as a module. If desired, I can spin it off into a
    separate patch and target it for net (not net-next).

  - The second patch updates the dt-bindings for the new compatible
    string.

  - The third patch adds the 8 byte tag protocol driver.

  - The fourth patch adds the rtl8365mb subdriver - the main feature of
    this patch series.

  - The fifth patch adds a PHY driver for the internal PHYs found in the
    RTL8365MB-VC. This is not strictly necessary for the rtl8365mb
    driver to work, but it avoids using the poll-only Generic PHY driver
    directly by hooking into the IRQs made available by the switch
    driver.

This is my first time in the DSA subsystem, so I am submitting this as
an RFC patch series for now. Apologies if I have made some terrible
mistakes along the way. All feedback - no matter how minor - is thus
very welcome.

There is a lot more work that can be done on this driver, particularly
when it comes to offloading certain DSA features to the hardware. I
intend to revisit this later when I have more time. In the mean time,
the driver seems to be in a good enough state for upstream submission.

Finally, there is an outstanding issue in probing the PHY driver when
fw_devlink=on. This seems to be a generic problem with DSA drivers which
create a dependency between an internal interrupt-controller and child
PHY nodes, realtek-smi being one of them. See [1] for an ongoing
discussion about that. Since this seems to be an existing problem and
not unique to this new driver, I hope that it will not impede the
upstreaming of this patch series.

[1] https://lore.kernel.org/netdev/cd0d9c40-d07b-e2ab-b068-d0bcb4685d09@bang-olufsen.dk/

Alvin Šipraga (5):
  net: dsa: realtek-smi: fix mdio_free bug on module unload
  dt-bindings: net: dsa: realtek-smi: document new compatible rtl8365mb
  net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag
  net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC
  net: phy: realtek: add support for RTL8365MB-VC internal PHYs

 .../bindings/net/dsa/realtek-smi.txt          |    1 +
 drivers/net/dsa/Kconfig                       |    1 +
 drivers/net/dsa/Makefile                      |    2 +-
 drivers/net/dsa/realtek-smi-core.c            |   10 +
 drivers/net/dsa/realtek-smi-core.h            |    2 +
 drivers/net/dsa/rtl8365mb.c                   | 2124 +++++++++++++++++
 drivers/net/dsa/rtl8366rb.c                   |    8 +
 drivers/net/phy/realtek.c                     |    8 +
 include/net/dsa.h                             |    2 +
 net/dsa/Kconfig                               |    6 +
 net/dsa/Makefile                              |    1 +
 net/dsa/tag_rtl8_4.c                          |  178 ++
 12 files changed, 2342 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/dsa/rtl8365mb.c
 create mode 100644 net/dsa/tag_rtl8_4.c

-- 
2.32.0

[RFC PATCH net-next 1/5] net: dsa: realtek-smi: fix mdio_free bug on module unload

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

realtek-smi-core fails to unregister the slave MII bus on module unload,
raising the following BUG warning:

    mdio_bus.c:650: BUG_ON(bus->state != MDIOBUS_UNREGISTERED);

    kernel BUG at drivers/net/phy/mdio_bus.c:650!
    Internal error: Oops - BUG: 0 [#1] PREEMPT_RT SMP
    Call trace:
     mdiobus_free+0x4c/0x50
     devm_mdiobus_free+0x18/0x20
     release_nodes.isra.0+0x1c0/0x2b0
     devres_release_all+0x38/0x58
     device_release_driver_internal+0x124/0x1e8
     driver_detach+0x54/0xe0
     bus_remove_driver+0x60/0xd8
     driver_unregister+0x34/0x60
     platform_driver_unregister+0x18/0x20
     realtek_smi_driver_exit+0x14/0x1c [realtek_smi]

Fix this by duly unregistering the slave MII bus with
mdiobus_unregister. We do this in the DSA teardown path, since
registration is performed in the DSA setup path.

Cc: Linus Walleij <linus.walleij@linaro.org>
Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/net/dsa/realtek-smi-core.c | 6 ++++++
 drivers/net/dsa/realtek-smi-core.h | 1 +
 drivers/net/dsa/rtl8366rb.c        | 8 ++++++++
 3 files changed, 15 insertions(+)

diff --git a/drivers/net/dsa/realtek-smi-core.c b/drivers/net/dsa/realtek-smi-core.c
index 8e49d4f85d48..6992b6b31db6 100644
--- a/drivers/net/dsa/realtek-smi-core.c
+++ b/drivers/net/dsa/realtek-smi-core.c
@@ -383,6 +383,12 @@ int realtek_smi_setup_mdio(struct realtek_smi *smi)
 	return ret;
 }
 
+void realtek_smi_teardown_mdio(struct realtek_smi *smi)
+{
+	if (smi->slave_mii_bus)
+		mdiobus_unregister(smi->slave_mii_bus);
+}
+
 static int realtek_smi_probe(struct platform_device *pdev)
 {
 	const struct realtek_smi_variant *var;
diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h
index fcf465f7f922..6cfa5f2df7ea 100644
--- a/drivers/net/dsa/realtek-smi-core.h
+++ b/drivers/net/dsa/realtek-smi-core.h
@@ -119,6 +119,7 @@ struct realtek_smi_variant {
 int realtek_smi_write_reg_noack(struct realtek_smi *smi, u32 addr,
 				u32 data);
 int realtek_smi_setup_mdio(struct realtek_smi *smi);
+void realtek_smi_teardown_mdio(struct realtek_smi *smi);
 
 /* RTL8366 library helpers */
 int rtl8366_mc_is_used(struct realtek_smi *smi, int mc_index, int *used);
diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index a89093bc6c6a..6537fac7aba4 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -982,6 +982,13 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	return 0;
 }
 
+static void rtl8366rb_teardown(struct dsa_switch *ds)
+{
+	struct realtek_smi *smi = ds->priv;
+
+	realtek_smi_teardown_mdio(smi);
+}
+
 static enum dsa_tag_protocol rtl8366_get_tag_protocol(struct dsa_switch *ds,
 						      int port,
 						      enum dsa_tag_protocol mp)
@@ -1505,6 +1512,7 @@ static int rtl8366rb_detect(struct realtek_smi *smi)
 static const struct dsa_switch_ops rtl8366rb_switch_ops = {
 	.get_tag_protocol = rtl8366_get_tag_protocol,
 	.setup = rtl8366rb_setup,
+	.teardown = rtl8366rb_teardown,
 	.phylink_mac_link_up = rtl8366rb_mac_link_up,
 	.phylink_mac_link_down = rtl8366rb_mac_link_down,
 	.get_strings = rtl8366_get_strings,
-- 
2.32.0

Re: [RFC PATCH net-next 1/5] net: dsa: realtek-smi: fix mdio_free bug on module unload

[Andrew Lunn]

On Sun, Aug 22, 2021 at 09:31:39PM +0200, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> realtek-smi-core fails to unregister the slave MII bus on module unload,
> raising the following BUG warning:
> 
>     mdio_bus.c:650: BUG_ON(bus->state != MDIOBUS_UNREGISTERED);
> 
>     kernel BUG at drivers/net/phy/mdio_bus.c:650!
>     Internal error: Oops - BUG: 0 [#1] PREEMPT_RT SMP
>     Call trace:
>      mdiobus_free+0x4c/0x50
>      devm_mdiobus_free+0x18/0x20
>      release_nodes.isra.0+0x1c0/0x2b0
>      devres_release_all+0x38/0x58
>      device_release_driver_internal+0x124/0x1e8
>      driver_detach+0x54/0xe0
>      bus_remove_driver+0x60/0xd8
>      driver_unregister+0x34/0x60
>      platform_driver_unregister+0x18/0x20
>      realtek_smi_driver_exit+0x14/0x1c [realtek_smi]
> 
> Fix this by duly unregistering the slave MII bus with
> mdiobus_unregister. We do this in the DSA teardown path, since
> registration is performed in the DSA setup path.

Looking at the setup code, is there anything undoing what
rtl8366rb_setup_cascaded_irq() does?

This patch however loos O.K.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [RFC PATCH net-next 1/5] net: dsa: realtek-smi: fix mdio_free bug on module unload

[Alvin Šipraga]

Hi Andrew,

On 8/22/21 11:40 PM, Andrew Lunn wrote:
> On Sun, Aug 22, 2021 at 09:31:39PM +0200, Alvin Šipraga wrote:
>> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>>
>> realtek-smi-core fails to unregister the slave MII bus on module unload,
>> raising the following BUG warning:
>>
>>      mdio_bus.c:650: BUG_ON(bus->state != MDIOBUS_UNREGISTERED);
>>
>>      kernel BUG at drivers/net/phy/mdio_bus.c:650!
>>      Internal error: Oops - BUG: 0 [#1] PREEMPT_RT SMP
>>      Call trace:
>>       mdiobus_free+0x4c/0x50
>>       devm_mdiobus_free+0x18/0x20
>>       release_nodes.isra.0+0x1c0/0x2b0
>>       devres_release_all+0x38/0x58
>>       device_release_driver_internal+0x124/0x1e8
>>       driver_detach+0x54/0xe0
>>       bus_remove_driver+0x60/0xd8
>>       driver_unregister+0x34/0x60
>>       platform_driver_unregister+0x18/0x20
>>       realtek_smi_driver_exit+0x14/0x1c [realtek_smi]
>>
>> Fix this by duly unregistering the slave MII bus with
>> mdiobus_unregister. We do this in the DSA teardown path, since
>> registration is performed in the DSA setup path.
> 
> Looking at the setup code, is there anything undoing what
> rtl8366rb_setup_cascaded_irq() does?

No, there isn't. I neglected to mention in the rtl8365mb patch that I 
reworked the IRQ setup (compared with rtl8366rb) so that it could be 
torn down in a neat way. So you will see that the new driver does it 
properly, but I did not touch rtl8366rb because I am not using it. I am 
happy to do the same to rtl8366rb but I don't think I should make it 
part of this series. What do you think?

> 
> This patch however loos O.K.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
>      Andrew
> 

Re: [RFC PATCH net-next 1/5] net: dsa: realtek-smi: fix mdio_free bug on module unload

[Andrew Lunn]

> No, there isn't. I neglected to mention in the rtl8365mb patch that I 
> reworked the IRQ setup (compared with rtl8366rb) so that it could be 
> torn down in a neat way. So you will see that the new driver does it 
> properly, but I did not touch rtl8366rb because I am not using it. I am 
> happy to do the same to rtl8366rb but I don't think I should make it 
> part of this series. What do you think?

Lets see if Linus has time. He can probably model the change based on
what you have done here.

     Andrew

Re: [RFC PATCH net-next 1/5] net: dsa: realtek-smi: fix mdio_free bug on module unload

[Linus Walleij]

On Mon, Aug 23, 2021 at 1:16 AM Andrew Lunn <andrew@lunn.ch> wrote:

> > No, there isn't. I neglected to mention in the rtl8365mb patch that I
> > reworked the IRQ setup (compared with rtl8366rb) so that it could be
> > torn down in a neat way. So you will see that the new driver does it
> > properly, but I did not touch rtl8366rb because I am not using it. I am
> > happy to do the same to rtl8366rb but I don't think I should make it
> > part of this series. What do you think?
>
> Lets see if Linus has time. He can probably model the change based on
> what you have done here.

I have limited bandwidth as I am effectively on parental leave, so
I can't do much of writing code, but I can certainly test a patch or
two.

Yours,
Linus Walleij

Re: [RFC PATCH net-next 1/5] net: dsa: realtek-smi: fix mdio_free bug on module unload

[Alvin Šipraga]

On 8/28/21 12:06 AM, Linus Walleij wrote:
> On Mon, Aug 23, 2021 at 1:16 AM Andrew Lunn <andrew@lunn.ch> wrote:
> 
>>> No, there isn't. I neglected to mention in the rtl8365mb patch that I
>>> reworked the IRQ setup (compared with rtl8366rb) so that it could be
>>> torn down in a neat way. So you will see that the new driver does it
>>> properly, but I did not touch rtl8366rb because I am not using it. I am
>>> happy to do the same to rtl8366rb but I don't think I should make it
>>> part of this series. What do you think?
>>
>> Lets see if Linus has time. He can probably model the change based on
>> what you have done here.
> 
> I have limited bandwidth as I am effectively on parental leave, so
> I can't do much of writing code, but I can certainly test a patch or
> two.

No problem - I will follow up on this and submit some patches when I get 
there.

	Alvin

> 
> Yours,
> Linus Walleij
> 

Re: [RFC PATCH net-next 1/5] net: dsa: realtek-smi: fix mdio_free bug on module unload

[Vladimir Oltean]

On Sun, Aug 22, 2021 at 09:31:39PM +0200, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> realtek-smi-core fails to unregister the slave MII bus on module unload,
> raising the following BUG warning:
> 
>     mdio_bus.c:650: BUG_ON(bus->state != MDIOBUS_UNREGISTERED);
> 
>     kernel BUG at drivers/net/phy/mdio_bus.c:650!
>     Internal error: Oops - BUG: 0 [#1] PREEMPT_RT SMP
>     Call trace:
>      mdiobus_free+0x4c/0x50
>      devm_mdiobus_free+0x18/0x20
>      release_nodes.isra.0+0x1c0/0x2b0
>      devres_release_all+0x38/0x58
>      device_release_driver_internal+0x124/0x1e8
>      driver_detach+0x54/0xe0
>      bus_remove_driver+0x60/0xd8
>      driver_unregister+0x34/0x60
>      platform_driver_unregister+0x18/0x20
>      realtek_smi_driver_exit+0x14/0x1c [realtek_smi]
> 
> Fix this by duly unregistering the slave MII bus with
> mdiobus_unregister. We do this in the DSA teardown path, since
> registration is performed in the DSA setup path.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  drivers/net/dsa/realtek-smi-core.c | 6 ++++++
>  drivers/net/dsa/realtek-smi-core.h | 1 +
>  drivers/net/dsa/rtl8366rb.c        | 8 ++++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/net/dsa/realtek-smi-core.c b/drivers/net/dsa/realtek-smi-core.c
> index 8e49d4f85d48..6992b6b31db6 100644
> --- a/drivers/net/dsa/realtek-smi-core.c
> +++ b/drivers/net/dsa/realtek-smi-core.c
> @@ -383,6 +383,12 @@ int realtek_smi_setup_mdio(struct realtek_smi *smi)
>  	return ret;
>  }
>  
> +void realtek_smi_teardown_mdio(struct realtek_smi *smi)
> +{
> +	if (smi->slave_mii_bus)
> +		mdiobus_unregister(smi->slave_mii_bus);
> +}
> +
>  static int realtek_smi_probe(struct platform_device *pdev)
>  {
>  	const struct realtek_smi_variant *var;
> diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h
> index fcf465f7f922..6cfa5f2df7ea 100644
> --- a/drivers/net/dsa/realtek-smi-core.h
> +++ b/drivers/net/dsa/realtek-smi-core.h
> @@ -119,6 +119,7 @@ struct realtek_smi_variant {
>  int realtek_smi_write_reg_noack(struct realtek_smi *smi, u32 addr,
>  				u32 data);
>  int realtek_smi_setup_mdio(struct realtek_smi *smi);
> +void realtek_smi_teardown_mdio(struct realtek_smi *smi);
>  
>  /* RTL8366 library helpers */
>  int rtl8366_mc_is_used(struct realtek_smi *smi, int mc_index, int *used);
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index a89093bc6c6a..6537fac7aba4 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -982,6 +982,13 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>  	return 0;
>  }
>  
> +static void rtl8366rb_teardown(struct dsa_switch *ds)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +
> +	realtek_smi_teardown_mdio(smi);
> +}
> +

Objection: dsa_switch_teardown has:

	if (ds->slave_mii_bus && ds->ops->phy_read)
		mdiobus_unregister(ds->slave_mii_bus);

The realtek_smi_setup_mdio function does:

	smi->ds->slave_mii_bus = smi->slave_mii_bus;

so I would expect that this would result in a double unregister on some
systems.

I haven't went through your new driver, but I wonder whether you have
the phy_read and phy_write methods implemented? Maybe that is the
difference?

>  static enum dsa_tag_protocol rtl8366_get_tag_protocol(struct dsa_switch *ds,
>  						      int port,
>  						      enum dsa_tag_protocol mp)
> @@ -1505,6 +1512,7 @@ static int rtl8366rb_detect(struct realtek_smi *smi)
>  static const struct dsa_switch_ops rtl8366rb_switch_ops = {
>  	.get_tag_protocol = rtl8366_get_tag_protocol,
>  	.setup = rtl8366rb_setup,
> +	.teardown = rtl8366rb_teardown,
>  	.phylink_mac_link_up = rtl8366rb_mac_link_up,
>  	.phylink_mac_link_down = rtl8366rb_mac_link_down,
>  	.get_strings = rtl8366_get_strings,
> -- 
> 2.32.0
> 

Re: [RFC PATCH net-next 1/5] net: dsa: realtek-smi: fix mdio_free bug on module unload

[Alvin Šipraga]

Hi Vladimir,

On 8/22/21 11:54 PM, Vladimir Oltean wrote:
> On Sun, Aug 22, 2021 at 09:31:39PM +0200, Alvin Šipraga wrote:
>> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>>
>> realtek-smi-core fails to unregister the slave MII bus on module unload,
>> raising the following BUG warning:
>>
>>      mdio_bus.c:650: BUG_ON(bus->state != MDIOBUS_UNREGISTERED);
>>
>>      kernel BUG at drivers/net/phy/mdio_bus.c:650!
>>      Internal error: Oops - BUG: 0 [#1] PREEMPT_RT SMP
>>      Call trace:
>>       mdiobus_free+0x4c/0x50
>>       devm_mdiobus_free+0x18/0x20
>>       release_nodes.isra.0+0x1c0/0x2b0
>>       devres_release_all+0x38/0x58
>>       device_release_driver_internal+0x124/0x1e8
>>       driver_detach+0x54/0xe0
>>       bus_remove_driver+0x60/0xd8
>>       driver_unregister+0x34/0x60
>>       platform_driver_unregister+0x18/0x20
>>       realtek_smi_driver_exit+0x14/0x1c [realtek_smi]
>>
>> Fix this by duly unregistering the slave MII bus with
>> mdiobus_unregister. We do this in the DSA teardown path, since
>> registration is performed in the DSA setup path.
>>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
>> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
>> ---
>>   drivers/net/dsa/realtek-smi-core.c | 6 ++++++
>>   drivers/net/dsa/realtek-smi-core.h | 1 +
>>   drivers/net/dsa/rtl8366rb.c        | 8 ++++++++
>>   3 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/net/dsa/realtek-smi-core.c b/drivers/net/dsa/realtek-smi-core.c
>> index 8e49d4f85d48..6992b6b31db6 100644
>> --- a/drivers/net/dsa/realtek-smi-core.c
>> +++ b/drivers/net/dsa/realtek-smi-core.c
>> @@ -383,6 +383,12 @@ int realtek_smi_setup_mdio(struct realtek_smi *smi)
>>   	return ret;
>>   }
>>   
>> +void realtek_smi_teardown_mdio(struct realtek_smi *smi)
>> +{
>> +	if (smi->slave_mii_bus)
>> +		mdiobus_unregister(smi->slave_mii_bus);
>> +}
>> +
>>   static int realtek_smi_probe(struct platform_device *pdev)
>>   {
>>   	const struct realtek_smi_variant *var;
>> diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h
>> index fcf465f7f922..6cfa5f2df7ea 100644
>> --- a/drivers/net/dsa/realtek-smi-core.h
>> +++ b/drivers/net/dsa/realtek-smi-core.h
>> @@ -119,6 +119,7 @@ struct realtek_smi_variant {
>>   int realtek_smi_write_reg_noack(struct realtek_smi *smi, u32 addr,
>>   				u32 data);
>>   int realtek_smi_setup_mdio(struct realtek_smi *smi);
>> +void realtek_smi_teardown_mdio(struct realtek_smi *smi);
>>   
>>   /* RTL8366 library helpers */
>>   int rtl8366_mc_is_used(struct realtek_smi *smi, int mc_index, int *used);
>> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
>> index a89093bc6c6a..6537fac7aba4 100644
>> --- a/drivers/net/dsa/rtl8366rb.c
>> +++ b/drivers/net/dsa/rtl8366rb.c
>> @@ -982,6 +982,13 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>>   	return 0;
>>   }
>>   
>> +static void rtl8366rb_teardown(struct dsa_switch *ds)
>> +{
>> +	struct realtek_smi *smi = ds->priv;
>> +
>> +	realtek_smi_teardown_mdio(smi);
>> +}
>> +
> 
> Objection: dsa_switch_teardown has:
> 
> 	if (ds->slave_mii_bus && ds->ops->phy_read)
> 		mdiobus_unregister(ds->slave_mii_bus);

This is unregistering an mdiobus registered in dsa_switch_setup:

	if (!ds->slave_mii_bus && ds->ops->phy_read) {
		ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
		if (!ds->slave_mii_bus) {
			err = -ENOMEM;
			goto teardown;
		}

		dsa_slave_mii_bus_init(ds);

		err = mdiobus_register(ds->slave_mii_bus);
		if (err < 0)
			goto teardown;
	}

However, we don't enter this codepath because:

- ds->slave_mii_bus is already set in the call to ds->ops->setup() 
before the code snippet above;
- ds->ops->phy_read is not set.

We don't want to either, since we want to use of_mdiobus_register().

> 
> The realtek_smi_setup_mdio function does:
> 
> 	smi->ds->slave_mii_bus = smi->slave_mii_bus;
> 
> so I would expect that this would result in a double unregister on some
> systems.
> 
> I haven't went through your new driver, but I wonder whether you have
> the phy_read and phy_write methods implemented? Maybe that is the
> difference?

Right, phy_read/phy_write are not set in the dsa_switch_ops of 
rtl8365mb. So we should be safe.

It did get me thinking that it would be nice if dsa_register_switch() 
could call of_mdiobus_register() when necessary, since the snippet above 
(and its call to dsa_slave_mii_bus_init()) is almost same as 
realtek_smi_setup_mdio(). It would simplify some logic in realtek-smi 
drivers and obviate the need for this patch. I am not sure what the 
right approach to this would be but with some pointers I can give it a shot.

> 
>>   static enum dsa_tag_protocol rtl8366_get_tag_protocol(struct dsa_switch *ds,
>>   						      int port,
>>   						      enum dsa_tag_protocol mp)
>> @@ -1505,6 +1512,7 @@ static int rtl8366rb_detect(struct realtek_smi *smi)
>>   static const struct dsa_switch_ops rtl8366rb_switch_ops = {
>>   	.get_tag_protocol = rtl8366_get_tag_protocol,
>>   	.setup = rtl8366rb_setup,
>> +	.teardown = rtl8366rb_teardown,
>>   	.phylink_mac_link_up = rtl8366rb_mac_link_up,
>>   	.phylink_mac_link_down = rtl8366rb_mac_link_down,
>>   	.get_strings = rtl8366_get_strings,
>> -- 
>> 2.32.0
>>

Re: [RFC PATCH net-next 1/5] net: dsa: realtek-smi: fix mdio_free bug on module unload

[Vladimir Oltean]

On Sun, Aug 22, 2021 at 10:42:23PM +0000, Alvin Šipraga wrote:
> > Objection: dsa_switch_teardown has:
> >
> > 	if (ds->slave_mii_bus && ds->ops->phy_read)
> > 		mdiobus_unregister(ds->slave_mii_bus);
>
> This is unregistering an mdiobus registered in dsa_switch_setup:
>
> 	if (!ds->slave_mii_bus && ds->ops->phy_read) {
> 		ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
> 		if (!ds->slave_mii_bus) {
> 			err = -ENOMEM;
> 			goto teardown;
> 		}
>
> 		dsa_slave_mii_bus_init(ds);
>
> 		err = mdiobus_register(ds->slave_mii_bus);
> 		if (err < 0)
> 			goto teardown;
> 	}
>
> However, we don't enter this codepath because:
>
> - ds->slave_mii_bus is already set in the call to ds->ops->setup()
> before the code snippet above;
> - ds->ops->phy_read is not set.
>
> We don't want to either, since we want to use of_mdiobus_register().
>
> >
> > The realtek_smi_setup_mdio function does:
> >
> > 	smi->ds->slave_mii_bus = smi->slave_mii_bus;
> >
> > so I would expect that this would result in a double unregister on some
> > systems.
> >
> > I haven't went through your new driver, but I wonder whether you have
> > the phy_read and phy_write methods implemented? Maybe that is the
> > difference?
>
> Right, phy_read/phy_write are not set in the dsa_switch_ops of
> rtl8365mb. So we should be safe.

Correct, DSA only unregisters the ds->slave_mii_bus it has registered,
which is provided when the driver implements phy_read and/or phy_write
but does not set/register ds->slave_mii_bus itself. The patch is fine.

>
> It did get me thinking that it would be nice if dsa_register_switch()
> could call of_mdiobus_register() when necessary, since the snippet above
> (and its call to dsa_slave_mii_bus_init()) is almost same as
> realtek_smi_setup_mdio(). It would simplify some logic in realtek-smi
> drivers and obviate the need for this patch. I am not sure what the
> right approach to this would be but with some pointers I can give it a shot.

I don't think DSA could call of_mdiobus_register, since you would need
to pass it the OF node you want and the read/write ops for the bus and
its name and a place to store it (one DSA switch might have more than
one MDIO bus), and I just fail to see the point of doing that.

The whole point of having ds->slave_mii_bus (either allocated by the
driver or by DSA) is to access the PHY registers of a port under a very
narrow set of assumptions: it implicitly assumes that the port N has a
PHY at MDIO address N, as opposed to doing the usual which is to follow
the phy-handle, and that there is a single internal MDIO bus. DSA will
do this as last resort in dsa_slave_phy_setup. But if you use
of_mdiobus_register, just put a phy-handle in the device tree and be
done, you don't need ds->ops->phy_read or ds->ops->phy_write, nor can
you/should you overload these pointers for DSA to do the
of_mdiobus_register for you.

> >
> >>   static enum dsa_tag_protocol rtl8366_get_tag_protocol(struct dsa_switch *ds,
> >>   						      int port,
> >>   						      enum dsa_tag_protocol mp)
> >> @@ -1505,6 +1512,7 @@ static int rtl8366rb_detect(struct realtek_smi *smi)
> >>   static const struct dsa_switch_ops rtl8366rb_switch_ops = {
> >>   	.get_tag_protocol = rtl8366_get_tag_protocol,
> >>   	.setup = rtl8366rb_setup,
> >> +	.teardown = rtl8366rb_teardown,
> >>   	.phylink_mac_link_up = rtl8366rb_mac_link_up,
> >>   	.phylink_mac_link_down = rtl8366rb_mac_link_down,
> >>   	.get_strings = rtl8366_get_strings,
> >> --
> >> 2.32.0
> >>

[RFC PATCH net-next 2/5] dt-bindings: net: dsa: realtek-smi: document new compatible rtl8365mb

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

rtl8365mb is a new realtek-smi subdriver for the RTL8365MB-VC 4+1 port
10/100/1000M Ethernet switch controller. Its compatible string is
"realtek,rtl8365mb".

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 Documentation/devicetree/bindings/net/dsa/realtek-smi.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt b/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt
index b6ae8541bd55..ee03eb40a488 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt
+++ b/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt
@@ -9,6 +9,7 @@ SMI-based Realtek devices.
 Required properties:
 
 - compatible: must be exactly one of:
+      "realtek,rtl8365mb" (4+1 ports)
       "realtek,rtl8366"
       "realtek,rtl8366rb" (4+1 ports)
       "realtek,rtl8366s"  (4+1 ports)
-- 
2.32.0

Re: [RFC PATCH net-next 2/5] dt-bindings: net: dsa: realtek-smi: document new compatible rtl8365mb

[Andrew Lunn]

On Sun, Aug 22, 2021 at 09:31:40PM +0200, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> rtl8365mb is a new realtek-smi subdriver for the RTL8365MB-VC 4+1 port
> 10/100/1000M Ethernet switch controller. Its compatible string is
> "realtek,rtl8365mb".
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [RFC PATCH net-next 2/5] dt-bindings: net: dsa: realtek-smi: document new compatible rtl8365mb

[Florian Fainelli]

On 8/22/2021 9:31 PM, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> rtl8365mb is a new realtek-smi subdriver for the RTL8365MB-VC 4+1 port
> 10/100/1000M Ethernet switch controller. Its compatible string is
> "realtek,rtl8365mb".
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [RFC PATCH net-next 2/5] dt-bindings: net: dsa: realtek-smi: document new compatible rtl8365mb

[Rob Herring]

On Sun, 22 Aug 2021 21:31:40 +0200, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> rtl8365mb is a new realtek-smi subdriver for the RTL8365MB-VC 4+1 port
> 10/100/1000M Ethernet switch controller. Its compatible string is
> "realtek,rtl8365mb".
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  Documentation/devicetree/bindings/net/dsa/realtek-smi.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [RFC PATCH net-next 2/5] dt-bindings: net: dsa: realtek-smi: document new compatible rtl8365mb

[Linus Walleij]

On Sun, Aug 22, 2021 at 9:32 PM Alvin Šipraga <alvin@pqrs.dk> wrote:

> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>
> rtl8365mb is a new realtek-smi subdriver for the RTL8365MB-VC 4+1 port
> 10/100/1000M Ethernet switch controller. Its compatible string is
> "realtek,rtl8365mb".
>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

And BTW: really happy to see kernel contributions from B&O!

Yours,
Linus Walleij

[RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

This commit implements a basic version of the 8 byte tag protocol used
in the Realtek RTL8365MB-VC switch, which carries with it a protocol
version of 0x04.

The implementation itself only handles the parsing of the EtherType
value and Realtek protocol version, together with the source or
destination port fields. The rest is left unimplemented for now.

The tag format is described in a confidential document provided to my
employer - Bang & Olufsen a/s - by Realtek Semiconductor Corp.
Permission has been granted by Realtek to publish this driver based on
that material, together with an extract from the document describing the
tag format and its fields.  It is hoped that this will help future
implementors who do not have access to the material but who wish to
extend the functionality of drivers for chips which use this protocol.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 include/net/dsa.h    |   2 +
 net/dsa/Kconfig      |   6 ++
 net/dsa/Makefile     |   1 +
 net/dsa/tag_rtl8_4.c | 178 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 187 insertions(+)
 create mode 100644 net/dsa/tag_rtl8_4.c

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 0c2cba45fa79..6d8b5a11d99a 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -51,6 +51,7 @@ struct phylink_link_state;
 #define DSA_TAG_PROTO_SEVILLE_VALUE		21
 #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE		22
 #define DSA_TAG_PROTO_SJA1110_VALUE		23
+#define DSA_TAG_PROTO_RTL8_4_VALUE		24
 
 enum dsa_tag_protocol {
 	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
@@ -77,6 +78,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_OCELOT_8021Q	= DSA_TAG_PROTO_OCELOT_8021Q_VALUE,
 	DSA_TAG_PROTO_SEVILLE		= DSA_TAG_PROTO_SEVILLE_VALUE,
 	DSA_TAG_PROTO_SJA1110		= DSA_TAG_PROTO_SJA1110_VALUE,
+	DSA_TAG_PROTO_RTL8_4		= DSA_TAG_PROTO_RTL8_4_VALUE,
 };
 
 struct dsa_switch;
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 548285539752..470a2f3e8f75 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -99,6 +99,12 @@ config NET_DSA_TAG_RTL4_A
 	  Realtek switches with 4 byte protocol A tags, sich as found in
 	  the Realtek RTL8366RB.
 
+config NET_DSA_TAG_RTL8_4
+	tristate "Tag driver for Realtek 8 byte protocol 4 tags"
+	help
+	  Say Y or M if you want to enable support for tagging frames for Realtek
+	  switches with 8 byte protocol 4 tags, such as the Realtek RTL8365MB-VC.
+
 config NET_DSA_TAG_OCELOT
 	tristate "Tag driver for Ocelot family of switches, using NPI port"
 	depends on MSCC_OCELOT_SWITCH_LIB || \
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 67ea009f242c..01282817e80e 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_NET_DSA_TAG_GSWIP) += tag_gswip.o
 obj-$(CONFIG_NET_DSA_TAG_HELLCREEK) += tag_hellcreek.o
 obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
 obj-$(CONFIG_NET_DSA_TAG_RTL4_A) += tag_rtl4_a.o
+obj-$(CONFIG_NET_DSA_TAG_RTL8_4) += tag_rtl8_4.o
 obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
 obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
 obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o
diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c
new file mode 100644
index 000000000000..1082bafb6a2e
--- /dev/null
+++ b/net/dsa/tag_rtl8_4.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Handler for Realtek 8 byte switch tags
+ *
+ * Copyright (C) 2021 Alvin Šipraga <alsi@bang-olufsen.dk>
+ *
+ * NOTE: Currently only supports protocol "4" found in the RTL8365MB, hence
+ * named tag_rtl8_4.
+ *
+ * This "proprietary tag" header has the following format:
+ *
+ *  -------------------------------------------
+ *  | MAC DA | MAC SA | 8 byte tag | Type | ...
+ *  -------------------------------------------
+ *     _______________/            \______________________________________
+ *    /                                                                   \
+ *  0                                  7|8                                 15
+ *  |-----------------------------------+-----------------------------------|---
+ *  |                               (16-bit)                                | ^
+ *  |                       Realtek EtherType [0x8899]                      | |
+ *  |-----------------------------------+-----------------------------------| 8
+ *  |              (8-bit)              |              (8-bit)              |
+ *  |          Protocol [0x04]          |              REASON               | b
+ *  |-----------------------------------+-----------------------------------| y
+ *  |   (1)  | (1) | (2) |   (1)  | (3) | (1)  | (1) |    (1)    |   (5)    | t
+ *  | FID_EN |  X  | FID | PRI_EN | PRI | KEEP |  X  | LEARN_DIS |    X     | e
+ *  |-----------------------------------+-----------------------------------| s
+ *  |   (1)  |                       (15-bit)                               | |
+ *  |  ALLOW |                        TX/RX                                 | v
+ *  |-----------------------------------+-----------------------------------|---
+ *
+ * With the following field descriptions:
+ *
+ *    field      | description
+ *   ------------+-------------
+ *    Realtek    | 0x8899: indicates that this is a proprietary Realtek tag;
+ *     EtherType |         note that Realtek uses the same EtherType for
+ *               |         other incompatible tag formats (e.g. tag_rtl4_a.c)
+ *    Protocol   | 0x04: indicates that this tag conforms to this format
+ *    X          | reserved
+ *   ------------+-------------
+ *    REASON     | reason for forwarding packet to CPU
+ *    FID_EN     | 1: packet has an FID
+ *               | 0: no FID
+ *    FID        | FID of packet (if FID_EN=1)
+ *    PRI_EN     | 1: force priority of packet
+ *               | 0: don't force priority
+ *    PRI        | priority of packet (if PRI_EN=1)
+ *    KEEP       | preserve packet VLAN tag format
+ *    LEARN_DIS  | don't learn the source MAC address of the packet
+ *    ALLOW      | 1: treat TX/RX field as an allowance port mask, meaning the
+ *               |    packet may only be forwarded to ports specified in the
+ *               |    mask
+ *               | 0: no allowance port mask, TX/RX field is the forwarding
+ *               |    port mask
+ *    TX/RX      | TX (switch->CPU): port number the packet was received on
+ *               | RX (CPU->switch): forwarding port mask (if ALLOW=0)
+ *               |                   allowance port mask (if ALLOW=1)
+ */
+
+#include <linux/etherdevice.h>
+#include <linux/bits.h>
+
+#include "dsa_priv.h"
+
+#define RTL8_4_TAG_LEN			8
+#define RTL8_4_ETHERTYPE		0x8899
+/* 0x04 = RTL8365MB DSA protocol
+ */
+#define RTL8_4_PROTOCOL_RTL8365MB	0x04
+
+static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb,
+				       struct net_device *dev)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	__be16 *p;
+	u8 *tag;
+	u16 out;
+
+	/* Pad out so that the (stripped) packet is at least 64 bytes long
+	 * (including FCS), otherwise the switch will drop the packet.
+	 * Then we need an additional 8 bytes for the Realtek tag.
+	 */
+	if (__skb_put_padto(skb, ETH_ZLEN + RTL8_4_TAG_LEN, false))
+		return NULL;
+
+	skb_push(skb, RTL8_4_TAG_LEN);
+
+	dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN);
+	tag = dsa_etype_header_pos_tx(skb);
+
+	/* Set Realtek EtherType */
+	p = (__be16 *)tag;
+	*p = htons(RTL8_4_ETHERTYPE);
+
+	/* Set Protocol; zero REASON */
+	p = (__be16 *)(tag + 2);
+	*p = htons(RTL8_4_PROTOCOL_RTL8365MB << 8);
+
+	/* Zero FID_EN, FID, PRI_EN, PRI, KEEP, LEARN_DIS */
+	p = (__be16 *)(tag + 4);
+	*p = 0;
+
+	/* Zero ALLOW; set RX (CPU->switch) forwarding port mask */
+	p = (__be16 *)(tag + 6);
+	out = BIT(dp->index);
+	*p = htons(~(1 << 15) & BIT(dp->index));
+
+	return skb;
+}
+
+static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
+				      struct net_device *dev)
+{
+	__be16 *p;
+	u16 etype;
+	u8 proto;
+	u8 *tag;
+	u8 port;
+	u16 tmp;
+
+	if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
+		return NULL;
+
+	tag = dsa_etype_header_pos_rx(skb);
+
+	/* Parse Realtek EtherType */
+	p = (__be16 *)tag;
+	etype = ntohs(*p);
+	if (unlikely(etype != RTL8_4_ETHERTYPE)) {
+		netdev_dbg(dev, "non-realtek ethertype 0x%04x\n", etype);
+		return NULL;
+	}
+
+	/* Parse Protocol; ignore REASON */
+	p = (__be16 *)(tag + 2);
+	tmp = ntohs(*p);
+	proto = tmp >> 8;
+	if (unlikely(proto != RTL8_4_PROTOCOL_RTL8365MB)) {
+		netdev_dbg(dev, "unknown realtek protocol 0x%02x\n", proto);
+		return NULL;
+	}
+
+	/* Ignore FID_EN, FID, PRI_EN, PRI, KEEP, LEARN_DIS */
+	p = (__be16 *)(tag + 4);
+
+	/* Ignore ALLOW; parse TX (switch->CPU) */
+	p = (__be16 *)(tag + 6);
+	tmp = ntohs(*p);
+	port = tmp & 0xf; /* Port number is the LSB 4 bits */
+
+	skb->dev = dsa_master_find_slave(dev, 0, port);
+	if (!skb->dev) {
+		netdev_dbg(dev, "could not find slave for port %d\n", port);
+		return NULL;
+	}
+
+	/* Remove tag and recalculate checksum */
+	skb_pull_rcsum(skb, RTL8_4_TAG_LEN);
+
+	dsa_strip_etype_header(skb, RTL8_4_TAG_LEN);
+
+	skb->offload_fwd_mark = 1;
+
+	return skb;
+}
+
+static const struct dsa_device_ops rtl8_4_netdev_ops = {
+	.name = "rtl8_4",
+	.proto = DSA_TAG_PROTO_RTL8_4,
+	.xmit = rtl8_4_tag_xmit,
+	.rcv = rtl8_4_tag_rcv,
+	.needed_headroom = RTL8_4_TAG_LEN,
+};
+module_dsa_tag_driver(rtl8_4_netdev_ops);
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL8_4);
-- 
2.32.0

Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Andrew Lunn]

On Sun, Aug 22, 2021 at 09:31:41PM +0200, Alvin Šipraga wrote:
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 548285539752..470a2f3e8f75 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -99,6 +99,12 @@ config NET_DSA_TAG_RTL4_A
>  	  Realtek switches with 4 byte protocol A tags, sich as found in
>  	  the Realtek RTL8366RB.
>  
> +config NET_DSA_TAG_RTL8_4
> +	tristate "Tag driver for Realtek 8 byte protocol 4 tags"
> +	help
> +	  Say Y or M if you want to enable support for tagging frames for Realtek
> +	  switches with 8 byte protocol 4 tags, such as the Realtek RTL8365MB-VC.
> +

Hi Alvin

This file is sorted based on the tristate text. As such, the
NET_DSA_TAG_RTL4_A is in the wrong place. So i can understand why you
put it here, but place move it after the Qualcom driver.

> @@ -11,6 +11,7 @@ obj-$(CONFIG_NET_DSA_TAG_GSWIP) += tag_gswip.o
>  obj-$(CONFIG_NET_DSA_TAG_HELLCREEK) += tag_hellcreek.o
>  obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
>  obj-$(CONFIG_NET_DSA_TAG_RTL4_A) += tag_rtl4_a.o
> +obj-$(CONFIG_NET_DSA_TAG_RTL8_4) += tag_rtl8_4.o
>  obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
>  obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
>  obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o

The CONFIG_NET_DSA_TAG_RTL4_A is also in the wrong place...

> diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c
> new file mode 100644
> index 000000000000..1082bafb6a2e
> --- /dev/null
> +++ b/net/dsa/tag_rtl8_4.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Handler for Realtek 8 byte switch tags
> + *
> + * Copyright (C) 2021 Alvin Šipraga <alsi@bang-olufsen.dk>
> + *
> + * NOTE: Currently only supports protocol "4" found in the RTL8365MB, hence
> + * named tag_rtl8_4.
> + *
> + * This "proprietary tag" header has the following format:

I think they are all proprietary. At least, there is no
standardization across vendors.

> + *
> + *  -------------------------------------------
> + *  | MAC DA | MAC SA | 8 byte tag | Type | ...
> + *  -------------------------------------------
> + *     _______________/            \______________________________________
> + *    /                                                                   \
> + *  0                                  7|8                                 15
> + *  |-----------------------------------+-----------------------------------|---
> + *  |                               (16-bit)                                | ^
> + *  |                       Realtek EtherType [0x8899]                      | |
> + *  |-----------------------------------+-----------------------------------| 8
> + *  |              (8-bit)              |              (8-bit)              |
> + *  |          Protocol [0x04]          |              REASON               | b
> + *  |-----------------------------------+-----------------------------------| y
> + *  |   (1)  | (1) | (2) |   (1)  | (3) | (1)  | (1) |    (1)    |   (5)    | t
> + *  | FID_EN |  X  | FID | PRI_EN | PRI | KEEP |  X  | LEARN_DIS |    X     | e
> + *  |-----------------------------------+-----------------------------------| s
> + *  |   (1)  |                       (15-bit)                               | |
> + *  |  ALLOW |                        TX/RX                                 | v
> + *  |-----------------------------------+-----------------------------------|---
> + *
> + * With the following field descriptions:
> + *
> + *    field      | description
> + *   ------------+-------------
> + *    Realtek    | 0x8899: indicates that this is a proprietary Realtek tag;
> + *     EtherType |         note that Realtek uses the same EtherType for
> + *               |         other incompatible tag formats (e.g. tag_rtl4_a.c)
> + *    Protocol   | 0x04: indicates that this tag conforms to this format
> + *    X          | reserved
> + *   ------------+-------------
> + *    REASON     | reason for forwarding packet to CPU

Are you allowed to document reason? This could be interesting for some
of the future work.

> + *    FID_EN     | 1: packet has an FID
> + *               | 0: no FID
> + *    FID        | FID of packet (if FID_EN=1)
> + *    PRI_EN     | 1: force priority of packet
> + *               | 0: don't force priority
> + *    PRI        | priority of packet (if PRI_EN=1)
> + *    KEEP       | preserve packet VLAN tag format
> + *    LEARN_DIS  | don't learn the source MAC address of the packet
> + *    ALLOW      | 1: treat TX/RX field as an allowance port mask, meaning the
> + *               |    packet may only be forwarded to ports specified in the
> + *               |    mask
> + *               | 0: no allowance port mask, TX/RX field is the forwarding
> + *               |    port mask
> + *    TX/RX      | TX (switch->CPU): port number the packet was received on
> + *               | RX (CPU->switch): forwarding port mask (if ALLOW=0)
> + *               |                   allowance port mask (if ALLOW=1)

There are some interesting fields here. It will be interesting to see
what we can do with the switch.

> + */
> +
> +#include <linux/etherdevice.h>
> +#include <linux/bits.h>
> +
> +#include "dsa_priv.h"
> +
> +#define RTL8_4_TAG_LEN			8
> +#define RTL8_4_ETHERTYPE		0x8899

Please add this to include/uapi/linux/if_ether.h

> +static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
> +				      struct net_device *dev)
> +{
> +	__be16 *p;
> +	u16 etype;
> +	u8 proto;
> +	u8 *tag;
> +	u8 port;
> +	u16 tmp;
> +
> +	if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
> +		return NULL;
> +
> +	tag = dsa_etype_header_pos_rx(skb);
> +
> +	/* Parse Realtek EtherType */
> +	p = (__be16 *)tag;
> +	etype = ntohs(*p);
> +	if (unlikely(etype != RTL8_4_ETHERTYPE)) {
> +		netdev_dbg(dev, "non-realtek ethertype 0x%04x\n", etype);

You probably want to rate limit these sorts of prints. You have
limited controller of what frames come in, and if somebody can craft
bad frames, they can make you send all your time printing messages to
the log.

    Andrew

Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Alvin Šipraga]

Hi Andrew,

On 8/23/21 12:02 AM, Andrew Lunn wrote:
> On Sun, Aug 22, 2021 at 09:31:41PM +0200, Alvin Šipraga wrote:
>> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
>> index 548285539752..470a2f3e8f75 100644
>> --- a/net/dsa/Kconfig
>> +++ b/net/dsa/Kconfig
>> @@ -99,6 +99,12 @@ config NET_DSA_TAG_RTL4_A
>>   	  Realtek switches with 4 byte protocol A tags, sich as found in
>>   	  the Realtek RTL8366RB.
>>   
>> +config NET_DSA_TAG_RTL8_4
>> +	tristate "Tag driver for Realtek 8 byte protocol 4 tags"
>> +	help
>> +	  Say Y or M if you want to enable support for tagging frames for Realtek
>> +	  switches with 8 byte protocol 4 tags, such as the Realtek RTL8365MB-VC.
>> +
> 
> Hi Alvin
> 
> This file is sorted based on the tristate text. As such, the
> NET_DSA_TAG_RTL4_A is in the wrong place. So i can understand why you
> put it here, but place move it after the Qualcom driver.

Thanks - I'll fix it in v2.

> 
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_NET_DSA_TAG_GSWIP) += tag_gswip.o
>>   obj-$(CONFIG_NET_DSA_TAG_HELLCREEK) += tag_hellcreek.o
>>   obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
>>   obj-$(CONFIG_NET_DSA_TAG_RTL4_A) += tag_rtl4_a.o
>> +obj-$(CONFIG_NET_DSA_TAG_RTL8_4) += tag_rtl8_4.o
>>   obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
>>   obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
>>   obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o
> 
> The CONFIG_NET_DSA_TAG_RTL4_A is also in the wrong place...

Ditto.

> 
>> diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c
>> new file mode 100644
>> index 000000000000..1082bafb6a2e
>> --- /dev/null
>> +++ b/net/dsa/tag_rtl8_4.c
>> @@ -0,0 +1,178 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Handler for Realtek 8 byte switch tags
>> + *
>> + * Copyright (C) 2021 Alvin Šipraga <alsi@bang-olufsen.dk>
>> + *
>> + * NOTE: Currently only supports protocol "4" found in the RTL8365MB, hence
>> + * named tag_rtl8_4.
>> + *
>> + * This "proprietary tag" header has the following format:
> 
> I think they are all proprietary. At least, there is no
> standardization across vendors.

I'll remove in v2.

> 
>> + *
>> + *  -------------------------------------------
>> + *  | MAC DA | MAC SA | 8 byte tag | Type | ...
>> + *  -------------------------------------------
>> + *     _______________/            \______________________________________
>> + *    /                                                                   \
>> + *  0                                  7|8                                 15
>> + *  |-----------------------------------+-----------------------------------|---
>> + *  |                               (16-bit)                                | ^
>> + *  |                       Realtek EtherType [0x8899]                      | |
>> + *  |-----------------------------------+-----------------------------------| 8
>> + *  |              (8-bit)              |              (8-bit)              |
>> + *  |          Protocol [0x04]          |              REASON               | b
>> + *  |-----------------------------------+-----------------------------------| y
>> + *  |   (1)  | (1) | (2) |   (1)  | (3) | (1)  | (1) |    (1)    |   (5)    | t
>> + *  | FID_EN |  X  | FID | PRI_EN | PRI | KEEP |  X  | LEARN_DIS |    X     | e
>> + *  |-----------------------------------+-----------------------------------| s
>> + *  |   (1)  |                       (15-bit)                               | |
>> + *  |  ALLOW |                        TX/RX                                 | v
>> + *  |-----------------------------------+-----------------------------------|---
>> + *
>> + * With the following field descriptions:
>> + *
>> + *    field      | description
>> + *   ------------+-------------
>> + *    Realtek    | 0x8899: indicates that this is a proprietary Realtek tag;
>> + *     EtherType |         note that Realtek uses the same EtherType for
>> + *               |         other incompatible tag formats (e.g. tag_rtl4_a.c)
>> + *    Protocol   | 0x04: indicates that this tag conforms to this format
>> + *    X          | reserved
>> + *   ------------+-------------
>> + *    REASON     | reason for forwarding packet to CPU
> 
> Are you allowed to document reason? This could be interesting for some
> of the future work.

Unfortunately the reason field is undocumented. The vendor driver 
doesn't contain any parsing code for the CPU tag so we are left to 
guess. One obvious example would be trapped packets, since the switch 
lets you configure what to do with those (drop, forward to CPU, etc.).

I have not had a reason to look into this yet, otherwise I would have 
documented whatever I knew about it. Hope it's OK for now.

> 
>> + *    FID_EN     | 1: packet has an FID
>> + *               | 0: no FID
>> + *    FID        | FID of packet (if FID_EN=1)
>> + *    PRI_EN     | 1: force priority of packet
>> + *               | 0: don't force priority
>> + *    PRI        | priority of packet (if PRI_EN=1)
>> + *    KEEP       | preserve packet VLAN tag format
>> + *    LEARN_DIS  | don't learn the source MAC address of the packet
>> + *    ALLOW      | 1: treat TX/RX field as an allowance port mask, meaning the
>> + *               |    packet may only be forwarded to ports specified in the
>> + *               |    mask
>> + *               | 0: no allowance port mask, TX/RX field is the forwarding
>> + *               |    port mask
>> + *    TX/RX      | TX (switch->CPU): port number the packet was received on
>> + *               | RX (CPU->switch): forwarding port mask (if ALLOW=0)
>> + *               |                   allowance port mask (if ALLOW=1)
> 
> There are some interesting fields here. It will be interesting to see
> what we can do with the switch.

This is exactly why I asked for Realtek's permission to publish the 
details. :-)

> 
>> + */
>> +
>> +#include <linux/etherdevice.h>
>> +#include <linux/bits.h>
>> +
>> +#include "dsa_priv.h"
>> +
>> +#define RTL8_4_TAG_LEN			8
>> +#define RTL8_4_ETHERTYPE		0x8899
> 
> Please add this to include/uapi/linux/if_ether.h

I believe Realtek uses this EtherType for a bunch of unrelated 
protocols, so I'm not sure this is a good idea. See [1] for a similar 
discussion on the mailing list a while back. What do you think?

[1] 
https://lore.kernel.org/netdev/CACRpkdYQthFgjwVzHyK3DeYUOdcYyWmdjDPG=Rf9B3VrJ12Rzg@mail.gmail.com/

> 
>> +static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
>> +				      struct net_device *dev)
>> +{
>> +	__be16 *p;
>> +	u16 etype;
>> +	u8 proto;
>> +	u8 *tag;
>> +	u8 port;
>> +	u16 tmp;
>> +
>> +	if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
>> +		return NULL;
>> +
>> +	tag = dsa_etype_header_pos_rx(skb);
>> +
>> +	/* Parse Realtek EtherType */
>> +	p = (__be16 *)tag;
>> +	etype = ntohs(*p);
>> +	if (unlikely(etype != RTL8_4_ETHERTYPE)) {
>> +		netdev_dbg(dev, "non-realtek ethertype 0x%04x\n", etype);
> 
> You probably want to rate limit these sorts of prints. You have
> limited controller of what frames come in, and if somebody can craft
> bad frames, they can make you send all your time printing messages to
> the log.

OK, I think I saw some rate limited version of netdev_dbg so I'll bring 
that in for v2. Thanks for the tip.

> 
>      Andrew
> 

Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Andrew Lunn]

> >> + */
> >> +
> >> +#include <linux/etherdevice.h>
> >> +#include <linux/bits.h>
> >> +
> >> +#include "dsa_priv.h"
> >> +
> >> +#define RTL8_4_TAG_LEN			8
> >> +#define RTL8_4_ETHERTYPE		0x8899
> > 
> > Please add this to include/uapi/linux/if_ether.h

Maybe call it ETH_P_REALTEK, and comment /* Multiple Proprietary
protocols */ ?

If you do it in an individual patch, you can explain more in the
commit message about it being used for different protocols by Realtek,
and that no assumptions should be made when trying to decode it.

	  Andrew

Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Alvin Šipraga]

Hi Andrew,

On 8/23/21 1:14 AM, Andrew Lunn wrote:
>>>> + */
>>>> +
>>>> +#include <linux/etherdevice.h>
>>>> +#include <linux/bits.h>
>>>> +
>>>> +#include "dsa_priv.h"
>>>> +
>>>> +#define RTL8_4_TAG_LEN			8
>>>> +#define RTL8_4_ETHERTYPE		0x8899
>>>
>>> Please add this to include/uapi/linux/if_ether.h
> 
> Maybe call it ETH_P_REALTEK, and comment /* Multiple Proprietary
> protocols */ ?
> 
> If you do it in an individual patch, you can explain more in the
> commit message about it being used for different protocols by Realtek,
> and that no assumptions should be made when trying to decode it.

Sounds good, I'll do that in v2.

> 
> 	  Andrew
> 

Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Vladimir Oltean]

On Sun, Aug 22, 2021 at 09:31:41PM +0200, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> This commit implements a basic version of the 8 byte tag protocol used
> in the Realtek RTL8365MB-VC switch, which carries with it a protocol
> version of 0x04.
> 
> The implementation itself only handles the parsing of the EtherType
> value and Realtek protocol version, together with the source or
> destination port fields. The rest is left unimplemented for now.
> 
> The tag format is described in a confidential document provided to my
> employer - Bang & Olufsen a/s - by Realtek Semiconductor Corp.
> Permission has been granted by Realtek to publish this driver based on
> that material, together with an extract from the document describing the
> tag format and its fields.  It is hoped that this will help future
> implementors who do not have access to the material but who wish to
> extend the functionality of drivers for chips which use this protocol.
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  include/net/dsa.h    |   2 +
>  net/dsa/Kconfig      |   6 ++
>  net/dsa/Makefile     |   1 +
>  net/dsa/tag_rtl8_4.c | 178 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 187 insertions(+)
>  create mode 100644 net/dsa/tag_rtl8_4.c
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 0c2cba45fa79..6d8b5a11d99a 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -51,6 +51,7 @@ struct phylink_link_state;
>  #define DSA_TAG_PROTO_SEVILLE_VALUE		21
>  #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE		22
>  #define DSA_TAG_PROTO_SJA1110_VALUE		23
> +#define DSA_TAG_PROTO_RTL8_4_VALUE		24
>  
>  enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
> @@ -77,6 +78,7 @@ enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_OCELOT_8021Q	= DSA_TAG_PROTO_OCELOT_8021Q_VALUE,
>  	DSA_TAG_PROTO_SEVILLE		= DSA_TAG_PROTO_SEVILLE_VALUE,
>  	DSA_TAG_PROTO_SJA1110		= DSA_TAG_PROTO_SJA1110_VALUE,
> +	DSA_TAG_PROTO_RTL8_4		= DSA_TAG_PROTO_RTL8_4_VALUE,
>  };
>  
>  struct dsa_switch;
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 548285539752..470a2f3e8f75 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -99,6 +99,12 @@ config NET_DSA_TAG_RTL4_A
>  	  Realtek switches with 4 byte protocol A tags, sich as found in
>  	  the Realtek RTL8366RB.
>  
> +config NET_DSA_TAG_RTL8_4
> +	tristate "Tag driver for Realtek 8 byte protocol 4 tags"
> +	help
> +	  Say Y or M if you want to enable support for tagging frames for Realtek
> +	  switches with 8 byte protocol 4 tags, such as the Realtek RTL8365MB-VC.
> +
>  config NET_DSA_TAG_OCELOT
>  	tristate "Tag driver for Ocelot family of switches, using NPI port"
>  	depends on MSCC_OCELOT_SWITCH_LIB || \
> diff --git a/net/dsa/Makefile b/net/dsa/Makefile
> index 67ea009f242c..01282817e80e 100644
> --- a/net/dsa/Makefile
> +++ b/net/dsa/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_NET_DSA_TAG_GSWIP) += tag_gswip.o
>  obj-$(CONFIG_NET_DSA_TAG_HELLCREEK) += tag_hellcreek.o
>  obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
>  obj-$(CONFIG_NET_DSA_TAG_RTL4_A) += tag_rtl4_a.o
> +obj-$(CONFIG_NET_DSA_TAG_RTL8_4) += tag_rtl8_4.o
>  obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
>  obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
>  obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o
> diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c
> new file mode 100644
> index 000000000000..1082bafb6a2e
> --- /dev/null
> +++ b/net/dsa/tag_rtl8_4.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Handler for Realtek 8 byte switch tags
> + *
> + * Copyright (C) 2021 Alvin Šipraga <alsi@bang-olufsen.dk>
> + *
> + * NOTE: Currently only supports protocol "4" found in the RTL8365MB, hence
> + * named tag_rtl8_4.
> + *
> + * This "proprietary tag" header has the following format:
> + *
> + *  -------------------------------------------
> + *  | MAC DA | MAC SA | 8 byte tag | Type | ...
> + *  -------------------------------------------
> + *     _______________/            \______________________________________
> + *    /                                                                   \
> + *  0                                  7|8                                 15
> + *  |-----------------------------------+-----------------------------------|---
> + *  |                               (16-bit)                                | ^
> + *  |                       Realtek EtherType [0x8899]                      | |
> + *  |-----------------------------------+-----------------------------------| 8
> + *  |              (8-bit)              |              (8-bit)              |
> + *  |          Protocol [0x04]          |              REASON               | b
> + *  |-----------------------------------+-----------------------------------| y
> + *  |   (1)  | (1) | (2) |   (1)  | (3) | (1)  | (1) |    (1)    |   (5)    | t
> + *  | FID_EN |  X  | FID | PRI_EN | PRI | KEEP |  X  | LEARN_DIS |    X     | e
> + *  |-----------------------------------+-----------------------------------| s
> + *  |   (1)  |                       (15-bit)                               | |
> + *  |  ALLOW |                        TX/RX                                 | v
> + *  |-----------------------------------+-----------------------------------|---
> + *
> + * With the following field descriptions:
> + *
> + *    field      | description
> + *   ------------+-------------
> + *    Realtek    | 0x8899: indicates that this is a proprietary Realtek tag;
> + *     EtherType |         note that Realtek uses the same EtherType for
> + *               |         other incompatible tag formats (e.g. tag_rtl4_a.c)
> + *    Protocol   | 0x04: indicates that this tag conforms to this format
> + *    X          | reserved
> + *   ------------+-------------
> + *    REASON     | reason for forwarding packet to CPU
> + *    FID_EN     | 1: packet has an FID
> + *               | 0: no FID
> + *    FID        | FID of packet (if FID_EN=1)
> + *    PRI_EN     | 1: force priority of packet
> + *               | 0: don't force priority
> + *    PRI        | priority of packet (if PRI_EN=1)
> + *    KEEP       | preserve packet VLAN tag format

What does it mean to preserve packet VLAN tag format? Trying to
understand if the sane thing is to clear or set this bit. Does it mean
to strip the VLAN tag on egress if the VLAN is configured as
egress-untagged on the port?

> + *    LEARN_DIS  | don't learn the source MAC address of the packet
> + *    ALLOW      | 1: treat TX/RX field as an allowance port mask, meaning the
> + *               |    packet may only be forwarded to ports specified in the
> + *               |    mask
> + *               | 0: no allowance port mask, TX/RX field is the forwarding
> + *               |    port mask
> + *    TX/RX      | TX (switch->CPU): port number the packet was received on
> + *               | RX (CPU->switch): forwarding port mask (if ALLOW=0)
> + *               |                   allowance port mask (if ALLOW=1)
> + */
> +
> +#include <linux/etherdevice.h>
> +#include <linux/bits.h>
> +
> +#include "dsa_priv.h"
> +
> +#define RTL8_4_TAG_LEN			8
> +#define RTL8_4_ETHERTYPE		0x8899
> +/* 0x04 = RTL8365MB DSA protocol
> + */
> +#define RTL8_4_PROTOCOL_RTL8365MB	0x04
> +
> +static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb,
> +				       struct net_device *dev)
> +{
> +	struct dsa_port *dp = dsa_slave_to_port(dev);
> +	__be16 *p;
> +	u8 *tag;
> +	u16 out;
> +
> +	/* Pad out so that the (stripped) packet is at least 64 bytes long
> +	 * (including FCS), otherwise the switch will drop the packet.
> +	 * Then we need an additional 8 bytes for the Realtek tag.
> +	 */
> +	if (__skb_put_padto(skb, ETH_ZLEN + RTL8_4_TAG_LEN, false))
> +		return NULL;
> +
> +	skb_push(skb, RTL8_4_TAG_LEN);
> +
> +	dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN);
> +	tag = dsa_etype_header_pos_tx(skb);
> +
> +	/* Set Realtek EtherType */
> +	p = (__be16 *)tag;

You would have much fewer (zero) type casts if "tag" was a "__be16 *" in
the first place. Additionally, you would not need "p" and you could work
with tag[0], tag[1], tag[2], tag[3].

> +	*p = htons(RTL8_4_ETHERTYPE);
> +
> +	/* Set Protocol; zero REASON */
> +	p = (__be16 *)(tag + 2);
> +	*p = htons(RTL8_4_PROTOCOL_RTL8365MB << 8);
> +
> +	/* Zero FID_EN, FID, PRI_EN, PRI, KEEP, LEARN_DIS */

Please set LEARN_DIS. DSA has better ways to control what needs to be
learned and what doesn't. Packets injected into a standalone port
surely shouldn't have their MAC SA learned. Grep for "tx_fwd_offload" in
the kernel, see what it takes to transmit a packet with
skb->offload_fwd_mark = true, and you can clear LEARN_DIS and set ALLOW
then.

> +	p = (__be16 *)(tag + 4);
> +	*p = 0;
> +
> +	/* Zero ALLOW; set RX (CPU->switch) forwarding port mask */
> +	p = (__be16 *)(tag + 6);
> +	out = BIT(dp->index);

Set but unused variable.

> +	*p = htons(~(1 << 15) & BIT(dp->index));

I am deeply confused by this line.

~(1 << 15) is GENMASK(14, 0)
By AND-ing it with BIT(dp->index), what do you gain?

> +
> +	return skb;
> +}
> +
> +static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
> +				      struct net_device *dev)
> +{
> +	__be16 *p;
> +	u16 etype;
> +	u8 proto;
> +	u8 *tag;
> +	u8 port;
> +	u16 tmp;
> +
> +	if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
> +		return NULL;
> +
> +	tag = dsa_etype_header_pos_rx(skb);
> +
> +	/* Parse Realtek EtherType */
> +	p = (__be16 *)tag;

Same comment about it being more productive for "tag" to be "__be16 *".

> +	etype = ntohs(*p);
> +	if (unlikely(etype != RTL8_4_ETHERTYPE)) {
> +		netdev_dbg(dev, "non-realtek ethertype 0x%04x\n", etype);
> +		return NULL;
> +	}
> +
> +	/* Parse Protocol; ignore REASON */
> +	p = (__be16 *)(tag + 2);
> +	tmp = ntohs(*p);
> +	proto = tmp >> 8;
> +	if (unlikely(proto != RTL8_4_PROTOCOL_RTL8365MB)) {
> +		netdev_dbg(dev, "unknown realtek protocol 0x%02x\n", proto);
> +		return NULL;
> +	}
> +
> +	/* Ignore FID_EN, FID, PRI_EN, PRI, KEEP, LEARN_DIS */
> +	p = (__be16 *)(tag + 4);

Delete then?

> +
> +	/* Ignore ALLOW; parse TX (switch->CPU) */
> +	p = (__be16 *)(tag + 6);
> +	tmp = ntohs(*p);
> +	port = tmp & 0xf; /* Port number is the LSB 4 bits */
> +
> +	skb->dev = dsa_master_find_slave(dev, 0, port);
> +	if (!skb->dev) {
> +		netdev_dbg(dev, "could not find slave for port %d\n", port);
> +		return NULL;
> +	}
> +
> +	/* Remove tag and recalculate checksum */
> +	skb_pull_rcsum(skb, RTL8_4_TAG_LEN);
> +
> +	dsa_strip_etype_header(skb, RTL8_4_TAG_LEN);
> +
> +	skb->offload_fwd_mark = 1;

At the very least, please use

	dsa_default_offload_fwd_mark(skb);

which does the right thing when the port is not offloading the bridge.

Also tell us more about REASON and ALLOW. Is there a bit in the RX tag
which denotes that the packet was forwarded only to the host?

> +
> +	return skb;
> +}
> +
> +static const struct dsa_device_ops rtl8_4_netdev_ops = {
> +	.name = "rtl8_4",
> +	.proto = DSA_TAG_PROTO_RTL8_4,
> +	.xmit = rtl8_4_tag_xmit,
> +	.rcv = rtl8_4_tag_rcv,
> +	.needed_headroom = RTL8_4_TAG_LEN,
> +};
> +module_dsa_tag_driver(rtl8_4_netdev_ops);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL8_4);
> -- 
> 2.32.0
> 

Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Alvin Šipraga]

Hi Vladimir,

On 8/23/21 12:13 AM, Vladimir Oltean wrote:
> On Sun, Aug 22, 2021 at 09:31:41PM +0200, Alvin Šipraga wrote:
>> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>>
>> This commit implements a basic version of the 8 byte tag protocol used
>> in the Realtek RTL8365MB-VC switch, which carries with it a protocol
>> version of 0x04.
>>
>> The implementation itself only handles the parsing of the EtherType
>> value and Realtek protocol version, together with the source or
>> destination port fields. The rest is left unimplemented for now.
>>
>> The tag format is described in a confidential document provided to my
>> employer - Bang & Olufsen a/s - by Realtek Semiconductor Corp.
>> Permission has been granted by Realtek to publish this driver based on
>> that material, together with an extract from the document describing the
>> tag format and its fields.  It is hoped that this will help future
>> implementors who do not have access to the material but who wish to
>> extend the functionality of drivers for chips which use this protocol.
>>
>> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
>> ---
>>   include/net/dsa.h    |   2 +
>>   net/dsa/Kconfig      |   6 ++
>>   net/dsa/Makefile     |   1 +
>>   net/dsa/tag_rtl8_4.c | 178 +++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 187 insertions(+)
>>   create mode 100644 net/dsa/tag_rtl8_4.c
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index 0c2cba45fa79..6d8b5a11d99a 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -51,6 +51,7 @@ struct phylink_link_state;
>>   #define DSA_TAG_PROTO_SEVILLE_VALUE		21
>>   #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE		22
>>   #define DSA_TAG_PROTO_SJA1110_VALUE		23
>> +#define DSA_TAG_PROTO_RTL8_4_VALUE		24
>>   
>>   enum dsa_tag_protocol {
>>   	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
>> @@ -77,6 +78,7 @@ enum dsa_tag_protocol {
>>   	DSA_TAG_PROTO_OCELOT_8021Q	= DSA_TAG_PROTO_OCELOT_8021Q_VALUE,
>>   	DSA_TAG_PROTO_SEVILLE		= DSA_TAG_PROTO_SEVILLE_VALUE,
>>   	DSA_TAG_PROTO_SJA1110		= DSA_TAG_PROTO_SJA1110_VALUE,
>> +	DSA_TAG_PROTO_RTL8_4		= DSA_TAG_PROTO_RTL8_4_VALUE,
>>   };
>>   
>>   struct dsa_switch;
>> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
>> index 548285539752..470a2f3e8f75 100644
>> --- a/net/dsa/Kconfig
>> +++ b/net/dsa/Kconfig
>> @@ -99,6 +99,12 @@ config NET_DSA_TAG_RTL4_A
>>   	  Realtek switches with 4 byte protocol A tags, sich as found in
>>   	  the Realtek RTL8366RB.
>>   
>> +config NET_DSA_TAG_RTL8_4
>> +	tristate "Tag driver for Realtek 8 byte protocol 4 tags"
>> +	help
>> +	  Say Y or M if you want to enable support for tagging frames for Realtek
>> +	  switches with 8 byte protocol 4 tags, such as the Realtek RTL8365MB-VC.
>> +
>>   config NET_DSA_TAG_OCELOT
>>   	tristate "Tag driver for Ocelot family of switches, using NPI port"
>>   	depends on MSCC_OCELOT_SWITCH_LIB || \
>> diff --git a/net/dsa/Makefile b/net/dsa/Makefile
>> index 67ea009f242c..01282817e80e 100644
>> --- a/net/dsa/Makefile
>> +++ b/net/dsa/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_NET_DSA_TAG_GSWIP) += tag_gswip.o
>>   obj-$(CONFIG_NET_DSA_TAG_HELLCREEK) += tag_hellcreek.o
>>   obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
>>   obj-$(CONFIG_NET_DSA_TAG_RTL4_A) += tag_rtl4_a.o
>> +obj-$(CONFIG_NET_DSA_TAG_RTL8_4) += tag_rtl8_4.o
>>   obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
>>   obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
>>   obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o
>> diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c
>> new file mode 100644
>> index 000000000000..1082bafb6a2e
>> --- /dev/null
>> +++ b/net/dsa/tag_rtl8_4.c
>> @@ -0,0 +1,178 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Handler for Realtek 8 byte switch tags
>> + *
>> + * Copyright (C) 2021 Alvin Šipraga <alsi@bang-olufsen.dk>
>> + *
>> + * NOTE: Currently only supports protocol "4" found in the RTL8365MB, hence
>> + * named tag_rtl8_4.
>> + *
>> + * This "proprietary tag" header has the following format:
>> + *
>> + *  -------------------------------------------
>> + *  | MAC DA | MAC SA | 8 byte tag | Type | ...
>> + *  -------------------------------------------
>> + *     _______________/            \______________________________________
>> + *    /                                                                   \
>> + *  0                                  7|8                                 15
>> + *  |-----------------------------------+-----------------------------------|---
>> + *  |                               (16-bit)                                | ^
>> + *  |                       Realtek EtherType [0x8899]                      | |
>> + *  |-----------------------------------+-----------------------------------| 8
>> + *  |              (8-bit)              |              (8-bit)              |
>> + *  |          Protocol [0x04]          |              REASON               | b
>> + *  |-----------------------------------+-----------------------------------| y
>> + *  |   (1)  | (1) | (2) |   (1)  | (3) | (1)  | (1) |    (1)    |   (5)    | t
>> + *  | FID_EN |  X  | FID | PRI_EN | PRI | KEEP |  X  | LEARN_DIS |    X     | e
>> + *  |-----------------------------------+-----------------------------------| s
>> + *  |   (1)  |                       (15-bit)                               | |
>> + *  |  ALLOW |                        TX/RX                                 | v
>> + *  |-----------------------------------+-----------------------------------|---
>> + *
>> + * With the following field descriptions:
>> + *
>> + *    field      | description
>> + *   ------------+-------------
>> + *    Realtek    | 0x8899: indicates that this is a proprietary Realtek tag;
>> + *     EtherType |         note that Realtek uses the same EtherType for
>> + *               |         other incompatible tag formats (e.g. tag_rtl4_a.c)
>> + *    Protocol   | 0x04: indicates that this tag conforms to this format
>> + *    X          | reserved
>> + *   ------------+-------------
>> + *    REASON     | reason for forwarding packet to CPU
>> + *    FID_EN     | 1: packet has an FID
>> + *               | 0: no FID
>> + *    FID        | FID of packet (if FID_EN=1)
>> + *    PRI_EN     | 1: force priority of packet
>> + *               | 0: don't force priority
>> + *    PRI        | priority of packet (if PRI_EN=1)
>> + *    KEEP       | preserve packet VLAN tag format
> 
> What does it mean to preserve packet VLAN tag format? Trying to
> understand if the sane thing is to clear or set this bit. Does it mean
> to strip the VLAN tag on egress if the VLAN is configured as
> egress-untagged on the port?

I suppose you mean "Does it mean _don't_ strip the VLAN tag on egress..."?

I'm not sure what the semantics of this KEEP are. When I configure the 
ports to be egress-untagged, the packets leave the port untagged. When I 
configure the ports without egress-untagged, the packets leave the port 
tagged. This is with the code as you see it - so KEEP=0. If I am to 
hazard a guess, maybe it overrides any port-based egress-untagged 
setting. I will run some tests tomorrow.

> 
>> + *    LEARN_DIS  | don't learn the source MAC address of the packet
>> + *    ALLOW      | 1: treat TX/RX field as an allowance port mask, meaning the
>> + *               |    packet may only be forwarded to ports specified in the
>> + *               |    mask
>> + *               | 0: no allowance port mask, TX/RX field is the forwarding
>> + *               |    port mask
>> + *    TX/RX      | TX (switch->CPU): port number the packet was received on
>> + *               | RX (CPU->switch): forwarding port mask (if ALLOW=0)
>> + *               |                   allowance port mask (if ALLOW=1)
>> + */
>> +
>> +#include <linux/etherdevice.h>
>> +#include <linux/bits.h>
>> +
>> +#include "dsa_priv.h"
>> +
>> +#define RTL8_4_TAG_LEN			8
>> +#define RTL8_4_ETHERTYPE		0x8899
>> +/* 0x04 = RTL8365MB DSA protocol
>> + */
>> +#define RTL8_4_PROTOCOL_RTL8365MB	0x04
>> +
>> +static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb,
>> +				       struct net_device *dev)
>> +{
>> +	struct dsa_port *dp = dsa_slave_to_port(dev);
>> +	__be16 *p;
>> +	u8 *tag;
>> +	u16 out;
>> +
>> +	/* Pad out so that the (stripped) packet is at least 64 bytes long
>> +	 * (including FCS), otherwise the switch will drop the packet.
>> +	 * Then we need an additional 8 bytes for the Realtek tag.
>> +	 */
>> +	if (__skb_put_padto(skb, ETH_ZLEN + RTL8_4_TAG_LEN, false))
>> +		return NULL;
>> +
>> +	skb_push(skb, RTL8_4_TAG_LEN);
>> +
>> +	dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN);
>> +	tag = dsa_etype_header_pos_tx(skb);
>> +
>> +	/* Set Realtek EtherType */
>> +	p = (__be16 *)tag;
> 
> You would have much fewer (zero) type casts if "tag" was a "__be16 *" in
> the first place. Additionally, you would not need "p" and you could work
> with tag[0], tag[1], tag[2], tag[3].

Thanks, I'll revisit this in v2.

> 
>> +	*p = htons(RTL8_4_ETHERTYPE);
>> +
>> +	/* Set Protocol; zero REASON */
>> +	p = (__be16 *)(tag + 2);
>> +	*p = htons(RTL8_4_PROTOCOL_RTL8365MB << 8);
>> +
>> +	/* Zero FID_EN, FID, PRI_EN, PRI, KEEP, LEARN_DIS */
> 
> Please set LEARN_DIS. DSA has better ways to control what needs to be
> learned and what doesn't. Packets injected into a standalone port
> surely shouldn't have their MAC SA learned. Grep for "tx_fwd_offload" in
> the kernel, see what it takes to transmit a packet with
> skb->offload_fwd_mark = true, and you can clear LEARN_DIS and set ALLOW
> then.

OK, I'll take a look.

> 
>> +	p = (__be16 *)(tag + 4);
>> +	*p = 0;
>> +
>> +	/* Zero ALLOW; set RX (CPU->switch) forwarding port mask */
>> +	p = (__be16 *)(tag + 6);
>> +	out = BIT(dp->index);
> 
> Set but unused variable.

Thought I scrubbed this but apparently not. Will address in v2.

> 
>> +	*p = htons(~(1 << 15) & BIT(dp->index));
> 
> I am deeply confused by this line.
> 
> ~(1 << 15) is GENMASK(14, 0)
> By AND-ing it with BIT(dp->index), what do you gain?

Deliberate verbosity for the human who was engaged in writing the 
tagging driver to begin with, but obviously stupid. I'll remove.

> 
>> +
>> +	return skb;
>> +}
>> +
>> +static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
>> +				      struct net_device *dev)
>> +{
>> +	__be16 *p;
>> +	u16 etype;
>> +	u8 proto;
>> +	u8 *tag;
>> +	u8 port;
>> +	u16 tmp;
>> +
>> +	if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
>> +		return NULL;
>> +
>> +	tag = dsa_etype_header_pos_rx(skb);
>> +
>> +	/* Parse Realtek EtherType */
>> +	p = (__be16 *)tag;
> 
> Same comment about it being more productive for "tag" to be "__be16 *".

Thanks.

> 
>> +	etype = ntohs(*p);
>> +	if (unlikely(etype != RTL8_4_ETHERTYPE)) {
>> +		netdev_dbg(dev, "non-realtek ethertype 0x%04x\n", etype);
>> +		return NULL;
>> +	}
>> +
>> +	/* Parse Protocol; ignore REASON */
>> +	p = (__be16 *)(tag + 2);
>> +	tmp = ntohs(*p);
>> +	proto = tmp >> 8;
>> +	if (unlikely(proto != RTL8_4_PROTOCOL_RTL8365MB)) {
>> +		netdev_dbg(dev, "unknown realtek protocol 0x%02x\n", proto);
>> +		return NULL;
>> +	}
>> +
>> +	/* Ignore FID_EN, FID, PRI_EN, PRI, KEEP, LEARN_DIS */
>> +	p = (__be16 *)(tag + 4);
> 
> Delete then?

Deliberate verbosity again - but I figure any half-decent compiler will 
optimize this out to begin with. I thought it serves as a perfectly fine 
"add stuff here" notice together with the comment, but I can remove in v2.

> 
>> +
>> +	/* Ignore ALLOW; parse TX (switch->CPU) */
>> +	p = (__be16 *)(tag + 6);
>> +	tmp = ntohs(*p);
>> +	port = tmp & 0xf; /* Port number is the LSB 4 bits */
>> +
>> +	skb->dev = dsa_master_find_slave(dev, 0, port);
>> +	if (!skb->dev) {
>> +		netdev_dbg(dev, "could not find slave for port %d\n", port);
>> +		return NULL;
>> +	}
>> +
>> +	/* Remove tag and recalculate checksum */
>> +	skb_pull_rcsum(skb, RTL8_4_TAG_LEN);
>> +
>> +	dsa_strip_etype_header(skb, RTL8_4_TAG_LEN);
>> +
>> +	skb->offload_fwd_mark = 1;
> 
> At the very least, please use
> 
> 	dsa_default_offload_fwd_mark(skb);
> 
> which does the right thing when the port is not offloading the bridge.

Sure. Can you elaborate on what you mean by "at the very least"? Can it 
be improved even further?

> 
> Also tell us more about REASON and ALLOW. Is there a bit in the RX tag
> which denotes that the packet was forwarded only to the host?

As I wrote to Andrew, REASON is undocumented and I have not investigated 
this field yet. I have addressed ALLOW upstairs in this email, but 
suffice to say I am not sure.

> 
>> +
>> +	return skb;
>> +}
>> +
>> +static const struct dsa_device_ops rtl8_4_netdev_ops = {
>> +	.name = "rtl8_4",
>> +	.proto = DSA_TAG_PROTO_RTL8_4,
>> +	.xmit = rtl8_4_tag_xmit,
>> +	.rcv = rtl8_4_tag_rcv,
>> +	.needed_headroom = RTL8_4_TAG_LEN,
>> +};
>> +module_dsa_tag_driver(rtl8_4_netdev_ops);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL8_4);
>> -- 
>> 2.32.0
>>

Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Vladimir Oltean]

On Sun, Aug 22, 2021 at 11:11:21PM +0000, Alvin Šipraga wrote:
> >> + *    KEEP       | preserve packet VLAN tag format
> >
> > What does it mean to preserve packet VLAN tag format? Trying to
> > understand if the sane thing is to clear or set this bit. Does it mean
> > to strip the VLAN tag on egress if the VLAN is configured as
> > egress-untagged on the port?
>
> I suppose you mean "Does it mean _don't_ strip the VLAN tag on egress..."?
>
> I'm not sure what the semantics of this KEEP are. When I configure the
> ports to be egress-untagged, the packets leave the port untagged. When I
> configure the ports without egress-untagged, the packets leave the port
> tagged. This is with the code as you see it - so KEEP=0. If I am to
> hazard a guess, maybe it overrides any port-based egress-untagged
> setting. I will run some tests tomorrow.

Ok, then it makes sense to set KEEP=0 and not override the port settings.

> >
> >> +	*p = htons(~(1 << 15) & BIT(dp->index));
> >
> > I am deeply confused by this line.
> >
> > ~(1 << 15) is GENMASK(14, 0)
> > By AND-ing it with BIT(dp->index), what do you gain?
>
> Deliberate verbosity for the human who was engaged in writing the
> tagging driver to begin with, but obviously stupid. I'll remove.

I wouldn't say "stupid", but it's non-obvious, hard to read and at the same time pointless.
I had to take out the abacus to see if I'm missing something.

> >> +	/* Ignore FID_EN, FID, PRI_EN, PRI, KEEP, LEARN_DIS */
> >> +	p = (__be16 *)(tag + 4);
> >
> > Delete then?
>
> Deliberate verbosity again - but I figure any half-decent compiler will
> optimize this out to begin with. I thought it serves as a perfectly fine
> "add stuff here" notice together with the comment, but I can remove in v2.

Keeping just the comment is fine, but having the line of code is pretty
pointless. Just like any half-decent compiler will optimize it out, any
developer with half a brain will figure out what to do to parse
FID_EN ... LEARN_DIS thanks to the other comments.

> >
> >> +
> >> +	/* Ignore ALLOW; parse TX (switch->CPU) */
> >> +	p = (__be16 *)(tag + 6);
> >> +	tmp = ntohs(*p);
> >> +	port = tmp & 0xf; /* Port number is the LSB 4 bits */
> >> +
> >> +	skb->dev = dsa_master_find_slave(dev, 0, port);
> >> +	if (!skb->dev) {
> >> +		netdev_dbg(dev, "could not find slave for port %d\n", port);
> >> +		return NULL;
> >> +	}
> >> +
> >> +	/* Remove tag and recalculate checksum */
> >> +	skb_pull_rcsum(skb, RTL8_4_TAG_LEN);
> >> +
> >> +	dsa_strip_etype_header(skb, RTL8_4_TAG_LEN);
> >> +
> >> +	skb->offload_fwd_mark = 1;
> >
> > At the very least, please use
> >
> > 	dsa_default_offload_fwd_mark(skb);
> >
> > which does the right thing when the port is not offloading the bridge.
>
> Sure. Can you elaborate on what you mean by "at the very least"? Can it
> be improved even further?

The elaboration is right below. skb->offload_fwd_mark should be set to
zero for packets that have been forwarded only to the host (like packets
that have hit a trapping rule). I guess the switch will denote this
piece of info through the REASON code.

This allows the software bridge data path to know to not flood packets
that have already been flooded by the switch in its hardware data path.

Control packets can still be re-forwarded by the software data path,
even if the switch has trapped/not forwarded them, through the
"group_fwd_mask" option in "man ip-link").

> >
> > Also tell us more about REASON and ALLOW. Is there a bit in the RX tag
> > which denotes that the packet was forwarded only to the host?
>
> As I wrote to Andrew, REASON is undocumented and I have not investigated
> this field yet. I have addressed ALLOW upstairs in this email, but
> suffice to say I am not sure.

On xmit, you have. On rcv (switch->CPU), I am not sure whether the
switch will ever set ALLOW to 1, and what is the meaning of that.

Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Alvin Šipraga]

Hi Vladimir,

On 8/23/21 1:25 AM, Vladimir Oltean wrote:
> On Sun, Aug 22, 2021 at 11:11:21PM +0000, Alvin Šipraga wrote:
>>>> + *    KEEP       | preserve packet VLAN tag format
>>>
>>> What does it mean to preserve packet VLAN tag format? Trying to
>>> understand if the sane thing is to clear or set this bit. Does it mean
>>> to strip the VLAN tag on egress if the VLAN is configured as
>>> egress-untagged on the port?
>>
>> I suppose you mean "Does it mean _don't_ strip the VLAN tag on egress..."?
>>
>> I'm not sure what the semantics of this KEEP are. When I configure the
>> ports to be egress-untagged, the packets leave the port untagged. When I
>> configure the ports without egress-untagged, the packets leave the port
>> tagged. This is with the code as you see it - so KEEP=0. If I am to
>> hazard a guess, maybe it overrides any port-based egress-untagged
>> setting. I will run some tests tomorrow.
> 
> Ok, then it makes sense to set KEEP=0 and not override the port settings.

OK, glad you agree.

> 
>>>
>>>> +	*p = htons(~(1 << 15) & BIT(dp->index));
>>>
>>> I am deeply confused by this line.
>>>
>>> ~(1 << 15) is GENMASK(14, 0)
>>> By AND-ing it with BIT(dp->index), what do you gain?
>>
>> Deliberate verbosity for the human who was engaged in writing the
>> tagging driver to begin with, but obviously stupid. I'll remove.
> 
> I wouldn't say "stupid", but it's non-obvious, hard to read and at the same time pointless.
> I had to take out the abacus to see if I'm missing something.
> 
>>>> +	/* Ignore FID_EN, FID, PRI_EN, PRI, KEEP, LEARN_DIS */
>>>> +	p = (__be16 *)(tag + 4);
>>>
>>> Delete then?
>>
>> Deliberate verbosity again - but I figure any half-decent compiler will
>> optimize this out to begin with. I thought it serves as a perfectly fine
>> "add stuff here" notice together with the comment, but I can remove in v2.
> 
> Keeping just the comment is fine, but having the line of code is pretty
> pointless. Just like any half-decent compiler will optimize it out, any
> developer with half a brain will figure out what to do to parse
> FID_EN ... LEARN_DIS thanks to the other comments.

Point well made :-) I'll clean up in v2. Thanks!

> 
>>>
>>>> +
>>>> +	/* Ignore ALLOW; parse TX (switch->CPU) */
>>>> +	p = (__be16 *)(tag + 6);
>>>> +	tmp = ntohs(*p);
>>>> +	port = tmp & 0xf; /* Port number is the LSB 4 bits */
>>>> +
>>>> +	skb->dev = dsa_master_find_slave(dev, 0, port);
>>>> +	if (!skb->dev) {
>>>> +		netdev_dbg(dev, "could not find slave for port %d\n", port);
>>>> +		return NULL;
>>>> +	}
>>>> +
>>>> +	/* Remove tag and recalculate checksum */
>>>> +	skb_pull_rcsum(skb, RTL8_4_TAG_LEN);
>>>> +
>>>> +	dsa_strip_etype_header(skb, RTL8_4_TAG_LEN);
>>>> +
>>>> +	skb->offload_fwd_mark = 1;
>>>
>>> At the very least, please use
>>>
>>> 	dsa_default_offload_fwd_mark(skb);
>>>
>>> which does the right thing when the port is not offloading the bridge.
>>
>> Sure. Can you elaborate on what you mean by "at the very least"? Can it
>> be improved even further?
> 
> The elaboration is right below. skb->offload_fwd_mark should be set to
> zero for packets that have been forwarded only to the host (like packets
> that have hit a trapping rule). I guess the switch will denote this
> piece of info through the REASON code.

Yes, I think it will be communicated in REASON too. I haven't gotten to 
deciphering the contents of this field since it has not been needed so 
far: the ports are fully isolated and all bridging is done in software.

> 
> This allows the software bridge data path to know to not flood packets
> that have already been flooded by the switch in its hardware data path.
> 
> Control packets can still be re-forwarded by the software data path,
> even if the switch has trapped/not forwarded them, through the
> "group_fwd_mask" option in "man ip-link").

Since the driver doesn't support any offloading right now (ports are 
isolated), would you be OK with me just setting 
dsa_default_offload_fwd_mark(skb) for now? I will revisit this in the 
future when I have more time at work to implement some of the offloading 
features, but it's not something I can commit to in the near future.

> 
>>>
>>> Also tell us more about REASON and ALLOW. Is there a bit in the RX tag
>>> which denotes that the packet was forwarded only to the host?
>>
>> As I wrote to Andrew, REASON is undocumented and I have not investigated
>> this field yet. I have addressed ALLOW upstairs in this email, but
>> suffice to say I am not sure.
> 
> On xmit, you have. On rcv (switch->CPU), I am not sure whether the
> switch will ever set ALLOW to 1, and what is the meaning of that.

I think ALLOW is only relevant on xmit (CPU->switch). I can make it more 
clear in the comment in v2.

Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Vladimir Oltean]

On Sun, Aug 22, 2021 at 11:37:28PM +0000, Alvin Šipraga wrote:
> >>>> +	skb->offload_fwd_mark = 1;
> >>>
> >>> At the very least, please use
> >>>
> >>> 	dsa_default_offload_fwd_mark(skb);
> >>>
> >>> which does the right thing when the port is not offloading the bridge.
> >>
> >> Sure. Can you elaborate on what you mean by "at the very least"? Can it
> >> be improved even further?
> >
> > The elaboration is right below. skb->offload_fwd_mark should be set to
> > zero for packets that have been forwarded only to the host (like packets
> > that have hit a trapping rule). I guess the switch will denote this
> > piece of info through the REASON code.
>
> Yes, I think it will be communicated in REASON too. I haven't gotten to
> deciphering the contents of this field since it has not been needed so
> far: the ports are fully isolated and all bridging is done in software.

In that case, setting skb->offload_fwd_mark to true is absolutely wrong,
since the bridge is told that no software forwarding should be done
between ports, as it was already done in hardware (see nbp_switchdev_allowed_egress).

I wonder how this has ever worked? Are you completely sure that bridging
is done in software?

Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Alvin Šipraga]

On 8/23/21 1:45 AM, Vladimir Oltean wrote:
> On Sun, Aug 22, 2021 at 11:37:28PM +0000, Alvin Šipraga wrote:
>>>>>> +	skb->offload_fwd_mark = 1;
>>>>>
>>>>> At the very least, please use
>>>>>
>>>>> 	dsa_default_offload_fwd_mark(skb);
>>>>>
>>>>> which does the right thing when the port is not offloading the bridge.
>>>>
>>>> Sure. Can you elaborate on what you mean by "at the very least"? Can it
>>>> be improved even further?
>>>
>>> The elaboration is right below. skb->offload_fwd_mark should be set to
>>> zero for packets that have been forwarded only to the host (like packets
>>> that have hit a trapping rule). I guess the switch will denote this
>>> piece of info through the REASON code.
>>
>> Yes, I think it will be communicated in REASON too. I haven't gotten to
>> deciphering the contents of this field since it has not been needed so
>> far: the ports are fully isolated and all bridging is done in software.
> 
> In that case, setting skb->offload_fwd_mark to true is absolutely wrong,
> since the bridge is told that no software forwarding should be done
> between ports, as it was already done in hardware (see nbp_switchdev_allowed_egress).
> 
> I wonder how this has ever worked? Are you completely sure that bridging
> is done in software?

You are absolutely right, and indeed I checked just now and the bridging 
is not working at all.

Deleting the line (i.e. skb->offload_fwd_mark = 0) restores the expected 
bridging behaviour.

Based on what you have said, do I understand correctly that 
offload_fwd_mark shouldn't be set until bridge hardware offloading has 
been implemented?

Thanks for your detailed review so far.

Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Vladimir Oltean]

On Mon, Aug 23, 2021 at 12:28:51AM +0000, Alvin Šipraga wrote:
> On 8/23/21 1:45 AM, Vladimir Oltean wrote:
> > On Sun, Aug 22, 2021 at 11:37:28PM +0000, Alvin Šipraga wrote:
> >>>>>> +	skb->offload_fwd_mark = 1;
> >>>>>
> >>>>> At the very least, please use
> >>>>>
> >>>>> 	dsa_default_offload_fwd_mark(skb);
> >>>>>
> >>>>> which does the right thing when the port is not offloading the bridge.
> >>>>
> >>>> Sure. Can you elaborate on what you mean by "at the very least"? Can it
> >>>> be improved even further?
> >>>
> >>> The elaboration is right below. skb->offload_fwd_mark should be set to
> >>> zero for packets that have been forwarded only to the host (like packets
> >>> that have hit a trapping rule). I guess the switch will denote this
> >>> piece of info through the REASON code.
> >>
> >> Yes, I think it will be communicated in REASON too. I haven't gotten to
> >> deciphering the contents of this field since it has not been needed so
> >> far: the ports are fully isolated and all bridging is done in software.
> >
> > In that case, setting skb->offload_fwd_mark to true is absolutely wrong,
> > since the bridge is told that no software forwarding should be done
> > between ports, as it was already done in hardware (see nbp_switchdev_allowed_egress).
> >
> > I wonder how this has ever worked? Are you completely sure that bridging
> > is done in software?
>
> You are absolutely right, and indeed I checked just now and the bridging
> is not working at all.
>
> Deleting the line (i.e. skb->offload_fwd_mark = 0) restores the expected
> bridging behaviour.
>
> Based on what you have said, do I understand correctly that
> offload_fwd_mark shouldn't be set until bridge hardware offloading has
> been implemented?
>
> Thanks for your detailed review so far.

So back to my initial suggestion:

| At the very least, please use
|
| 	dsa_default_offload_fwd_mark(skb);
|
| which does the right thing when the port is not offloading the bridge.

This way, you won't have to touch this code even after you start
implementing .port_bridge_join and .port_bridge_leave. It deals with
both cases.

[RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

This patch adds a realtek-smi subdriver for the RTL8365MB-VC 4+1 port
10/100/1000M Ethernet switch controller. The driver has been developed
based on a GPL-licensed OS-agnostic Realtek vendor driver known as
rtl8367c found in the OpenWrt source tree.

Despite the name, the RTL8365MB-VC has an entirely different register
layout to the already-supported RTL8366RB ASIC. Notwithstanding this,
the structure of the rtl8365mb subdriver is based on the rtl8366rb
subdriver and makes use of the rtl8366 helper library for VLAN
configuration. Like the 'rb, it establishes its own irqchip to handle
cascaded PHY link status interrupts.

The RTL8365MB-VC is a capable switch and provides a number of offload
features that are not yet supported by this subdriver. However, the
basic functionality should be the same as that the rtl8366rb, modulo LED
support, Energy-Efficient Ethernet (EEE), and MTU configuration. Support
for these features - and more - may follow.

One more thing. Realtek's nomenclature for switches makes it hard to
know exactly what other ASICs might be supported by this driver. The
vendor driver goes by the name rtl8367c, but as far as I can tell, no
chip actually exists under this name. As such, the subdriver is named
rtl8365mb to emphasize the potentially limited support. But it is clear
from the vendor sources that a number of other more advanced switches
share a similar register layout, and further support should not be too
hard to add given access to the relevant hardware. With this in mind,
the subdriver has been written with as few assumptions about the
particular chip as is reasonable. But the RTL8365MB-VC is the only
hardware I have available, so some further work is surely needed.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
Co-developed-by: Michael Rasmussen <mir@bang-olufsen.dk>
Signed-off-by: Michael Rasmussen <mir@bang-olufsen.dk>
---
 drivers/net/dsa/Kconfig            |    1 +
 drivers/net/dsa/Makefile           |    2 +-
 drivers/net/dsa/realtek-smi-core.c |    4 +
 drivers/net/dsa/realtek-smi-core.h |    1 +
 drivers/net/dsa/rtl8365mb.c        | 2124 ++++++++++++++++++++++++++++
 5 files changed, 2131 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/dsa/rtl8365mb.c

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index a5f1aa911fe2..7b1457a6e327 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -70,6 +70,7 @@ config NET_DSA_QCA8K
 config NET_DSA_REALTEK_SMI
 	tristate "Realtek SMI Ethernet switch family support"
 	select NET_DSA_TAG_RTL4_A
+	select NET_DSA_TAG_RTL8_4
 	select FIXED_PHY
 	select IRQ_DOMAIN
 	select REALTEK_PHY
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index f3598c040994..8da1569a34e6 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -10,7 +10,7 @@ obj-$(CONFIG_NET_DSA_MT7530)	+= mt7530.o
 obj-$(CONFIG_NET_DSA_MV88E6060) += mv88e6060.o
 obj-$(CONFIG_NET_DSA_QCA8K)	+= qca8k.o
 obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
-realtek-smi-objs		:= realtek-smi-core.o rtl8366.o rtl8366rb.o
+realtek-smi-objs		:= realtek-smi-core.o rtl8366.o rtl8366rb.o rtl8365mb.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303) += lan9303-core.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303_I2C) += lan9303_i2c.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
diff --git a/drivers/net/dsa/realtek-smi-core.c b/drivers/net/dsa/realtek-smi-core.c
index 6992b6b31db6..d64f85c47891 100644
--- a/drivers/net/dsa/realtek-smi-core.c
+++ b/drivers/net/dsa/realtek-smi-core.c
@@ -490,6 +490,10 @@ static const struct of_device_id realtek_smi_of_match[] = {
 		.compatible = "realtek,rtl8366s",
 		.data = NULL,
 	},
+	{
+		.compatible = "realtek,rtl8365mb",
+		.data = &rtl8365mb_variant,
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, realtek_smi_of_match);
diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h
index 6cfa5f2df7ea..005890ff1e4b 100644
--- a/drivers/net/dsa/realtek-smi-core.h
+++ b/drivers/net/dsa/realtek-smi-core.h
@@ -144,5 +144,6 @@ int rtl8366_get_sset_count(struct dsa_switch *ds, int port, int sset);
 void rtl8366_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data);
 
 extern const struct realtek_smi_variant rtl8366rb_variant;
+extern const struct realtek_smi_variant rtl8365mb_variant;
 
 #endif /*  _REALTEK_SMI_H */
diff --git a/drivers/net/dsa/rtl8365mb.c b/drivers/net/dsa/rtl8365mb.c
new file mode 100644
index 000000000000..7698c0e3e33e
--- /dev/null
+++ b/drivers/net/dsa/rtl8365mb.c
@@ -0,0 +1,2124 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Realtek SMI subdriver for the Realtek RTL8365MB-VC Ethernet switch.
+ *
+ * Copyright (C) 2021 Alvin Šipraga <alsi@bang-olufsen.dk>
+ * Copyright (C) 2021 Michael Rasmussen <mir@bang-olufsen.dk>
+ *
+ * The RTL8365MB-VC is a 4+1 port 10/100/1000M switch controller. It includes 4
+ * integrated PHYs for the user facing ports, and an extension interface which
+ * can be connected to the CPU - or another PHY - via either MII, RMII, or
+ * RGMII. The switch is configured via the Realtek Simple Management Interface
+ * (SMI), which uses the MDIO/MDC lines.
+ *
+ * Below is a simplified block diagram of the chip and its relevant interfaces.
+ *
+ *                          .-----------------------------------.
+ *                          |                                   |
+ *         UTP <---------------> Giga PHY <-> PCS <-> P0 GMAC   |
+ *         UTP <---------------> Giga PHY <-> PCS <-> P1 GMAC   |
+ *         UTP <---------------> Giga PHY <-> PCS <-> P2 GMAC   |
+ *         UTP <---------------> Giga PHY <-> PCS <-> P3 GMAC   |
+ *                          |                                   |
+ *     CPU/PHY <-MII/RMII/RGMII--->  Extension  <---> Extension |
+ *                          |       interface 1        GMAC 1   |
+ *                          |                                   |
+ *     SMI driver/ <-MDC/SCL---> Management    ~~~~~~~~~~~~~~   |
+ *        EEPROM   <-MDIO/SDA--> interface     ~REALTEK ~~~~~   |
+ *                          |                  ~RTL8365MB ~~~   |
+ *                          |                  ~GXXXC TAIWAN~   |
+ *        GPIO <--------------> Reset          ~~~~~~~~~~~~~~   |
+ *                          |                                   |
+ *      Interrupt  <----------> Link UP/DOWN events             |
+ *      controller          |                                   |
+ *                          '-----------------------------------'
+ *
+ * The driver uses DSA to integrate the 4 user and 1 extension ports into the
+ * kernel. Netdevices are created for the user ports, as are PHY devices for
+ * their integrated PHYs. The device tree firmware should also specify the link
+ * partner of the extension port - either via a fixed-link or other phy-handle.
+ * See the device tree bindings for more detailed information. Note that the
+ * driver has only been tested with a fixed-link, but in principle it should not
+ * matter.
+ *
+ * NOTE: Currently, only the RGMII interface is implemented in this driver.
+ *
+ * The interrupt line is asserted on link UP/DOWN events. The driver creates a
+ * custom irqchip to handle this interrupt and demultiplex the events by reading
+ * the status registers via SMI. Interrupts are then propagated to the relevant
+ * PHY device.
+ *
+ * The EEPROM contains initial register values which the chip will read over I2C
+ * upon hardware reset. It is also possible to omit the EEPROM. In both cases,
+ * the driver will manually reprogram some registers using jam tables to reach
+ * an initial state defined by the vendor driver.
+ *
+ * This Linux driver is written based on an OS-agnostic vendor driver from
+ * Realtek. The reference GPL-licensed sources can be found in the OpenWrt
+ * source tree under the name rtl8367c. The vendor driver claims to support a
+ * number of similar switch controllers from Realtek, but the only hardware we
+ * have is the RTL8365MB-VC. Moreover, there does not seem to be any chip under
+ * the name RTL8367C. Although one wishes that the 'C' stood for some kind of
+ * common hardware revision, there exist examples of chips with the suffix -VC
+ * which are explicitly not supported by the rtl8367c driver and which instead
+ * require the rtl8367d vendor driver. With all this uncertainty, the driver has
+ * been modestly named rtl8365mb. Future implementors may wish to rename things
+ * accordingly.
+ *
+ * In the same family of chips, some carry up to 8 user ports and up to 2
+ * extension ports. Where possible this driver tries to make things generic, but
+ * more work must be done to support these configurations. According to
+ * documentation from Realtek, the family should include the following chips:
+ *
+ *  - RTL8363NB
+ *  - RTL8363NB-VB
+ *  - RTL8363SC
+ *  - RTL8363SC-VB
+ *  - RTL8364NB
+ *  - RTL8364NB-VB
+ *  - RTL8365MB-VC
+ *  - RTL8366SC
+ *  - RTL8367RB-VB
+ *  - RTL8367SB
+ *  - RTL8367S
+ *  - RTL8370MB
+ *  - RTL8310SR
+ *
+ * Some of the register logic for these additional chips has been skipped over
+ * while implementing this driver. It is therefore not possible to assume that
+ * things will work out-of-the-box for other chips, and a careful review of the
+ * vendor driver may be needed to expand support. The RTL8365MB-VC seems to be
+ * one of the simpler chips.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/of_irq.h>
+#include <linux/regmap.h>
+
+#include "realtek-smi-core.h"
+
+/* Port mapping macros
+ *
+ * PORT_NUM_x2y: map a port number from domain x to domain y
+ * PORT_MASK_x2y: map a port mask from domain x to domain y
+ *
+ * L = logical port domain, i.e. dsa_port.index
+ * P = physical port domain, used by the Realtek ASIC for port indexing;
+ *     for ports with internal PHYs, this is also the PHY index
+ * E = extension port domain, used by the Realtek ASIC for managing EXT ports
+ *
+ * The terminology is borrowed from the vendor driver. The extension port domain
+ * is mostly used to navigate the labyrinthine layout of EXT port configuration
+ * registers and is not considered intuitive by the author.
+ *
+ * Unless a function is accessing chip registers, it should be using the logical
+ * port domain. Moreover, function arguments for port numbers and port masks
+ * must always be in the logical domain. The conversion must be done as close as
+ * possible to the register access to avoid chaos.
+ *
+ * The mappings vary between chips in the family supported by this driver. Here
+ * is an example of the mapping for the RTL8365MB-VC:
+ *
+ *    L | P | E | remark
+ *   ---+---+---+--------
+ *    0 | 0 |   | user port
+ *    1 | 1 |   | user port
+ *    2 | 2 |   | user port
+ *    3 | 3 |   | user port
+ *    4 | 6 | 1 | extension (CPU) port
+ *
+ * NOTE: Currently this is hardcoded for the RTL8365MB-VC. This will probably
+ * require a rework when adding support for other chips.
+ */
+#define CPU_PORT_LOGICAL_NUM	4
+#define CPU_PORT_LOGICAL_MASK	BIT(CPU_PORT_LOGICAL_NUM)
+#define CPU_PORT_PHYSICAL_NUM	6
+#define CPU_PORT_PHYSICAL_MASK	BIT(CPU_PORT_PHYSICAL_NUM)
+#define CPU_PORT_EXTENSION_NUM	1
+
+#define PORT_NUM_L2P(_p)  \
+		((_p) == CPU_PORT_LOGICAL_NUM ? CPU_PORT_PHYSICAL_NUM : (_p))
+#define PORT_NUM_P2L(_p)  \
+		((_p) == CPU_PORT_PHYSICAL_NUM ? CPU_PORT_LOGICAL_NUM : (_p))
+#define PORT_NUM_L2E(_p) (CPU_PORT_EXTENSION_NUM)
+
+#define PORT_MASK_L2P(_m)                          \
+		(((_m) & ~CPU_PORT_LOGICAL_MASK) | \
+		 (((_m) & CPU_PORT_LOGICAL_MASK) ? CPU_PORT_PHYSICAL_MASK : 0))
+#define PORT_MASK_P2L(_m)                           \
+		(((_m) & ~CPU_PORT_PHYSICAL_MASK) | \
+		 (((_m) & CPU_PORT_PHYSICAL_MASK) ? CPU_PORT_LOGICAL_MASK : 0))
+
+/* Chip-specific data and limits */
+#define RTL8365MB_CHIP_ID_8365MB_VC		0x6367
+#define RTL8365MB_NUM_PORTS_8365MB_VC		5
+#define RTL8365MB_NUM_PHYS_8365MB_VC		4
+#define RTL8365MB_CPU_PORT_NUM_8365MB_VC	4
+#define RTL8365MB_CPU_PORT_MASK_8365MB_VC       BIT(4)
+#define RTL8365MB_PHY_PORT_MASK_8365MB_VC	GENMASK(3, 0)
+#define RTL8365MB_PORT_MASK_8365MB_VC                \
+		(RTL8365MB_CPU_PORT_MASK_8365MB_VC | \
+		 RTL8365MB_PHY_PORT_MASK_8365MB_VC)
+
+/* Family-specific data and limits */
+#define RTL8365MB_NUM_PHYREGS	32
+#define RTL8365MB_PHYREGMAX	(RTL8365MB_NUM_PHYREGS - 1)
+#define RTL8365MB_NUM_VLANS	32
+#define RTL8365MB_NUM_VIDS	4096
+#define RTL8365MB_VIDMAX	(RTL8365MB_NUM_VIDS - 1)
+#define RTL8365MB_NUM_EVIDS	8192
+#define RTL8365MB_EVIDMAX	(RTL8365MB_NUM_EVIDS - 1)
+#define RTL8365MB_PRIORITYMAX	7
+#define RTL8365MB_FIDMAX	15
+#define RTL8365MB_NUM_METERS	64
+#define RTL8365MB_METERMAX	(RTL8365MB_NUM_METERS - 1)
+
+/* Chip identification registers */
+#define RTL8365MB_CHIP_ID_REG		0x1300
+
+#define RTL8365MB_CHIP_VER_REG		0x1301
+
+#define RTL8365MB_MAGIC_REG		0x13C2
+#define   RTL8365MB_MAGIC_VALUE		0x0249
+
+/* Chip reset register */
+#define RTL8365MB_CHIP_RESET_REG	0x1322
+#define RTL8365MB_CHIP_RESET_SW_MASK	0x0002
+#define RTL8365MB_CHIP_RESET_HW_MASK	0x0001
+
+/* Interrupt polarity register */
+#define RTL8365MB_INTR_POLARITY_REG	0x1100
+#define   RTL8365MB_INTR_POLARITY_MASK	0x0001
+#define   RTL8365MB_INTR_POLARITY_HIGH	0
+#define   RTL8365MB_INTR_POLARITY_LOW	1
+
+/* Interrupt control register - enable or disable specific interrupt types */
+#define RTL8365MB_INTR_CTRL				0x1101
+#define   RTL8365MB_INTR_CTRL_SLIENT_START_2_MASK	0x1000
+#define   RTL8365MB_INTR_CTRL_SLIENT_START_MASK		0x800
+#define   RTL8365MB_INTR_CTRL_ACL_ACTION_MASK		0x200
+#define   RTL8365MB_INTR_CTRL_CABLE_DIAG_FIN_MASK	0x100
+#define   RTL8365MB_INTR_CTRL_INTERRUPT_8051_MASK	0x80
+#define   RTL8365MB_INTR_CTRL_LOOP_DETECTION_MASK	0x40
+#define   RTL8365MB_INTR_CTRL_GREEN_TIMER_MASK		0x20
+#define   RTL8365MB_INTR_CTRL_SPECIAL_CONGEST_MASK	0x10
+#define   RTL8365MB_INTR_CTRL_SPEED_CHANGE_MASK		0x8
+#define   RTL8365MB_INTR_CTRL_LEARN_OVER_MASK		0x4
+#define   RTL8365MB_INTR_CTRL_METER_EXCEEDED_MASK	0x2
+#define   RTL8365MB_INTR_CTRL_LINK_CHANGE_MASK		0x1
+
+
+/* Interrupt status register */
+#define RTL8365MB_INTR_STATUS_REG			0x1102
+#define   RTL8365MB_INTR_STATUS_SLIENT_START_2_MASK	0x1000
+#define   RTL8365MB_INTR_STATUS_SLIENT_START_MASK	0x800
+#define   RTL8365MB_INTR_STATUS_ACL_ACTION_MASK		0x200
+#define   RTL8365MB_INTR_STATUS_CABLE_DIAG_FIN_MASK	0x100
+#define   RTL8365MB_INTR_STATUS_INTERRUPT_8051_MASK	0x80
+#define   RTL8365MB_INTR_STATUS_LOOP_DETECTION_MASK	0x40
+#define   RTL8365MB_INTR_STATUS_GREEN_TIMER_MASK	0x20
+#define   RTL8365MB_INTR_STATUS_SPECIAL_CONGEST_MASK	0x10
+#define   RTL8365MB_INTR_STATUS_SPEED_CHANGE_MASK	0x8
+#define   RTL8365MB_INTR_STATUS_LEARN_OVER_MASK		0x4
+#define   RTL8365MB_INTR_STATUS_METER_EXCEEDED_MASK	0x2
+#define   RTL8365MB_INTR_STATUS_LINK_CHANGE_MASK	0x1
+#define   RTL8365MB_INTR_STATUS_ALL_MASK                      \
+		(RTL8365MB_INTR_STATUS_SLIENT_START_2_MASK |  \
+		 RTL8365MB_INTR_STATUS_SLIENT_START_MASK |    \
+		 RTL8365MB_INTR_STATUS_ACL_ACTION_MASK |      \
+		 RTL8365MB_INTR_STATUS_CABLE_DIAG_FIN_MASK |  \
+		 RTL8365MB_INTR_STATUS_INTERRUPT_8051_MASK |  \
+		 RTL8365MB_INTR_STATUS_LOOP_DETECTION_MASK |  \
+		 RTL8365MB_INTR_STATUS_GREEN_TIMER_MASK |     \
+		 RTL8365MB_INTR_STATUS_SPECIAL_CONGEST_MASK | \
+		 RTL8365MB_INTR_STATUS_SPEED_CHANGE_MASK |    \
+		 RTL8365MB_INTR_STATUS_LEARN_OVER_MASK |      \
+		 RTL8365MB_INTR_STATUS_METER_EXCEEDED_MASK |  \
+		 RTL8365MB_INTR_STATUS_LINK_CHANGE_MASK)
+
+/* Per-port interrupt type status registers */
+#define RTL8365MB_PORT_LINKDOWN_IND_REG		0x1106
+#define   RTL8365MB_PORT_LINKDOWN_IND_MASK	0x7FF
+
+#define RTL8365MB_PORT_LINKUP_IND_REG		0x1107
+#define   RTL8365MB_PORT_LINKUP_IND_MASK	0x7FF
+
+/* PHY indirect access registers */
+#define RTL8365MB_INDIRECT_ACCESS_CTRL_REG			0x1F00
+#define   RTL8365MB_INDIRECT_ACCESS_CTRL_RW_MASK		0x0002
+#define   RTL8365MB_INDIRECT_ACCESS_CTRL_RW_READ		0
+#define   RTL8365MB_INDIRECT_ACCESS_CTRL_RW_WRITE		1
+#define   RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_MASK		0x1
+#define   RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_VALUE		1
+#define RTL8365MB_INDIRECT_ACCESS_STATUS_REG			0x1F01
+#define RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG			0x1F02
+#define   RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_5_1_MASK	GENMASK(4, 0)
+#define   RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK		GENMASK(6, 5)
+#define   RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK	GENMASK(11, 8)
+#define   RTL8365MB_PHY_BASE					0x2000
+#define RTL8365MB_INDIRECT_ACCESS_WRITE_DATA_REG		0x1F03
+#define RTL8365MB_INDIRECT_ACCESS_READ_DATA_REG			0x1F04
+
+/* PHY OCP address prefix register */
+#define RTL8365MB_GPHY_OCP_MSB_0_REG			0x1D15
+#define   RTL8365MB_GPHY_OCP_MSB_0_CFG_CPU_OCPADR_MASK	0x0FC0
+#define RTL8365MB_PHY_OCP_ADDR_PREFIX_MASK		0xFC00
+
+/* The PHY OCP addresses of PHY registers 0~31 start here */
+#define RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE		0xA400
+
+/* EXT port interface mode values - used in DIGITAL_INTERFACE_SELECT */
+#define RTL8365MB_EXT_PORT_MODE_DISABLE		0
+#define RTL8365MB_EXT_PORT_MODE_RGMII		1
+#define RTL8365MB_EXT_PORT_MODE_MII_MAC		2
+#define RTL8365MB_EXT_PORT_MODE_MII_PHY		3
+#define RTL8365MB_EXT_PORT_MODE_TMII_MAC	4
+#define RTL8365MB_EXT_PORT_MODE_TMII_PHY	5
+#define RTL8365MB_EXT_PORT_MODE_GMII		6
+#define RTL8365MB_EXT_PORT_MODE_RMII_MAC	7
+#define RTL8365MB_EXT_PORT_MODE_RMII_PHY	8
+#define RTL8365MB_EXT_PORT_MODE_SGMII		9
+#define RTL8365MB_EXT_PORT_MODE_HSGMII		10
+#define RTL8365MB_EXT_PORT_MODE_1000X_100FX	11
+#define RTL8365MB_EXT_PORT_MODE_1000X		12
+#define RTL8365MB_EXT_PORT_MODE_100FX		13
+
+/* EXT port interface mode configuration registers 0~1 */
+#define RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0		0x1305
+#define RTL8365MB_DIGITAL_INTERFACE_SELECT_REG1		0x13C3
+#define RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(_extport)   \
+		(RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0 + \
+		 ((_extport) >> 1) * (0x13C3 - 0x1305))
+#define   RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(_extport) \
+		(0xF << (((_extport) % 2)))
+#define   RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(_extport) \
+		(((_extport) % 2) * 4)
+
+/* EXT port RGMII TX/RX delay configuration registers 1~2 */
+#define RTL8365MB_EXT_RGMXF_REG1		0x1307
+#define RTL8365MB_EXT_RGMXF_REG2		0x13C5
+#define RTL8365MB_EXT_RGMXF_REG(_extport)   \
+		(RTL8365MB_EXT_RGMXF_REG1 + \
+		 (((_extport) >> 1) * (0x13C5 - 0x1307)))
+#define   RTL8365MB_EXT_RGMXF_RXDELAY_MASK	0x0007
+#define   RTL8365MB_EXT_RGMXF_TXDELAY_MASK	0x0008
+
+/* External port speed values - used in DIGITAL_INTERFACE_FORCE */
+#define RTL8365MB_PORT_SPEED_10M	0
+#define RTL8365MB_PORT_SPEED_100M	1
+#define RTL8365MB_PORT_SPEED_1000M	2
+
+/* EXT port force configuration registers 0~2 */
+#define RTL8365MB_DIGITAL_INTERFACE_FORCE_REG0			0x1310
+#define RTL8365MB_DIGITAL_INTERFACE_FORCE_REG1			0x1311
+#define RTL8365MB_DIGITAL_INTERFACE_FORCE_REG2			0x13c4
+#define RTL8365MB_DIGITAL_INTERFACE_FORCE_REG(_extport)   \
+		(RTL8365MB_DIGITAL_INTERFACE_FORCE_REG0 + \
+		 ((_extport) & 0x1) +                     \
+		 ((((_extport) >> 1) & 0x1) * (0x13C4 - 0x1310)))
+#define   RTL8365MB_DIGITAL_INTERFACE_FORCE_EN_MASK		0x1000
+#define   RTL8365MB_DIGITAL_INTERFACE_FORCE_NWAY_MASK		0x0080
+#define   RTL8365MB_DIGITAL_INTERFACE_FORCE_TXPAUSE_MASK	0x0040
+#define   RTL8365MB_DIGITAL_INTERFACE_FORCE_RXPAUSE_MASK	0x0020
+#define   RTL8365MB_DIGITAL_INTERFACE_FORCE_LINK_MASK		0x0010
+#define   RTL8365MB_DIGITAL_INTERFACE_FORCE_DUPLEX_MASK		0x0004
+#define   RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK		0x0003
+
+/* CPU port mask register - controls which ports are treated as CPU ports */
+#define RTL8365MB_CPU_PORT_MASK_REG	0x1219
+#define   RTL8365MB_CPU_PORT_MASK_MASK	0x07FF
+
+/* CPU control register */
+#define RTL8365MB_CPU_CTRL_REG			0x121a
+#define   RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK	0x400
+#define   RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK	0x200
+#define   RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK	0x80
+#define   RTL8365MB_CPU_CTRL_TAG_POSITION_MASK	0x40
+#define   RTL8365MB_CPU_CTRL_TRAP_PORT_MASK	0x38
+#define   RTL8365MB_CPU_CTRL_INSERTMODE_MASK	0x6
+#define   RTL8365MB_CPU_CTRL_EN_MASK		0x1
+
+/* Maximum packet length register */
+#define RTL8365MB_CFG0_MAX_LEN_REG	0x088c
+#define   RTL8365MB_CFG0_MAX_LEN_MASK	0x3FFF
+
+/* Table read/write registers */
+#define RTL8365MB_TABLE_READ_BASE	0x0520
+#define RTL8365MB_TABLE_READ_REG(_x) \
+		(RTL8365MB_TABLE_READ_BASE + (_x))
+#define RTL8365MB_TABLE_WRITE_BASE	0x0510
+#define RTL8365MB_TABLE_WRITE_REG(_x) \
+		(RTL8365MB_TABLE_WRITE_BASE + (_x))
+/* The generic table size is actually 10, but the uppermost table register has
+ * space for only half a word (1 byte). There doesn't seem to be any table with
+ * such large entries, so cap it at 9 to simplify the read/write logic below.
+ */
+#define RTL8365MB_TABLE_ENTRY_SIZE	9
+
+/* Table access control register
+ * NOTE: Some tables seem to support alternative access methods. For now the
+ * method field is not used by rtl8365mb_table_{read,write}.
+ */
+#define RTL8365MB_TABLE_ACCESS_CTRL_REG				0x0500
+#define   RTL8365MB_TABLE_ACCESS_CTRL_TARGET_MASK		0x0007
+#define     RTL8365MB_TABLE_ACCESS_CTRL_TARGET_ACLRULE		1
+#define     RTL8365MB_TABLE_ACCESS_CTRL_TARGET_ACLACT		2
+#define     RTL8365MB_TABLE_ACCESS_CTRL_TARGET_CVLAN		3
+#define     RTL8365MB_TABLE_ACCESS_CTRL_TARGET_L2		4
+#define     RTL8365MB_TABLE_ACCESS_CTRL_TARGET_IGMP_GROUP	5
+#define   RTL8365MB_TABLE_ACCESS_CTRL_CMD_TYPE_MASK		0x0008
+#define     RTL8365MB_TABLE_ACCESS_CTRL_CMD_TYPE_READ		0
+#define     RTL8365MB_TABLE_ACCESS_CTRL_CMD_TYPE_WRITE		1
+#define   RTL8365MB_TABLE_ACCESS_CTRL_METHOD_MASK		0x0070
+
+/* Table access address register */
+#define RTL8365MB_TABLE_ACCESS_ADDR_REG			0x0501
+#define   RTL8365MB_TABLE_ACCESS_ADDR_MASK		0x1FFF
+
+/* Table LUT (look-up-table) address register */
+#define RTL8365MB_TABLE_LUT_ADDR_REG			0x0502
+#define   RTL8365MB_TABLE_LUT_ADDR_ADDRESS2_MASK	0x4000
+#define   RTL8365MB_TABLE_LUT_ADDR_BUSY_FLAG_MASK	0x2000
+#define   RTL8365MB_TABLE_LUT_ADDR_HIT_STATUS_MASK	0x1000
+#define   RTL8365MB_TABLE_LUT_ADDR_TYPE_MASK		0x0800
+#define   RTL8365MB_TABLE_LUT_ADDR_ADDRESS_MASK		0x07FF
+
+/* CVLAN (i.e. vlan4k) table entry layout, u16[4] */
+#define RTL8365MB_CVLAN_ENTRY_D0_MBR_MASK		0x00FF
+#define RTL8365MB_CVLAN_ENTRY_D0_UNTAG_MASK		0xFF00
+#define RTL8365MB_CVLAN_ENTRY_D1_FID_MASK		0x000F
+#define RTL8365MB_CVLAN_ENTRY_D1_VBPEN_MASK		0x0010
+#define RTL8365MB_CVLAN_ENTRY_D1_VBPRI_MASK		0x00E0
+#define RTL8365MB_CVLAN_ENTRY_D1_ENVLANPOL_MASK		0x0100
+#define RTL8365MB_CVLAN_ENTRY_D1_METERIDX_MASK		0x3E00
+#define RTL8365MB_CVLAN_ENTRY_D1_IVL_SVL_MASK		0x4000
+#define RTL8365MB_CVLAN_ENTRY_D2_MBR_EXT_MASK		0x0007
+#define RTL8365MB_CVLAN_ENTRY_D2_UNTAG_EXT_MASK		0x0038
+#define RTL8365MB_CVLAN_ENTRY_D2_METERIDX_EXT_MASK	0x0040
+
+/* Port isolation (forwarding mask) registers */
+#define RTL8365MB_PORT_ISOLATION_REG_BASE		0x08A2
+#define RTL8365MB_PORT_ISOLATION_REG(_physport) \
+		(RTL8365MB_PORT_ISOLATION_REG_BASE + (_physport))
+#define   RTL8365MB_PORT_ISOLATION_MASK			0x07FF
+
+/* Port-based VID registers 0~5 - each one holds an MC index for two ports */
+#define RTL8365MB_VLAN_PVID_CTRL_BASE			0x0700
+
+#define RTL8365MB_VLAN_PVID_CTRL_REG(_physport) \
+		(RTL8365MB_VLAN_PVID_CTRL_BASE + ((_physport) >> 1))
+#define   RTL8365MB_VLAN_PVID_CTRL_PORT0_VIDX_MASK	0x001F
+#define   RTL8365MB_VLAN_PVID_CTRL_PORT1_VIDX_MASK	0x1F00
+#define   RTL8365MB_VLAN_PVID_CTRL_PORT_VIDX_OFFSET(_physport) \
+		(((_physport) & 1) << 3)
+#define   RTL8365MB_VLAN_PVID_CTRL_PORT_VIDX_MASK(_physport) \
+		(0x1F << RTL8365MB_VLAN_PVID_CTRL_PORT_VIDX_OFFSET(_physport))
+
+/* VLAN control register */
+#define RTL8365MB_VLAN_CTRL_REG			0x07A8
+#define   RTL8365MB_VLAN_CTRL_EN_MASK		0x0001
+
+/* VLAN ingress filter register - enable ingress filter on masked ports */
+#define RTL8365MB_VLAN_INGRESS_REG		0x07A9
+#define   RTL8365MB_VLAN_INGRESS_FILTER_PORT_EN_OFFSET(_physport) (_physport)
+#define   RTL8365MB_VLAN_INGRESS_FILTER_PORT_EN_MASK(_physport) BIT(_physport)
+
+/* VLAN member configuration registers 0~31, u16[3] */
+#define RTL8365MB_VLAN_MC_BASE			0x0728
+#define RTL8365MB_VLAN_MC_REG(_x)  \
+		(RTL8365MB_VLAN_MC_BASE + (_x) * 4)
+#define   RTL8365MB_VLAN_MC_D0_MBR_MASK			0x07FF
+#define   RTL8365MB_VLAN_MC_D1_FID_MASK			0x000F
+#define   RTL8365MB_VLAN_MC_D2_METERIDX_MASK		0x07E0
+#define   RTL8365MB_VLAN_MC_D2_ENVLANPOL_MASK		0x0010
+#define   RTL8365MB_VLAN_MC_D2_VBPRI_MASK		0x000E
+#define   RTL8365MB_VLAN_MC_D2_VBPEN_MASK		0x0001
+#define   RTL8365MB_VLAN_MC_D3_EVID_MASK		0x1FFF
+
+enum rtl8365mb_table {
+	RTL8365MB_TABLE_ACLRULE = RTL8365MB_TABLE_ACCESS_CTRL_TARGET_ACLRULE,
+	RTL8365MB_TABLE_ACLACT = RTL8365MB_TABLE_ACCESS_CTRL_TARGET_ACLACT,
+	RTL8365MB_TABLE_CVLAN = RTL8365MB_TABLE_ACCESS_CTRL_TARGET_CVLAN,
+	RTL8365MB_TABLE_L2 = RTL8365MB_TABLE_ACCESS_CTRL_TARGET_L2,
+	RTL8365MB_TABLE_IGMP_GROUP =
+		RTL8365MB_TABLE_ACCESS_CTRL_TARGET_IGMP_GROUP,
+};
+
+/* MIB counter value registers */
+#define RTL8365MB_MIB_COUNTER_BASE	0x1000
+#define RTL8365MB_MIB_COUNTER_REG(_x)	(RTL8365MB_MIB_COUNTER_BASE + (_x))
+
+/* MIB counter address register */
+#define RTL8365MB_MIB_ADDRESS_REG		0x1004
+#define   RTL8365MB_MIB_ADDRESS_PORT_OFFSET	0x7C
+#define   RTL8365MB_MIB_ADDRESS(_p, _x) \
+		(((RTL8365MB_MIB_ADDRESS_PORT_OFFSET) * (_p) + (_x)) >> 2)
+
+#define RTL8365MB_MIB_CTRL0_REG			0x1005
+#define   RTL8365MB_MIB_CTRL0_RESET_FLAG_MASK	0x2
+#define   RTL8365MB_MIB_CTRL0_BUSY_FLAG_MASK	0x0001
+
+static struct rtl8366_mib_counter rtl8365mb_mib_counters[] = {
+	{ 0, 0, 4, "ifInOctets" },
+	{ 0, 4, 2, "dot3StatsFCSErrors" },
+	{ 0, 6, 2, "dot3StatsSymbolErrors" },
+	{ 0, 8, 2, "dot3InPauseFrames" },
+	{ 0, 10, 2, "dot3ControlInUnknownOpcodes" },
+	{ 0, 12, 2, "etherStatsFragments" },
+	{ 0, 14, 2, "etherStatsJabbers" },
+	{ 0, 16, 2, "ifInUcastPkts" },
+	{ 0, 18, 2, "etherStatsDropEvents" },
+	{ 0, 20, 2, "ifInMulticastPkts" },
+	{ 0, 22, 2, "ifInBroadcastPkts" },
+	{ 0, 24, 2, "inMldChecksumError" },
+	{ 0, 26, 2, "inIgmpChecksumError" },
+	{ 0, 28, 2, "inMldSpecificQuery" },
+	{ 0, 30, 2, "inMldGeneralQuery" },
+	{ 0, 32, 2, "inIgmpSpecificQuery" },
+	{ 0, 34, 2, "inIgmpGeneralQuery" },
+	{ 0, 36, 2, "inMldLeaves" },
+	{ 0, 38, 2, "inIgmpLeaves" },
+	{ 0, 40, 4, "etherStatsOctets" },
+	{ 0, 44, 2, "etherStatsUnderSizePkts" },
+	{ 0, 46, 2, "etherOversizeStats" },
+	{ 0, 48, 2, "etherStatsPkts64Octets" },
+	{ 0, 50, 2, "etherStatsPkts65to127Octets" },
+	{ 0, 52, 2, "etherStatsPkts128to255Octets" },
+	{ 0, 54, 2, "etherStatsPkts256to511Octets" },
+	{ 0, 56, 2, "etherStatsPkts512to1023Octets" },
+	{ 0, 58, 2, "etherStatsPkts1024to1518Octets" },
+	{ 0, 60, 4, "ifOutOctets" },
+	{ 0, 64, 2, "dot3StatsSingleCollisionFrames" },
+	{ 0, 66, 2, "dot3StatMultipleCollisionFrames" },
+	{ 0, 68, 2, "dot3sDeferredTransmissions" },
+	{ 0, 70, 2, "dot3StatsLateCollisions" },
+	{ 0, 72, 2, "etherStatsCollisions" },
+	{ 0, 74, 2, "dot3StatsExcessiveCollisions" },
+	{ 0, 76, 2, "dot3OutPauseFrames" },
+	{ 0, 78, 2, "ifOutDiscards" },
+	{ 0, 80, 2, "dot1dTpPortInDiscards" },
+	{ 0, 82, 2, "ifOutUcastPkts" },
+	{ 0, 84, 2, "ifOutMulticastPkts" },
+	{ 0, 86, 2, "ifOutBroadcastPkts" },
+	{ 0, 88, 2, "outOampduPkts" },
+	{ 0, 90, 2, "inOampduPkts" },
+	{ 0, 92, 4, "inIgmpJoinsSuccess" },
+	{ 0, 96, 2, "inIgmpJoinsFail" },
+	{ 0, 98, 2, "inMldJoinsSuccess" },
+	{ 0, 100, 2, "inMldJoinsFail" },
+	{ 0, 102, 2, "inReportSuppressionDrop" },
+	{ 0, 104, 2, "inLeaveSuppressionDrop" },
+	{ 0, 106, 2, "outIgmpReports" },
+	{ 0, 108, 2, "outIgmpLeaves" },
+	{ 0, 110, 2, "outIgmpGeneralQuery" },
+	{ 0, 112, 2, "outIgmpSpecificQuery" },
+	{ 0, 114, 2, "outMldReports" },
+	{ 0, 116, 2, "outMldLeaves" },
+	{ 0, 118, 2, "outMldGeneralQuery" },
+	{ 0, 120, 2, "outMldSpecificQuery" },
+	{ 0, 122, 2, "inKnownMulticastPkts" },
+};
+
+struct rtl8365mb_vlan_mc {
+	u16 evid;
+	u16 member;
+	u8 fid;
+	u8 priority;
+	u8 priority_en : 1;
+	u8 policing_en : 1;
+	u16 meteridx;
+};
+
+struct rtl8365mb_vlan_4k {
+	u16 vid;
+	u16 member;
+	u16 untag;
+	u8 fid;
+	u8 priority;
+	u8 priority_en : 1;
+	u8 policing_en : 1;
+	u8 ivl_en : 1;
+	u8 meteridx;
+};
+
+struct rtl8365mb_jam_tbl_entry {
+	u16 reg;
+	u16 val;
+};
+
+/* Lifted from the vendor driver sources */
+static const struct rtl8365mb_jam_tbl_entry rtl8365mb_init_jam_8365mb_vc[] = {
+	{ 0x13EB, 0x15BB }, { 0x1303, 0x06D6 }, { 0x1304, 0x0700 },
+	{ 0x13E2, 0x003F }, { 0x13F9, 0x0090 }, { 0x121E, 0x03CA },
+	{ 0x1233, 0x0352 }, { 0x1237, 0x00A0 }, { 0x123A, 0x0030 },
+	{ 0x1239, 0x0084 }, { 0x0301, 0x1000 }, { 0x1349, 0x001F },
+	{ 0x18E0, 0x4004 }, { 0x122B, 0x241C }, { 0x1305, 0xC000 },
+	{ 0x13F0, 0x0000 },
+};
+
+static const struct rtl8365mb_jam_tbl_entry rtl8365mb_init_jam_common[] = {
+	{ 0x1200, 0x7FCB }, { 0x0884, 0x0003 }, { 0x06EB, 0x0001 },
+	{ 0x03Fa, 0x0007 }, { 0x08C8, 0x00C0 }, { 0x0A30, 0x020E },
+	{ 0x0800, 0x0000 }, { 0x0802, 0x0000 }, { 0x09DA, 0x0013 },
+	{ 0x1D32, 0x0002 },
+};
+
+enum rtl8365mb_cpu_insert {
+	RTL8365MB_CPU_INSERT_TO_ALL = 0,
+	RTL8365MB_CPU_INSERT_TO_TRAPPING = 1,
+	RTL8365MB_CPU_INSERT_TO_NONE = 2,
+};
+
+enum rtl8365mb_cpu_position {
+	RTL8365MB_CPU_POS_AFTER_SA = 0,
+	RTL8365MB_CPU_POS_BEFORE_CRC = 1,
+};
+
+enum rtl8365mb_cpu_format {
+	RTL8365MB_CPU_FORMAT_8BYTES = 0,
+	RTL8365MB_CPU_FORMAT_4BYTES = 1,
+};
+
+enum rtl8365mb_cpu_rxlen {
+	RTL8365MB_CPU_RXLEN_72BYTES = 0,
+	RTL8365MB_CPU_RXLEN_64BYTES = 1,
+};
+
+struct rtl8365mb_cpu {
+	bool enable; /* enable CPU tagging */
+	u32 mask; /* port mask of ports that should parse CPU tags */
+	u32 trap_port; /* forward trapped frames to this port */
+	enum rtl8365mb_cpu_insert insert; /* tag insertion mode */
+	enum rtl8365mb_cpu_position position; /* position of CPU tag in frame */
+	enum rtl8365mb_cpu_rxlen rx_length; /* minimum CPU RX length */
+	enum rtl8365mb_cpu_format format; /* CPU tag format */
+};
+
+/* Private chip-specific driver data */
+struct rtl8365mb {
+	int irq; /* registered IRQ or zero */
+	u32 chip_id;
+	u32 chip_ver;
+	unsigned int num_phys; /* number of integrated PHYs */
+	u32 port_mask; /* mask of all ports */
+	u32 phy_port_mask; /* mask of ports with integrated PHYs */
+	struct rtl8365mb_cpu cpu; /* CPU configuration */
+	const struct rtl8365mb_jam_tbl_entry *jam_table;
+	size_t jam_size;
+};
+
+static int rtl8365mb_table_poll_busy(struct realtek_smi *smi)
+{
+	u32 val;
+
+	return regmap_read_poll_timeout(
+		smi->map, RTL8365MB_TABLE_LUT_ADDR_REG, val,
+		(val & RTL8365MB_TABLE_LUT_ADDR_BUSY_FLAG_MASK) == 0, 10, 100);
+}
+
+static int rtl8365mb_table_read(struct realtek_smi *smi,
+				enum rtl8365mb_table target, u32 addr,
+				u16 *data, size_t size)
+{
+	u32 val;
+	int ret;
+	int i;
+
+	if (!FIELD_FIT(RTL8365MB_TABLE_ACCESS_ADDR_MASK, addr))
+		return -EINVAL;
+
+	if (size > RTL8365MB_TABLE_ENTRY_SIZE)
+		return -E2BIG;
+
+	ret = rtl8365mb_table_poll_busy(smi);
+	if (ret)
+		return ret;
+
+	/* Set read address */
+	ret = regmap_write(smi->map, RTL8365MB_TABLE_ACCESS_ADDR_REG,
+			   FIELD_PREP(RTL8365MB_TABLE_ACCESS_ADDR_MASK, addr));
+	if (ret)
+		return ret;
+
+	/* Execute read operation */
+	ret = regmap_write_bits(
+		smi->map, RTL8365MB_TABLE_ACCESS_CTRL_REG,
+		RTL8365MB_TABLE_ACCESS_CTRL_TARGET_MASK |
+			RTL8365MB_TABLE_ACCESS_CTRL_CMD_TYPE_MASK,
+		FIELD_PREP(RTL8365MB_TABLE_ACCESS_CTRL_TARGET_MASK, target) |
+			FIELD_PREP(RTL8365MB_TABLE_ACCESS_CTRL_CMD_TYPE_MASK,
+				   RTL8365MB_TABLE_ACCESS_CTRL_CMD_TYPE_READ));
+	if (ret)
+		return ret;
+
+	ret = rtl8365mb_table_poll_busy(smi);
+	if (ret)
+		return ret;
+
+	/* Read table entry */
+	for (i = 0; i < size; i++) {
+		ret = regmap_read(smi->map, RTL8365MB_TABLE_READ_REG(i), &val);
+		if (ret)
+			return ret;
+
+		data[i] = val & 0xFFFF;
+	}
+
+	return 0;
+}
+
+static int rtl8365mb_table_write(struct realtek_smi *smi,
+				 enum rtl8365mb_table target, u32 addr,
+				 u16 *data, size_t size)
+{
+	int ret;
+	int i;
+
+	if (!FIELD_FIT(RTL8365MB_TABLE_ACCESS_ADDR_MASK, addr))
+		return -EINVAL;
+
+	if (size > RTL8365MB_TABLE_ENTRY_SIZE)
+		return -E2BIG;
+
+	/* Write table entry */
+	for (i = 0; i < size; i++) {
+		ret = regmap_write(smi->map, RTL8365MB_TABLE_WRITE_REG(i),
+				   data[i]);
+		if (ret)
+			return ret;
+	}
+
+	/* Set write address */
+	ret = regmap_write(smi->map, RTL8365MB_TABLE_ACCESS_ADDR_REG,
+			   FIELD_PREP(RTL8365MB_TABLE_ACCESS_ADDR_MASK, addr));
+	if (ret)
+		return ret;
+
+	/* Execute write operation */
+	ret = regmap_write_bits(
+		smi->map, RTL8365MB_TABLE_ACCESS_CTRL_REG,
+		RTL8365MB_TABLE_ACCESS_CTRL_TARGET_MASK |
+			RTL8365MB_TABLE_ACCESS_CTRL_CMD_TYPE_MASK,
+		FIELD_PREP(RTL8365MB_TABLE_ACCESS_CTRL_TARGET_MASK, target) |
+			FIELD_PREP(RTL8365MB_TABLE_ACCESS_CTRL_CMD_TYPE_MASK,
+				   RTL8365MB_TABLE_ACCESS_CTRL_CMD_TYPE_WRITE));
+
+	return ret;
+}
+
+static int rtl8365mb_phy_poll_busy(struct realtek_smi *smi)
+{
+	u32 val;
+
+	return regmap_read_poll_timeout(smi->map,
+					RTL8365MB_INDIRECT_ACCESS_STATUS_REG,
+					val, val == 0, 10, 100);
+}
+
+static int rtl8365mb_phy_ocp_prepare(struct realtek_smi *smi, int phy,
+				     u32 ocp_addr)
+{
+	u32 val;
+	int ret;
+
+	/* Set OCP prefix */
+	val = FIELD_GET(RTL8365MB_PHY_OCP_ADDR_PREFIX_MASK, ocp_addr);
+	ret = regmap_update_bits(
+		smi->map, RTL8365MB_GPHY_OCP_MSB_0_REG,
+		RTL8365MB_GPHY_OCP_MSB_0_CFG_CPU_OCPADR_MASK,
+		FIELD_PREP(RTL8365MB_GPHY_OCP_MSB_0_CFG_CPU_OCPADR_MASK, val));
+	if (ret)
+		return ret;
+
+	/* Set PHY register address */
+	val = RTL8365MB_PHY_BASE;
+	val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK, phy);
+	val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_5_1_MASK,
+			  ocp_addr >> 1);
+	val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK,
+			  ocp_addr >> 6);
+	ret = regmap_write(smi->map, RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG,
+			   val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rtl8365mb_phy_ocp_read(struct realtek_smi *smi, int phy,
+				  u32 ocp_addr, u16 *data)
+{
+	u32 val;
+	int ret;
+
+	ret = rtl8365mb_phy_poll_busy(smi);
+	if (ret)
+		return ret;
+
+	ret = rtl8365mb_phy_ocp_prepare(smi, phy, ocp_addr);
+	if (ret)
+		return ret;
+
+	/* Execute read operation */
+	val = FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_MASK,
+			 RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_VALUE) |
+	      FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_RW_MASK,
+			 RTL8365MB_INDIRECT_ACCESS_CTRL_RW_READ);
+	ret = regmap_write(smi->map, RTL8365MB_INDIRECT_ACCESS_CTRL_REG, val);
+	if (ret)
+		return ret;
+
+	ret = rtl8365mb_phy_poll_busy(smi);
+	if (ret)
+		return ret;
+
+	/* Get PHY register data */
+	ret = regmap_read(smi->map, RTL8365MB_INDIRECT_ACCESS_READ_DATA_REG,
+			  &val);
+	if (ret)
+		return ret;
+
+	*data = val & 0xFFFF;
+
+	return 0;
+}
+
+static int rtl8365mb_phy_ocp_write(struct realtek_smi *smi, int phy,
+				   u32 ocp_addr, u16 data)
+{
+	u32 val;
+	int ret;
+
+	ret = rtl8365mb_phy_poll_busy(smi);
+	if (ret)
+		return ret;
+
+	ret = rtl8365mb_phy_ocp_prepare(smi, phy, ocp_addr);
+	if (ret)
+		return ret;
+
+	/* Set PHY register data */
+	ret = regmap_write(smi->map, RTL8365MB_INDIRECT_ACCESS_WRITE_DATA_REG,
+			   data);
+	if (ret)
+		return ret;
+
+	/* Execute write operation */
+	val = FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_MASK,
+			 RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_VALUE) |
+	      FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_RW_MASK,
+			 RTL8365MB_INDIRECT_ACCESS_CTRL_RW_WRITE);
+	ret = regmap_write(smi->map, RTL8365MB_INDIRECT_ACCESS_CTRL_REG, val);
+	if (ret)
+		return ret;
+
+	ret = rtl8365mb_phy_poll_busy(smi);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rtl8365mb_phy_read(struct realtek_smi *smi, int phy, int regnum)
+{
+	struct rtl8365mb *mb = smi->chip_data;
+	u32 ocp_addr;
+	u16 val;
+	int ret;
+
+	if (phy >= mb->num_phys || regnum > RTL8365MB_PHYREGMAX)
+		return -EINVAL;
+
+	ocp_addr = RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE + regnum * 2;
+
+	ret = rtl8365mb_phy_ocp_read(smi, phy, ocp_addr, &val);
+	if (ret) {
+		dev_err(smi->dev,
+			"failed to read PHY%d reg %02x @ %04x, ret %d\n", phy,
+			regnum, ocp_addr, ret);
+		return ret;
+	}
+
+	dev_dbg(smi->dev, "read PHY%d register 0x%02x @ %04x, val <- %04x\n",
+		phy, regnum, ocp_addr, val);
+
+	return val;
+}
+
+static int rtl8365mb_phy_write(struct realtek_smi *smi, int phy, int regnum,
+			       u16 val)
+{
+	struct rtl8365mb *mb = smi->chip_data;
+	u32 ocp_addr;
+	int ret;
+
+	if (phy >= mb->num_phys || regnum > RTL8365MB_PHYREGMAX)
+		return -EINVAL;
+
+	ocp_addr = RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE + regnum * 2;
+
+	ret = rtl8365mb_phy_ocp_write(smi, phy, ocp_addr, val);
+	if (ret) {
+		dev_err(smi->dev,
+			"failed to write PHY%d reg %02x @ %04x, ret %d\n", phy,
+			regnum, ocp_addr, ret);
+		return ret;
+	}
+
+	dev_dbg(smi->dev, "write PHY%d register 0x%02x @ %04x, val -> %04x\n",
+		phy, regnum, ocp_addr, val);
+
+	return 0;
+}
+
+static enum dsa_tag_protocol
+rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
+			   enum dsa_tag_protocol mp)
+{
+	return DSA_TAG_PROTO_RTL8_4;
+}
+
+static int rtl8365mb_ext_config_rgmii(struct realtek_smi *smi, int port,
+				      phy_interface_t interface)
+{
+	int tx_delay = 0;
+	int rx_delay = 0;
+	int ext_port;
+	int ret;
+
+	if (port == smi->cpu_port) {
+		ext_port = PORT_NUM_L2E(port);
+	} else {
+		dev_err(smi->dev, "only one EXT port is currently supported\n");
+		return -EINVAL;
+	}
+
+	/* Set the RGMII TX/RX delay
+	 *
+	 * The Realtek vendor driver indicates the following possible
+	 * configuration settings:
+	 *
+	 *   TX delay:
+	 *     0 = no delay, 1 = 2 ns delay
+	 *   RX delay:
+	 *     0 = no delay, 7 = maximum delay
+	 *     No units are specified, but there are a total of 8 steps.
+	 *
+	 * The vendor driver also states that this must be configured *before*
+	 * forcing the external interface into a particular mode, which is done
+	 * in the rtl8365mb_phylink_mac_link_{up,down} functions.
+	 *
+	 * NOTE: For now this is hardcoded to tx_delay = 1, rx_delay = 4.
+	 */
+	if (interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    interface == PHY_INTERFACE_MODE_RGMII_TXID)
+		tx_delay = 1; /* 2 ns */
+
+	if (interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    interface == PHY_INTERFACE_MODE_RGMII_RXID)
+		rx_delay = 4;
+
+	ret = regmap_update_bits(
+		smi->map, RTL8365MB_EXT_RGMXF_REG(ext_port),
+		RTL8365MB_EXT_RGMXF_TXDELAY_MASK |
+			RTL8365MB_EXT_RGMXF_RXDELAY_MASK,
+		FIELD_PREP(RTL8365MB_EXT_RGMXF_TXDELAY_MASK, tx_delay) |
+			FIELD_PREP(RTL8365MB_EXT_RGMXF_RXDELAY_MASK, rx_delay));
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(
+		smi->map, RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(ext_port),
+		RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(ext_port),
+		RTL8365MB_EXT_PORT_MODE_RGMII
+			<< RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(
+				   ext_port));
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rtl8365mb_ext_config_forcemode(struct realtek_smi *smi, int port,
+					  bool link, int speed, int duplex,
+					  bool tx_pause, bool rx_pause)
+{
+	u32 r_tx_pause;
+	u32 r_rx_pause;
+	u32 r_duplex;
+	u32 r_speed;
+	u32 r_link;
+	int ext_port;
+	int val;
+	int ret;
+
+	if (port == smi->cpu_port) {
+		ext_port = PORT_NUM_L2E(port);
+	} else {
+		dev_err(smi->dev, "only one EXT port is currently supported\n");
+		return -EINVAL;
+	}
+
+	if (link) {
+		/* Force the link up with the desired configuration */
+		r_link = 1;
+		r_rx_pause = rx_pause ? 1 : 0;
+		r_tx_pause = tx_pause ? 1 : 0;
+
+		if (speed == SPEED_1000) {
+			r_speed = RTL8365MB_PORT_SPEED_1000M;
+		} else if (speed == SPEED_100) {
+			r_speed = RTL8365MB_PORT_SPEED_100M;
+		} else if (speed == SPEED_10) {
+			r_speed = RTL8365MB_PORT_SPEED_10M;
+		} else {
+			dev_err(smi->dev, "unsupported port speed %s\n",
+				phy_speed_to_str(speed));
+			return -EINVAL;
+		}
+
+		if (duplex == DUPLEX_FULL) {
+			r_duplex = 1;
+		} else if (duplex == DUPLEX_HALF) {
+			r_duplex = 0;
+		} else {
+			dev_err(smi->dev, "unsupported duplex %s\n",
+				phy_duplex_to_str(duplex));
+			return -EINVAL;
+		}
+	} else {
+		/* Force the link down and reset any programmed configuration */
+		r_link = 0;
+		r_tx_pause = 0;
+		r_rx_pause = 0;
+		r_speed = 0;
+		r_duplex = 0;
+	}
+
+	val = FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_EN_MASK, 1) |
+	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_TXPAUSE_MASK,
+			 r_tx_pause) |
+	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_RXPAUSE_MASK,
+			 r_rx_pause) |
+	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_LINK_MASK, r_link) |
+	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_DUPLEX_MASK,
+			 r_duplex) |
+	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK, r_speed);
+	ret = regmap_write(
+		smi->map, RTL8365MB_DIGITAL_INTERFACE_FORCE_REG(ext_port), val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
+					 phy_interface_t interface)
+{
+	if (dsa_is_user_port(ds, port) &&
+	    (interface == PHY_INTERFACE_MODE_NA ||
+	     interface == PHY_INTERFACE_MODE_INTERNAL))
+		/* Internal PHY */
+		return true;
+	else if (dsa_is_cpu_port(ds, port) &&
+		 phy_interface_mode_is_rgmii(interface))
+		/* Extension MAC */
+		return true;
+
+	return false;
+}
+
+static void rtl8365mb_phylink_validate(struct dsa_switch *ds, int port,
+				       unsigned long *supported,
+				       struct phylink_link_state *state)
+{
+	struct realtek_smi *smi = ds->priv;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0 };
+
+	/* include/linux/phylink.h says:
+	 *     When @state->interface is %PHY_INTERFACE_MODE_NA, phylink
+	 *     expects the MAC driver to return all supported link modes.
+	 */
+	if (state->interface != PHY_INTERFACE_MODE_NA &&
+	    !rtl8365mb_phy_mode_supported(ds, port, state->interface)) {
+		dev_err(smi->dev, "phy mode %s is unsupported on port %d\n",
+			phy_modes(state->interface), port);
+		linkmode_zero(supported);
+		return;
+	}
+
+	phylink_set_port_modes(mask);
+
+	phylink_set(mask, Autoneg);
+	phylink_set(mask, Pause);
+	phylink_set(mask, Asym_Pause);
+
+	phylink_set(mask, 10baseT_Half);
+	phylink_set(mask, 10baseT_Full);
+	phylink_set(mask, 100baseT_Half);
+	phylink_set(mask, 100baseT_Full);
+	phylink_set(mask, 1000baseT_Full);
+	phylink_set(mask, 1000baseT_Half);
+
+	linkmode_and(supported, supported, mask);
+	linkmode_and(state->advertising, state->advertising, mask);
+}
+
+static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
+					 unsigned int mode,
+					 const struct phylink_link_state *state)
+{
+	struct realtek_smi *smi = ds->priv;
+	int ret;
+
+	if (!rtl8365mb_phy_mode_supported(ds, port, state->interface)) {
+		dev_err(smi->dev, "phy mode %s is unsupported on port %d\n",
+			phy_modes(state->interface), port);
+		return;
+	}
+
+	/* If port MAC is connected to an internal PHY, we have nothing to do */
+	if (dsa_is_user_port(ds, port))
+		return;
+
+	if (mode != MLO_AN_PHY && mode != MLO_AN_FIXED) {
+		dev_err(smi->dev,
+			"port %d supports only conventional PHY or fixed-link\n",
+			port);
+		return;
+	}
+
+	if (phy_interface_mode_is_rgmii(state->interface)) {
+		ret = rtl8365mb_ext_config_rgmii(smi, port, state->interface);
+		if (ret)
+			dev_err(smi->dev,
+				"failed to configure RGMII mode on port %d: %d\n",
+				port, ret);
+		return;
+	}
+
+	/* TODO: Implement MII and RMII modes, which the RTL8365MB-VC also
+	 * supports
+	 */
+}
+
+static void rtl8365mb_phylink_mac_link_down(struct dsa_switch *ds, int port,
+					    unsigned int mode,
+					    phy_interface_t interface)
+{
+	struct realtek_smi *smi = ds->priv;
+	int ret;
+
+	if (dsa_is_cpu_port(ds, port)) {
+		ret = rtl8365mb_ext_config_forcemode(smi, port, false, 0, 0,
+						     false, false);
+		if (ret)
+			dev_err(smi->dev,
+				"failed to reset forced mode on port %d: %d\n",
+				port, ret);
+
+		return;
+	}
+}
+
+static void rtl8365mb_phylink_mac_link_up(struct dsa_switch *ds, int port,
+					  unsigned int mode,
+					  phy_interface_t interface,
+					  struct phy_device *phydev, int speed,
+					  int duplex, bool tx_pause,
+					  bool rx_pause)
+{
+	struct realtek_smi *smi = ds->priv;
+	int ret;
+
+	if (dsa_is_cpu_port(ds, port)) {
+		ret = rtl8365mb_ext_config_forcemode(
+			smi, port, true, speed, duplex, tx_pause, rx_pause);
+		if (ret)
+			dev_err(smi->dev,
+				"failed to force mode on port %d: %d\n", port,
+				ret);
+
+		return;
+	}
+}
+
+static int rtl8365mb_port_enable(struct dsa_switch *ds, int port,
+				 struct phy_device *phy)
+{
+	struct realtek_smi *smi = ds->priv;
+	int val;
+
+	if (dsa_is_user_port(ds, port)) {
+		/* Power up the internal PHY and restart autonegotiation */
+		val = rtl8365mb_phy_read(smi, port, MII_BMCR);
+		if (val < 0)
+			return val;
+
+		val &= ~BMCR_PDOWN;
+		val |= BMCR_ANRESTART;
+
+		return rtl8365mb_phy_write(smi, port, MII_BMCR, val);
+	}
+
+	return 0;
+}
+
+static void rtl8365mb_port_disable(struct dsa_switch *ds, int port)
+{
+	struct realtek_smi *smi = ds->priv;
+	int val;
+
+	if (dsa_is_user_port(ds, port)) {
+		/* Power down the internal PHY */
+		val = rtl8365mb_phy_read(smi, port, MII_BMCR);
+		if (val < 0)
+			return;
+
+		val |= BMCR_PDOWN;
+
+		rtl8365mb_phy_write(smi, port, MII_BMCR, val);
+
+		return;
+	}
+}
+
+int rtl8365mb_port_vlan_filtering(struct dsa_switch *ds, int port,
+				  bool vlan_filtering,
+				  struct netlink_ext_ack *extack)
+{
+	struct realtek_smi *smi = ds->priv;
+	u32 phys_port = PORT_NUM_L2P(port);
+	int ret;
+
+	dev_info(smi->dev, "%s filtering on port %d\n",
+		 vlan_filtering ? "enable" : "disable", port);
+
+	/* vlan_filtering on: Discard VLAN tagged frames if the port is not a
+	 * member of the VLAN with which the packet is associated. Untagged
+	 * packets should also be discarded unless the port has a PVID
+	 * programmed.
+	 *
+	 * vlan_filtering off: Accept all VLAN tagged frames, including
+	 * untagged.
+	 */
+	ret = regmap_update_bits(
+		smi->map, RTL8365MB_VLAN_INGRESS_REG,
+		RTL8365MB_VLAN_INGRESS_FILTER_PORT_EN_MASK(phys_port),
+		(vlan_filtering ? 1 : 0)
+			<< RTL8365MB_VLAN_INGRESS_FILTER_PORT_EN_OFFSET(
+				   phys_port));
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int _rtl8365mb_get_vlan_4k(struct realtek_smi *smi, u32 vid,
+				  struct rtl8365mb_vlan_4k *vlan4k)
+{
+	u32 phys_member;
+	u32 phys_untag;
+	u16 data[3];
+	int ret;
+
+	if (vid > RTL8365MB_VIDMAX)
+		return -EINVAL;
+
+	ret = rtl8365mb_table_read(smi, RTL8365MB_TABLE_CVLAN, vid, data,
+				   ARRAY_SIZE(data));
+	if (ret)
+		return ret;
+
+	memset(vlan4k, 0, sizeof(*vlan4k));
+
+	/* Unpack table entry */
+	vlan4k->vid = vid;
+	phys_member = FIELD_GET(RTL8365MB_CVLAN_ENTRY_D0_MBR_MASK, data[0]) |
+		      (FIELD_GET(RTL8365MB_CVLAN_ENTRY_D2_MBR_EXT_MASK, data[2])
+		       << 8);
+	phys_untag =
+		FIELD_GET(RTL8365MB_CVLAN_ENTRY_D0_UNTAG_MASK, data[0]) |
+		(FIELD_GET(RTL8365MB_CVLAN_ENTRY_D2_UNTAG_EXT_MASK, data[2])
+		 << 8);
+	vlan4k->member = PORT_MASK_P2L(phys_member);
+	vlan4k->untag = PORT_MASK_P2L(phys_untag);
+	vlan4k->fid = FIELD_GET(RTL8365MB_CVLAN_ENTRY_D1_FID_MASK, data[1]);
+	vlan4k->priority_en =
+		FIELD_GET(RTL8365MB_CVLAN_ENTRY_D1_VBPEN_MASK, data[1]);
+	vlan4k->priority =
+		FIELD_GET(RTL8365MB_CVLAN_ENTRY_D1_VBPRI_MASK, data[1]);
+	vlan4k->policing_en =
+		FIELD_GET(RTL8365MB_CVLAN_ENTRY_D1_ENVLANPOL_MASK, data[1]);
+	vlan4k->meteridx =
+		FIELD_GET(RTL8365MB_CVLAN_ENTRY_D1_METERIDX_MASK, data[1]) |
+		(FIELD_GET(RTL8365MB_CVLAN_ENTRY_D2_METERIDX_EXT_MASK, data[2])
+		 << 5);
+	vlan4k->ivl_en =
+		FIELD_GET(RTL8365MB_CVLAN_ENTRY_D1_IVL_SVL_MASK, data[1]);
+
+	return 0;
+}
+
+static int rtl8365mb_get_vlan_4k(struct realtek_smi *smi, u32 vid,
+				 struct rtl8366_vlan_4k *vlan4k)
+{
+	struct rtl8365mb_vlan_4k vlan4k_65mb;
+	int ret;
+
+	ret = _rtl8365mb_get_vlan_4k(smi, vid, &vlan4k_65mb);
+	if (ret)
+		return ret;
+
+	memset(vlan4k, 0, sizeof(*vlan4k));
+
+	vlan4k->vid = vlan4k_65mb.vid;
+	vlan4k->untag = vlan4k_65mb.untag;
+	vlan4k->member = vlan4k_65mb.member;
+	vlan4k->fid = vlan4k_65mb.fid;
+
+	return 0;
+}
+
+static int _rtl8365mb_set_vlan_4k(struct realtek_smi *smi,
+				  const struct rtl8365mb_vlan_4k *vlan4k)
+{
+	struct rtl8365mb *mb = smi->chip_data;
+	u16 data[3] = { 0 };
+	u32 phys_member;
+	u32 phys_untag;
+
+	if ((vlan4k->member & ~(mb->port_mask)) ||
+	    (vlan4k->untag & ~(mb->port_mask)) ||
+	    vlan4k->vid > RTL8365MB_VIDMAX || vlan4k->fid > RTL8365MB_FIDMAX ||
+	    vlan4k->priority > RTL8365MB_PRIORITYMAX ||
+	    vlan4k->meteridx > RTL8365MB_METERMAX)
+		return -EINVAL;
+
+	phys_member = PORT_MASK_L2P(vlan4k->member);
+	phys_untag = PORT_MASK_L2P(vlan4k->untag);
+
+	/* Pack table entry value */
+	data[0] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D0_MBR_MASK, phys_member);
+	data[0] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D0_UNTAG_MASK, phys_untag);
+	data[1] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D1_FID_MASK, vlan4k->fid);
+	data[1] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D1_VBPEN_MASK,
+			      vlan4k->priority_en);
+	data[1] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D1_VBPRI_MASK,
+			      vlan4k->priority);
+	data[1] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D1_ENVLANPOL_MASK,
+			      vlan4k->policing_en);
+	data[1] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D1_METERIDX_MASK,
+			      vlan4k->meteridx);
+	data[1] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D1_IVL_SVL_MASK,
+			      vlan4k->ivl_en);
+	data[2] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D2_MBR_EXT_MASK,
+			      phys_member >> 8);
+	data[2] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D2_UNTAG_EXT_MASK,
+			      phys_untag >> 8);
+	data[2] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D2_METERIDX_EXT_MASK,
+			      vlan4k->meteridx >> 5);
+
+	return rtl8365mb_table_write(smi, RTL8365MB_TABLE_CVLAN, vlan4k->vid,
+				     data, ARRAY_SIZE(data));
+}
+
+static int rtl8365mb_set_vlan_4k(struct realtek_smi *smi,
+				 const struct rtl8366_vlan_4k *vlan4k)
+{
+	struct rtl8365mb_vlan_4k vlan4k_65mb = { 0 };
+
+	/* The RTL8365MB-VC has a more granular VLAN configuration than what is
+	 * offered by the RTL8366x helper library. Extend this as you wish.
+	 */
+	vlan4k_65mb.vid = vlan4k->vid;
+	vlan4k_65mb.member = vlan4k->member;
+	vlan4k_65mb.untag = vlan4k->untag;
+	vlan4k_65mb.fid = vlan4k->fid;
+
+	return _rtl8365mb_set_vlan_4k(smi, &vlan4k_65mb);
+}
+
+static int _rtl8365mb_get_vlan_mc(struct realtek_smi *smi, u32 index,
+				  struct rtl8365mb_vlan_mc *vlanmc)
+{
+	u32 phys_member;
+	u16 data[4];
+	u32 val;
+	int ret;
+	int i;
+
+	if (index >= smi->num_vlan_mc)
+		return -EINVAL;
+
+	for (i = 0; i < 4; i++) {
+		ret = regmap_read(smi->map, RTL8365MB_VLAN_MC_REG(index) + i,
+				  &val);
+		if (ret)
+			return ret;
+
+		data[i] = val & 0xFFFF;
+	}
+
+	memset(vlanmc, 0, sizeof(*vlanmc));
+
+	phys_member = FIELD_GET(RTL8365MB_VLAN_MC_D0_MBR_MASK, data[0]);
+	vlanmc->member = PORT_MASK_P2L(phys_member);
+	vlanmc->fid = FIELD_GET(RTL8365MB_VLAN_MC_D1_FID_MASK, data[1]);
+	vlanmc->meteridx =
+		FIELD_GET(RTL8365MB_VLAN_MC_D2_METERIDX_MASK, data[2]);
+	vlanmc->policing_en =
+		FIELD_GET(RTL8365MB_VLAN_MC_D2_ENVLANPOL_MASK, data[2]);
+	vlanmc->priority = FIELD_GET(RTL8365MB_VLAN_MC_D2_VBPRI_MASK, data[2]);
+	vlanmc->priority_en =
+		FIELD_GET(RTL8365MB_VLAN_MC_D2_VBPEN_MASK, data[2]);
+	vlanmc->evid = FIELD_GET(RTL8365MB_VLAN_MC_D3_EVID_MASK, data[3]);
+
+	return 0;
+}
+
+static int rtl8365mb_get_vlan_mc(struct realtek_smi *smi, u32 index,
+				 struct rtl8366_vlan_mc *vlanmc)
+{
+	struct rtl8365mb_vlan_mc vlanmc_65mb;
+	struct rtl8365mb_vlan_4k vlan4k_65mb;
+	int ret;
+
+	ret = _rtl8365mb_get_vlan_mc(smi, index, &vlanmc_65mb);
+	if (ret)
+		return ret;
+
+	/* The untag field is not stored in the membership config, so grab it
+	 * from the vlan4k table instead. This is mostly for compatibility with
+	 * the RTL8366x helper library.
+	 */
+	ret = _rtl8365mb_get_vlan_4k(smi, vlanmc_65mb.evid, &vlan4k_65mb);
+	if (ret)
+		return ret;
+
+	memset(vlanmc, 0, sizeof(*vlanmc));
+
+	vlanmc->vid = vlanmc_65mb.evid;
+	vlanmc->untag = vlan4k_65mb.untag;
+	vlanmc->member = vlanmc_65mb.member;
+	vlanmc->fid = vlanmc_65mb.fid;
+	vlanmc->priority = vlanmc_65mb.priority;
+
+	return 0;
+}
+
+static int _rtl8365mb_set_vlan_mc(struct realtek_smi *smi, u32 index,
+				  const struct rtl8365mb_vlan_mc *vlanmc)
+{
+	struct rtl8365mb *mb = smi->chip_data;
+	u16 data[4] = { 0 };
+	u32 phys_member;
+	int ret;
+	int i;
+
+	if (index >= smi->num_vlan_mc || (vlanmc->member & ~(mb->port_mask)) ||
+	    vlanmc->evid > RTL8365MB_EVIDMAX ||
+	    vlanmc->fid > RTL8365MB_FIDMAX ||
+	    vlanmc->priority > RTL8365MB_PRIORITYMAX ||
+	    vlanmc->meteridx > RTL8365MB_METERMAX)
+		return -EINVAL;
+
+	phys_member = PORT_MASK_L2P(vlanmc->member);
+
+	data[0] |= FIELD_PREP(RTL8365MB_VLAN_MC_D0_MBR_MASK, phys_member);
+	data[1] |= FIELD_PREP(RTL8365MB_VLAN_MC_D1_FID_MASK, vlanmc->fid);
+	data[2] |= FIELD_PREP(RTL8365MB_VLAN_MC_D2_METERIDX_MASK,
+			      vlanmc->meteridx);
+	data[2] |= FIELD_PREP(RTL8365MB_VLAN_MC_D2_ENVLANPOL_MASK,
+			      vlanmc->policing_en);
+	data[2] |=
+		FIELD_PREP(RTL8365MB_VLAN_MC_D2_VBPRI_MASK, vlanmc->priority);
+	data[2] |= FIELD_PREP(RTL8365MB_VLAN_MC_D2_VBPEN_MASK,
+			      vlanmc->priority_en);
+	data[3] |= FIELD_PREP(RTL8365MB_VLAN_MC_D3_EVID_MASK, vlanmc->evid);
+
+	for (i = 0; i < 4; i++) {
+		ret = regmap_write(smi->map, RTL8365MB_VLAN_MC_REG(index) + i,
+				   data[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int rtl8365mb_set_vlan_mc(struct realtek_smi *smi, u32 index,
+				 const struct rtl8366_vlan_mc *vlanmc)
+{
+	struct rtl8365mb_vlan_mc vlanmc_65mb = { 0 };
+
+	vlanmc_65mb.evid = vlanmc->vid;
+	/* The untag field is set in the corresponding vlan4k entry */
+	vlanmc_65mb.member = vlanmc->member;
+	vlanmc_65mb.fid = vlanmc->fid;
+	vlanmc_65mb.priority = vlanmc->priority;
+
+	return _rtl8365mb_set_vlan_mc(smi, index, &vlanmc_65mb);
+}
+
+static int rtl8365mb_get_mc_index(struct realtek_smi *smi, int port, int *index)
+{
+	int phys_port = PORT_NUM_L2P(port);
+	u32 val;
+	int ret;
+
+	if (port >= smi->num_ports || *index >= smi->num_vlan_mc)
+		return -EINVAL;
+
+	ret = regmap_read(smi->map, RTL8365MB_VLAN_PVID_CTRL_REG(phys_port),
+			  &val);
+	if (ret)
+		return ret;
+
+	*index = (val & (RTL8365MB_VLAN_PVID_CTRL_PORT_VIDX_MASK(phys_port))) >>
+		 RTL8365MB_VLAN_PVID_CTRL_PORT_VIDX_OFFSET(phys_port);
+
+	return 0;
+}
+
+static int rtl8365mb_set_mc_index(struct realtek_smi *smi, int port, int index)
+{
+	int phys_port = PORT_NUM_L2P(port);
+
+	if (port >= smi->num_ports || index >= smi->num_vlan_mc)
+		return -EINVAL;
+
+	return regmap_update_bits(
+		smi->map, RTL8365MB_VLAN_PVID_CTRL_REG(phys_port),
+		RTL8365MB_VLAN_PVID_CTRL_PORT_VIDX_MASK(phys_port),
+		index << RTL8365MB_VLAN_PVID_CTRL_PORT_VIDX_OFFSET(phys_port));
+}
+
+static int rtl8365mb_ports_isolate(struct realtek_smi *smi)
+{
+	u32 phys_mask = PORT_MASK_L2P(BIT(smi->cpu_port));
+	int ret;
+	int i;
+
+	/* Isolate all ports by setting the forwarding mask for each port to
+	 * BIT(cpu_port). The switch will then forward all frames from the user
+	 * ports to the CPU port only.
+	 *
+	 * For the CPU port, the forwarding mask is effectively zero. The switch
+	 * will then reject all non-CPU tagged frames from the CPU port, and
+	 * honour the TX mask in all CPU tagged frames from the CPU port
+	 * irrespective of the forwarding mask.
+	 */
+	for (i = 0; i < smi->num_ports; i++) {
+		u32 phys_port = PORT_NUM_L2P(i);
+
+		ret = regmap_write(smi->map,
+				   RTL8365MB_PORT_ISOLATION_REG(phys_port),
+				   phys_mask);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int rtl8365mb_get_mib_counter(struct realtek_smi *smi, int port,
+				     struct rtl8366_mib_counter *mib,
+				     u64 *mibvalue)
+{
+	int phys_port = PORT_NUM_L2P(port);
+	u32 offset;
+	u32 val;
+	int ret;
+	int i;
+
+	/* The MIB address is an SRAM address. We request a particular address
+	 * and then poll the control register before reading the value from some
+	 * counter registers.
+	 */
+	ret = regmap_write(smi->map, RTL8365MB_MIB_ADDRESS_REG,
+			   RTL8365MB_MIB_ADDRESS(phys_port, mib->offset));
+	if (ret)
+		return ret;
+
+	/* Poll for completion */
+	ret = regmap_read_poll_timeout(
+		smi->map, RTL8365MB_MIB_CTRL0_REG, val,
+		(val & RTL8365MB_MIB_CTRL0_BUSY_FLAG_MASK) == 0, 10, 100);
+	if (ret)
+		return ret;
+
+	/* Presumably this indicates a MIB counter read failure */
+	if (val & RTL8365MB_MIB_CTRL0_RESET_FLAG_MASK)
+		return -EIO;
+
+	/* There are four MIB counter registers each holding a 16 bit word of a
+	 * MIB counter. Depending on the offset, we should read from the upper
+	 * two or lower two registers. In case the MIB counter is 4 words, we
+	 * read from all four registers.
+	 */
+	if (mib->length == 4)
+		offset = 3;
+	else
+		offset = (mib->offset + 1) % 4;
+
+	/* Read the MIB counter 16 bits at a time */
+	*mibvalue = 0;
+	for (i = 0; i < mib->length; i++) {
+		ret = regmap_read(smi->map,
+				  RTL8365MB_MIB_COUNTER_REG(offset - i), &val);
+		if (ret)
+			return ret;
+
+		*mibvalue = ((*mibvalue) << 16) | (val & 0xFFFF);
+	}
+
+	return 0;
+}
+
+static bool rtl8365mb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
+{
+	if (vlan > RTL8365MB_VIDMAX)
+		return false;
+
+	return true;
+}
+
+static int rtl8365mb_enable_vlan(struct realtek_smi *smi, bool enable)
+{
+	dev_dbg(smi->dev, "%s VLAN\n", enable ? "enable" : "disable");
+	return regmap_update_bits(
+		smi->map, RTL8365MB_VLAN_CTRL_REG, RTL8365MB_VLAN_CTRL_EN_MASK,
+		FIELD_PREP(RTL8365MB_VLAN_CTRL_EN_MASK, enable ? 1 : 0));
+}
+
+static int rtl8365mb_enable_vlan4k(struct realtek_smi *smi, bool enable)
+{
+	return rtl8365mb_enable_vlan(smi, enable);
+}
+
+static int rtl8365mb_get_and_clear_status_reg(struct realtek_smi *smi, u32 reg,
+					      u32 *val)
+{
+	int ret;
+
+	ret = regmap_read(smi->map, reg, val);
+	if (ret)
+		return ret;
+
+	return regmap_write(smi->map, reg, *val);
+}
+
+static irqreturn_t rtl8365mb_irq(int irq, void *data)
+{
+	struct realtek_smi *smi = data;
+	struct rtl8365mb *mb;
+	u32 line_changes = 0;
+	u32 linkdown_ind;
+	u32 linkup_ind;
+	u32 stat;
+	u32 val;
+	int ret;
+
+	mb = smi->chip_data;
+
+	ret = rtl8365mb_get_and_clear_status_reg(smi, RTL8365MB_INTR_STATUS_REG,
+						 &stat);
+	if (ret)
+		goto out_error;
+
+	if (stat & RTL8365MB_INTR_STATUS_LINK_CHANGE_MASK) {
+		ret = rtl8365mb_get_and_clear_status_reg(
+			smi, RTL8365MB_PORT_LINKUP_IND_REG, &val);
+		if (ret)
+			goto out_error;
+
+		linkup_ind = FIELD_GET(RTL8365MB_PORT_LINKUP_IND_MASK, val);
+
+		ret = rtl8365mb_get_and_clear_status_reg(
+			smi, RTL8365MB_PORT_LINKDOWN_IND_REG, &val);
+		if (ret)
+			goto out_error;
+
+		linkdown_ind = FIELD_GET(RTL8365MB_PORT_LINKDOWN_IND_MASK, val);
+
+		line_changes = PORT_MASK_P2L(linkup_ind | linkdown_ind) &
+			       mb->phy_port_mask;
+	}
+
+	if (!line_changes)
+		goto out_none;
+
+	while (line_changes) {
+		int line = __ffs(line_changes);
+		int child_irq;
+
+		line_changes &= ~BIT(line);
+
+		child_irq = irq_find_mapping(smi->irqdomain, line);
+		handle_nested_irq(child_irq);
+	}
+
+	return IRQ_HANDLED;
+
+out_error:
+	dev_err(smi->dev, "failed to read interrupt status: %d\n", ret);
+
+out_none:
+	return IRQ_NONE;
+}
+
+static struct irq_chip rtl8365mb_irq_chip = {
+	.name = "rtl8365mb",
+	/* The hardware doesn't support masking IRQs on a per-port basis */
+};
+
+static int rtl8365mb_irq_map(struct irq_domain *domain, unsigned int irq,
+			     irq_hw_number_t hwirq)
+{
+	irq_set_chip_data(irq, domain->host_data);
+	irq_set_chip_and_handler(irq, &rtl8365mb_irq_chip, handle_simple_irq);
+	irq_set_nested_thread(irq, 1);
+	irq_set_noprobe(irq);
+
+	return 0;
+}
+
+static void rtl8365mb_irq_unmap(struct irq_domain *d, unsigned int irq)
+{
+	irq_set_nested_thread(irq, 0);
+	irq_set_chip_and_handler(irq, NULL, NULL);
+	irq_set_chip_data(irq, NULL);
+}
+
+static const struct irq_domain_ops rtl8365mb_irqdomain_ops = {
+	.map = rtl8365mb_irq_map,
+	.unmap = rtl8365mb_irq_unmap,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static int _rtl8365mb_irq_enable(struct realtek_smi *smi, bool enable)
+{
+	return regmap_update_bits(
+		smi->map, RTL8365MB_INTR_CTRL,
+		RTL8365MB_INTR_CTRL_LINK_CHANGE_MASK,
+		FIELD_PREP(RTL8365MB_INTR_CTRL_LINK_CHANGE_MASK,
+			   enable ? 1 : 0));
+}
+
+static int rtl8365mb_irq_enable(struct realtek_smi *smi)
+{
+	return _rtl8365mb_irq_enable(smi, true);
+}
+
+static int rtl8365mb_irq_disable(struct realtek_smi *smi)
+{
+	return _rtl8365mb_irq_enable(smi, false);
+}
+
+static int rtl8365mb_irq_setup(struct realtek_smi *smi)
+{
+	struct rtl8365mb *mb = smi->chip_data;
+	struct device_node *intc;
+	u32 irq_trig;
+	int virq;
+	int irq;
+	u32 val;
+	int ret;
+	int i;
+
+	intc = of_get_child_by_name(smi->dev->of_node, "interrupt-controller");
+	if (!intc) {
+		dev_err(smi->dev, "missing child interrupt-controller node\n");
+		return -EINVAL;
+	}
+
+	smi->irqdomain = irq_domain_add_linear(intc, mb->num_phys,
+					       &rtl8365mb_irqdomain_ops, smi);
+	if (!smi->irqdomain) {
+		dev_err(smi->dev, "failed to add irq domain\n");
+		ret = -ENOMEM;
+		goto out_put_node;
+	}
+
+	for (i = 0; i < mb->num_phys; i++) {
+		virq = irq_create_mapping(smi->irqdomain, i);
+		if (!virq) {
+			dev_err(smi->dev,
+				"failed to create irq domain mapping\n");
+			ret = -EINVAL;
+			goto out_remove_irqdomain;
+		}
+
+		irq_set_parent(virq, irq);
+	}
+
+	/* rtl8365mb IRQs cascade off this one */
+	irq = of_irq_get(intc, 0);
+	if (irq <= 0) {
+		if (irq != -EPROBE_DEFER)
+			dev_err(smi->dev, "failed to get parent irq: %d\n",
+				irq);
+		ret = irq ? irq : -EINVAL;
+		goto out_remove_irqdomain;
+	}
+
+	/* Configure chip interrupt signal polarity */
+	irq_trig = irqd_get_trigger_type(irq_get_irq_data(irq));
+	switch (irq_trig) {
+	case IRQF_TRIGGER_RISING:
+	case IRQF_TRIGGER_HIGH:
+		val = RTL8365MB_INTR_POLARITY_HIGH;
+		break;
+	case IRQF_TRIGGER_FALLING:
+	case IRQF_TRIGGER_LOW:
+		val = RTL8365MB_INTR_POLARITY_LOW;
+		break;
+	default:
+		dev_err(smi->dev, "unsupported irq trigger type %u\n",
+			irq_trig);
+		ret = -EINVAL;
+		goto out_remove_irqdomain;
+	}
+
+	ret = regmap_update_bits(smi->map, RTL8365MB_INTR_POLARITY_REG,
+				 RTL8365MB_INTR_POLARITY_MASK,
+				 FIELD_PREP(RTL8365MB_INTR_POLARITY_MASK, val));
+	if (ret)
+		goto out_remove_irqdomain;
+
+	/* Disable the interrupt in case the chip has it enabled on reset */
+	ret = rtl8365mb_irq_disable(smi);
+	if (ret)
+		goto out_remove_irqdomain;
+
+	/* Clear the interrupt status register */
+	ret = regmap_write(smi->map, RTL8365MB_INTR_STATUS_REG,
+			   RTL8365MB_INTR_STATUS_ALL_MASK);
+	if (ret)
+		goto out_remove_irqdomain;
+
+	ret = request_threaded_irq(irq, NULL, rtl8365mb_irq, IRQF_ONESHOT,
+				   "rtl8365mb", smi);
+	if (ret) {
+		dev_err(smi->dev, "failed to request irq: %d\n", ret);
+		goto out_remove_irqdomain;
+	}
+
+	/* Store the irq so that we know to free it during teardown */
+	mb->irq = irq;
+
+	ret = rtl8365mb_irq_enable(smi);
+	if (ret)
+		goto out_free_irq;
+
+	of_node_put(intc);
+
+	return 0;
+
+out_free_irq:
+	free_irq(mb->irq, smi);
+	mb->irq = 0;
+
+out_remove_irqdomain:
+	for (i = 0; i < mb->num_phys; i++) {
+		virq = irq_find_mapping(smi->irqdomain, i);
+		irq_dispose_mapping(virq);
+	}
+
+	irq_domain_remove(smi->irqdomain);
+	smi->irqdomain = NULL;
+
+out_put_node:
+	of_node_put(intc);
+
+	return ret;
+}
+
+static void rtl8365mb_irq_teardown(struct realtek_smi *smi)
+{
+	struct rtl8365mb *mb = smi->chip_data;
+	int virq;
+	int i;
+
+	if (mb->irq) {
+		free_irq(mb->irq, smi);
+		mb->irq = 0;
+	}
+
+	if (smi->irqdomain) {
+		for (i = 0; i < mb->num_phys; i++) {
+			virq = irq_find_mapping(smi->irqdomain, i);
+			irq_dispose_mapping(virq);
+		}
+
+		irq_domain_remove(smi->irqdomain);
+		smi->irqdomain = NULL;
+	}
+}
+
+static int rtl8365mb_cpu_config(struct realtek_smi *smi)
+{
+	struct rtl8365mb *mb = smi->chip_data;
+	struct rtl8365mb_cpu *cpu = &mb->cpu;
+	u32 phys_mask;
+	u32 phys_trap_port;
+	u32 val;
+	int ret;
+
+	phys_mask = PORT_MASK_L2P(cpu->mask);
+	phys_trap_port = PORT_NUM_L2P(cpu->trap_port);
+
+	ret = regmap_update_bits(smi->map, RTL8365MB_CPU_PORT_MASK_REG,
+				 RTL8365MB_CPU_PORT_MASK_MASK,
+				 FIELD_PREP(RTL8365MB_CPU_PORT_MASK_MASK,
+					    phys_mask));
+	if (ret)
+		return ret;
+
+	val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) |
+	      FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) |
+	      FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) |
+	      FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) |
+	      FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) |
+	      FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, phys_trap_port) |
+	      FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
+			 phys_trap_port >> 3);
+	ret = regmap_write(smi->map, RTL8365MB_CPU_CTRL_REG, val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rtl8365mb_switch_init(struct realtek_smi *smi)
+{
+	struct rtl8365mb *mb = smi->chip_data;
+	int ret;
+	int i;
+
+	/* Do any chip-specific init jam before getting to the common stuff */
+	if (mb->jam_table) {
+		for (i = 0; i < mb->jam_size; i++) {
+			ret = regmap_write(smi->map, mb->jam_table[i].reg,
+					   mb->jam_table[i].val);
+			if (ret)
+				return ret;
+		}
+	}
+
+	/* Common init jam */
+	for (i = 0; i < ARRAY_SIZE(rtl8365mb_init_jam_common); i++) {
+		ret = regmap_write(smi->map, rtl8365mb_init_jam_common[i].reg,
+				   rtl8365mb_init_jam_common[i].val);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int rtl8365mb_reset_chip(struct realtek_smi *smi)
+{
+	u32 val;
+
+	realtek_smi_write_reg_noack(smi, RTL8365MB_CHIP_RESET_REG,
+				    FIELD_PREP(RTL8365MB_CHIP_RESET_HW_MASK,
+					       1));
+
+	/* Realtek documentation says the chip needs 1 second to reset. Sleep
+	 * for 100 ms before accessing any registers to prevent ACK timeouts.
+	 */
+	msleep(100);
+	return regmap_read_poll_timeout(
+		smi->map, RTL8365MB_CHIP_RESET_REG, val,
+		(val & RTL8365MB_CHIP_RESET_HW_MASK) == 0, 20000, 1e6);
+}
+
+static int rtl8365mb_setup(struct dsa_switch *ds)
+{
+	struct realtek_smi *smi = ds->priv;
+	struct rtl8365mb *mb;
+	int ret;
+
+	mb = smi->chip_data;
+
+	ret = rtl8365mb_reset_chip(smi);
+	if (ret) {
+		dev_err(smi->dev, "failed to reset chip: %d\n", ret);
+		return ret;
+	}
+
+	/* Configure switch to vendor-defined initial state */
+	ret = rtl8365mb_switch_init(smi);
+	if (ret) {
+		dev_err(smi->dev, "failed to initialize switch: %d\n", ret);
+		return ret;
+	}
+
+	/* Configure CPU tagging */
+	ret = rtl8365mb_cpu_config(smi);
+	if (ret)
+		return ret;
+
+	/* Isolate all ports per DSA setup requirements */
+	ret = rtl8365mb_ports_isolate(smi);
+	if (ret)
+		return ret;
+
+	/* Set maximum packet length to 1536 bytes */
+	ret = regmap_update_bits(smi->map, RTL8365MB_CFG0_MAX_LEN_REG,
+				 RTL8365MB_CFG0_MAX_LEN_MASK,
+				 FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, 1536));
+	if (ret)
+		return ret;
+
+	/* Set up cascading IRQs */
+	ret = rtl8365mb_irq_setup(smi);
+	if (ret == -EPROBE_DEFER)
+		return ret;
+	else if (ret)
+		dev_info(smi->dev, "no interrupt support\n");
+
+	ret = realtek_smi_setup_mdio(smi);
+	if (ret) {
+		dev_err(smi->dev, "could not set up MDIO bus\n");
+		return -ENODEV;
+	}
+
+	return ret;
+}
+
+static void rtl8365mb_teardown(struct dsa_switch *ds)
+{
+	struct realtek_smi *smi = ds->priv;
+
+	realtek_smi_teardown_mdio(smi);
+	rtl8365mb_irq_teardown(smi);
+}
+
+static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver)
+{
+	int ret;
+
+	/* For some reason we have to write a magic value to an arbitrary
+	 * register whenever accessing the chip ID/version registers.
+	 */
+	ret = regmap_write(map, RTL8365MB_MAGIC_REG, RTL8365MB_MAGIC_VALUE);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(map, RTL8365MB_CHIP_ID_REG, id);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(map, RTL8365MB_CHIP_VER_REG, ver);
+	if (ret)
+		return ret;
+
+	/* Reset magic register */
+	ret = regmap_write(map, RTL8365MB_MAGIC_REG, 0);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rtl8365mb_detect(struct realtek_smi *smi)
+{
+	struct rtl8365mb *mb = smi->chip_data;
+	u32 chip_id;
+	u32 chip_ver;
+	int ret;
+
+	ret = rtl8365mb_get_chip_id_and_ver(smi->map, &chip_id, &chip_ver);
+	if (ret) {
+		dev_err(smi->dev, "failed to read chip id and version: %d\n",
+			ret);
+		return ret;
+	}
+
+	switch (chip_id) {
+	case RTL8365MB_CHIP_ID_8365MB_VC:
+		dev_info(smi->dev,
+			 "found an RTL8365MB-VC switch (ver=0x%04x)\n",
+			 chip_ver);
+
+		smi->cpu_port = RTL8365MB_CPU_PORT_NUM_8365MB_VC;
+		smi->num_ports = RTL8365MB_NUM_PORTS_8365MB_VC;
+		smi->num_vlan_mc = RTL8365MB_NUM_VLANS;
+		smi->mib_counters = rtl8365mb_mib_counters;
+		smi->num_mib_counters = ARRAY_SIZE(rtl8365mb_mib_counters);
+
+		mb->chip_id = chip_id;
+		mb->chip_ver = chip_ver;
+		mb->num_phys = RTL8365MB_NUM_PHYS_8365MB_VC;
+		mb->port_mask = RTL8365MB_PORT_MASK_8365MB_VC;
+		mb->phy_port_mask = RTL8365MB_PHY_PORT_MASK_8365MB_VC;
+		mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
+		mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
+
+		mb->cpu.enable = 1;
+		mb->cpu.mask = BIT(smi->cpu_port);
+		mb->cpu.trap_port = smi->cpu_port;
+		mb->cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
+		mb->cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
+		mb->cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
+		mb->cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;
+
+		break;
+	default:
+		dev_err(smi->dev,
+			"found an unknown Realtek switch (id=0x%04x, ver=0x%04x)\n",
+			chip_id, chip_ver);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static const struct dsa_switch_ops rtl8365mb_switch_ops = {
+	.get_tag_protocol = rtl8365mb_get_tag_protocol,
+	.setup = rtl8365mb_setup,
+	.teardown = rtl8365mb_teardown,
+	.phylink_validate = rtl8365mb_phylink_validate,
+	.phylink_mac_config = rtl8365mb_phylink_mac_config,
+	.phylink_mac_link_down = rtl8365mb_phylink_mac_link_down,
+	.phylink_mac_link_up = rtl8365mb_phylink_mac_link_up,
+	.get_strings = rtl8366_get_strings,
+	.get_ethtool_stats = rtl8366_get_ethtool_stats,
+	.get_sset_count = rtl8366_get_sset_count,
+	.port_vlan_filtering = rtl8365mb_port_vlan_filtering,
+	.port_vlan_add = rtl8366_vlan_add,
+	.port_vlan_del = rtl8366_vlan_del,
+	.port_enable = rtl8365mb_port_enable,
+	.port_disable = rtl8365mb_port_disable,
+};
+
+static const struct realtek_smi_ops rtl8365mb_smi_ops = {
+	.detect = rtl8365mb_detect,
+	.get_vlan_mc = rtl8365mb_get_vlan_mc,
+	.set_vlan_mc = rtl8365mb_set_vlan_mc,
+	.get_vlan_4k = rtl8365mb_get_vlan_4k,
+	.set_vlan_4k = rtl8365mb_set_vlan_4k,
+	.get_mc_index = rtl8365mb_get_mc_index,
+	.set_mc_index = rtl8365mb_set_mc_index,
+	.get_mib_counter = rtl8365mb_get_mib_counter,
+	.is_vlan_valid = rtl8365mb_is_vlan_valid,
+	.enable_vlan = rtl8365mb_enable_vlan,
+	.enable_vlan4k = rtl8365mb_enable_vlan4k,
+	.phy_read = rtl8365mb_phy_read,
+	.phy_write = rtl8365mb_phy_write,
+};
+
+const struct realtek_smi_variant rtl8365mb_variant = {
+	.ds_ops = &rtl8365mb_switch_ops,
+	.ops = &rtl8365mb_smi_ops,
+	.clk_delay = 10,
+	.cmd_read = 0xb9,
+	.cmd_write = 0xb8,
+	.chip_data_sz = sizeof(struct rtl8365mb),
+};
+EXPORT_SYMBOL_GPL(rtl8365mb_variant);
-- 
2.32.0

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Vladimir Oltean]

On Sun, Aug 22, 2021 at 09:31:42PM +0200, Alvin Šipraga wrote:
> +static bool rtl8365mb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)

Maybe it would be more efficient to make smi->ops->is_vlan_valid optional?

> +{
> +	if (vlan > RTL8365MB_VIDMAX)
> +		return false;
> +
> +	return true;
> +}
> +
> +static int rtl8365mb_enable_vlan(struct realtek_smi *smi, bool enable)
> +{
> +	dev_dbg(smi->dev, "%s VLAN\n", enable ? "enable" : "disable");
> +	return regmap_update_bits(
> +		smi->map, RTL8365MB_VLAN_CTRL_REG, RTL8365MB_VLAN_CTRL_EN_MASK,
> +		FIELD_PREP(RTL8365MB_VLAN_CTRL_EN_MASK, enable ? 1 : 0));
> +}
> +
> +static int rtl8365mb_enable_vlan4k(struct realtek_smi *smi, bool enable)
> +{
> +	return rtl8365mb_enable_vlan(smi, enable);
> +}

I'm not going to lie, the realtek_smi_ops VLAN methods seem highly
cryptic to me. Why do you do the same thing from .enable_vlan4k as from
.enable_vlan? What are these supposed to do in the first place?
Or to quote from rtl8366_vlan_add: "what's with this 4k business?"

Also, stupid question: what do you need the VLAN ops for if you haven't
implemented .port_bridge_join and .port_bridge_leave? How have you
tested them?

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

On 8/23/21 12:48 AM, Vladimir Oltean wrote:
> On Sun, Aug 22, 2021 at 09:31:42PM +0200, Alvin Šipraga wrote:
>> +static bool rtl8365mb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
> 
> Maybe it would be more efficient to make smi->ops->is_vlan_valid optional?

That would work. I'll make a note to do it for v2.

> 
>> +{
>> +	if (vlan > RTL8365MB_VIDMAX)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static int rtl8365mb_enable_vlan(struct realtek_smi *smi, bool enable)
>> +{
>> +	dev_dbg(smi->dev, "%s VLAN\n", enable ? "enable" : "disable");
>> +	return regmap_update_bits(
>> +		smi->map, RTL8365MB_VLAN_CTRL_REG, RTL8365MB_VLAN_CTRL_EN_MASK,
>> +		FIELD_PREP(RTL8365MB_VLAN_CTRL_EN_MASK, enable ? 1 : 0));
>> +}
>> +
>> +static int rtl8365mb_enable_vlan4k(struct realtek_smi *smi, bool enable)
>> +{
>> +	return rtl8365mb_enable_vlan(smi, enable);
>> +}
> 
> I'm not going to lie, the realtek_smi_ops VLAN methods seem highly
> cryptic to me. Why do you do the same thing from .enable_vlan4k as from
> .enable_vlan? What are these supposed to do in the first place?
> Or to quote from rtl8366_vlan_add: "what's with this 4k business?"

I think realtek-smi was written with rtl8366rb.c in mind, which appears 
to have different control registers for VLAN and VLAN4k modes, whatever 
that's supposed to mean. Since the RTL8365MB doesn't distinguish between 
the two, I just route one to the other. The approach is one of caution, 
since I don't want to break the other driver (I don't have hardware to 
test for regressions). Maybe Linus can chime in?

> 
> Also, stupid question: what do you need the VLAN ops for if you haven't
> implemented .port_bridge_join and .port_bridge_leave? How have you
> tested them?

I have to admit that I am also in some doubt about that. To illustrate, 
this is a typical configuration I have been testing:

                               br0
                                +
                                |
               +----------+-----+-----+----------+
               |          |           |          |
(DHCP)        +          +           +          +      (static IP)
  wan0      brwan0       swp2        swp3     brpriv0      priv0
   |           + 1 P u    + 1 P u     + 1 P u    +           +
   |           |          |           | 2        | 2 P u     |
   |           |          |           |          |           |
   +-----------+          +           +          +-----------+
                         LAN         PRIV

          n P u
          ^ ^ ^
          | | |
          | | `--- Egress Untagged
          | `----- Port VLAN ID (PVID)
          `------- VLAN ID n

In this configuration, priv0 is used to communicate directly with the 
PRIV device over VLAN2. PRIV can also access the wider LAN by 
transmitting untagged frames. My understanding was that the VLAN 
configuration is necessary for e.g. packets to be untagged properly on 
swp2 egress. But are you suggesting that this is being done in software 
already? I.e. we are sending untagged frames from CPU->switch without 
any VLAN tag?

In case you think the VLAN ops are unnecessary given that 
.port_bridge_{join,leave} aren't implemented, do you think they should 
be removed in their entirety from the current patch?

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Vladimir Oltean]

On Sun, Aug 22, 2021 at 11:56:04PM +0000, Alvin Šipraga wrote:
> > I'm not going to lie, the realtek_smi_ops VLAN methods seem highly
> > cryptic to me. Why do you do the same thing from .enable_vlan4k as from
> > .enable_vlan? What are these supposed to do in the first place?
> > Or to quote from rtl8366_vlan_add: "what's with this 4k business?"
>
> I think realtek-smi was written with rtl8366rb.c in mind, which appears
> to have different control registers for VLAN and VLAN4k modes, whatever
> that's supposed to mean. Since the RTL8365MB doesn't distinguish between
> the two, I just route one to the other. The approach is one of caution,
> since I don't want to break the other driver (I don't have hardware to
> test for regressions). Maybe Linus can chime in?

You don't _have_ to use the rtl8366 ops for VLAN, especially if they
don't make sense, do you?

> > Also, stupid question: what do you need the VLAN ops for if you haven't
> > implemented .port_bridge_join and .port_bridge_leave? How have you
> > tested them?
>
> I have to admit that I am also in some doubt about that. To illustrate,
> this is a typical configuration I have been testing:
>
>                                br0
>                                 +
>                                 |
>                +----------+-----+-----+----------+
>                |          |           |          |
> (DHCP)         +          +           +          +      (static IP)
>   wan0      brwan0       swp2        swp3     brpriv0      priv0
>    |           + 1 P u    + 1 P u     + 1 P u    +           +
>    |           |          |           | 2        | 2 P u     |
>    |           |          |           |          |           |
>    +-----------+          +           +          +-----------+
>                          LAN         PRIV
>
>           n P u
>           ^ ^ ^
>           | | |
>           | | `--- Egress Untagged
>           | `----- Port VLAN ID (PVID)
>           `------- VLAN ID n

What are priv0 and wan0? Are they local interfaces of your board, put in
loopback with switch ports? Are they external devices?

What does DHCP mean? Is there a server there, or does it mean that the
wan0 interface gets IP over DHCP? Where is the DHCP server? Why is "DHCP"
relevant?

>
> In this configuration, priv0 is used to communicate directly with the
> PRIV device over VLAN2. PRIV can also access the wider LAN by
> transmitting untagged frames. My understanding was that the VLAN
> configuration is necessary for e.g. packets to be untagged properly on
> swp2 egress.

swp2 egresses packets only in VLAN 1. In your example, how would any
packet become tagged in VLAN 1? VLAN 1 is a pvid on all ports which are
members of it.

> But are you suggesting that this is being done in software
> already? I.e. we are sending untagged frames from CPU->switch without
> any VLAN tag?

With the exception of ports with the TX_FWD_OFFLOAD feature where the
VLAN is always left in the packet, the bridge will pop the VLAN ID on
transmission if that VLAN is configured as egress-untagged in the
software VLAN database corresponding to the destination bridge port.
See br_handle_vlan:

	/* If the skb will be sent using forwarding offload, the assumption is
	 * that the switchdev will inject the packet into hardware together
	 * with the bridge VLAN, so that it can be forwarded according to that
	 * VLAN. The switchdev should deal with popping the VLAN header in
	 * hardware on each egress port as appropriate. So only strip the VLAN
	 * header if forwarding offload is not being used.
	 */
	if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED &&
	    !br_switchdev_frame_uses_tx_fwd_offload(skb))
		__vlan_hwaccel_clear_tag(skb);

>
> In case you think the VLAN ops are unnecessary given that
> .port_bridge_{join,leave} aren't implemented, do you think they should
> be removed in their entirety from the current patch?

I don't think it's a matter of whether _I_ think that they are
unnecessary. Are they necessary? Are these code paths really exercised?
What happens if you delete them? These are unanswered questions.


My best guess is: you have a problem with transmitting VLAN-tagged
packets on a port, even if that port doesn't offload the bridge
forwarding process. You keep transmitting the packet to the switch as
VLAN-tagged and the switch keeps stripping the tag. You need the VLAN
ops to configure the VLAN 2 as egress-tagged on the port, so the switch
will leave it alone.
It all has to do with the KEEP bit from the xmit DSA header. The switch
has VLAN ingress filtering disabled but is not VLAN-unaware. A standalone
port (one which does not offload a Linux bridge) is expected to be
completely VLAN-unaware and not inject or strip any VLAN header from any
packet, at least not in any user-visible manner. It should behave just
like any other network interface. Packet in, packet out, and the skb
that the network stack sees, after stripping the DSA tag, should look
like the packet that was on the wire (and similarly in the reverse direction).

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

On 8/23/21 2:19 AM, Vladimir Oltean wrote:
> On Sun, Aug 22, 2021 at 11:56:04PM +0000, Alvin Šipraga wrote:
>>> I'm not going to lie, the realtek_smi_ops VLAN methods seem highly
>>> cryptic to me. Why do you do the same thing from .enable_vlan4k as from
>>> .enable_vlan? What are these supposed to do in the first place?
>>> Or to quote from rtl8366_vlan_add: "what's with this 4k business?"
>>
>> I think realtek-smi was written with rtl8366rb.c in mind, which appears
>> to have different control registers for VLAN and VLAN4k modes, whatever
>> that's supposed to mean. Since the RTL8365MB doesn't distinguish between
>> the two, I just route one to the other. The approach is one of caution,
>> since I don't want to break the other driver (I don't have hardware to
>> test for regressions). Maybe Linus can chime in?
> 
> You don't _have_ to use the rtl8366 ops for VLAN, especially if they
> don't make sense, do you?

I was mainly referring to the enablement of VLAN versus VLAN4k when I 
said it didn't make sense. The add/del ops seemed to fit well though, so 
I sought to reuse code where I could. But they make use of 
.enable_{vlan,vlan4k}, hence the weird stub you see.

> 
>>> Also, stupid question: what do you need the VLAN ops for if you haven't
>>> implemented .port_bridge_join and .port_bridge_leave? How have you
>>> tested them?
>>
>> I have to admit that I am also in some doubt about that. To illustrate,
>> this is a typical configuration I have been testing:
>>
>>                                 br0
>>                                  +
>>                                  |
>>                 +----------+-----+-----+----------+
>>                 |          |           |          |
>> (DHCP)         +          +           +          +      (static IP)
>>    wan0      brwan0       swp2        swp3     brpriv0      priv0
>>     |           + 1 P u    + 1 P u     + 1 P u    +           +
>>     |           |          |           | 2        | 2 P u     |
>>     |           |          |           |          |           |
>>     +-----------+          +           +          +-----------+
>>                           LAN         PRIV
>>
>>            n P u
>>            ^ ^ ^
>>            | | |
>>            | | `--- Egress Untagged
>>            | `----- Port VLAN ID (PVID)
>>            `------- VLAN ID n
> 
> What are priv0 and wan0? Are they local interfaces of your board, put in
> loopback with switch ports? Are they external devices?

Sorry, I should have clarified. They are veth interface pairs, with one 
end added to the bridge, and the other end used as a standalone "VLAN 
unaware" interface to perform specific functions. wan0 is used as a 
typical Ethernet interface for contacting the Internet and other devices 
on the LAN. priv0 is an interface used to communicate with a private 
device PRIV over VLAN2.

> 
> What does DHCP mean? Is there a server there, or does it mean that the
> wan0 interface gets IP over DHCP? Where is the DHCP server? Why is "DHCP"
> relevant?

It was meant to indicate that it is getting an IP address over DHCP over 
the LAN.

> 
>>
>> In this configuration, priv0 is used to communicate directly with the
>> PRIV device over VLAN2. PRIV can also access the wider LAN by
>> transmitting untagged frames. My understanding was that the VLAN
>> configuration is necessary for e.g. packets to be untagged properly on
>> swp2 egress.
> 
> swp2 egresses packets only in VLAN 1. In your example, how would any
> packet become tagged in VLAN 1? VLAN 1 is a pvid on all ports which are
> members of it.

I thought that frames forwarded by the bridge from brwan0 to swp2 would 
be tagged VLAN 1, and that the switch port should untag it. Is that not 
the case?

> 
>> But are you suggesting that this is being done in software
>> already? I.e. we are sending untagged frames from CPU->switch without
>> any VLAN tag?
> 
> With the exception of ports with the TX_FWD_OFFLOAD feature where the
> VLAN is always left in the packet, the bridge will pop the VLAN ID on
> transmission if that VLAN is configured as egress-untagged in the
> software VLAN database corresponding to the destination bridge port.
> See br_handle_vlan:
> 
> 	/* If the skb will be sent using forwarding offload, the assumption is
> 	 * that the switchdev will inject the packet into hardware together
> 	 * with the bridge VLAN, so that it can be forwarded according to that
> 	 * VLAN. The switchdev should deal with popping the VLAN header in
> 	 * hardware on each egress port as appropriate. So only strip the VLAN
> 	 * header if forwarding offload is not being used.
> 	 */
> 	if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED &&
> 	    !br_switchdev_frame_uses_tx_fwd_offload(skb))
> 		__vlan_hwaccel_clear_tag(skb);

Right, so that answers my question immediately above: of course it's not 
the case - the bridge will pop the tag before sending it to swp2.

> 
>>
>> In case you think the VLAN ops are unnecessary given that
>> .port_bridge_{join,leave} aren't implemented, do you think they should
>> be removed in their entirety from the current patch?
> 
> I don't think it's a matter of whether _I_ think that they are
> unnecessary. Are they necessary? Are these code paths really exercised?
> What happens if you delete them? These are unanswered questions.

The code paths are exercised, insofar as they are called when I 
configure my bridge. Perhaps I could rephrase my question as follows: If 
the switch driver behaves properly (i.e. does not strip or tag frames) 
despite the switch being VLAN-aware, is it a problem?

(We can of course argue whether the switch is behaving correctly with my 
driver, but the question assumes that it is.)

The VLAN code will be of use when implementing bridge offload, so I'm 
seeking some advice from you with regards to the process. I can remove 
all the VLAN stuff and resubmit the driver such that the switch behaves 
in a completely VLAN-unaware fashion, but that will require some 
backtracking and the work will have to be done again if any offloading 
is to be implemented. So if we can agree that it doesn't cause any harm, 
I would think that it's OK to keep it in.

> 
> 
> My best guess is: you have a problem with transmitting VLAN-tagged
> packets on a port, even if that port doesn't offload the bridge
> forwarding process. You keep transmitting the packet to the switch as
> VLAN-tagged and the switch keeps stripping the tag. You need the VLAN
> ops to configure the VLAN 2 as egress-tagged on the port, so the switch
> will leave it alone.
> It all has to do with the KEEP bit from the xmit DSA header. The switch
> has VLAN ingress filtering disabled but is not VLAN-unaware. A standalone
> port (one which does not offload a Linux bridge) is expected to be
> completely VLAN-unaware and not inject or strip any VLAN header from any
> packet, at least not in any user-visible manner. It should behave just
> like any other network interface. Packet in, packet out, and the skb
> that the network stack sees, after stripping the DSA tag, should look
> like the packet that was on the wire (and similarly in the reverse direction).
> 

I am actually enabling VLAN ingress filtering. And I don't have a 
problem transmitting VLAN 2-tagged packets on swp3 in my example. 
Whether or not the driver is following the best practices - I'm not 
sure. Following on from above: is the best practice to make the switch 
completely VLAN-unaware if I am submitting a driver which does not 
support any bridge offloading?

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Vladimir Oltean]

On Mon, Aug 23, 2021 at 01:22:53AM +0000, Alvin Šipraga wrote:
> On 8/23/21 2:19 AM, Vladimir Oltean wrote:
> > On Sun, Aug 22, 2021 at 11:56:04PM +0000, Alvin Šipraga wrote:
> >>> I'm not going to lie, the realtek_smi_ops VLAN methods seem highly
> >>> cryptic to me. Why do you do the same thing from .enable_vlan4k as from
> >>> .enable_vlan? What are these supposed to do in the first place?
> >>> Or to quote from rtl8366_vlan_add: "what's with this 4k business?"
> >>
> >> I think realtek-smi was written with rtl8366rb.c in mind, which appears
> >> to have different control registers for VLAN and VLAN4k modes, whatever
> >> that's supposed to mean. Since the RTL8365MB doesn't distinguish between
> >> the two, I just route one to the other. The approach is one of caution,
> >> since I don't want to break the other driver (I don't have hardware to
> >> test for regressions). Maybe Linus can chime in?
> >
> > You don't _have_ to use the rtl8366 ops for VLAN, especially if they
> > don't make sense, do you?
>
> I was mainly referring to the enablement of VLAN versus VLAN4k when I
> said it didn't make sense. The add/del ops seemed to fit well though, so
> I sought to reuse code where I could. But they make use of
> .enable_{vlan,vlan4k}, hence the weird stub you see.
>
> >
> >>> Also, stupid question: what do you need the VLAN ops for if you haven't
> >>> implemented .port_bridge_join and .port_bridge_leave? How have you
> >>> tested them?
> >>
> >> I have to admit that I am also in some doubt about that. To illustrate,
> >> this is a typical configuration I have been testing:
> >>
> >>                                 br0
> >>                                  +
> >>                                  |
> >>                 +----------+-----+-----+----------+
> >>                 |          |           |          |
> >> (DHCP)          +          +           +          +      (static IP)
> >>    wan0      brwan0       swp2        swp3     brpriv0      priv0
> >>     |           + 1 P u    + 1 P u     + 1 P u    +           +
> >>     |           |          |           | 2        | 2 P u     |
> >>     |           |          |           |          |           |
> >>     +-----------+          +           +          +-----------+
> >>                           LAN         PRIV
> >>
> >>            n P u
> >>            ^ ^ ^
> >>            | | |
> >>            | | `--- Egress Untagged
> >>            | `----- Port VLAN ID (PVID)
> >>            `------- VLAN ID n
> >
> > What are priv0 and wan0? Are they local interfaces of your board, put in
> > loopback with switch ports? Are they external devices?
>
> Sorry, I should have clarified. They are veth interface pairs, with one
> end added to the bridge, and the other end used as a standalone "VLAN
> unaware" interface to perform specific functions. wan0 is used as a
> typical Ethernet interface for contacting the Internet and other devices
> on the LAN. priv0 is an interface used to communicate with a private
> device PRIV over VLAN2.

Ok, understood.

> > What does DHCP mean? Is there a server there, or does it mean that the
> > wan0 interface gets IP over DHCP? Where is the DHCP server? Why is "DHCP"
> > relevant?
>
> It was meant to indicate that it is getting an IP address over DHCP over
> the LAN.

Ok, so not relevant.

> >> In this configuration, priv0 is used to communicate directly with the
> >> PRIV device over VLAN2. PRIV can also access the wider LAN by
> >> transmitting untagged frames. My understanding was that the VLAN
> >> configuration is necessary for e.g. packets to be untagged properly on
> >> swp2 egress.
> >
> > swp2 egresses packets only in VLAN 1. In your example, how would any
> > packet become tagged in VLAN 1? VLAN 1 is a pvid on all ports which are
> > members of it.
>
> I thought that frames forwarded by the bridge from brwan0 to swp2 would
> be tagged VLAN 1, and that the switch port should untag it. Is that not
> the case?

Your hardware switch does not know about the existence of brwan0. It
just sees what packets the tagger is sending to it. And in your example,
no one will be sending packets to the switch that the switch must untag.
Packets in VLAN 1 are sent as untagged by the bridge, as a tcpdump on
swp2 will show.

> >> But are you suggesting that this is being done in software
> >> already? I.e. we are sending untagged frames from CPU->switch without
> >> any VLAN tag?
> >
> > With the exception of ports with the TX_FWD_OFFLOAD feature where the
> > VLAN is always left in the packet, the bridge will pop the VLAN ID on
> > transmission if that VLAN is configured as egress-untagged in the
> > software VLAN database corresponding to the destination bridge port.
> > See br_handle_vlan:
> >
> > 	/* If the skb will be sent using forwarding offload, the assumption is
> > 	 * that the switchdev will inject the packet into hardware together
> > 	 * with the bridge VLAN, so that it can be forwarded according to that
> > 	 * VLAN. The switchdev should deal with popping the VLAN header in
> > 	 * hardware on each egress port as appropriate. So only strip the VLAN
> > 	 * header if forwarding offload is not being used.
> > 	 */
> > 	if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED &&
> > 	    !br_switchdev_frame_uses_tx_fwd_offload(skb))
> > 		__vlan_hwaccel_clear_tag(skb);
>
> Right, so that answers my question immediately above: of course it's not
> the case - the bridge will pop the tag before sending it to swp2.
>
> >
> >>
> >> In case you think the VLAN ops are unnecessary given that
> >> .port_bridge_{join,leave} aren't implemented, do you think they should
> >> be removed in their entirety from the current patch?
> >
> > I don't think it's a matter of whether _I_ think that they are
> > unnecessary. Are they necessary? Are these code paths really exercised?
> > What happens if you delete them? These are unanswered questions.
>
> The code paths are exercised, insofar as they are called when I
> configure my bridge.

See? That's exactly where the problem is: "they are called". Let me
explain why they shouldn't.

When a port joins a bridge, dsa_slave_changeupper() will call
dsa_port_bridge_join(). This will dive a bit into DSA internals but will
finally return -EOPNOTSUPP because ds->ops->port_bridge_join is NULL.
This triggers the error path of dsa_broadcast(DSA_NOTIFIER_BRIDGE_JOIN)
which sets back dp->bridge_dev to NULL. The port should behave as
standalone.

Now I just went through the whole code path and this does not happen for
plain bridging: in lack of a ds->ops->port_bridge_join method, DSA is
still happy to return zero, although I don't understand why - I recall
writing a patch specifically for that. Anyway. I just rewrote it and
posted it to the list.

I can understand why a lot of things didn't make sense for you. I thought
we were on the same page about what is happening, but we weren't.

> Perhaps I could rephrase my question as follows: If
> the switch driver behaves properly (i.e. does not strip or tag frames)
> despite the switch being VLAN-aware, is it a problem?
>
> (We can of course argue whether the switch is behaving correctly with my
> driver, but the question assumes that it is.)
>
> The VLAN code will be of use when implementing bridge offload, so I'm
> seeking some advice from you with regards to the process. I can remove
> all the VLAN stuff and resubmit the driver such that the switch behaves
> in a completely VLAN-unaware fashion, but that will require some
> backtracking and the work will have to be done again if any offloading
> is to be implemented. So if we can agree that it doesn't cause any harm,
> I would think that it's OK to keep it in.

With DSA now doing the right thing with the patch I just sent, I hope it is
now clearer why having VLAN ops does not make sense if you don't offload
the bridge. They were not supposed to be called.

> > My best guess is: you have a problem with transmitting VLAN-tagged
> > packets on a port, even if that port doesn't offload the bridge
> > forwarding process. You keep transmitting the packet to the switch as
> > VLAN-tagged and the switch keeps stripping the tag. You need the VLAN
> > ops to configure the VLAN 2 as egress-tagged on the port, so the switch
> > will leave it alone.
> > It all has to do with the KEEP bit from the xmit DSA header. The switch
> > has VLAN ingress filtering disabled but is not VLAN-unaware. A standalone
> > port (one which does not offload a Linux bridge) is expected to be
> > completely VLAN-unaware and not inject or strip any VLAN header from any
> > packet, at least not in any user-visible manner. It should behave just
> > like any other network interface. Packet in, packet out, and the skb
> > that the network stack sees, after stripping the DSA tag, should look
> > like the packet that was on the wire (and similarly in the reverse direction).
> >
>
> I am actually enabling VLAN ingress filtering. And I don't have a
> problem transmitting VLAN 2-tagged packets on swp3 in my example.
> Whether or not the driver is following the best practices - I'm not
> sure. Following on from above: is the best practice to make the switch
> completely VLAN-unaware if I am submitting a driver which does not
> support any bridge offloading?

VLAN unaware, no ingress filtering, no address learning, all ports
forward to the CPU port and only to the CPU port.

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

Hi Vladimir,

On 8/23/21 4:12 AM, Vladimir Oltean wrote:
> Your hardware switch does not know about the existence of brwan0. It
> just sees what packets the tagger is sending to it. And in your example,
> no one will be sending packets to the switch that the switch must untag.
> Packets in VLAN 1 are sent as untagged by the bridge, as a tcpdump on
> swp2 will show.

Understood.

> 
>>>> But are you suggesting that this is being done in software
>>>> already? I.e. we are sending untagged frames from CPU->switch without
>>>> any VLAN tag?
>>>
>>> With the exception of ports with the TX_FWD_OFFLOAD feature where the
>>> VLAN is always left in the packet, the bridge will pop the VLAN ID on
>>> transmission if that VLAN is configured as egress-untagged in the
>>> software VLAN database corresponding to the destination bridge port.
>>> See br_handle_vlan:
>>>
>>> 	/* If the skb will be sent using forwarding offload, the assumption is
>>> 	 * that the switchdev will inject the packet into hardware together
>>> 	 * with the bridge VLAN, so that it can be forwarded according to that
>>> 	 * VLAN. The switchdev should deal with popping the VLAN header in
>>> 	 * hardware on each egress port as appropriate. So only strip the VLAN
>>> 	 * header if forwarding offload is not being used.
>>> 	 */
>>> 	if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED &&
>>> 	    !br_switchdev_frame_uses_tx_fwd_offload(skb))
>>> 		__vlan_hwaccel_clear_tag(skb);
>>
>> Right, so that answers my question immediately above: of course it's not
>> the case - the bridge will pop the tag before sending it to swp2.
>>
>>>
>>>>
>>>> In case you think the VLAN ops are unnecessary given that
>>>> .port_bridge_{join,leave} aren't implemented, do you think they should
>>>> be removed in their entirety from the current patch?
>>>
>>> I don't think it's a matter of whether _I_ think that they are
>>> unnecessary. Are they necessary? Are these code paths really exercised?
>>> What happens if you delete them? These are unanswered questions.
>>
>> The code paths are exercised, insofar as they are called when I
>> configure my bridge.
> 
> See? That's exactly where the problem is: "they are called". Let me
> explain why they shouldn't.
> 
> When a port joins a bridge, dsa_slave_changeupper() will call
> dsa_port_bridge_join(). This will dive a bit into DSA internals but will
> finally return -EOPNOTSUPP because ds->ops->port_bridge_join is NULL.
> This triggers the error path of dsa_broadcast(DSA_NOTIFIER_BRIDGE_JOIN)
> which sets back dp->bridge_dev to NULL. The port should behave as
> standalone.
> 
> Now I just went through the whole code path and this does not happen for
> plain bridging: in lack of a ds->ops->port_bridge_join method, DSA is
> still happy to return zero, although I don't understand why - I recall
> writing a patch specifically for that. Anyway. I just rewrote it and
> posted it to the list.

I tested your patch with some small modifications to make it apply (I'm 
running 5.14-rc5 right now and it's not so trivial to bump right now - 
let me know if you think it's important).

However I still observe the VLAN ops of my driver getting called (now 
with "tagged, no PVID", which is not what I thought was intended - 
previously it was "untagged, PVID"):

[   45.727777] realtek-smi ethernet-switch swp2: configuring for phy/ 
link mode
[   45.730173] realtek-smi ethernet-switch: add VLAN 1 on port 2, 
tagged, no PVID
[   45.733457] CPU: 1 PID: 595 Comm: systemd-network Tainted: G 
   O      5.14.0-rc5-20210811-1-r
t6 #1
[   45.733477] Hardware name: B&O (DT)
[   45.733481] Call trace:
[   45.733482]  dump_backtrace+0x0/0x1f8
[   45.733500]  show_stack+0x1c/0x28
[   45.733508]  dump_stack_lvl+0x64/0x7c
[   45.733516]  dump_stack+0x14/0x2c
[   45.733524]  rtl8365mb_set_vlan_4k+0x3c/0xa6c [realtek_smi]
[   45.733547]  rtl8366_set_vlan+0xb8/0x1f8 [realtek_smi]
[   45.733564]  rtl8366_vlan_add+0x174/0x228 [realtek_smi]
[   45.733582]  dsa_switch_event+0x2c4/0xde8
[   45.733591]  notifier_call_chain+0x80/0xd8
[   45.733598]  raw_notifier_call_chain+0x1c/0x28
[   45.733603]  dsa_tree_notify+0x18/0x38
[   45.733612]  dsa_port_vlan_add+0x54/0x78
[   45.733620]  dsa_slave_vlan_rx_add_vid+0x80/0x130
[   45.733627]  vlan_add_rx_filter_info+0x5c/0x80
[   45.733636]  vlan_vid_add+0xec/0x1c8
[   45.733643]  __vlan_add+0x748/0x8c8
[   45.733650]  nbp_vlan_add+0xf4/0x170
[   45.733656]  br_vlan_info.isra.0+0x6c/0x120
[   45.733662]  br_process_vlan_info+0x244/0x368
[   45.733669]  br_afspec+0x170/0x190
[   45.733674]  br_setlink+0x174/0x218
[   45.733679]  rtnl_bridge_setlink+0xbc/0x258
[   45.733688]  rtnetlink_rcv_msg+0x11c/0x338
...

I hope it's clear that even with software bridging, I still want to use 
VLAN to achieve the network topology I described in one of my previous 
replies. I think we are in agreement now that this should be handled 
entirely in software, with the switch being completely VLAN-unaware and 
not touching the VLAN tags. To that end I think I will strip all the 
VLAN ops from the v2 series to make this unambiguous. But regardless of 
that, shouldn't your patch ensure that no VLAN operations are offloaded 
to the switch hardware if .port_bridge_{join,leave} are not implemented?

> 
> I can understand why a lot of things didn't make sense for you. I thought
> we were on the same page about what is happening, but we weren't.

Yeah, the fact that my VLAN ops were still getting called led me to 
believe that there was still utility in keeping them there. I was not 
aware of the details of the implementation, but your explanation is 
making things a lot clearer to me. I hope you can answer the above 
question which I think will clear up any other misunderstandings I might 
have here.

> 
>> Perhaps I could rephrase my question as follows: If
>> the switch driver behaves properly (i.e. does not strip or tag frames)
>> despite the switch being VLAN-aware, is it a problem?
>>
>> (We can of course argue whether the switch is behaving correctly with my
>> driver, but the question assumes that it is.)
>>
>> The VLAN code will be of use when implementing bridge offload, so I'm
>> seeking some advice from you with regards to the process. I can remove
>> all the VLAN stuff and resubmit the driver such that the switch behaves
>> in a completely VLAN-unaware fashion, but that will require some
>> backtracking and the work will have to be done again if any offloading
>> is to be implemented. So if we can agree that it doesn't cause any harm,
>> I would think that it's OK to keep it in.
> 
> With DSA now doing the right thing with the patch I just sent, I hope it is
> now clearer why having VLAN ops does not make sense if you don't offload
> the bridge. They were not supposed to be called.

Per the above, your explanation makes sense, except that my VLAN ops are 
still getting called. If I can understand why that's (not) supposed to 
happen, I think we'll be on the same page.

> 
>>> My best guess is: you have a problem with transmitting VLAN-tagged
>>> packets on a port, even if that port doesn't offload the bridge
>>> forwarding process. You keep transmitting the packet to the switch as
>>> VLAN-tagged and the switch keeps stripping the tag. You need the VLAN
>>> ops to configure the VLAN 2 as egress-tagged on the port, so the switch
>>> will leave it alone.
>>> It all has to do with the KEEP bit from the xmit DSA header. The switch
>>> has VLAN ingress filtering disabled but is not VLAN-unaware. A standalone
>>> port (one which does not offload a Linux bridge) is expected to be
>>> completely VLAN-unaware and not inject or strip any VLAN header from any
>>> packet, at least not in any user-visible manner. It should behave just
>>> like any other network interface. Packet in, packet out, and the skb
>>> that the network stack sees, after stripping the DSA tag, should look
>>> like the packet that was on the wire (and similarly in the reverse direction).
>>>
>>
>> I am actually enabling VLAN ingress filtering. And I don't have a
>> problem transmitting VLAN 2-tagged packets on swp3 in my example.
>> Whether or not the driver is following the best practices - I'm not
>> sure. Following on from above: is the best practice to make the switch
>> completely VLAN-unaware if I am submitting a driver which does not
>> support any bridge offloading?
> 
> VLAN unaware, no ingress filtering, no address learning, all ports
> forward to the CPU port and only to the CPU port.

Got it. I'll make sure this is the case in v2 unless I find the time to 
work on the offloading functionality in the interim. Thanks again.

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Vladimir Oltean]

On Mon, Aug 23, 2021 at 10:06:39AM +0000, Alvin Šipraga wrote:
> I tested your patch with some small modifications to make it apply (I'm
> running 5.14-rc5 right now and it's not so trivial to bump right now -
> let me know if you think it's important).
>
> However I still observe the VLAN ops of my driver getting called (now
> with "tagged, no PVID", which is not what I thought was intended -
> previously it was "untagged, PVID"):
>
> [   45.727777] realtek-smi ethernet-switch swp2: configuring for phy/link mode
> [   45.730173] realtek-smi ethernet-switch: add VLAN 1 on port 2, tagged, no PVID
> [   45.733457] CPU: 1 PID: 595 Comm: systemd-network Tainted: G   O      5.14.0-rc5-20210811-1-rt6 #1
> [   45.733477] Hardware name: B&O (DT)
> [   45.733481] Call trace:
> [   45.733482]  dump_backtrace+0x0/0x1f8
> [   45.733500]  show_stack+0x1c/0x28
> [   45.733508]  dump_stack_lvl+0x64/0x7c
> [   45.733516]  dump_stack+0x14/0x2c
> [   45.733524]  rtl8365mb_set_vlan_4k+0x3c/0xa6c [realtek_smi]
> [   45.733547]  rtl8366_set_vlan+0xb8/0x1f8 [realtek_smi]
> [   45.733564]  rtl8366_vlan_add+0x174/0x228 [realtek_smi]
> [   45.733582]  dsa_switch_event+0x2c4/0xde8
> [   45.733591]  notifier_call_chain+0x80/0xd8
> [   45.733598]  raw_notifier_call_chain+0x1c/0x28
> [   45.733603]  dsa_tree_notify+0x18/0x38
> [   45.733612]  dsa_port_vlan_add+0x54/0x78
> [   45.733620]  dsa_slave_vlan_rx_add_vid+0x80/0x130
> [   45.733627]  vlan_add_rx_filter_info+0x5c/0x80
> [   45.733636]  vlan_vid_add+0xec/0x1c8

This is an unintended consequence for sure. The bridge is persistent and
finds a leak in our defense, see __vlan_vid_add:

	/* Try switchdev op first. In case it is not supported, fallback to
	 * 8021q add.
	 */
	err = br_switchdev_port_vlan_add(dev, v->vid, flags, extack);
	if (err == -EOPNOTSUPP)
		return vlan_vid_add(dev, br->vlan_proto, v->vid);

We return -EOPNOTSUPP to br_switchdev_port_vlan_add, then the bridge
tries with vlan_vid_add, which makes us think it's an 8021q upper, and
we say "oh, yes, but sure!"

Btw, the fact that DSA thinks it's an 8021q upper is also the reason why
your VLAN gets added with different flags, see dsa_slave_vlan_rx_add_vid:

	/* This API only allows programming tagged, non-PVID VIDs */

There is a larger problem at hand, which is that the logic behind
dsa_slave_vlan_rx_add_vid currently adds VLANs to hardware even for many
switch drivers that don't need that. It does not even give the switch
driver the opportunity to distinguish between a bridge VLAN and a VLAN
coming from a VLAN upper interface. I need to think about that too.

This should work if you replace all:

	case SWITCHDEV_OBJ_ID_PORT_VLAN:
		if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
			return -EOPNOTSUPP;

with:

	case SWITCHDEV_OBJ_ID_PORT_VLAN:
		if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
			return 0;

but I need a bit more time to think of any drawbacks of doing that.

> [   45.733643]  __vlan_add+0x748/0x8c8
> [   45.733650]  nbp_vlan_add+0xf4/0x170
> [   45.733656]  br_vlan_info.isra.0+0x6c/0x120
> [   45.733662]  br_process_vlan_info+0x244/0x368
> [   45.733669]  br_afspec+0x170/0x190
> [   45.733674]  br_setlink+0x174/0x218
> [   45.733679]  rtnl_bridge_setlink+0xbc/0x258
> [   45.733688]  rtnetlink_rcv_msg+0x11c/0x338
> ...
>
> I hope it's clear that even with software bridging, I still want to use
> VLAN to achieve the network topology I described in one of my previous
> replies. I think we are in agreement now that this should be handled
> entirely in software, with the switch being completely VLAN-unaware and
> not touching the VLAN tags. To that end I think I will strip all the
> VLAN ops from the v2 series to make this unambiguous. But regardless of
> that, shouldn't your patch ensure that no VLAN operations are offloaded
> to the switch hardware if .port_bridge_{join,leave} are not implemented?

See above for the 2 corner cases that exist. The only reason why
dsa_slave_vlan_rx_add_vid() exists is to work around some hardware
quirks where some switches cannot put their standalone ports in
VLAN-unaware mode. So to accept VLAN-tagged packets, DSA needs to trap
the vlan_vid_add() calls to perform VLAN RX filtering on these
standalone ports. You do not need this functionality at all, but we do
not distinguish between switches that need it and switches that don't,
hence the issues.

> > I can understand why a lot of things didn't make sense for you. I thought
> > we were on the same page about what is happening, but we weren't.
>
> Yeah, the fact that my VLAN ops were still getting called led me to
> believe that there was still utility in keeping them there. I was not
> aware of the details of the implementation, but your explanation is
> making things a lot clearer to me. I hope you can answer the above
> question which I think will clear up any other misunderstandings I might
> have here.

I fail to see any reason why any external factors would modify the state
of a standalone switch port.

> >> Perhaps I could rephrase my question as follows: If
> >> the switch driver behaves properly (i.e. does not strip or tag frames)
> >> despite the switch being VLAN-aware, is it a problem?
> >>
> >> (We can of course argue whether the switch is behaving correctly with my
> >> driver, but the question assumes that it is.)
> >>
> >> The VLAN code will be of use when implementing bridge offload, so I'm
> >> seeking some advice from you with regards to the process. I can remove
> >> all the VLAN stuff and resubmit the driver such that the switch behaves
> >> in a completely VLAN-unaware fashion, but that will require some
> >> backtracking and the work will have to be done again if any offloading
> >> is to be implemented. So if we can agree that it doesn't cause any harm,
> >> I would think that it's OK to keep it in.
> >
> > With DSA now doing the right thing with the patch I just sent, I hope it is
> > now clearer why having VLAN ops does not make sense if you don't offload
> > the bridge. They were not supposed to be called.
>
> Per the above, your explanation makes sense, except that my VLAN ops are
> still getting called. If I can understand why that's (not) supposed to
> happen, I think we'll be on the same page.

See above.

> >>> My best guess is: you have a problem with transmitting VLAN-tagged
> >>> packets on a port, even if that port doesn't offload the bridge
> >>> forwarding process. You keep transmitting the packet to the switch as
> >>> VLAN-tagged and the switch keeps stripping the tag. You need the VLAN
> >>> ops to configure the VLAN 2 as egress-tagged on the port, so the switch
> >>> will leave it alone.
> >>> It all has to do with the KEEP bit from the xmit DSA header. The switch
> >>> has VLAN ingress filtering disabled but is not VLAN-unaware. A standalone
> >>> port (one which does not offload a Linux bridge) is expected to be
> >>> completely VLAN-unaware and not inject or strip any VLAN header from any
> >>> packet, at least not in any user-visible manner. It should behave just
> >>> like any other network interface. Packet in, packet out, and the skb
> >>> that the network stack sees, after stripping the DSA tag, should look
> >>> like the packet that was on the wire (and similarly in the reverse direction).
> >>>
> >>
> >> I am actually enabling VLAN ingress filtering. And I don't have a
> >> problem transmitting VLAN 2-tagged packets on swp3 in my example.
> >> Whether or not the driver is following the best practices - I'm not
> >> sure. Following on from above: is the best practice to make the switch
> >> completely VLAN-unaware if I am submitting a driver which does not
> >> support any bridge offloading?
> >
> > VLAN unaware, no ingress filtering, no address learning, all ports
> > forward to the CPU port and only to the CPU port.
>
> Got it. I'll make sure this is the case in v2 unless I find the time to
> work on the offloading functionality in the interim. Thanks again.

Even if you find the time to work on bridge offloading, standalone ports
should still behave like that: no learning, VLAN-unaware, no ingress
filtering, forward only to the CPU, flood all packets. You may find that
the switchover from one state to the other is a bit tricky, but it needs
to be consistent.

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

On 8/23/21 12:31 PM, Vladimir Oltean wrote:
> On Mon, Aug 23, 2021 at 10:06:39AM +0000, Alvin Šipraga wrote:
>> I tested your patch with some small modifications to make it apply (I'm
>> running 5.14-rc5 right now and it's not so trivial to bump right now -
>> let me know if you think it's important).
>>
>> However I still observe the VLAN ops of my driver getting called (now
>> with "tagged, no PVID", which is not what I thought was intended -
>> previously it was "untagged, PVID"):
>>
>> [   45.727777] realtek-smi ethernet-switch swp2: configuring for phy/link mode
>> [   45.730173] realtek-smi ethernet-switch: add VLAN 1 on port 2, tagged, no PVID
>> [   45.733457] CPU: 1 PID: 595 Comm: systemd-network Tainted: G   O      5.14.0-rc5-20210811-1-rt6 #1
>> [   45.733477] Hardware name: B&O (DT)
>> [   45.733481] Call trace:
>> [   45.733482]  dump_backtrace+0x0/0x1f8
>> [   45.733500]  show_stack+0x1c/0x28
>> [   45.733508]  dump_stack_lvl+0x64/0x7c
>> [   45.733516]  dump_stack+0x14/0x2c
>> [   45.733524]  rtl8365mb_set_vlan_4k+0x3c/0xa6c [realtek_smi]
>> [   45.733547]  rtl8366_set_vlan+0xb8/0x1f8 [realtek_smi]
>> [   45.733564]  rtl8366_vlan_add+0x174/0x228 [realtek_smi]
>> [   45.733582]  dsa_switch_event+0x2c4/0xde8
>> [   45.733591]  notifier_call_chain+0x80/0xd8
>> [   45.733598]  raw_notifier_call_chain+0x1c/0x28
>> [   45.733603]  dsa_tree_notify+0x18/0x38
>> [   45.733612]  dsa_port_vlan_add+0x54/0x78
>> [   45.733620]  dsa_slave_vlan_rx_add_vid+0x80/0x130
>> [   45.733627]  vlan_add_rx_filter_info+0x5c/0x80
>> [   45.733636]  vlan_vid_add+0xec/0x1c8
> 
> This is an unintended consequence for sure. The bridge is persistent and
> finds a leak in our defense, see __vlan_vid_add:
> 
> 	/* Try switchdev op first. In case it is not supported, fallback to
> 	 * 8021q add.
> 	 */
> 	err = br_switchdev_port_vlan_add(dev, v->vid, flags, extack);
> 	if (err == -EOPNOTSUPP)
> 		return vlan_vid_add(dev, br->vlan_proto, v->vid);
> 
> We return -EOPNOTSUPP to br_switchdev_port_vlan_add, then the bridge
> tries with vlan_vid_add, which makes us think it's an 8021q upper, and
> we say "oh, yes, but sure!"
> 
> Btw, the fact that DSA thinks it's an 8021q upper is also the reason why
> your VLAN gets added with different flags, see dsa_slave_vlan_rx_add_vid:
> 
> 	/* This API only allows programming tagged, non-PVID VIDs */
> 
> There is a larger problem at hand, which is that the logic behind
> dsa_slave_vlan_rx_add_vid currently adds VLANs to hardware even for many
> switch drivers that don't need that. It does not even give the switch
> driver the opportunity to distinguish between a bridge VLAN and a VLAN
> coming from a VLAN upper interface. I need to think about that too.
> 
> This should work if you replace all:
> 
> 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
> 		if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
> 			return -EOPNOTSUPP;
> 
> with:
> 
> 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
> 		if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
> 			return 0;
> 
> but I need a bit more time to think of any drawbacks of doing that.

OK, I expected something like that. Removing the VLAN ops should allow 
me to ignore this particular edge case, so I will not dwell on this. But 
thanks for the explanation.

> 
>> [   45.733643]  __vlan_add+0x748/0x8c8
>> [   45.733650]  nbp_vlan_add+0xf4/0x170
>> [   45.733656]  br_vlan_info.isra.0+0x6c/0x120
>> [   45.733662]  br_process_vlan_info+0x244/0x368
>> [   45.733669]  br_afspec+0x170/0x190
>> [   45.733674]  br_setlink+0x174/0x218
>> [   45.733679]  rtnl_bridge_setlink+0xbc/0x258
>> [   45.733688]  rtnetlink_rcv_msg+0x11c/0x338
>> ...
>>
>> I hope it's clear that even with software bridging, I still want to use
>> VLAN to achieve the network topology I described in one of my previous
>> replies. I think we are in agreement now that this should be handled
>> entirely in software, with the switch being completely VLAN-unaware and
>> not touching the VLAN tags. To that end I think I will strip all the
>> VLAN ops from the v2 series to make this unambiguous. But regardless of
>> that, shouldn't your patch ensure that no VLAN operations are offloaded
>> to the switch hardware if .port_bridge_{join,leave} are not implemented?
> 
> See above for the 2 corner cases that exist. The only reason why
> dsa_slave_vlan_rx_add_vid() exists is to work around some hardware
> quirks where some switches cannot put their standalone ports in
> VLAN-unaware mode. So to accept VLAN-tagged packets, DSA needs to trap
> the vlan_vid_add() calls to perform VLAN RX filtering on these
> standalone ports. You do not need this functionality at all, but we do
> not distinguish between switches that need it and switches that don't,
> hence the issues.

Understood.

> 
>>> I can understand why a lot of things didn't make sense for you. I thought
>>> we were on the same page about what is happening, but we weren't.
>>
>> Yeah, the fact that my VLAN ops were still getting called led me to
>> believe that there was still utility in keeping them there. I was not
>> aware of the details of the implementation, but your explanation is
>> making things a lot clearer to me. I hope you can answer the above
>> question which I think will clear up any other misunderstandings I might
>> have here.
> 
> I fail to see any reason why any external factors would modify the state
> of a standalone switch port.

You have explained the requirements of a standalone switch port in clear 
terms, so I can see now why my belief was misguided.

> 
>>>> Perhaps I could rephrase my question as follows: If
>>>> the switch driver behaves properly (i.e. does not strip or tag frames)
>>>> despite the switch being VLAN-aware, is it a problem?
>>>>
>>>> (We can of course argue whether the switch is behaving correctly with my
>>>> driver, but the question assumes that it is.)
>>>>
>>>> The VLAN code will be of use when implementing bridge offload, so I'm
>>>> seeking some advice from you with regards to the process. I can remove
>>>> all the VLAN stuff and resubmit the driver such that the switch behaves
>>>> in a completely VLAN-unaware fashion, but that will require some
>>>> backtracking and the work will have to be done again if any offloading
>>>> is to be implemented. So if we can agree that it doesn't cause any harm,
>>>> I would think that it's OK to keep it in.
>>>
>>> With DSA now doing the right thing with the patch I just sent, I hope it is
>>> now clearer why having VLAN ops does not make sense if you don't offload
>>> the bridge. They were not supposed to be called.
>>
>> Per the above, your explanation makes sense, except that my VLAN ops are
>> still getting called. If I can understand why that's (not) supposed to
>> happen, I think we'll be on the same page.
> 
> See above.
> 
>>>>> My best guess is: you have a problem with transmitting VLAN-tagged
>>>>> packets on a port, even if that port doesn't offload the bridge
>>>>> forwarding process. You keep transmitting the packet to the switch as
>>>>> VLAN-tagged and the switch keeps stripping the tag. You need the VLAN
>>>>> ops to configure the VLAN 2 as egress-tagged on the port, so the switch
>>>>> will leave it alone.
>>>>> It all has to do with the KEEP bit from the xmit DSA header. The switch
>>>>> has VLAN ingress filtering disabled but is not VLAN-unaware. A standalone
>>>>> port (one which does not offload a Linux bridge) is expected to be
>>>>> completely VLAN-unaware and not inject or strip any VLAN header from any
>>>>> packet, at least not in any user-visible manner. It should behave just
>>>>> like any other network interface. Packet in, packet out, and the skb
>>>>> that the network stack sees, after stripping the DSA tag, should look
>>>>> like the packet that was on the wire (and similarly in the reverse direction).
>>>>>
>>>>
>>>> I am actually enabling VLAN ingress filtering. And I don't have a
>>>> problem transmitting VLAN 2-tagged packets on swp3 in my example.
>>>> Whether or not the driver is following the best practices - I'm not
>>>> sure. Following on from above: is the best practice to make the switch
>>>> completely VLAN-unaware if I am submitting a driver which does not
>>>> support any bridge offloading?
>>>
>>> VLAN unaware, no ingress filtering, no address learning, all ports
>>> forward to the CPU port and only to the CPU port.
>>
>> Got it. I'll make sure this is the case in v2 unless I find the time to
>> work on the offloading functionality in the interim. Thanks again.
> 
> Even if you find the time to work on bridge offloading, standalone ports
> should still behave like that: no learning, VLAN-unaware, no ingress
> filtering, forward only to the CPU, flood all packets. You may find that
> the switchover from one state to the other is a bit tricky, but it needs
> to be consistent.

Understood.

You've given me a lot to think about now, and I think I have got the 
answers I need to follow up on the feedback. Thanks for taking the time 
to explain all of this - it was incredibly helpful.

Please expect some delay before I come back with a v2 series, as I have 
other priorities nagging me at work. But I will definitely follow up in 
due course.

Kind regards,
Alvin

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Andrew Lunn]

> I tested your patch with some small modifications to make it apply (I'm 
> running 5.14-rc5 right now and it's not so trivial to bump right now - 
> let me know if you think it's important).

Patches submitted to netdev should be against net-next. Before you
submit a version which gets merged, you need to update. Please mark
all submissions until then as RFC in the Subject line.

    Andrew

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

Hi Andrew,

On 8/23/21 3:13 PM, Andrew Lunn wrote:
>> I tested your patch with some small modifications to make it apply (I'm
>> running 5.14-rc5 right now and it's not so trivial to bump right now -
>> let me know if you think it's important).
> 
> Patches submitted to netdev should be against net-next. Before you
> submit a version which gets merged, you need to update. Please mark
> all submissions until then as RFC in the Subject line.

Yes, I'm aware of that requirement - another reason for the RFC tag. If 
I submit v2 (or vN) sans RFC, you will know that it's been built and 
tested against net-next, so no worries.

Kind regards,
Alvin

> 
>      Andrew
> 

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Linus Walleij]

On Mon, Aug 23, 2021 at 1:56 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
> On 8/23/21 12:48 AM, Vladimir Oltean wrote:
> > On Sun, Aug 22, 2021 at 09:31:42PM +0200, Alvin Šipraga wrote:

> >> +static int rtl8365mb_enable_vlan(struct realtek_smi *smi, bool enable)
> >> +{
> >> +    dev_dbg(smi->dev, "%s VLAN\n", enable ? "enable" : "disable");
> >> +    return regmap_update_bits(
> >> +            smi->map, RTL8365MB_VLAN_CTRL_REG, RTL8365MB_VLAN_CTRL_EN_MASK,
> >> +            FIELD_PREP(RTL8365MB_VLAN_CTRL_EN_MASK, enable ? 1 : 0));
> >> +}
> >> +
> >> +static int rtl8365mb_enable_vlan4k(struct realtek_smi *smi, bool enable)
> >> +{
> >> +    return rtl8365mb_enable_vlan(smi, enable);
> >> +}
> >
> > I'm not going to lie, the realtek_smi_ops VLAN methods seem highly
> > cryptic to me. Why do you do the same thing from .enable_vlan4k as from
> > .enable_vlan? What are these supposed to do in the first place?
> > Or to quote from rtl8366_vlan_add: "what's with this 4k business?"
>
> I think realtek-smi was written with rtl8366rb.c in mind, which appears
> to have different control registers for VLAN and VLAN4k modes, whatever
> that's supposed to mean. Since the RTL8365MB doesn't distinguish between
> the two, I just route one to the other. The approach is one of caution,
> since I don't want to break the other driver (I don't have hardware to
> test for regressions). Maybe Linus can chime in?

Sigh, I have zero documentation, I just mimic what the code dump from
Realtek does.

But my interpretation is that the RTL8366RB can operate with either
16 or 4096 VLAN (VID) member entries. (Called "mc", member configs)
The support for 4096 "4k" entries need to be enabled explicitly,
in succession after enabling the 16 entries, and this is what the
code in rtl8366.c does, and we always enable all 4096 "mc:s"
of course.

I guess some older switch only supported 16 members and this
is a hardware compatibility mode.

Yours,
Linus Walleij

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Andrew Lunn]

> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> Co-developed-by: Michael Rasmussen <mir@bang-olufsen.dk>
> Signed-off-by: Michael Rasmussen <mir@bang-olufsen.dk>

Hi Alvin

Since you are submitting the patch, your SOB goes last.

> +/* Port mapping macros
> + *
> + * PORT_NUM_x2y: map a port number from domain x to domain y
> + * PORT_MASK_x2y: map a port mask from domain x to domain y
> + *
> + * L = logical port domain, i.e. dsa_port.index
> + * P = physical port domain, used by the Realtek ASIC for port indexing;
> + *     for ports with internal PHYs, this is also the PHY index
> + * E = extension port domain, used by the Realtek ASIC for managing EXT ports
> + *
> + * The terminology is borrowed from the vendor driver. The extension port domain
> + * is mostly used to navigate the labyrinthine layout of EXT port configuration
> + * registers and is not considered intuitive by the author.
> + *
> + * Unless a function is accessing chip registers, it should be using the logical
> + * port domain. Moreover, function arguments for port numbers and port masks
> + * must always be in the logical domain. The conversion must be done as close as
> + * possible to the register access to avoid chaos.
> + *
> + * The mappings vary between chips in the family supported by this driver. Here
> + * is an example of the mapping for the RTL8365MB-VC:
> + *
> + *    L | P | E | remark
> + *   ---+---+---+--------
> + *    0 | 0 |   | user port
> + *    1 | 1 |   | user port
> + *    2 | 2 |   | user port
> + *    3 | 3 |   | user port
> + *    4 | 6 | 1 | extension (CPU) port

Did you consider not bothering with this. Just always use the Physical
port number? The DSA framework does not care if there are ports
missing. If it makes the code simpler, ignore the logical number, and
just enforce that the missing ports are not used, by returning -EINVAL
in the port_enable() callback.

> +/* Interrupt control register - enable or disable specific interrupt types */
> +#define RTL8365MB_INTR_CTRL				0x1101
> +#define   RTL8365MB_INTR_CTRL_SLIENT_START_2_MASK	0x1000
> +#define   RTL8365MB_INTR_CTRL_SLIENT_START_MASK		0x800
> +#define   RTL8365MB_INTR_CTRL_ACL_ACTION_MASK		0x200
> +#define   RTL8365MB_INTR_CTRL_CABLE_DIAG_FIN_MASK	0x100
> +#define   RTL8365MB_INTR_CTRL_INTERRUPT_8051_MASK	0x80
> +#define   RTL8365MB_INTR_CTRL_LOOP_DETECTION_MASK	0x40
> +#define   RTL8365MB_INTR_CTRL_GREEN_TIMER_MASK		0x20
> +#define   RTL8365MB_INTR_CTRL_SPECIAL_CONGEST_MASK	0x10
> +#define   RTL8365MB_INTR_CTRL_SPEED_CHANGE_MASK		0x8
> +#define   RTL8365MB_INTR_CTRL_LEARN_OVER_MASK		0x4
> +#define   RTL8365MB_INTR_CTRL_METER_EXCEEDED_MASK	0x2
> +#define   RTL8365MB_INTR_CTRL_LINK_CHANGE_MASK		0x1
> +
> +
> +/* Interrupt status register */
> +#define RTL8365MB_INTR_STATUS_REG			0x1102
> +#define   RTL8365MB_INTR_STATUS_SLIENT_START_2_MASK	0x1000
> +#define   RTL8365MB_INTR_STATUS_SLIENT_START_MASK	0x800
> +#define   RTL8365MB_INTR_STATUS_ACL_ACTION_MASK		0x200
> +#define   RTL8365MB_INTR_STATUS_CABLE_DIAG_FIN_MASK	0x100
> +#define   RTL8365MB_INTR_STATUS_INTERRUPT_8051_MASK	0x80
> +#define   RTL8365MB_INTR_STATUS_LOOP_DETECTION_MASK	0x40
> +#define   RTL8365MB_INTR_STATUS_GREEN_TIMER_MASK	0x20
> +#define   RTL8365MB_INTR_STATUS_SPECIAL_CONGEST_MASK	0x10
> +#define   RTL8365MB_INTR_STATUS_SPEED_CHANGE_MASK	0x8
> +#define   RTL8365MB_INTR_STATUS_LEARN_OVER_MASK		0x4
> +#define   RTL8365MB_INTR_STATUS_METER_EXCEEDED_MASK	0x2
> +#define   RTL8365MB_INTR_STATUS_LINK_CHANGE_MASK	0x1
> +#define   RTL8365MB_INTR_STATUS_ALL_MASK                      \

Interrupt control and status registers are generally identical. So you
don't need to define the values twice.

 +static void rtl8365mb_phylink_validate(struct dsa_switch *ds, int port,
> +				       unsigned long *supported,
> +				       struct phylink_link_state *state)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0 };
> +
> +	/* include/linux/phylink.h says:
> +	 *     When @state->interface is %PHY_INTERFACE_MODE_NA, phylink
> +	 *     expects the MAC driver to return all supported link modes.
> +	 */
> +	if (state->interface != PHY_INTERFACE_MODE_NA &&
> +	    !rtl8365mb_phy_mode_supported(ds, port, state->interface)) {
> +		dev_err(smi->dev, "phy mode %s is unsupported on port %d\n",
> +			phy_modes(state->interface), port);
> +		linkmode_zero(supported);
> +		return;
> +	}
> +
> +	phylink_set_port_modes(mask);
> +
> +	phylink_set(mask, Autoneg);
> +	phylink_set(mask, Pause);
> +	phylink_set(mask, Asym_Pause);
> +
> +	phylink_set(mask, 10baseT_Half);
> +	phylink_set(mask, 10baseT_Full);
> +	phylink_set(mask, 100baseT_Half);
> +	phylink_set(mask, 100baseT_Full);
> +	phylink_set(mask, 1000baseT_Full);
> +	phylink_set(mask, 1000baseT_Half);

Does the documentation actually mention 1000baseT_Half? Often it is
not implemented.

> +static int rtl8365mb_port_enable(struct dsa_switch *ds, int port,
> +				 struct phy_device *phy)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	int val;
> +
> +	if (dsa_is_user_port(ds, port)) {
> +		/* Power up the internal PHY and restart autonegotiation */
> +		val = rtl8365mb_phy_read(smi, port, MII_BMCR);
> +		if (val < 0)
> +			return val;
> +
> +		val &= ~BMCR_PDOWN;
> +		val |= BMCR_ANRESTART;
> +
> +		return rtl8365mb_phy_write(smi, port, MII_BMCR, val);
> +	}

There should not be any need to do this. phylib should do it. In
generally, you should not need to touch any PHY registers, you just
need to allow access to the PHY registers.

I want to take another look at the interrupt code. But this looks
pretty nice in general.

       Andrew

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

Hi Andrew,

On 8/23/21 1:04 AM, Andrew Lunn wrote:
>> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
>> Co-developed-by: Michael Rasmussen <mir@bang-olufsen.dk>
>> Signed-off-by: Michael Rasmussen <mir@bang-olufsen.dk>
> 
> Hi Alvin
> 
> Since you are submitting the patch, your SOB goes last.

Will fix in v2.

> 
>> +/* Port mapping macros
>> + *
>> + * PORT_NUM_x2y: map a port number from domain x to domain y
>> + * PORT_MASK_x2y: map a port mask from domain x to domain y
>> + *
>> + * L = logical port domain, i.e. dsa_port.index
>> + * P = physical port domain, used by the Realtek ASIC for port indexing;
>> + *     for ports with internal PHYs, this is also the PHY index
>> + * E = extension port domain, used by the Realtek ASIC for managing EXT ports
>> + *
>> + * The terminology is borrowed from the vendor driver. The extension port domain
>> + * is mostly used to navigate the labyrinthine layout of EXT port configuration
>> + * registers and is not considered intuitive by the author.
>> + *
>> + * Unless a function is accessing chip registers, it should be using the logical
>> + * port domain. Moreover, function arguments for port numbers and port masks
>> + * must always be in the logical domain. The conversion must be done as close as
>> + * possible to the register access to avoid chaos.
>> + *
>> + * The mappings vary between chips in the family supported by this driver. Here
>> + * is an example of the mapping for the RTL8365MB-VC:
>> + *
>> + *    L | P | E | remark
>> + *   ---+---+---+--------
>> + *    0 | 0 |   | user port
>> + *    1 | 1 |   | user port
>> + *    2 | 2 |   | user port
>> + *    3 | 3 |   | user port
>> + *    4 | 6 | 1 | extension (CPU) port
> 
> Did you consider not bothering with this. Just always use the Physical
> port number? The DSA framework does not care if there are ports
> missing. If it makes the code simpler, ignore the logical number, and
> just enforce that the missing ports are not used, by returning -EINVAL
> in the port_enable() callback.

Nice, I didn't know I could do that. I'll review this before sending v2, 
although I might end up keeping it if it leads to sticky logic elsewhere 
in the code. In general the PORT_{NUM,MASK}_L2P() calls are not too painfil.

Just to clarify, this means I should set the physical port number in the 
reg field of the device tree? i.e.:

	port@4 {
		reg = <6>; /* <--- instead of <4>? */
		label = "cpu";
		ethernet = <&fec1>;
		phy-mode = "rgmii-id";
		fixed-link {
			speed = <1000>;
			full-duplex;
			pause;
		};
	};


> 
>> +/* Interrupt control register - enable or disable specific interrupt types */
>> +#define RTL8365MB_INTR_CTRL				0x1101
>> +#define   RTL8365MB_INTR_CTRL_SLIENT_START_2_MASK	0x1000
>> +#define   RTL8365MB_INTR_CTRL_SLIENT_START_MASK		0x800
>> +#define   RTL8365MB_INTR_CTRL_ACL_ACTION_MASK		0x200
>> +#define   RTL8365MB_INTR_CTRL_CABLE_DIAG_FIN_MASK	0x100
>> +#define   RTL8365MB_INTR_CTRL_INTERRUPT_8051_MASK	0x80
>> +#define   RTL8365MB_INTR_CTRL_LOOP_DETECTION_MASK	0x40
>> +#define   RTL8365MB_INTR_CTRL_GREEN_TIMER_MASK		0x20
>> +#define   RTL8365MB_INTR_CTRL_SPECIAL_CONGEST_MASK	0x10
>> +#define   RTL8365MB_INTR_CTRL_SPEED_CHANGE_MASK		0x8
>> +#define   RTL8365MB_INTR_CTRL_LEARN_OVER_MASK		0x4
>> +#define   RTL8365MB_INTR_CTRL_METER_EXCEEDED_MASK	0x2
>> +#define   RTL8365MB_INTR_CTRL_LINK_CHANGE_MASK		0x1
>> +
>> +
>> +/* Interrupt status register */
>> +#define RTL8365MB_INTR_STATUS_REG			0x1102
>> +#define   RTL8365MB_INTR_STATUS_SLIENT_START_2_MASK	0x1000
>> +#define   RTL8365MB_INTR_STATUS_SLIENT_START_MASK	0x800
>> +#define   RTL8365MB_INTR_STATUS_ACL_ACTION_MASK		0x200
>> +#define   RTL8365MB_INTR_STATUS_CABLE_DIAG_FIN_MASK	0x100
>> +#define   RTL8365MB_INTR_STATUS_INTERRUPT_8051_MASK	0x80
>> +#define   RTL8365MB_INTR_STATUS_LOOP_DETECTION_MASK	0x40
>> +#define   RTL8365MB_INTR_STATUS_GREEN_TIMER_MASK	0x20
>> +#define   RTL8365MB_INTR_STATUS_SPECIAL_CONGEST_MASK	0x10
>> +#define   RTL8365MB_INTR_STATUS_SPEED_CHANGE_MASK	0x8
>> +#define   RTL8365MB_INTR_STATUS_LEARN_OVER_MASK		0x4
>> +#define   RTL8365MB_INTR_STATUS_METER_EXCEEDED_MASK	0x2
>> +#define   RTL8365MB_INTR_STATUS_LINK_CHANGE_MASK	0x1
>> +#define   RTL8365MB_INTR_STATUS_ALL_MASK                      \
> 
> Interrupt control and status registers are generally identical. So you
> don't need to define the values twice.

Will remove in v2.

> 
>   +static void rtl8365mb_phylink_validate(struct dsa_switch *ds, int port,
>> +				       unsigned long *supported,
>> +				       struct phylink_link_state *state)
>> +{
>> +	struct realtek_smi *smi = ds->priv;
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0 };
>> +
>> +	/* include/linux/phylink.h says:
>> +	 *     When @state->interface is %PHY_INTERFACE_MODE_NA, phylink
>> +	 *     expects the MAC driver to return all supported link modes.
>> +	 */
>> +	if (state->interface != PHY_INTERFACE_MODE_NA &&
>> +	    !rtl8365mb_phy_mode_supported(ds, port, state->interface)) {
>> +		dev_err(smi->dev, "phy mode %s is unsupported on port %d\n",
>> +			phy_modes(state->interface), port);
>> +		linkmode_zero(supported);
>> +		return;
>> +	}
>> +
>> +	phylink_set_port_modes(mask);
>> +
>> +	phylink_set(mask, Autoneg);
>> +	phylink_set(mask, Pause);
>> +	phylink_set(mask, Asym_Pause);
>> +
>> +	phylink_set(mask, 10baseT_Half);
>> +	phylink_set(mask, 10baseT_Full);
>> +	phylink_set(mask, 100baseT_Half);
>> +	phylink_set(mask, 100baseT_Full);
>> +	phylink_set(mask, 1000baseT_Full);
>> +	phylink_set(mask, 1000baseT_Half);
> 
> Does the documentation actually mention 1000baseT_Half? Often it is
> not implemented.

I think you're right that it isn't supported - although the 
documentation I have is very terse. I'll remove it in v2.

> 
>> +static int rtl8365mb_port_enable(struct dsa_switch *ds, int port,
>> +				 struct phy_device *phy)
>> +{
>> +	struct realtek_smi *smi = ds->priv;
>> +	int val;
>> +
>> +	if (dsa_is_user_port(ds, port)) {
>> +		/* Power up the internal PHY and restart autonegotiation */
>> +		val = rtl8365mb_phy_read(smi, port, MII_BMCR);
>> +		if (val < 0)
>> +			return val;
>> +
>> +		val &= ~BMCR_PDOWN;
>> +		val |= BMCR_ANRESTART;
>> +
>> +		return rtl8365mb_phy_write(smi, port, MII_BMCR, val);
>> +	}
> 
> There should not be any need to do this. phylib should do it. In
> generally, you should not need to touch any PHY registers, you just
> need to allow access to the PHY registers.

Will phylib also the opposite when the interface is administratively put 
down (e.g. ip link set dev swp2 down)? The switch doesn't seem to have 
any other handle for stopping the flow of packets from a port except to 
power down the internal PHY, hence why I put this in for port_disable. 
For port_enable I just did the opposite but I can see why it's not 
necessary.

> 
> I want to take another look at the interrupt code. But this looks
> pretty nice in general.
> 
>         Andrew
> 

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Andrew Lunn]

> Just to clarify, this means I should set the physical port number in the 
> reg field of the device tree? i.e.:
> 
> 	port@4 {
> 		reg = <6>; /* <--- instead of <4>? */
> 		label = "cpu";
> 		ethernet = <&fec1>;
> 		phy-mode = "rgmii-id";
> 		fixed-link {
> 			speed = <1000>;
> 			full-duplex;
> 			pause;
> 		};
> 	};

Yes, this is fine.

> >> +static int rtl8365mb_port_enable(struct dsa_switch *ds, int port,
> >> +				 struct phy_device *phy)
> >> +{
> >> +	struct realtek_smi *smi = ds->priv;
> >> +	int val;
> >> +
> >> +	if (dsa_is_user_port(ds, port)) {
> >> +		/* Power up the internal PHY and restart autonegotiation */
> >> +		val = rtl8365mb_phy_read(smi, port, MII_BMCR);
> >> +		if (val < 0)
> >> +			return val;
> >> +
> >> +		val &= ~BMCR_PDOWN;
> >> +		val |= BMCR_ANRESTART;
> >> +
> >> +		return rtl8365mb_phy_write(smi, port, MII_BMCR, val);
> >> +	}
> > 
> > There should not be any need to do this. phylib should do it. In
> > generally, you should not need to touch any PHY registers, you just
> > need to allow access to the PHY registers.
> 
> Will phylib also the opposite when the interface is administratively put 
> down (e.g. ip link set dev swp2 down)? The switch doesn't seem to have 
> any other handle for stopping the flow of packets from a port except to 
> power down the internal PHY, hence why I put this in for port_disable. 
> For port_enable I just did the opposite but I can see why it's not 
> necessary.

When the MAC and PHY are attached phy_attach_direct() gets called,
which calls phy_resume(phydev); The generic implementation clears the
BMCR_PDOWN bit.

phy_detach() will suspend the PHY, which sets the BMCR_PDOWN bit.

But there are two different schemes for doing this.  Some MAC drivers
connect the PHY during probe. Others do it at open. DSA does it at
probe. So this is generic feature is not going to work for you. You
might want to put some printk() in phy_suspend and phy_resume to check
that.

So, setting/clearing the PDOWN bit does seems reasonable. But please
document in the functions why you are doing this. Also, don't restart
autoneg. That really should be up to phylib, and it should be
triggered by phylink_start() which should be called directly after
port_enable().

	Andrew

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

On 8/23/21 3:14 AM, Andrew Lunn wrote:
>> Just to clarify, this means I should set the physical port number in the
>> reg field of the device tree? i.e.:
>>
>> 	port@4 {
>> 		reg = <6>; /* <--- instead of <4>? */
>> 		label = "cpu";
>> 		ethernet = <&fec1>;
>> 		phy-mode = "rgmii-id";
>> 		fixed-link {
>> 			speed = <1000>;
>> 			full-duplex;
>> 			pause;
>> 		};
>> 	};
> 
> Yes, this is fine.
> 
>>>> +static int rtl8365mb_port_enable(struct dsa_switch *ds, int port,
>>>> +				 struct phy_device *phy)
>>>> +{
>>>> +	struct realtek_smi *smi = ds->priv;
>>>> +	int val;
>>>> +
>>>> +	if (dsa_is_user_port(ds, port)) {
>>>> +		/* Power up the internal PHY and restart autonegotiation */
>>>> +		val = rtl8365mb_phy_read(smi, port, MII_BMCR);
>>>> +		if (val < 0)
>>>> +			return val;
>>>> +
>>>> +		val &= ~BMCR_PDOWN;
>>>> +		val |= BMCR_ANRESTART;
>>>> +
>>>> +		return rtl8365mb_phy_write(smi, port, MII_BMCR, val);
>>>> +	}
>>>
>>> There should not be any need to do this. phylib should do it. In
>>> generally, you should not need to touch any PHY registers, you just
>>> need to allow access to the PHY registers.
>>
>> Will phylib also the opposite when the interface is administratively put
>> down (e.g. ip link set dev swp2 down)? The switch doesn't seem to have
>> any other handle for stopping the flow of packets from a port except to
>> power down the internal PHY, hence why I put this in for port_disable.
>> For port_enable I just did the opposite but I can see why it's not
>> necessary.
> 
> When the MAC and PHY are attached phy_attach_direct() gets called,
> which calls phy_resume(phydev); The generic implementation clears the
> BMCR_PDOWN bit.
> 
> phy_detach() will suspend the PHY, which sets the BMCR_PDOWN bit.
> 
> But there are two different schemes for doing this.  Some MAC drivers
> connect the PHY during probe. Others do it at open. DSA does it at
> probe. So this is generic feature is not going to work for you. You
> might want to put some printk() in phy_suspend and phy_resume to check
> that.

Thanks, I will double check.

> 
> So, setting/clearing the PDOWN bit does seems reasonable. But please
> document in the functions why you are doing this. Also, don't restart
> autoneg. That really should be up to phylib, and it should be
> triggered by phylink_start() which should be called directly after
> port_enable().

Understood. I'll review the necessity of these calls and add a comment 
in v2 if I keep them.

> 
> 	Andrew
> 

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[DENG Qingfang]

On Sun, Aug 22, 2021 at 09:31:42PM +0200, Alvin Šipraga wrote:
> +/* Table LUT (look-up-table) address register */
> +#define RTL8365MB_TABLE_LUT_ADDR_REG			0x0502
> +#define   RTL8365MB_TABLE_LUT_ADDR_ADDRESS2_MASK	0x4000
> +#define   RTL8365MB_TABLE_LUT_ADDR_BUSY_FLAG_MASK	0x2000
> +#define   RTL8365MB_TABLE_LUT_ADDR_HIT_STATUS_MASK	0x1000
> +#define   RTL8365MB_TABLE_LUT_ADDR_TYPE_MASK		0x0800
> +#define   RTL8365MB_TABLE_LUT_ADDR_ADDRESS_MASK		0x07FF

FDB/MDB operations should be possible.

> +/* Port isolation (forwarding mask) registers */
> +#define RTL8365MB_PORT_ISOLATION_REG_BASE		0x08A2
> +#define RTL8365MB_PORT_ISOLATION_REG(_physport) \
> +		(RTL8365MB_PORT_ISOLATION_REG_BASE + (_physport))
> +#define   RTL8365MB_PORT_ISOLATION_MASK			0x07FF

Bridge offload should be implemented with these isolation registers.



FYI:
https://cdn.jsdelivr.net/gh/libc0607/Realtek_switch_hacking@files/Realtek_Unmanaged_Switch_ProgrammingGuide.pdf

Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

On 8/23/21 6:37 AM, DENG Qingfang wrote:
> On Sun, Aug 22, 2021 at 09:31:42PM +0200, Alvin Šipraga wrote:
>> +/* Table LUT (look-up-table) address register */
>> +#define RTL8365MB_TABLE_LUT_ADDR_REG			0x0502
>> +#define   RTL8365MB_TABLE_LUT_ADDR_ADDRESS2_MASK	0x4000
>> +#define   RTL8365MB_TABLE_LUT_ADDR_BUSY_FLAG_MASK	0x2000
>> +#define   RTL8365MB_TABLE_LUT_ADDR_HIT_STATUS_MASK	0x1000
>> +#define   RTL8365MB_TABLE_LUT_ADDR_TYPE_MASK		0x0800
>> +#define   RTL8365MB_TABLE_LUT_ADDR_ADDRESS_MASK		0x07FF
> 
> FDB/MDB operations should be possible.

Yes, the switch supports these.

> 
>> +/* Port isolation (forwarding mask) registers */
>> +#define RTL8365MB_PORT_ISOLATION_REG_BASE		0x08A2
>> +#define RTL8365MB_PORT_ISOLATION_REG(_physport) \
>> +		(RTL8365MB_PORT_ISOLATION_REG_BASE + (_physport))
>> +#define   RTL8365MB_PORT_ISOLATION_MASK			0x07FF
> 
> Bridge offload should be implemented with these isolation registers.

That was my plan, albeit not yet implemented in this series.

> 
> 
> 
> FYI:
> https://cdn.jsdelivr.net/gh/libc0607/Realtek_switch_hacking@files/Realtek_Unmanaged_Switch_ProgrammingGuide.pdf>> 

Yes, this is the document we got from Realtek and where the layout of 
the CPU tag format was lifted from. Other reviewers might find it 
helpful to check this out. Thanks!

Kind regards,
Alvin

[RFC PATCH net-next 5/5] net: phy: realtek: add support for RTL8365MB-VC internal PHYs

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

The RTL8365MB-VC ethernet switch controller has 4 internal PHYs for its
user-facing ports. All that is needed is to let the PHY driver core
pick up the IRQ made available by the switch driver.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/net/phy/realtek.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 11be60333fa8..a5671ab896b3 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -1023,6 +1023,14 @@ static struct phy_driver realtek_drvs[] = {
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
+	}, {
+		PHY_ID_MATCH_EXACT(0x001cc942),
+		.name		= "RTL8365MB-VC Gigabit Ethernet",
+		/* Interrupt handling analogous to RTL8366RB */
+		.config_intr	= genphy_no_config_intr,
+		.handle_interrupt = genphy_handle_interrupt_no_ack,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
 	},
 };
 
-- 
2.32.0

Re: [RFC PATCH net-next 5/5] net: phy: realtek: add support for RTL8365MB-VC internal PHYs

[Florian Fainelli]

On 8/22/2021 9:31 PM, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> The RTL8365MB-VC ethernet switch controller has 4 internal PHYs for its
> user-facing ports. All that is needed is to let the PHY driver core
> pick up the IRQ made available by the switch driver.
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [RFC PATCH net-next 5/5] net: phy: realtek: add support for RTL8365MB-VC internal PHYs

[Linus Walleij]

On Sun, Aug 22, 2021 at 9:32 PM Alvin Šipraga <alvin@pqrs.dk> wrote:

> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>
> The RTL8365MB-VC ethernet switch controller has 4 internal PHYs for its
> user-facing ports. All that is needed is to let the PHY driver core
> pick up the IRQ made available by the switch driver.
>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij