[PATCH net-next 0/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[PATCH net-next 0/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Egil Hjelmeland]

When CPU transmit directly to port using tag, the LAN9303 does not
learn MAC addresses received on the CPU port into the ALR table.
ALR learning is performed only when transmitting using ALR lookup.

Solution:
If the two external ports are bridged and the packet is not STP BPDU,
then use ALR lookup to allow ALR learning on CPU port.
Otherwise transmit directly to port with STP state override.

The first patch moves struct lan9303 to include/linux/lan9303.h in order
to prepare for the second patch. 

Egil Hjelmeland (2):
  net: dsa: lan9303: Move struct lan9303 to include/linux/lan9303.h
  net: dsa: lan9303: Learn addresses on CPU port when bridged

 MAINTAINERS               |  1 +
 drivers/net/dsa/lan9303.h | 34 +---------------------------------
 include/linux/lan9303.h   | 35 +++++++++++++++++++++++++++++++++++
 net/dsa/tag_lan9303.c     | 23 ++++++++++++++++++++++-
 4 files changed, 59 insertions(+), 34 deletions(-)
 create mode 100644 include/linux/lan9303.h

-- 
2.11.0

[PATCH net-next 1/2] net: dsa: lan9303: Move struct lan9303 to include/linux/lan9303.h

[Egil Hjelmeland]

The next patch require net/dsa/tag_lan9303.c to access struct lan9303.
Therefore move struct lan9303 definitions from drivers/net/dsa/lan9303.h
to new file include/linux/lan9303.h.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 MAINTAINERS               |  1 +
 drivers/net/dsa/lan9303.h | 34 +---------------------------------
 include/linux/lan9303.h   | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 33 deletions(-)
 create mode 100644 include/linux/lan9303.h

diff --git a/MAINTAINERS b/MAINTAINERS
index e3a7ca9d2783..9535e32bd421 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9416,6 +9416,7 @@ S:	Maintained
 F:	net/dsa/
 F:	include/net/dsa.h
 F:	drivers/net/dsa/
+F:	include/linux/lan9303.h
 
 NETWORKING [GENERAL]
 M:	"David S. Miller" <davem@davemloft.net>
diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index d807b1be35f2..e6675e11c833 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -2,39 +2,7 @@
 #include <linux/device.h>
 #include <net/dsa.h>
 
-struct lan9303;
-
-struct lan9303_phy_ops {
-	/* PHY 1 and 2 access*/
-	int	(*phy_read)(struct lan9303 *chip, int port, int regnum);
-	int	(*phy_write)(struct lan9303 *chip, int port,
-			     int regnum, u16 val);
-};
-
-#define LAN9303_NUM_ALR_RECORDS 512
-struct lan9303_alr_cache_entry {
-	u8  mac_addr[ETH_ALEN];
-	u8  port_map;           /* Bitmap of ports. Zero if unused entry */
-	u8  stp_override;       /* non zero if set ALR_DAT1_AGE_OVERRID */
-};
-
-struct lan9303 {
-	struct device *dev;
-	struct regmap *regmap;
-	struct regmap_irq_chip_data *irq_data;
-	struct gpio_desc *reset_gpio;
-	u32 reset_duration; /* in [ms] */
-	bool phy_addr_sel_strap;
-	struct dsa_switch *ds;
-	struct mutex indirect_mutex; /* protect indexed register access */
-	const struct lan9303_phy_ops *ops;
-	bool is_bridged; /* true if port 1 and 2 are bridged */
-	u32 swe_port_state; /* remember SWE_PORT_STATE while not bridged */
-	/* LAN9303 do not offer reading specific ALR entry. Cache all
-	 * static entries in a flat table
-	 **/
-	struct lan9303_alr_cache_entry alr_cache[LAN9303_NUM_ALR_RECORDS];
-};
+#include <linux/lan9303.h>
 
 extern const struct regmap_access_table lan9303_register_set;
 extern const struct lan9303_phy_ops lan9303_indirect_phy_ops;
diff --git a/include/linux/lan9303.h b/include/linux/lan9303.h
new file mode 100644
index 000000000000..5810bdb43581
--- /dev/null
+++ b/include/linux/lan9303.h
@@ -0,0 +1,35 @@
+/* Included by drivers/net/dsa/lan9303.h and net/dsa/tag_lan9303.c */
+
+struct lan9303;
+
+struct lan9303_phy_ops {
+	/* PHY 1 and 2 access*/
+	int	(*phy_read)(struct lan9303 *chip, int port, int regnum);
+	int	(*phy_write)(struct lan9303 *chip, int port,
+			     int regnum, u16 val);
+};
+
+#define LAN9303_NUM_ALR_RECORDS 512
+struct lan9303_alr_cache_entry {
+	u8  mac_addr[ETH_ALEN];
+	u8  port_map;           /* Bitmap of ports. Zero if unused entry */
+	u8  stp_override;       /* non zero if set ALR_DAT1_AGE_OVERRID */
+};
+
+struct lan9303 {
+	struct device *dev;
+	struct regmap *regmap;
+	struct regmap_irq_chip_data *irq_data;
+	struct gpio_desc *reset_gpio;
+	u32 reset_duration; /* in [ms] */
+	bool phy_addr_sel_strap;
+	struct dsa_switch *ds;
+	struct mutex indirect_mutex; /* protect indexed register access */
+	const struct lan9303_phy_ops *ops;
+	bool is_bridged; /* true if port 1 and 2 are bridged */
+	u32 swe_port_state; /* remember SWE_PORT_STATE while not bridged */
+	/* LAN9303 do not offer reading specific ALR entry. Cache all
+	 * static entries in a flat table
+	 **/
+	struct lan9303_alr_cache_entry alr_cache[LAN9303_NUM_ALR_RECORDS];
+};
-- 
2.11.0

Re: [PATCH net-next 1/2] net: dsa: lan9303: Move struct lan9303 to include/linux/lan9303.h

[Andrew Lunn]

On Tue, Oct 24, 2017 at 11:35:14AM +0200, Egil Hjelmeland wrote:
> The next patch require net/dsa/tag_lan9303.c to access struct lan9303.
> Therefore move struct lan9303 definitions from drivers/net/dsa/lan9303.h
> to new file include/linux/lan9303.h.
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

O.K, so not too bad.

I am now however wondering if include/linux/dsa/lan9303.h would be
better?

Anyway,

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

    Andrew

Re: [PATCH net-next 1/2] net: dsa: lan9303: Move struct lan9303 to include/linux/lan9303.h

[Egil Hjelmeland]

On 24. okt. 2017 18:31, Andrew Lunn wrote:
> On Tue, Oct 24, 2017 at 11:35:14AM +0200, Egil Hjelmeland wrote:
>> The next patch require net/dsa/tag_lan9303.c to access struct lan9303.
>> Therefore move struct lan9303 definitions from drivers/net/dsa/lan9303.h
>> to new file include/linux/lan9303.h.
>>
>> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> 
> O.K, so not too bad.
> 
> I am now however wondering if include/linux/dsa/lan9303.h would be
> better?
> 

Me too. I can do that.


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

Egil

[PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Egil Hjelmeland]

When CPU transmit directly to port using tag, the LAN9303 does not
learn MAC addresses received on the CPU port into the ALR.
ALR learning is performed only when transmitting using ALR lookup.

Solution:
If the two external ports are bridged and the packet is not STP BPDU,
then use ALR lookup to allow ALR learning on CPU port.
Otherwise transmit directly to port with STP state override.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 net/dsa/tag_lan9303.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 57519597c6fc..174721293a34 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -12,6 +12,7 @@
  *
  */
 #include <linux/etherdevice.h>
+#include <linux/lan9303.h>
 #include <linux/list.h>
 #include <linux/slab.h>
 
@@ -39,6 +40,23 @@
  */
 
 #define LAN9303_TAG_LEN 4
+# define LAN9303_TAG_TX_USE_ALR BIT(3)
+# define LAN9303_TAG_TX_STP_OVERRIDE BIT(4)
+#define eth_stp_addr eth_reserved_addr_base
+
+/* Decide whether to transmit using ALR lookup, or transmit directly to
+ * port using tag. ALR learning is performed only when using ALR lookup.
+ * If the two external ports are bridged and the packet is not STP BPDU,
+ * then use ALR lookup to allow ALR learning on CPU port.
+ * Otherwise transmit directly to port with STP state override.
+ * See also: lan9303_separate_ports() and lan9303.pdf 6.4.10.1
+ */
+static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)
+{
+	struct lan9303 *chip = dp->ds->priv;
+
+	return chip->is_bridged && !ether_addr_equal(dest_addr, eth_stp_addr);
+}
 
 static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
 {
@@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
 	lan9303_tag[0] = htons(ETH_P_8021Q);
-	lan9303_tag[1] = htons(dp->index | BIT(4));
+	lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?
+				LAN9303_TAG_TX_USE_ALR :
+				dp->index | LAN9303_TAG_TX_STP_OVERRIDE;
+	lan9303_tag[1] = htons(lan9303_tag[1]);
 
 	return skb;
 }
