Re: [PATCH v2] net/hsr: Add support for the High-availability Seamless Redundancy protocol (HSRv0)

[Arvid Brodin <arvid.brodin@xdin.com>]

On 2013-06-29 06:16, David Miller wrote:
> From: Arvid Brodin <arvid.brodin@xdin.com>
> Date: Wed, 26 Jun 2013 17:16:11 +0200
>
>> +static int hsr_dev_open(struct net_device *dev)
>> +{
>> +	struct hsr_priv *hsr_priv;
>> +
>> +	hsr_priv = netdev_priv(dev);
>> +
>> +	if (hsr_priv->slave[0])
>> +		dev_open(hsr_priv->slave[0]);
>> +	if (hsr_priv->slave[1])
>> +		dev_open(hsr_priv->slave[1]);
>> +
>> +	return 0;
>> +}
>
> dev_open() can and does fail, you must thus check for error returns, undo any
> necessary state, and propagate that error to callers of hsr_dev_open.
>

I'm not sure it's an error (from the HSR interface's point of view) if the slave(s)
won't come up here. The calls to dev_open() can be seen more like a convenience
than a necessity (I actually left them out to begin with). If none of the slaves
could go up the HSR interface would end up as admin UP but with operstate
IF_OPER_LOWERLAYERDOWN until any of the slaves went up.

I see that the VLAN code, in its vlan_dev_open(), checks the underlying interface's
IFF_UP flag and returns -ENETDOWN if it's not set. That might be an option here -
but then one would have to set three interfaces up to get a working HSR interface,
which is a bit tedious... Also, the HSR interface could end up in the state above
anyway if both slaves went down after the HSR interface went up successfully.

What about checking the return values and printing a warning if a slave dev_open()
fails, but still return success for the HSR interface no matter what?


-- 
Arvid Brodin | Consultant (Linux)
XDIN AB | Knarrarnäsgatan 7 | SE-164 40 Kista | Sweden | xdin.com