On Tue Nov 10, 2020 at 3:59 PM CET, Vladimir Oltean wrote:
> On Tue, Nov 10, 2020 at 10:13:25AM +0100, Tobias Waldekranz wrote:
> > +config NET_DSA_TAG_DSA_COMMON
> > + tristate
> > + default n
>
> I think that "default n" is implicit and should be omitted.
Correct, will fix!
> > +/**
> > + * enum dsa_cmd - DSA Command
> > + * @DSA_CMD_TO_CPU: Set on packets that where trapped or mirrored to
>
> s/where/were/
ACK
> > + /* Construct tagged FROM_CPU DSA tag from 802.1Q tag. */
> > + dsa_header = skb->data + 2 * ETH_ALEN + extra;
> > + dsa_header[0] = (DSA_CMD_FROM_CPU << 6) | 0x20 | dp->ds->index;
>
> What is 0x20, BIT(5)? To denote that it's an 802.1Q tagged frame I
> suppose?
> Could it have a macro?
It could, there are loads of bare shifts and masks inherited from the
old taggers though. I suppose it would be nice to replace them with
symbolic names. Then again they are never used for anything else so
I'm not sure it adds that much. Andrew?
> > - /*
> > - * The ethertype field is part of the DSA header.
> > - */
> > + /* The ethertype field is part of the DSA header. */
>
> Could these comment style changes be a separate patch?
Sure, I'll separate them in v2.
> > +static const struct dsa_device_ops edsa_netdev_ops = {
> > + .name = "edsa",
> > + .proto = DSA_TAG_PROTO_EDSA,
> > + .xmit = edsa_xmit,
> > + .rcv = edsa_rcv,
> > + .overhead = EDSA_HLEN,
>
> Could you reindent these to be aligned?
Yeah, absolutely.
Thanks for the review!