-- 
2.11.0

Re: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Andrew Lunn]

On Tue, Oct 24, 2017 at 11:35:15AM +0200, Egil Hjelmeland wrote:
> When CPU transmit directly to port using tag, the LAN9303 does not
> learn MAC addresses received on the CPU port into the ALR.
> ALR learning is performed only when transmitting using ALR lookup.
> 
> Solution:
> If the two external ports are bridged and the packet is not STP BPDU,
> then use ALR lookup to allow ALR learning on CPU port.
> Otherwise transmit directly to port with STP state override.
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
>  net/dsa/tag_lan9303.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
> index 57519597c6fc..174721293a34 100644
> --- a/net/dsa/tag_lan9303.c
> +++ b/net/dsa/tag_lan9303.c
> @@ -12,6 +12,7 @@
>   *
>   */
>  #include <linux/etherdevice.h>
> +#include <linux/lan9303.h>
>  #include <linux/list.h>
>  #include <linux/slab.h>
>  
> @@ -39,6 +40,23 @@
>   */
>  
>  #define LAN9303_TAG_LEN 4
> +# define LAN9303_TAG_TX_USE_ALR BIT(3)
> +# define LAN9303_TAG_TX_STP_OVERRIDE BIT(4)
> +#define eth_stp_addr eth_reserved_addr_base
> +
> +/* Decide whether to transmit using ALR lookup, or transmit directly to
> + * port using tag. ALR learning is performed only when using ALR lookup.
> + * If the two external ports are bridged and the packet is not STP BPDU,
> + * then use ALR lookup to allow ALR learning on CPU port.
> + * Otherwise transmit directly to port with STP state override.
> + * See also: lan9303_separate_ports() and lan9303.pdf 6.4.10.1
> + */
> +static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)

