Received-SPF: pass (google.com: domain of linux-kernel+bounces-88208-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 1 Mar 2024 10:55:50 +0100
From: Lukasz Majewski <lukma@denx.de>
To: "Ziyang Xuan (William)" <william.xuanziyang@huawei.com>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>, Andrew Lunn <andrew@lunn.ch>,
 Eric Dumazet <edumazet@google.com>, "Florian Fainelli"
 <f.fainelli@gmail.com>, Vladimir Oltean <olteanv@gmail.com>, "David S.
 Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>,
 <netdev@vger.kernel.org>, <Tristram.Ha@microchip.com>, "Sebastian Andrzej
 Siewior" <bigeasy@linutronix.de>, Paolo Abeni <pabeni@redhat.com>, "Ravi
 Gunasekaran" <r-gunasekaran@ti.com>, Simon Horman <horms@kernel.org>,
 "Wojciech Drewek" <wojciech.drewek@intel.com>, Nikita Zhandarovich
 <n.zhandarovich@fintech.ru>, Murali Karicheri <m-karicheri2@ti.com>, "Dan
 Carpenter" <dan.carpenter@linaro.org>, Kristian Overskeid
 <koverskeid@gmail.com>, Matthieu Baerts <matttbe@kernel.org>,
 <linux-kernel@vger.kernel.org>
Subject: Re: [RFC] net: hsr: Provide RedBox support
Message-ID: <20240301105550.26de5271@wsk>
In-Reply-To: <64ddec9f-42a8-089a-fb4a-a49a2e80337c@huawei.com>
References: <20240228150735.3647892-1-lukma@denx.de>
	<64ddec9f-42a8-089a-fb4a-a49a2e80337c@huawei.com>
Organization: denx.de
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="Sig_/FQHjZg_Cf39XqB1x9daHbJ/";
 protocol="application/pgp-signature"; micalg=pgp-sha512

--Sig_/FQHjZg_Cf39XqB1x9daHbJ/
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable

Hi William,

> Give opinions only from the code level.
>=20
> > =20
> >  void hsr_debugfs_rename(struct net_device *dev)
> >  {
> > @@ -95,6 +114,19 @@ void hsr_debugfs_init(struct hsr_priv *priv,
> > struct net_device *hsr_dev) priv->node_tbl_root =3D NULL;
> >  		return;
> >  	}
> > +
> > +	if (!priv->redbox)
> > +		return;
> > +
> > +	de =3D debugfs_create_file("proxy_node_table", S_IFREG |
> > 0444,
> > +				 priv->node_tbl_root, priv,
> > +				 &hsr_proxy_node_table_fops);
> > +	if (IS_ERR(de)) {
> > +		pr_err("Cannot create hsr proxy node_table
> > file\n");
> > +		debugfs_remove(priv->node_tbl_root);
> > +		priv->node_tbl_root =3D NULL;
> > +		return;
> > +	} =20
> I think we can use "goto label" to reduce duplicate codes for error
> handling.

This code is already NAK'ed from network maintainers, as debugfs is not
acceptable to give any "API like" information.

Instead the "netlink" API shall be used to provide this information.

>=20
> >  }
> > =20
> > @@ -296,6 +298,7 @@ static void send_hsr_supervision_frame(struct
> > hsr_port *master, struct hsr_priv *hsr =3D master->hsr;
> >  	__u8 type =3D HSR_TLV_LIFE_CHECK;
> >  	struct hsr_sup_payload *hsr_sp;
> > +	struct hsr_sup_tlv *hsr_stlv;
> >  	struct hsr_sup_tag *hsr_stag;
> >  	struct sk_buff *skb;
> > =20
> > @@ -335,6 +338,16 @@ static void send_hsr_supervision_frame(struct
> > hsr_port *master, hsr_sp =3D skb_put(skb, sizeof(struct
> > hsr_sup_payload)); ether_addr_copy(hsr_sp->macaddress_A,
> > master->dev->dev_addr);=20
> > +	if (hsr->redbox) {
> > +		hsr_stlv =3D skb_put(skb, sizeof(struct
> > hsr_sup_tlv));
> > +		hsr_stlv->HSR_TLV_type =3D PRP_TLV_REDBOX_MAC;
> > +		hsr_stlv->HSR_TLV_length =3D sizeof(struct
> > hsr_sup_payload); +
> > +		/* Payload: MacAddressRedBox */
> > +		hsr_sp =3D skb_put(skb, sizeof(struct
> > hsr_sup_payload));
> > +		ether_addr_copy(hsr_sp->macaddress_A,
> > hsr->macaddress_redbox);
> > +	} =20
> If hsr->redbox is true, hsr_sp->macaddress_A will be covered. Do
> ether_addr_copy() twice. Is it better like this:
>=20
> hsr_sp =3D skb_put(skb, sizeof(struct hsr_sup_payload));
>=20
> if (hsr->redbox) {
> 	...
> 	ether_addr_copy(hsr_sp->macaddress_A, hsr->macaddress_redbox);
> } else {
> 	ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
> }

It may be a bit misleading, as the hsr_sp is the payload for TLV fields
in HSR supervisory frames.

It shall be done as in the code as:

1. First you need to send this HSR "node" MAC address (the
ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr))

2. If the box is a RedBox, then this supervisory frame shall have
_appended_ another TLV with RedBox mac address (it can be the same
as the HSR node in the special case).

The hsr_sp->macaddress_A holds the address to the V (value) field of
the TLV.

>=20
> > +
> >  	if (skb_put_padto(skb, ETH_ZLEN)) {
> >  		spin_unlock_bh(&hsr->seqnr_lock);
> >  		return; =20
>=20
> > =20
> > @@ -448,13 +455,14 @@ static void hsr_forward_do(struct
> > hsr_frame_info *frame) }
> > =20
> >  		/* Check if frame is to be dropped. Eg. for PRP no
> > forward
> > -		 * between ports.
> > +		 * between ports, or sending HSR supervision to
> > RedBox. */
> >  		if (hsr->proto_ops->drop_frame &&
> >  		    hsr->proto_ops->drop_frame(frame, port))
> >  			continue;
> > =20
> > -		if (port->type !=3D HSR_PT_MASTER)
> > +		if (port->type =3D=3D HSR_PT_SLAVE_A ||
> > +		    port->type =3D=3D HSR_PT_SLAVE_B) =20
>=20
> (port->type !=3D HSR_PT_MASTER) is not equivalent to (port->type =3D=3D
> HSR_PT_SLAVE_A || port->type =3D=3D HSR_PT_SLAVE_B). port->type may be
> HSR_PT_INTERLINK or others. Or here is a bugfix? Please check.

Without Redbox support you don't use HSR_PT_INTERLINK. This port shall
behave in similar way to HSR_PT_MASTER - e.g. it will send/receive HSR
untagged frames. That is why the condition has been altered to only
prepare HSR tagged frames for HSR ring port A/B.

>=20
> >  			skb =3D
> > hsr->proto_ops->create_tagged_frame(frame, port); else
> >  			skb =3D
> > hsr->proto_ops->get_untagged_frame(frame, port); @@ -469,7 +477,9
> > @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> > hsr_deliver_master(skb, port->dev, frame->node_src); } else {
> >  			if (!hsr_xmit(skb, port, frame))
> > -				sent =3D true;
> > +				if (port->type =3D=3D HSR_PT_SLAVE_A ||
> > +				    port->type =3D=3D HSR_PT_SLAVE_B)
> > +					sent =3D true;
> >  		}
> >  	}
> >  } =20
>=20
> If my opinions be accepted, Can you add "Reviewed-by: Ziyang Xuan
> <william.xuanziyang@huawei.com>" at next version of patch?

