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

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

On 2013-10-28 10:17, David Laight wrote:
>> Subject: Re: [PATCH v5] net/hsr: Add support for the High-availability Seamless Redundancy protocol
>> (HSRv0)
>>
>> On Fri, 2013-10-25 at 20:20 +0200, Arvid Brodin wrote:
>>> On 2013-10-23 18:52, Joe Perches wrote:
>>>> On Wed, 2013-10-23 at 18:09 +0200, Arvid Brodin wrote:
>> []
>>>>> +/* above(a, b) - return 1 if a > b, 0 otherwise.
>>>>> + */
>>>>> +static bool above(u16 a, u16 b)
>>>>> +{
>>>>> +	/* Remove inconsistency where above(a, b) == below(a, b) */
>>>>> +	if ((int) b - a == 32768)
>>>>> +		return 0;
>>>>> +
>>>>> +	return (((s16) (b - a)) < 0);
>>>>> +}
>>>>> +#define below(a, b)		above((b), (a))
>>>>> +#define above_or_eq(a, b)	(!below((a), (b)))
>>>>> +#define below_or_eq(a, b)	(!above((a), (b)))
>>>>
>>>> This looks odd.
>>>
>>> It relies on unsigned arithmetic to compare two values that may wrap. I.e.,
>>> it doesn't care about the absolute sizes, but only about the distance
>>> between the numbers.
> 
> I'd have thought it wasn't really worth worrying about what happens
> when a ^ b == 0x8000. Anything using modulo 16 distances shouldn't
> really see such values.
> Perhaps just document the effect.

Unfortunately there are circumstances where such values can be seen - 
specifically when there is a network problem. And since HSR is meant to
be a high-availability protocol, it is imperative that such cases are 
handled consistently. (That would have been the case with modulo 32 as
well, even if the occations when it happened would be a lot rarer, and
the resulting bugs would be much harder to find.)

> 
> Also, if these definitions are in a .h file they need better names

They're in a .c file, but between your's and Joe Perches' comments, I can
see that the names and description needs a face-lift.

Thanks for looking at the code!

> 
> 	David
> 

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