linux-hardening - Re: [PATCH net-next v3 04/14] net: ethernet: qualcomm: Initialize PPE buffer management for IPQ9574

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: <cffdd8e8-76bc-4424-8cdb-d48f5010686d@quicinc.com>
Date: Thu, 6 Mar 2025 18:01:49 +0800
From: Jie Luo <quic_luoj@...cinc.com>
To: Andrew Lunn <andrew@...n.ch>
CC: 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>, Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
	<conor+dt@...nel.org>, Lei Wei <quic_leiwei@...cinc.com>,
        Suruchi Agarwal
	<quic_suruchia@...cinc.com>,
        Pavithra R <quic_pavir@...cinc.com>,
        "Simon
 Horman" <horms@...nel.org>, Jonathan Corbet <corbet@....net>,
        Kees Cook
	<kees@...nel.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        "Philipp
 Zabel" <p.zabel@...gutronix.de>,
        <linux-arm-msm@...r.kernel.org>, <netdev@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <linux-hardening@...r.kernel.org>,
        <quic_kkumarcs@...cinc.com>, <quic_linchen@...cinc.com>,
        <srinivas.kandagatla@...aro.org>, <bartosz.golaszewski@...aro.org>,
        <john@...ozen.org>
Subject: Re: [PATCH net-next v3 04/14] net: ethernet: qualcomm: Initialize PPE
 buffer management for IPQ9574



On 2/20/2025 11:09 PM, Andrew Lunn wrote:
> On Thu, Feb 20, 2025 at 10:38:03PM +0800, Jie Luo wrote:
>>
>>
>> On 2/11/2025 9:22 PM, Andrew Lunn wrote:
>>>> +	/* Configure BM flow control related threshold. */
>>>> +	PPE_BM_PORT_FC_SET_WEIGHT(bm_fc_val, port_cfg.weight);
>>>> +	PPE_BM_PORT_FC_SET_RESUME_OFFSET(bm_fc_val, port_cfg.resume_offset);
>>>> +	PPE_BM_PORT_FC_SET_RESUME_THRESHOLD(bm_fc_val, port_cfg.resume_ceil);
>>>> +	PPE_BM_PORT_FC_SET_DYNAMIC(bm_fc_val, port_cfg.dynamic);
>>>> +	PPE_BM_PORT_FC_SET_REACT_LIMIT(bm_fc_val, port_cfg.in_fly_buf);
>>>> +	PPE_BM_PORT_FC_SET_PRE_ALLOC(bm_fc_val, port_cfg.pre_alloc);
>>>
>>> ...
>>>
>>>> +#define PPE_BM_PORT_FC_CFG_TBL_ADDR		0x601000
>>>> +#define PPE_BM_PORT_FC_CFG_TBL_ENTRIES		15
>>>> +#define PPE_BM_PORT_FC_CFG_TBL_INC		0x10
>>>> +#define PPE_BM_PORT_FC_W0_REACT_LIMIT		GENMASK(8, 0)
>>>> +#define PPE_BM_PORT_FC_W0_RESUME_THRESHOLD	GENMASK(17, 9)
>>>> +#define PPE_BM_PORT_FC_W0_RESUME_OFFSET		GENMASK(28, 18)
>>>> +#define PPE_BM_PORT_FC_W0_CEILING_LOW		GENMASK(31, 29)
>>>> +#define PPE_BM_PORT_FC_W1_CEILING_HIGH		GENMASK(7, 0)
>>>> +#define PPE_BM_PORT_FC_W1_WEIGHT		GENMASK(10, 8)
>>>> +#define PPE_BM_PORT_FC_W1_DYNAMIC		BIT(11)
>>>> +#define PPE_BM_PORT_FC_W1_PRE_ALLOC		GENMASK(22, 12)
>>>> +
>>>> +#define PPE_BM_PORT_FC_SET_REACT_LIMIT(tbl_cfg, value)	\
>>>> +	u32p_replace_bits((u32 *)tbl_cfg, value, PPE_BM_PORT_FC_W0_REACT_LIMIT)
>>>> +#define PPE_BM_PORT_FC_SET_RESUME_THRESHOLD(tbl_cfg, value)	\
>>>> +	u32p_replace_bits((u32 *)tbl_cfg, value, PPE_BM_PORT_FC_W0_RESUME_THRESHOLD)
>>>
>>> Where is u32p_replace_bits()?
>>
>> u32p_replace_bits is defined by the macro __MAKE_OP(32) in the header
>> file "include/linux/bitfield.h".
> 
> Given it is pretty well hidden, and not documented, it makes me think
> you should not be using it. The macros you are expected to use from
> that file are all well documented.