I will add your Reviewed-by: tag, no problem.

Thanks for the input :-)

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@denx.de

--Sig_/FQHjZg_Cf39XqB1x9daHbJ/
Content-Type: application/pgp-signature
Content-Description: OpenPGP digital signature

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEEgAyFJ+N6uu6+XupJAR8vZIA0zr0FAmXhpiYACgkQAR8vZIA0
zr3S1gf/WjpsxibQboT9bZVmYqBI42CusZQPAtv+BKCCQPIAKeU+uxZJYFNdY9X1
1RvPcYudaPtPYdz7MXbVOjFojxHh46PltWoUM3SH+B7NvHFTdFHibnjE//GvDxP1
zEJ3yf2Yk24L9U/F6gc0l4aXAbIlIwF3ptTxUFZBaEEC51XEvPpU+f6PEeABHiK6
seqRQGq6nlU6YUAFpPpvmSTjIx8RYNem/nMApysdVynQeY5J5WyAFNb2Rh2bN6KY
m9wFDWUzXH81aj+nyZ65h11QqIkh5XLhv10AcSFjHpswgbUNZCV5G/zQ8N37mLRX
yxUR5tYuntWOhUG6A6QzcyDp0CNkfw==
=xyoM
-----END PGP SIGNATURE-----

--Sig_/FQHjZg_Cf39XqB1x9daHbJ/--