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: <608850a9-966e-420b-8d16-1ab6baa65025@lunn.ch>
Date: Wed, 28 Feb 2024 18:13:08 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Lukasz Majewski <lukma@...x.de>
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

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

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

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

	Andrew

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