Re: [PATCH v2 net-next 3/9] net: dsa: tag_ksz: add tag handling for Microchip LAN937x

[Prasanna Vengateshan <prasanna.vengateshan@microchip.com>]

On Thu, 2021-04-22 at 22:18 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Thu, Apr 22, 2021 at 03:12:51PM +0530, Prasanna Vengateshan wrote:
> > The Microchip LAN937X switches have a tagging protocol which is
> > very similar to KSZ tagging. So that the implementation is added to
> > tag_ksz.c and reused common APIs
> > 
> > Signed-off-by: Prasanna Vengateshan <prasanna.vengateshan@microchip.com>
> > ---
> >  include/net/dsa.h |  2 ++
> >  net/dsa/Kconfig   |  4 ++--
> >  net/dsa/tag_ksz.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 62 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index 507082959aa4..98c1ab6dc4dc 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -50,6 +50,7 @@ struct phylink_link_state;
> >  #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE     20
> >  #define DSA_TAG_PROTO_SEVILLE_VALUE          21
> >  #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE              22
> > +#define DSA_TAG_PROTO_LAN937X_VALUE          23
> > 
> >  enum dsa_tag_protocol {
> >       DSA_TAG_PROTO_NONE              = DSA_TAG_PROTO_NONE_VALUE,
> > @@ -75,6 +76,7 @@ enum dsa_tag_protocol {
> >       DSA_TAG_PROTO_XRS700X           = DSA_TAG_PROTO_XRS700X_VALUE,
> >       DSA_TAG_PROTO_OCELOT_8021Q      = DSA_TAG_PROTO_OCELOT_8021Q_VALUE,
> >       DSA_TAG_PROTO_SEVILLE           = DSA_TAG_PROTO_SEVILLE_VALUE,
> > +     DSA_TAG_PROTO_LAN937X           = DSA_TAG_PROTO_LAN937X_VALUE,
> >  };
> > 
> >  struct packet_type;
> > diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> > index cbc2bd643ab2..888eb79a85f2 100644
> > --- a/net/dsa/Kconfig
> > +++ b/net/dsa/Kconfig
> > @@ -97,10 +97,10 @@ config NET_DSA_TAG_MTK
> >         Mediatek switches.
> > 
> >  config NET_DSA_TAG_KSZ
> > -     tristate "Tag driver for Microchip 8795/9477/9893 families of
> > switches"
> > +     tristate "Tag driver for Microchip 8795/937x/9477/9893 families of
> > switches"
> >       help
> >         Say Y if you want to enable support for tagging frames for the
> > -       Microchip 8795/9477/9893 families of switches.
> > +       Microchip 8795/937x/9477/9893 families of switches.
> > 
> >  config NET_DSA_TAG_RTL4_A
> >       tristate "Tag driver for Realtek 4 byte protocol A tags"
> > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> > index 4820dbcedfa2..a67f21bdab8f 100644
> > --- a/net/dsa/tag_ksz.c
> > +++ b/net/dsa/tag_ksz.c
> > @@ -190,10 +190,68 @@ static const struct dsa_device_ops ksz9893_netdev_ops
> > = {
> >  DSA_TAG_DRIVER(ksz9893_netdev_ops);
> >  MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9893);
> > 
> > +/* For xmit, 2 bytes are added before FCS.
> > + * ------------------------------------------------------------------------
> > ---
> > + *
> > DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
> > + * ------------------------------------------------------------------------
> > ---
> > + * tag0 : represents tag override, lookup and valid
> > + * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x80=port8)
> > + *
> > + * For rcv, 1 byte is added before FCS.
> > + * ------------------------------------------------------------------------
> > ---
> > + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes)
> > + * ------------------------------------------------------------------------
> > ---
> > + * tag0 : zero-based value represents port
> > + *     (eg, 0x00=port1, 0x02=port3, 0x07=port8)
> > + */
> > +#define LAN937X_INGRESS_TAG_LEN              2
> > +
> > +#define LAN937X_TAIL_TAG_OVERRIDE    BIT(11)
> 
> I had to look up the datasheet, to see what this is, because "override"
> can mean many things, not all of them are desirable.
> 
> Port Blocking Override
> When set, packets will be sent from the specified port(s) regardless, and any
> port
> blocking (see Port Transmit Enable in Port MSTP State Register) is ignored.
> 
> Do you think you can name it LAN937X_TAIL_TAG_BLOCKING_OVERRIDE? I know
> it's longer, but it's also more suggestive.
Thanks for reviewing the patch series. Suggestion looks meaningful. Noted for
next rev.

> 
> > +#define LAN937X_TAIL_TAG_LOOKUP              BIT(12)
> > +#define LAN937X_TAIL_TAG_VALID               BIT(13)
> > +#define LAN937X_TAIL_TAG_PORT_MASK   7
> > +
> > +static struct sk_buff *lan937x_xmit(struct sk_buff *skb,
> > +                                 struct net_device *dev)
> > +{
> > +     struct dsa_port *dp = dsa_slave_to_port(dev);
> > +     __be16 *tag;
> > +     u8 *addr;
> > +     u16 val;
> > +r
> > +     tag = skb_put(skb, LAN937X_INGRESS_TAG_LEN);
> 
> Here we go again.. why is it called INGRESS_TAG_LEN if it is used during
> xmit, and only during xmit?
Definition is ingress to the LAN937x since it has different tag length for
ingress and egress of LAN937x. Do you think should it be changed? 


> 
> > +     addr = skb_mac_header(skb);
> 
> My personal choice would have been:
> 
>         const struct ethhdr *hdr = eth_hdr(skb);
> 
>         if (is_link_local_ether_addr(hdr->h_dest))
It makes more understandable since h_dest is passed. Noted.

> 
> > 
> > 2.27.0
> >