Re: [PATCH v3 net-next] net: phy: air_en8811h: select LED GPIO pins based on AN8811HB package variant

[Paolo Abeni]

On 9/9/26 6:57 PM, netdev-bot+sashiko@xxxxxxxxxx wrote:
>> @@ -1115,6 +1167,12 @@ static int an8811hb_probe(struct phy_device *phydev)
>>  	/* MDIO_DEVS1/2 empty, so set mmds_present bits here */
>>  	phydev->c45_ids.mmds_present |= MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
>>  
>> +	/* Detect package variant */
>> +	ret = air_phy_buckpbus_reg_read(phydev, AN8811HB_HWTRAP2, &reg_val);
>> +	if (ret < 0)
>> +		goto err_dev_create;
>> +	priv->is_an8811hbcn = FIELD_GET(AN8811HB_HWTRAP2_PKG, reg_val);
>> +
>>  	ret = en8811h_leds_setup(phydev);
>>  	if (ret < 0)
>>  		goto err_dev_create;
> 
> [Severity: Medium]
> AN8811HB_HWTRAP2_PKG is GENMASK(14, 12), so FIELD_GET() yields a
> 3-bit value here, but it is stored straight into the bool
> priv->is_an8811hbcn.  Any non-zero package code therefore collapses to
> true, which means the whole variant decision in
> an8811hb_led_gpio_setup() rests on the unstated assumption that
> AN8811HBCN is the only encoding with a non-zero value and AN8811HBN
> reads back exactly 0.
> 
> If the field can take any other value - a second AN8811HBN encoding, a
> reserved code, or a future package - it lands in the
> AN8811HB_GPIO_OUTPUT_0115 branch and the driver drives GPIO 0/1/15
> plus the SEL1/SEL2 mux for a part whose LEDs are not on those pads.
> That failure mode is silent: the LEDs simply do not work, and there is
> nothing in the source that documents what the encoding actually is.
> 
> Could you define the expected package identifiers as named constants
> and compare against them explicitly, e.g. something like
> 
> 	pkg = FIELD_GET(AN8811HB_HWTRAP2_PKG, reg_val);
> 	priv->is_an8811hbcn = (pkg == AN8811HB_HWTRAP2_PKG_HBCN);
> 
> so that unknown codes fall into the conservative branch rather than
> being treated as AN8811HBCN?  If the hardware really only ever reports
> two values and 0 is guaranteed to mean AN8811HBN, please say so in a
> comment next to the assignment, since the reader cannot tell that from
> the mask alone.  Also, is it worth warning (or failing probe) on an
> unrecognised package code rather than silently picking a GPIO mapping?

I think we are better off without per pkg macros until there are only
2 of them around.

All others are doubtful nit-picks.

@Weiting: note that you are requested to address sashiko comments
proactively.

/P


>