Hi Egil

There is no need for the inline. The compiler will do that anyway, for
a function like this.

    Andrew

Re: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Egil Hjelmeland]

On 24. okt. 2017 18:33, Andrew Lunn wrote:
> On Tue, Oct 24, 2017 at 11:35:15AM +0200, Egil Hjelmeland wrote:
>> + */
>> +static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)
> 
> Hi Egil
> 
> There is no need for the inline. The compiler will do that anyway, for
> a function like this. >
Will fix.

>      Andrew
> 

Thanks
Egil

RE: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Woojung.Huh]

Hi Egil,

> +static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)
> +{
> +	struct lan9303 *chip = dp->ds->priv;
> +
> +	return chip->is_bridged && !ether_addr_equal(dest_addr,
> eth_stp_addr);
> +}
> 
>  static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device
> *dev)
>  {
> @@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb,
> struct net_device *dev)
> 
>  	lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
>  	lan9303_tag[0] = htons(ETH_P_8021Q);
> -	lan9303_tag[1] = htons(dp->index | BIT(4));
> +	lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?

How about using skb_mac_header(skb) than skb->data?

> +				LAN9303_TAG_TX_USE_ALR :
> +				dp->index |

Thanks.
Woojung

Re: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Egil Hjelmeland]

Hi Woojung!

