linux-kernel - Re: [RFC] net: hsr: Provide RedBox support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240229103048.45ecc1de@wsk>
Date: Thu, 29 Feb 2024 10:30:48 +0100
From: Lukasz Majewski <lukma@...x.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: Oleksij Rempel <o.rempel@...gutronix.de>, Eric Dumazet
 <edumazet@...gle.com>, Florian Fainelli <f.fainelli@...il.com>, Vladimir
 Oltean <olteanv@...il.com>, "David S. Miller" <davem@...emloft.net>, Jakub
 Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
 Tristram.Ha@...rochip.com, Sebastian Andrzej Siewior
 <bigeasy@...utronix.de>, Paolo Abeni <pabeni@...hat.com>, Ravi Gunasekaran
 <r-gunasekaran@...com>, Simon Horman <horms@...nel.org>, Wojciech Drewek
 <wojciech.drewek@...el.com>, Nikita Zhandarovich
 <n.zhandarovich@...tech.ru>, Murali Karicheri <m-karicheri2@...com>, Dan
 Carpenter <dan.carpenter@...aro.org>, Ziyang Xuan
 <william.xuanziyang@...wei.com>, Kristian Overskeid <koverskeid@...il.com>,
 Matthieu Baerts <matttbe@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [RFC] net: hsr: Provide RedBox support

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 = NULL;
> >  		return;
> >  	}
> > +
> > +	if (!priv->redbox)
> > +		return;
> > +
> > +	de = 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 = NULL;
> > +		return;  
> 
> 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.
> 
> > --- 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 = NULL;  
> 
> 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.
> 

Ok.

> >  	hsr = netdev_priv(dev);
> > -	designation = '\0';
> >  
> >  	hsr_for_each_port(hsr, port) {
> >  		if (port->type == HSR_PT_MASTER)
> >  			continue;
> >  		switch (port->type) {
> >  		case HSR_PT_SLAVE_A:
> > -			designation = 'A';
> > +			designation = "Slave A";
> >  			break;
> >  		case HSR_PT_SLAVE_B:
> > -			designation = 'B';
> > +			designation = "Slave B";
> > +			break;
> > +		case HSR_PT_INTERLINK:
> > +			designation = "Interlink";
> >  			break;
> >  		default:
> > -			designation = '?';
> > +			designation = "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); }
> >  
> > -	if (designation == '\0')
> > +	if (designation == NULL)
> >  		netdev_warn(dev, "No slave devices configured\n");
> >  
> 
> 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.
> 

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

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

Content of type "application/pgp-signature" skipped