Received-SPF: pass (google.com: domain of linux-kernel+bounces-86429-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 29 Feb 2024 10:30:48 +0100
From: Lukasz Majewski <lukma@denx.de>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>, 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>, Ziyang Xuan
 <william.xuanziyang@huawei.com>, 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: <20240229103048.45ecc1de@wsk>
In-Reply-To: <608850a9-966e-420b-8d16-1ab6baa65025@lunn.ch>
References: <20240228150735.3647892-1-lukma@denx.de>
	<608850a9-966e-420b-8d16-1ab6baa65025@lunn.ch>
Organization: denx.de
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="Sig_/ugh8IGWgId5MP.WkOG54Lmt";
 protocol="application/pgp-signature"; micalg=pgp-sha512

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

Hi Andrew,

> >  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
>=20
> You should not be checking return values from debugfs and not printing
> error messages etc. debugfs is totally option, so you should just keep
> going. The debugfs API should not explode because of a previous
> failure.
>=20
> > --- a/net/hsr/hsr_device.c
> > +++ b/net/hsr/hsr_device.c
> > @@ -142,30 +142,32 @@ static int hsr_dev_open(struct net_device
> > *dev) {
> >  	struct hsr_priv *hsr;
> >  	struct hsr_port *port;
> > -	char designation;
> > +	char *designation =3D NULL; =20
>=20
> Revere christmas tree place. When you go from RFC to a real
> submission, issues like this need fixing. For an RFC its not so bad.
>=20

Ok.

> >  	hsr =3D netdev_priv(dev);
> > -	designation =3D '\0';
> > =20
> >  	hsr_for_each_port(hsr, port) {
> >  		if (port->type =3D=3D HSR_PT_MASTER)
> >  			continue;
> >  		switch (port->type) {
> >  		case HSR_PT_SLAVE_A:
> > -			designation =3D 'A';
> > +			designation =3D "Slave A";
> >  			break;
> >  		case HSR_PT_SLAVE_B:
> > -			designation =3D 'B';
> > +			designation =3D "Slave B";
> > +			break;
> > +		case HSR_PT_INTERLINK:
> > +			designation =3D "Interlink";
> >  			break;
> >  		default:
> > -			designation =3D '?';
> > +			designation =3D "Unknown";
> >  		}
> >  		if (!is_slave_up(port->dev))
> > -			netdev_warn(dev, "Slave %c (%s) is not up;
> > please bring it up to get a fully working HSR network\n",
> > +			netdev_warn(dev, "%s (%s) is not up;
> > please bring it up to get a fully working HSR network\n",
> > designation, port->dev->name); }
> > =20
> > -	if (designation =3D=3D '\0')
> > +	if (designation =3D=3D NULL)
> >  		netdev_warn(dev, "No slave devices configured\n");
> > =20
>=20
> It would be good to split this into multiple patches. Do the A to
> Slave A, B to Slave B, etc as one patch. Then add Interlink.
>=20

Ok. This is particular one can be extracted to separate patch.

> Ideally you want lots of simple patches which are obviously correct,
> each with a good commit message explaining why the change is being
> made.

+1

>=20
> 	Andrew


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_/ugh8IGWgId5MP.WkOG54Lmt
Content-Type: application/pgp-signature
Content-Description: OpenPGP digital signature

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

iQEzBAEBCgAdFiEEgAyFJ+N6uu6+XupJAR8vZIA0zr0FAmXgTsgACgkQAR8vZIA0
zr3DuQgAoKWa5XcWdNa0MSFdLtsAI/2lJu6Tfw52RJEKxtvKx12Qx0QYdyl6a8+l
6pF4aTNIDoQtANv4nrdDBFNSkXWEgc+CYNKooBqx8SEtSQe4mz/+2v4CutXB9PBi
m0vm4NZoVrqcmCnDEp3BfnQETfvFiE/cujMIzf9GP5eKEd0B5YzJ23QW34bMpVwy
ku4t7ZTsgploN7Yds8Z9d8hpsPkgabpmrbxUNdjBTnsNu5k6EBaVk9sIrJkvAjml
sos5d7oxKvKNFuZSqMktj5kNY53iCwEOhm0Ngc8GJcMG3sRs8JfhhMm9Yf88m4C+
y5RRRLNrKl+Jw5d3HQP8bMsTxoAkyw==
=NDUn
-----END PGP SIGNATURE-----

--Sig_/ugh8IGWgId5MP.WkOG54Lmt--