Re: [PATCH v2 net-next] net/hsr: add protocol version to fill_info output

[Fernando Fernandez Mancera <fmancera@suse.de>]

On 9/25/25 1:40 AM, Jakub Kicinski wrote:
> On Wed, 24 Sep 2025 13:21:32 +0200 Ján Václav wrote:
>> On Wed, Sep 24, 2025 at 2:06 AM Jakub Kicinski <kuba@kernel.org> wrote:
>>> On Mon, 22 Sep 2025 11:37:45 +0200 Jan Vaclav wrote:
>>>>        if (hsr->prot_version == PRP_V1)
>>>>                proto = HSR_PROTOCOL_PRP;
>>>> +     if (nla_put_u8(skb, IFLA_HSR_VERSION, hsr->prot_version))
>>>> +             goto nla_put_failure;
>>>
>>> Looks like configuration path does not allow setting version if proto
>>> is PRP. Should we add an else before the if? since previous if is
>>> checking for PRP already
>>>   
>>
>> The way HSR configuration is currently handled seems very confusing to
>> me, because it allows setting the protocol version, but for PRP_V1
>> only as a byproduct of setting the protocol to PRP. If you configure
>> an interface with (proto = PRP, version = PRP_V1), it will fail, which
>> seems wrong to me, considering this is the end result of configuring
>> only with proto = PRP anyways.
> 
> I'm not very familiar with HSR or PRP. But The PRP_V1 which has value
> of 3 looks like a kernel-internal hack. Or does the protocol actually
> specify value 3 to mean PRP?
> 
> I don't think there's anything particularly wrong with the code.
> The version is for HSR because PRP only has one version, there's no
> ambiguity.
> 
> But again, I'm just glancing at the code I could be wrong..
> 

No you are right, this is a hack made to integrate PRP with HSR driver. 
PRP does not have a version other than PRP_V1 therefore it does not make 
much sense to configure it. Having said that, I think it's weird to 
report HSR_VERSION 3 but fail when configuring it.

IMHO HSR_VERSION should be hidden for PRP or it should be possible to 
configure it to "3" (which now that you say it, it looks weird).