Re: [RESEND PATCH v1] net: dsa: motorcomm: add yt92xx dsa driver

[Andrew Lunn <andrew@lunn.ch>]

> >> +#define CMM_PARAM_CHK(expr, err_code)    \
> >> +       do {                             \
> >> +               if ((u32)(expr)) {       \
> >> +                       return err_code; \
> >> +               }                        \
> >> +       } while (0)
> >> +
> >> +#define CMM_ERR_CHK(op, ret)           \
> >> +       do {                           \
> >> +               ret = (op);            \
> >> +               if (ret != CMM_ERR_OK) \
> >> +                       return ret;    \
> >> +       } while (0)
> > 
> > Do not use macros like this.
> 
> Ans: Acknowledged, i will consider how to optimize them in the future.

It is not about optimization. Hiding a return statement in a macro is
very bad style. It will lead to locking bugs, and resource leaks,
because nobody knows the return is there.

> >> +/*
> >> + * Macro Definition
> >> + */
> >> +#ifndef NULL
> >> +#define NULL 0
> >> +#endif
> >> +
> >> +#ifndef FALSE
> >> +#define FALSE 0
> >> +#endif
> >> +
> >> +#ifndef TRUE
> >> +#define TRUE 1
> >> +#endif
> > 
> > Nonsense.
> 
> Ans: Acknowledge, will be fixed later.

No. They will be fixed now.

> >> +       /* Print chipid here since we are interested in lower 16 bits */
> >> +       dev_info(dev,
> >> +                "Motorcomm %s ethernet switch.\n",
> >> +                info->name);
> > 
> > Stop copy-n-paste.
> 
> Ans: Sry for this, i will recheck the code to make sure each line of comments and code
> meaningful again.

Also, consider the comments. Do the comments add anything useful which
is not already obvious from the code. Comments should be about "Why?".

> >> --- a/include/uapi/linux/if_ether.h
> >> +++ b/include/uapi/linux/if_ether.h
> >> @@ -118,7 +118,7 @@
> >>  #define ETH_P_QINQ1    0x9100          /* deprecated QinQ VLAN [ NOT AN OFFICIALLY REGISTERED ID ] */
> >>  #define ETH_P_QINQ2    0x9200          /* deprecated QinQ VLAN [ NOT AN OFFICIALLY REGISTERED ID ] */
> >>  #define ETH_P_QINQ3    0x9300          /* deprecated QinQ VLAN [ NOT AN OFFICIALLY REGISTERED ID ] */
> >> -#define ETH_P_YT921X   0x9988          /* Motorcomm YT921x DSA [ NOT AN OFFICIALLY REGISTERED ID ] */
> >> +#define ETH_P_YT92XX   0x9988          /* Motorcomm YT92xx DSA [ NOT AN OFFICIALLY REGISTERED ID ] */
> >>  #define ETH_P_EDSA     0xDADA          /* Ethertype DSA [ NOT AN OFFICIALLY REGISTERED ID ] */
> >>  #define ETH_P_DSA_8021Q        0xDADB          /* Fake VLAN Header for DSA [ NOT AN OFFICIALLY REGISTERED ID ] */
> >>  #define ETH_P_DSA_A5PSW        0xE001          /* A5PSW Tag Value [ NOT AN OFFICIALLY REGISTERED ID ] */
> > 
> > UAPI stands for User-space API. Do not change it unless there is a
> > very very good reason.
> > 
> 
> Ans: The default tpid both yt921x and yt922x is 0x9988. I have modified this to 
> allow for simultaneous use in both yt922x and yt921x scenarios.

As pointed out, this is UAPI. Any changes to this file need a good
explanation how it does not change the user API. Do this break
backwards compatibility with user space applications? Maybe tcpdump or
wireshark has a dissector which expects ETH_P_YT921X and you have just
broken it?

> >> +#define YT922X_TAG_FORMAT2_NAME "yt922x-8b"
> >> +#define YT922X_FORMAT2_TAG_LEN                  8
> >> +#define YT922X_PKT_TYPE          GENMASK(15, 14)
> >> +#define YT922X_8B_CPUTAG_PKT_FROM_CPU      0x1
> >> +#define YT922X_8B_CPUTAG_SRC_PORT          GENMASK(6, 2)
> >> +#define YT922X_8B_CPUTAG_DST_PORTMASK      GENMASK(8, 0)
> >> +#define YT922X_8B_CPUTAG_DST_PORTMASK_0      BIT(15)
> >> +#define YT922X_8B_CPUTAG_DST_PORTMASK_0_EN      0x1
> >> +#define YT922X_8B_CPUTAG_FORCE_DST         BIT(9)
> >> +#define YT922X_8B_CPUTAG_FORCE_DST_EN      0x1
> > 
> > If yt922x tag format shares no common with yt921x, make a new tag driver.
> 
> Ans: thank you for your suggestion, we will consider whether to create a new driver in the new file.

When you look at other tag drivers, you will also notice some drivers
implement two taggers in one file. So consider this if there is any
shared code.

> >> +static struct dsa_tag_driver *dsa_tag_driver_array[] = {
> >> +       &DSA_TAG_DRIVER_NAME(yt921x_netdev_ops),
> >> +       &DSA_TAG_DRIVER_NAME(yt922x_4b_netdev_ops),
> >> +       &DSA_TAG_DRIVER_NAME(yt922x_8b_netdev_ops),
> >> +};
> > 
> > If both are supported by the chip and 4b does nothing more than 8b
> > does, do not bother with it.
> 
> Ans: 4b and 8b dsa tag may have different application scenarios. from my opinion,
>      1. 4b dsa tag can save 4 bytes of payload
>      2. 8b dsa tag carry more package info.

How do you plan to swap between the different formats?

The user perspective is that the machine has a collection of interface
which are used just as normal, using Linux tools likes like
iproute2. If the user enables a feature which requires the 8b tag
format, will you change the format from the DSA driver? And swap back
to the 4 byte format when the feature is no longer needed?

	Andrew