Message-ID: <20240429120904.2ab5248c@wsk>
Date: Mon, 29 Apr 2024 12:09:04 +0200
From: Lukasz Majewski <lukma@...x.de>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>, Casper
 Andersson <casper.casan@...il.com>, Andrew Lunn <andrew@...n.ch>, Eric
 Dumazet <edumazet@...gle.com>, Vladimir Oltean <olteanv@...il.com>, "David
 S. Miller" <davem@...emloft.net>, Oleksij Rempel <o.rempel@...gutronix.de>,
 Tristram.Ha@...rochip.com, Sebastian Andrzej Siewior
 <bigeasy@...utronix.de>, Ravi Gunasekaran <r-gunasekaran@...com>, Simon
 Horman <horms@...nel.org>, Nikita Zhandarovich <n.zhandarovich@...tech.ru>,
 Murali Karicheri <m-karicheri2@...com>, Jiri Pirko <jiri@...nulli.us>, Dan
 Carpenter <dan.carpenter@...aro.org>, Ziyang Xuan
 <william.xuanziyang@...wei.com>, Shigeru Yoshida <syoshida@...hat.com>,
 "Ricardo B. Marliere" <ricardo@...liere.net>, linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH] hsr: Simplify code for announcing HSR nodes
 timer setup

Hi Jakub,

> On Thu, 25 Apr 2024 17:39:58 +0200 Lukasz Majewski wrote:
> > Up till now the code to start HSR announce timer, which triggers
> > sending supervisory frames, was assuming that hsr_netdev_notify()
> > would be called at least twice for hsrX interface. This was
> > required to have different values for old and current values of
> > network device's operstate.
> > 
> > This is problematic for a case where hsrX interface is already in
> > the operational state when hsr_netdev_notify() is called, so timer
> > is not configured to trigger and as a result the hsrX is not
> > sending supervisory frames to HSR ring.
> > 
> > This error has been discovered when hsr_ping.sh script was run. To
> > be more specific - for the hsr1 and hsr2 the hsr_netdev_notify() was
> > called at least twice with different IF_OPER_{LOWERDOWN|DOWN|UP}
> > states assigned in hsr_check_carrier_and_operstate(hsr). As a
> > result there was no issue with sending supervisory frames.
> > However, with hsr3, the notify function was called only once with
> > operstate set to IF_OPER_UP and timer responsible for triggering
> > supervisory frames was not fired.
> > 
> > The solution is to use netif_oper_up() helper function to assess if
> > network device is up and then setup timer. Otherwise the timer is
> > activated.  
> 
> NETDEV_CHANGE can get called for multiple trivial reasons,

I've assumed that NETDEV_CHANGE would be called when the link has
changed - i.e. it is down/up or carrier is down/up.

The timer shall be running _only_ when the hsrX port is fully
operational (i.e. at least one of 'slave' ports is up and running).

The motivation for this patch was to enable HSR announce timer not only
on state change, but also when the ethernet device is already up (as it
happens with QEMU + netns setup).
 

> if the
> timer is already running we'll mess with the spacing of the frames,
> no?

When NETDEV_CHANGE is trigger for reason different than carrier (or
port state) change and the netif_oper_up() returns true, the period for
HSR supervisory frames (i.e. HSR_ANNOUNCE_INTEVAL) would be violated.

What are here the potential threads?

> 
> If there is a path where the device may get activated without the
> notifier firing - maybe we can check carrier there and schedule the
> timer?

As I've stated above - IMHO the "announce" supervisory frames shall be
send only when HSR interface is up and running.

> 
> Also sounds like a bug fix, so please add a Fixes tag.

Ok.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists