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

[Lukasz Majewski]

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@xxxxxxx

Attachment: pgpOA5hXnieO6.pgp
Description: OpenPGP digital signature