OK, understand.

> 
>>> This cast does not look good.
>>
>> Yes, we can remove the cast.
> 
> To some extent, this is a symptom. Why is the cast there in the first
> place? Cast suggest bad design, not thinking about types, thinking it
> is actual O.K. to cast between types. Please look at all the casts you
> have. Is it because of bad design? If so, please fix your types to
> eliminate the casts.

Sure, this cast is actually redundant, the type of value passed to this
macro is already defined as the type u32. I will review and remove the
remaining casts in the ppe_reg.h file.

> 
>>> And this does not look like anything any
>>> other driver does. I suspect you are not using FIELD_PREP() etc when
>>> you should.
>>>
>>> https://elixir.bootlin.com/linux/v6.14-rc2/source/include/linux/bitfield.h
>>>
>>> 	Andrew
>>
>> The PPE_BM_XXX macros defined here write to either of two different
>> 32bit words in the register table, and the actual word used (0 or 1)
>> is hidden within the macro. For example, the below macro.
>>
>> #define PPE_BM_PORT_FC_SET_CEILING_HIGH(tbl_cfg, value)	\
>> 	u32p_replace_bits((u32 *)(tbl_cfg) + 0x1, value,
>> 	PPE_BM_PORT_FC_W1_CEILING_HIGH)
>>
>> We could have used FIELD_PREP as well for this purpose. However using
>> u32p_replace_bits() seemed more convenient and cleaner in this case,
>> since with FIELD_PREP, we would have needed an assignment statement to
>> be defined in the macro implementation. We also noticed many other
>> drivers using u32_replace_bits(). Hope this is ok.
> 
> Please extend the set of FIELD_{GET,PREP} macros to cover your use
> case. Document them to the level of the existing macros. Submit the
> patch to:
> 
> Yury Norov <yury.norov@...il.com> (maintainer:BITMAP API)
> Rasmus Villemoes <linux@...musvillemoes.dk> (reviewer:BITMAP API)
> etc
> 
> and see what they say about this.
> 
> 	Andrew

Thanks for the suggestion. Just to clarify, we preferred
u32p_replace_bits() over FIELD_PREP() because the former does
a clear-and-set operation against a given mask, where as with
FIELD_PREP(), we need to clear the bits first before we use the
macro and then set it. Due to this, we preferred using
u32_replace_bits() since it made the macro definitions to modify
the registers simpler. Given this, would it be acceptable to
document u32p_replace_bits() better, as it is already being used
by other drivers as well?

If you prefer to use FIELD_PREP() over u32p_replace_bits(), we
can update the driver to change the macros to use FIELD_PREP().
Please note that all our macros for register modifications
operate only on 32bit values. so we do not have any necessity
for casts in the code.

Below is one example per my understanding, implemented for
both cases - u32p_replace_bits() and FIELD_PREP:

#define PPE_BM_PORT_FC_SET_WEIGHT(tbl_cfg, value) \
       u32p_replace_bits(tbl_cfg + 0x1, value, PPE_BM_PORT_FC_W1_WEIGHT)
		
#define PPE_BM_PORT_FC_SET_WEIGHT(tbl_cfg, value) \
do { \
      *(tbl_cfg + 0x1) &= ~PPE_BM_PORT_FC_W1_WEIGHT; \
      *(tbl_cfg + 0x1) |= FIELD_PREP(PPE_BM_PORT_FC_W1_WEIGHT, value); \
} while (0)