linux-kernel - Re: [PATCH] phy: stm32: work around constant-value overflow assertion

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ef1ea5da-4344-4c59-8d2b-1b52533ef0cd@foss.st.com>
Date: Fri, 10 Jan 2025 13:57:31 +0100
From: Christian Bruel <christian.bruel@...s.st.com>
To: Arnd Bergmann <arnd@...nel.org>, Vinod Koul <vkoul@...nel.org>,
        Kishon
 Vijay Abraham I <kishon@...nel.org>,
        Maxime Coquelin
	<mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>
CC: Arnd Bergmann <arnd@...db.de>, Yang Li <yang.lee@...ux.alibaba.com>,
        <linux-phy@...ts.infradead.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] phy: stm32: work around constant-value overflow assertion

Hello,

Sorry, I missed this patch.

The gcc warning bellow is a false positive as the range is necessarily 
between the min_imp/max_imp values, thus imp_lookup will catch, or 
returned -EINVAL earlier

So maybe a more explicit way to shut-up the warning would be to use a 
boolean flag with a WARN_ON, something like:

     bool found;

     for (imp_of = 0; imp_of < ARRAY_SIZE(imp_lookup); imp_of++) {
      if (imp_lookup[imp_of].microohm <= val) {
          found = true;
          break;
        }
     }

     dev_dbg(combophy->dev, "Set %u micro-ohms output impedance\n",
                         imp_lookup[imp_of].microohm);

     if (WARN_ON(! found))
        return;

     regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
                        STM32MP25_PCIEPRG_IMPCTRL_OHM,
                        FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_OHM, imp_of));

Regards

Christian

On 11/11/24 11:37, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.de>
> 
> FIELD_PREP() checks that a constant fits into the available bitfield,
> but if one of the two lookup tables in stm32_impedance_tune() does
> not find a matching entry, the index is out of range, which gcc
> correctly complains about:
> 
> In file included from <command-line>:
> In function 'stm32_impedance_tune',
>      inlined from 'stm32_combophy_pll_init' at drivers/phy/st/phy-stm32-combophy.c:247:9:
> include/linux/compiler_types.h:517:38: error: call to '__compiletime_assert_447' declared with attribute error: FIELD_PREP: value too large for the field
>    517 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>        |                                      ^
> include/linux/bitfield.h:68:3: note: in expansion of macro 'BUILD_BUG_ON_MSG'
>     68 |   BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?  \
>    115 |   __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
>        |   ^~~~~~~~~~~~~~~~
> drivers/phy/st/phy-stm32-combophy.c:162:8: note: in expansion of macro 'FIELD_PREP'
>    162 |        FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_VSWING, vswing_of));
>        |        ^~~~~~~~~~
> 
> Rework this so the field value gets set inside of the loop and otherwise
> set to zero.
> 
> Fixes: 47e1bb6b4ba0 ("phy: stm32: Add support for STM32MP25 COMBOPHY.")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
>   drivers/phy/st/phy-stm32-combophy.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/st/phy-stm32-combophy.c b/drivers/phy/st/phy-stm32-combophy.c
> index 765bb34fe358..49e9fa90a681 100644
> --- a/drivers/phy/st/phy-stm32-combophy.c
> +++ b/drivers/phy/st/phy-stm32-combophy.c
> @@ -122,6 +122,7 @@ static int stm32_impedance_tune(struct stm32_combophy *combophy)
>   	u32 max_vswing = imp_lookup[imp_size - 1].vswing[vswing_size - 1];
>   	u32 min_vswing = imp_lookup[0].vswing[0];
>   	u32 val;
> +	u32 regval;
>   
>   	if (!of_property_read_u32(combophy->dev->of_node, "st,output-micro-ohms", &val)) {
>   		if (val < min_imp || val > max_imp) {
> @@ -129,16 +130,20 @@ static int stm32_impedance_tune(struct stm32_combophy *combophy)
>   			return -EINVAL;
>   		}
>   
> -		for (imp_of = 0; imp_of < ARRAY_SIZE(imp_lookup); imp_of++)
> -			if (imp_lookup[imp_of].microohm <= val)
> +		regval = 0;
> +		for (imp_of = 0; imp_of < ARRAY_SIZE(imp_lookup); imp_of++) {
> +			if (imp_lookup[imp_of].microohm <= val) {
> +				regval = FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_OHM, imp_of);
>   				break;
> +			}
> +		}
>   
>   		dev_dbg(combophy->dev, "Set %u micro-ohms output impedance\n",
>   			imp_lookup[imp_of].microohm);
>   
>   		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
>   				   STM32MP25_PCIEPRG_IMPCTRL_OHM,
> -				   FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_OHM, imp_of));
> +				   regval);
>   	} else {
>   		regmap_read(combophy->regmap, SYSCFG_PCIEPRGCR, &val);
>   		imp_of = FIELD_GET(STM32MP25_PCIEPRG_IMPCTRL_OHM, val);
> @@ -150,16 +155,20 @@ static int stm32_impedance_tune(struct stm32_combophy *combophy)
>   			return -EINVAL;
>   		}
>   
> -		for (vswing_of = 0; vswing_of < ARRAY_SIZE(imp_lookup[imp_of].vswing); vswing_of++)
> -			if (imp_lookup[imp_of].vswing[vswing_of] >= val)
> +		regval = 0;
> +		for (vswing_of = 0; vswing_of < ARRAY_SIZE(imp_lookup[imp_of].vswing); vswing_of++) {
> +			if (imp_lookup[imp_of].vswing[vswing_of] >= val) {
> +				regval = FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_VSWING, vswing_of);
>   				break;
> +			}
> +		}
>   
>   		dev_dbg(combophy->dev, "Set %u microvolt swing\n",
>   			 imp_lookup[imp_of].vswing[vswing_of]);
>   
>   		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
>   				   STM32MP25_PCIEPRG_IMPCTRL_VSWING,
> -				   FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_VSWING, vswing_of));
> +				   regval);
>   	}
>   
>   	return 0;