Re: [PATCH net-next v21 02/14] net: ethtool: Introduce ETHTOOL_LINK_MEDIUM_* values

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Paolo

On 02/12/2025 14:03, Paolo Abeni wrote:
> On 11/29/25 9:22 AM, Maxime Chevallier wrote:
>> @@ -298,138 +321,149 @@ static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
>>  		.speed	= SPEED_UNKNOWN, \
>>  		.lanes	= 0, \
>>  		.duplex	= DUPLEX_UNKNOWN, \
>> +		.mediums = BIT(ETHTOOL_LINK_MEDIUM_NONE), \
>>  	}
>>  
>>  const struct link_mode_info link_mode_params[] = {
>> -	__DEFINE_LINK_MODE_PARAMS(10, T, Half),
>> -	__DEFINE_LINK_MODE_PARAMS(10, T, Full),
>> -	__DEFINE_LINK_MODE_PARAMS(100, T, Half),
>> -	__DEFINE_LINK_MODE_PARAMS(100, T, Full),
>> -	__DEFINE_LINK_MODE_PARAMS(1000, T, Half),
>> -	__DEFINE_LINK_MODE_PARAMS(1000, T, Full),
>> +	__DEFINE_LINK_MODE_PARAMS_PAIRS(10, T, 2, 4, Half, T),
>> +	__DEFINE_LINK_MODE_PARAMS_PAIRS(10, T, 2, 4, Full, T),
>> +	__DEFINE_LINK_MODE_PARAMS_PAIRS(100, T, 2, 4, Half, T),
>> +	__DEFINE_LINK_MODE_PARAMS_PAIRS(100, T, 2, 4, Full, T),
>> +	__DEFINE_LINK_MODE_PARAMS_PAIRS(1000, T, 4, 4, Half, T),
>> +	__DEFINE_LINK_MODE_PARAMS_PAIRS(1000, T, 4, 4, Full, T),
>>  	__DEFINE_SPECIAL_MODE_PARAMS(Autoneg),
>>  	__DEFINE_SPECIAL_MODE_PARAMS(TP),
>>  	__DEFINE_SPECIAL_MODE_PARAMS(AUI),
>>  	__DEFINE_SPECIAL_MODE_PARAMS(MII),
>>  	__DEFINE_SPECIAL_MODE_PARAMS(FIBRE),
>>  	__DEFINE_SPECIAL_MODE_PARAMS(BNC),
>> -	__DEFINE_LINK_MODE_PARAMS(10000, T, Full),
>> +	__DEFINE_LINK_MODE_PARAMS_PAIRS(10000, T, 4, 4, Full, T),
>>  	__DEFINE_SPECIAL_MODE_PARAMS(Pause),
>>  	__DEFINE_SPECIAL_MODE_PARAMS(Asym_Pause),
>> -	__DEFINE_LINK_MODE_PARAMS(2500, X, Full),
>> +	__DEFINE_LINK_MODE_PARAMS_MEDIUMS(2500, X, Full,
>> +					  __MED(C) | __MED(S) | __MED(L)),
>>  	__DEFINE_SPECIAL_MODE_PARAMS(Backplane),
>> -	__DEFINE_LINK_MODE_PARAMS(1000, KX, Full),
>> -	__DEFINE_LINK_MODE_PARAMS(10000, KX4, Full),
>> -	__DEFINE_LINK_MODE_PARAMS(10000, KR, Full),
>> +	__DEFINE_LINK_MODE_PARAMS(1000, KX, Full, K),
>> +	__DEFINE_LINK_MODE_PARAMS(10000, KX4, Full, K),
>> +	__DEFINE_LINK_MODE_PARAMS(10000, KR, Full, K),
>>  	[ETHTOOL_LINK_MODE_10000baseR_FEC_BIT] = {
>>  		.speed	= SPEED_10000,
>>  		.lanes	= 1,
>>  		.duplex = DUPLEX_FULL,
> 
> The AI review points that medium is not initialized here:
> 
> https://netdev-ai.bots.linux.dev/ai-review.html?id=437cd013-c6a6-49e1-bec1-de4869930c7a#patch-1
> 
> Is that intentional? It should deserve at least an explanation in the
> commit message.

Yes it is OK, however I don't really know how to answer AI on that. I'm
sorry it's still a bit blurry to me what's the right way to proceed with
these reviews.

Should I paste the AI report, then reply to it ?

I'd rather add more comments to the code than say in my commit log "AI
says xxx, it's wrong because blabla" though.

> 
> Somewhat related, AI raised on the first patch the same question raised
> on a previous iteration, and I assumed you considered that valid,
> according to:
> 
> https://lore.kernel.org/netdev/f753719e-2370-401d-a001-821bdd5ee838@bootlin.com/

So I don't know either how to proceed with this. dt_binding_check is
fine with the current state, and Rob acked the patch. I am not sure how
it's going to be received if I reach out to DT maintainers saying "the
netdev LLM said XXX, is this correct or hallucination ?", but it may
very well have a good point. I did try to dive into the yaml and then
json schema specs, but I wasn't able to go far enough to reach a proper
conclusion on wether we must remove "contains" for scalar :(

All of that would be a bit clearer if the AI review was on the ML, but I
also understand the risk for pollution with that, especially at an early
stage of adoption.

> 
> Otherwise I think some wording in the commit message explaining why the
> AI feedback is incorrect would be useful.
> 
> /P
> 

I'll update the commits and add comments anyways. I guess in a month
though, with net-next closed then the end of year.

Thanks,

Maxime