[driver-discuss] Codereview for CR# 6655415 sfe auto-negotiate

[Steven Stallion]

Masa Murayama wrote:
> Steven,
>
> Thank you very much for your review.
> I answer in lines.

My pleasure; I am glad I could help.

>> (the changes on lines 1140 - 1180 really bug me):
>>
>> 1146	tmp0 is not meaningful - why was the assignment broken out? IMHO
>> 	having this broken apart makes the code more difficult to read.
>> 1147	tmp1 is not meaningful - see comments above (line 1146)
>> 1177	this line should not have changed; using a tmp variable here has
>> 	no benefit
>> 1178	I would have handled this differently. On line 1170, I would
>> 	have: 'mark |= dcp->dmac_size;', and then
>> 	'tdp->d_cmdsts = LE_32(mark);' on line 1180.
> 
> The reason of changing line 1140 - 1180 is for performance.
> As LE_32() is a macro, LE_32(p->xxx) will cause several memory
> references to the same location by a pointer, that are difficult for
> compilers to optimize.

This is very interesting - I honestly would not have considered
dereferencing a pointer from within a macro to be a performance issue -
now that you have pointed it out, I can understand why the assignments
were broken apart.

>> 1243	See comments on line 1188
>>
>> 1256 & 1257	Don't break the statements apart; clutter
>
> The reason is for supporting big endian CPUs, i.e. sparc.
> LE_32(tdp->d_cmdsts) is not atomic in sparc. LE_32() macro
> generates several memory references to read the same word in the same
> descriptor.
> It will corrupt the result of LE_32() if the nic will update the
> descriptor at the same time.

Makes sense, I did not even consider the side-effects of the inline.

(Minor rant...)

It's cases like the ones above that really make me wonder if LE/BE_32
and friends should be amended.

It seems that the behavior would be far more consistent if LE_32 on a BE
system would simply call a function rather than attempt to convert the
value inline. Pass-by-value would insulate the caller from any
non-atomic side-effects of the inline (at the additional cost of
performing a function call).

Then again, I suppose it depends on what is considered acceptable
overhead when converting from LE to BE (and vice versa) and how often it
is performed.

Regards,

Steve

--
Yet magic and hierarchy
arise from the same source,
and this source has a null pointer.

Reference the NULL within NULL,
it is the gateway to all wizardry.