On 24. okt. 2017 19:18, Woojung.Huh@microchip.com wrote:
> Hi Egil,
> 
>> +static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)
>> +{
>> +	struct lan9303 *chip = dp->ds->priv;
>> +
>> +	return chip->is_bridged && !ether_addr_equal(dest_addr,
>> eth_stp_addr);
>> +}
>>
>>   static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device
>> *dev)
>>   {
>> @@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb,
>> struct net_device *dev)
>>
>>   	lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
>>   	lan9303_tag[0] = htons(ETH_P_8021Q);
>> -	lan9303_tag[1] = htons(dp->index | BIT(4));
>> +	lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?
> 
> How about using skb_mac_header(skb) than skb->data?
> 
>> +				LAN9303_TAG_TX_USE_ALR :
>> +				dp->index |
> 

I am not the expert here.

I see that skb_mac_header() is (skb->head + skb->mac_header). So it will
cost a few nano seconds per packet. Not the end of the world though.
But I see that other net/dsa/tag_*.c use skb->data, assuming that
skb->data point to mac header.

Anyway, it may be an idea to decrement skb->mac_header, in case the
master interface driver uses it? What about skb->mac_len?

If to use skb_mac_header() at all, I would replace all use of skb->data,
like this:

---
/* provide 'LAN9303_TAG_LEN' bytes additional space */
skb_push(skb, LAN9303_TAG_LEN);

/* make room between MACs and Ether-Type */
memmove(skb_mac_header(skb) - LAN9303_TAG_LEN, skb_mac_header(skb),
		2 * ETH_ALEN);
skb->mac_header -= LAN9303_TAG_LEN;

lan9303_tag = (u16 *)(skb_mac_header(skb) + 2 * ETH_ALEN);
lan9303_tag[0] = htons(ETH_P_8021Q);
lan9303_tag[1] = lan9303_tx_use_arl(dp, skb_mac_header(skb)) ?
			LAN9303_TAG_TX_USE_ALR :
			dp->index | LAN9303_TAG_TX_STP_OVERRIDE;
lan9303_tag[1] = htons(lan9303_tag[1]);
---

But I will really like to hear the opinion from more people on this
before going down that road. Anyway, I think it would belong to a
separate patch.


> Thanks.
> Woojung
> 
> 

Regards
Egil

RE: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Woojung.Huh]

Hi Egil,

> >> @@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff
> *skb,
> >> struct net_device *dev)
> >>
> >>   	lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
> >>   	lan9303_tag[0] = htons(ETH_P_8021Q);
> >> -	lan9303_tag[1] = htons(dp->index | BIT(4));
> >> +	lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?
> >
> > How about using skb_mac_header(skb) than skb->data?
> >
> >> +				LAN9303_TAG_TX_USE_ALR :
> >> +				dp->index |
> >
> 
> I am not the expert here.
> 
> I see that skb_mac_header() is (skb->head + skb->mac_header). So it will
> cost a few nano seconds per packet. Not the end of the world though.
> But I see that other net/dsa/tag_*.c use skb->data, assuming that
> skb->data point to mac header.
> 

Revisited skb_mac_header(). It is basically skb->data after math.
Understand that it would be extra steps than referring skb->data directly.
Unless no one comments on this, please keep first patch.

Thanks.
Woojung