| 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
| ||
|
Message-ID: <36ea74e1-9caa-47b9-a614-5633f38f1444@quicinc.com>
Date: Fri, 10 Jan 2025 23:42:08 +0800
From: Jie Luo <quic_luoj@...cinc.com>
To: Simon Horman <horms@...nel.org>
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>,
"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 v2 04/14] net: ethernet: qualcomm: Initialize PPE
buffer management for IPQ9574
On 1/10/2025 4:11 AM, Simon Horman wrote:
> On Thu, Jan 09, 2025 at 05:27:14PM +0000, Simon Horman wrote:
>> On Wed, Jan 08, 2025 at 09:47:11PM +0800, Luo Jie wrote:
>>> The BM (Buffer Management) config controls the pause frame generated
>>> on the PPE port. There are maximum 15 BM ports and 4 groups supported,
>>> all BM ports are assigned to group 0 by default. The number of hardware
>>> buffers configured for the port influence the threshold of the flow
>>> control for that port.
>>>
>>> Signed-off-by: Luo Jie <quic_luoj@...cinc.com>
>>
>> ...
>>
>>> diff --git a/drivers/net/ethernet/qualcomm/ppe/ppe_config.c b/drivers/net/ethernet/qualcomm/ppe/ppe_config.c
>>
>> ...
>>
>>> +/* The buffer configurations per PPE port. There are 15 BM ports and
>>> + * 4 BM groups supported by PPE. BM port (0-7) is for EDMA port 0,
>>> + * BM port (8-13) is for PPE physical port 1-6 and BM port 14 is for
>>> + * EIP port.
>>> + */
>>> +static struct ppe_bm_port_config ipq9574_ppe_bm_port_config[] = {
>>> + {
>>> + /* Buffer configuration for the BM port ID 0 of EDMA. */
>>> + .port_id_start = 0,
>>> + .port_id_end = 0,
>>> + .pre_alloc = 0,
>>> + .in_fly_buf = 100,
>>> + .ceil = 1146,
>>> + .weight = 7,
>>> + .resume_offset = 8,
>>> + .resume_ceil = 0,
>>> + .dynamic = true,
>>> + },
>>> + {
>>> + /* Buffer configuration for the BM port ID 1-7 of EDMA. */
>>> + .port_id_start = 1,
>>> + .port_id_end = 7,
>>> + .pre_alloc = 0,
>>> + .in_fly_buf = 100,
>>> + .ceil = 250,
>>> + .weight = 4,
>>> + .resume_offset = 36,
>>> + .resume_ceil = 0,
>>> + .dynamic = true,
>>> + },
>>> + {
>>> + /* Buffer configuration for the BM port ID 8-13 of PPE ports. */
>>> + .port_id_start = 8,
>>> + .port_id_end = 13,
>>> + .pre_alloc = 0,
>>> + .in_fly_buf = 128,
>>> + .ceil = 250,
>>> + .weight = 4,
>>> + .resume_offset = 36,
>>> + .resume_ceil = 0,
>>> + .dynamic = true,
>>> + },
>>> + {
>>> + /* Buffer configuration for the BM port ID 14 of EIP. */
>>> + .port_id_start = 14,
>>> + .port_id_end = 14,
>>> + .pre_alloc = 0,
>>> + .in_fly_buf = 40,
>>> + .ceil = 250,
>>> + .weight = 4,
>>> + .resume_offset = 36,
>>> + .resume_ceil = 0,
>>> + .dynamic = true,
>>> + },
>>> +};
>>> +
>>> +static int ppe_config_bm_threshold(struct ppe_device *ppe_dev, int bm_port_id,
>>> + struct ppe_bm_port_config port_cfg)
>>> +{
>>> + u32 reg, val, bm_fc_val[2];
>>> + int ret;
>>> +
>>> + /* Configure BM flow control related threshold. */
>>> + PPE_BM_PORT_FC_SET_WEIGHT(bm_fc_val, port_cfg.weight);
>>
>> Hi Luo Jie,
>>
>> When compiling with W=1 for x86_32 and ARM (32bit)
>> (but, curiously not x86_64 or arm64), gcc-14 complains that
>> bm_fc_val is uninitialised, I believe due to the line above and
>> similar lines below.
>>
>> In file included from drivers/net/ethernet/qualcomm/ppe/ppe_config.c:10:
>> In function 'u32p_replace_bits',
>> inlined from 'ppe_config_bm_threshold' at drivers/net/ethernet/qualcomm/ppe/ppe_config.c:112:2:
>> ./include/linux/bitfield.h:189:15: warning: 'bm_fc_val' is used uninitialized [-Wuninitialized]
>> 189 | *p = (*p & ~to(field)) | type##_encode_bits(val, field); \
>> | ^~
>> ./include/linux/bitfield.h:198:9: note: in expansion of macro '____MAKE_OP'
>> 198 | ____MAKE_OP(u##size,u##size,,)
>> | ^~~~~~~~~~~
>> ./include/linux/bitfield.h:201:1: note: in expansion of macro '__MAKE_OP'
>> 201 | __MAKE_OP(32)
>> | ^~~~~~~~~
>> drivers/net/ethernet/qualcomm/ppe/ppe_config.c: In function 'ppe_config_bm_threshold':
>> drivers/net/ethernet/qualcomm/ppe/ppe_config.c:108:23: note: 'bm_fc_val' declared here
>> 108 | u32 reg, val, bm_fc_val[2];
>> | ^~~~~~~~~
>>
>>> + 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);
>>> +
>>> + /* Configure low/high bits of the ceiling for the BM port. */
>>> + val = FIELD_PREP(GENMASK(2, 0), port_cfg.ceil);
>>
>> The value of port_cfg.ceil is 250 or 1146, as set in
>> ipq9574_ppe_bm_port_config. clang-19 W=1 builds complain that this
>> value is too large for the field (3 bits).
>>
>> drivers/net/ethernet/qualcomm/ppe/ppe_config.c:120:8: error: call to '__compiletime_assert_925' declared with 'error' attribute: FIELD_PREP: value too large for the field
>> 120 | val = FIELD_PREP(GENMASK(2, 0), port_cfg.ceil);
>> | ^
>> ./include/linux/bitfield.h:115:3: note: expanded from macro 'FIELD_PREP'
>> 115 | __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
>> | ^
>> ./include/linux/bitfield.h:68:3: note: expanded from macro '__BF_FIELD_CHECK'
>> 68 | BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
>> | ^
>> ./include/linux/build_bug.h:39:37: note: expanded from macro 'BUILD_BUG_ON_MSG'
>> 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>> | ^
>> note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
>> ././include/linux/compiler_types.h:530:2: note: expanded from macro '_compiletime_assert'
>> 530 | __compiletime_assert(condition, msg, prefix, suffix)
>> | ^
>> ././include/linux/compiler_types.h:523:4: note: expanded from macro '__compiletime_assert'
>> 523 | prefix ## suffix(); \
>> | ^
>> <scratch space>:95:1: note: expanded from here
>> 95 | __compiletime_assert_925
>> | ^
>> 1 error generated
>>
>>> + PPE_BM_PORT_FC_SET_CEILING_LOW(bm_fc_val, val);
>>> + val = FIELD_PREP(GENMASK(10, 3), port_cfg.ceil);
>
> One more thing - I was pondering this over dinner.
>
> I believe that the above will write the bits 0-7 of port_cfg.ceil
> to bits 3-10 of val. I am guessing that the reverse mapping of
> bits is intended.
>
Thanks for catching this, yes, I will update this code. And also run the
32 bit ARCH check on the patches.
>>> + PPE_BM_PORT_FC_SET_CEILING_HIGH(bm_fc_val, val);
>>> +
>>> + reg = PPE_BM_PORT_FC_CFG_TBL_ADDR + PPE_BM_PORT_FC_CFG_TBL_INC * bm_port_id;
>>> + ret = regmap_bulk_write(ppe_dev->regmap, reg,
>>> + bm_fc_val, ARRAY_SIZE(bm_fc_val));
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* Assign the default group ID 0 to the BM port. */
>>> + val = FIELD_PREP(PPE_BM_PORT_GROUP_ID_SHARED_GROUP_ID, 0);
>>> + reg = PPE_BM_PORT_GROUP_ID_ADDR + PPE_BM_PORT_GROUP_ID_INC * bm_port_id;
>>> + ret = regmap_update_bits(ppe_dev->regmap, reg,
>>> + PPE_BM_PORT_GROUP_ID_SHARED_GROUP_ID,
>>> + val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* Enable BM port flow control. */
>>> + val = FIELD_PREP(PPE_BM_PORT_FC_MODE_EN, true);
>>> + reg = PPE_BM_PORT_FC_MODE_ADDR + PPE_BM_PORT_FC_MODE_INC * bm_port_id;
>>> +
>>> + return regmap_update_bits(ppe_dev->regmap, reg,
>>> + PPE_BM_PORT_FC_MODE_EN,
>>> + val);
>>> +}
>>
>> ...
>>
>> --
>> pw-bot: changes-requested
>>
Powered by blists - more mailing lists