Re: [RFC] net: hsr: Provide RedBox support

[Andrew Lunn]

>  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