netdev - Re: [PATCH] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <C6ZQR6IP6FDI.19PS7RY5FUQIG@wkz-x280>
Date:   Tue, 10 Nov 2020 17:57:54 +0100
From:   "Tobias Waldekranz" <tobias@...dekranz.com>
To:     "Vladimir Oltean" <olteanv@...il.com>
Cc:     <davem@...emloft.net>, <kuba@...nel.org>, <andrew@...n.ch>,
        <vivien.didelot@...il.com>, <f.fainelli@...il.com>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: dsa: tag_dsa: Unify regular and ethertype DSA
 taggers

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!