netdev - Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: enable DSCP to priority map for RX

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: <208a1472-c69b-4c20-9bb2-25158edfd7d8@ti.com>
Date: Fri, 8 Nov 2024 20:12:40 +0530
From: Siddharth Vadapalli <s-vadapalli@...com>
To: Roger Quadros <rogerq@...nel.org>
CC: Siddharth Vadapalli <s-vadapalli@...com>,
        Andrew Lunn
	<andrew+netdev@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni
	<pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
        <linux-omap@...r.kernel.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <srk@...com>,
        Pekka Varis <p-varis@...com>
Subject: Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: enable DSCP
 to priority map for RX

On Fri, Nov 08, 2024 at 02:55:18PM +0200, Roger Quadros wrote:
> Hi Siddharth,
> 
> On 08/11/2024 14:30, Siddharth Vadapalli wrote:

[...]

> >> +#define AM65_CPSW_PORTN_REG_CTL			0x004
> > 
> > nitpick: indentation needs to be fixed here to align with the macros
> > below.
> 
> It is fine in the code and in my editor in this reply email.

That's strange. But it appears the same to me as seen at:
https://lore.kernel.org/r/20241105-am65-cpsw-multi-rx-dscp-v1-2-38db85333c88@kernel.org/
where the indentation looks incorrect.

[...]

> 
> >> +
> >> +	if (dscp > AM65_CPSW_DSCP_MAX)
> >> +		return -EINVAL;
> > 
> > am65_cpsw_port_set_dscp_map() seems to be invoked by
> > am65_cpsw_port_enable_dscp_map() below, where the above check is guaranteed
> > to be satisfied. Is the check added for future-proofing this function?
> > 
> 
> Right, future callers can't be guaranteed to do the check so I'd prefer
> to have the check here.

Thank you for the confirmation.

> 
> >> +
> >> +	if (pri > AM65_CPSW_PRI_MAX)
> >> +		return -EINVAL;
> >> +
> >> +	reg_ofs = (dscp / 8) * 4;	/* reg offset to this dscp */
> >> +	bit_ofs = 4 * (dscp % 8);	/* bit offset to this dscp */
> > 
> > Maybe a macro can be used for the "4" since it is not clear what it
> 
> First 4 was for 4 bytes per register. Not sure if we need a macro for this.
> The comment already mentions register offset and we know each register is
> 32-bits wide.
> 
> We could add a macro for the 8 though
> #define AM65_CPSW_DSCP_PRI_PER_REG	8
> 
> The second 4 is actually 4 bits per DSCP field. I could add a macro for this.
> #define AM65_CPSW_DSCP_PRI_FIELD_WIDTH	4

This looks good to me, but I am fine either way, in case you prefer to
drop the macros.

> 
> 
> > corresponds to. Or maybe two macros can be used for "reg_ofs" and
> > "bit_ofs".
> > 
> >> +	val = readl(slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs);
> >> +	val &= ~(AM65_CPSW_PRI_MAX << bit_ofs);	/* clear */
> >> +	val |= pri << bit_ofs;			/* set */
> >> +	writel(val, slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs);
> >> +	val = readl(slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs);
> > 
> > The above readback seems to be just to flush the writel(). A comment of
> > the form:
> > /* flush */
> > might help, considering that other drivers do the same. Also, assigning
> > the returned value to "val" might not be required unless it is intended to
> > be checked.
> 
> This was actually left over debug code. I'll drop the readl.

Ok.

Regards,
Siddharth.