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

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

[Jan Vaclav]

Currently, it is possible to configure IFLA_HSR_VERSION, but
there is no way to check in userspace what the currently
configured HSR protocol version is.

Add it to the output of hsr_fill_info().

This info could then be used by e.g. ip(8), like so:
$ ip -d link show hsr0
12: hsr0: <BROADCAST,MULTICAST> mtu ...
    ...
    hsr slave1 veth0 slave2 veth1 ... proto 0 version 1

Signed-off-by: Jan Vaclav <jvaclav@redhat.com>
---
v2: Added an example to the commit message to show
    how userspace programs could use this property.

v1: https://lore.kernel.org/netdev/20250918125337.111641-2-jvaclav@redhat.com/

 net/hsr/hsr_netlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/hsr/hsr_netlink.c b/net/hsr/hsr_netlink.c
index b12047024..3d0bd24c6 100644
--- a/net/hsr/hsr_netlink.c
+++ b/net/hsr/hsr_netlink.c
@@ -166,6 +166,8 @@ static int hsr_fill_info(struct sk_buff *skb, const struct net_device *dev)
 		goto nla_put_failure;
 	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;
 	if (nla_put_u8(skb, IFLA_HSR_PROTOCOL, proto))
 		goto nla_put_failure;
 
-- 
2.51.0

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

[Simon Horman]

On Mon, Sep 22, 2025 at 11:37:45AM +0200, Jan Vaclav wrote:
> Currently, it is possible to configure IFLA_HSR_VERSION, but
> there is no way to check in userspace what the currently
> configured HSR protocol version is.
> 
> Add it to the output of hsr_fill_info().
> 
> This info could then be used by e.g. ip(8), like so:
> $ ip -d link show hsr0
> 12: hsr0: <BROADCAST,MULTICAST> mtu ...
>     ...
>     hsr slave1 veth0 slave2 veth1 ... proto 0 version 1
> 
> Signed-off-by: Jan Vaclav <jvaclav@redhat.com>
> ---
> v2: Added an example to the commit message to show
>     how userspace programs could use this property.
> 
> v1: https://lore.kernel.org/netdev/20250918125337.111641-2-jvaclav@redhat.com/

Hi Jan,

thanks for the update.

Reviewed-by: Simon Horman <horms@kernel.org>

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

[Jakub Kicinski]

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

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

[Ján Václav]

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 think the best solution would be to introduce another change that
allows explicitly setting the version to PRP_V1 if the protocol is set
to PRP.

What do you think?

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

[Fernando Fernandez Mancera]

On 9/24/25 1:21 PM, 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.
> 

IMO, that is a good point.

> I think the best solution would be to introduce another change that
> allows explicitly setting the version to PRP_V1 if the protocol is set
> to PRP.
> 

I think it would be nice to unify the logic inside the else statement 
when parsing IFLA_HSR_VERSION and the later check for HSR_PROTOCOL_PRP. 
It could be simplified while supporting PRP_V1 explicitly set when PRP 
protocol is set.

Which also makes me wonder, should the enum hsr_version be exposed as 
UAPI? Currently it is under include/linux/if_hsr.h. But then, the naming 
should be something like "HSR_VERSION_HSR_0" or similar.

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

[Jakub Kicinski]

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..

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

[Fernando Fernandez Mancera]

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).

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

[Jakub Kicinski]

On Thu, 25 Sep 2025 10:37:38 +0200 Fernando Fernandez Mancera wrote:
> > 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).

I think we're in agreement then? i was suggesting:

--- a/net/hsr/hsr_netlink.c
+++ b/net/hsr/hsr_netlink.c
@@ -166,6 +166,8 @@ static int hsr_fill_info(struct sk_buff *skb, const struct net_device *dev)
 		goto nla_put_failure;
 	if (hsr->prot_version == PRP_V1)
 		proto = HSR_PROTOCOL_PRP;
+	else if (nla_put_u8(skb, IFLA_HSR_VERSION, hsr->prot_version))
+		goto nla_put_failure;
 	if (nla_put_u8(skb, IFLA_HSR_PROTOCOL, proto))
 		goto nla_put_failure;

This will not report the HSR version if prot is PRP

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

[Fernando Fernandez Mancera]

On 9/26/25 4:00 AM, Jakub Kicinski wrote:
> On Thu, 25 Sep 2025 10:37:38 +0200 Fernando Fernandez Mancera wrote:
>>> 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).
> 
> I think we're in agreement then? i was suggesting:
> 
> --- a/net/hsr/hsr_netlink.c
> +++ b/net/hsr/hsr_netlink.c
> @@ -166,6 +166,8 @@ static int hsr_fill_info(struct sk_buff *skb, const struct net_device *dev)
>   		goto nla_put_failure;
>   	if (hsr->prot_version == PRP_V1)
>   		proto = HSR_PROTOCOL_PRP;
> +	else if (nla_put_u8(skb, IFLA_HSR_VERSION, hsr->prot_version))
> +		goto nla_put_failure;
>   	if (nla_put_u8(skb, IFLA_HSR_PROTOCOL, proto))
>   		goto nla_put_failure;
> 
> This will not report the HSR version if prot is PRP

Looks good to me. Jan could you send a v3? Thanks!