RE: [PATCH net v2 1/1] igc: read before write to SRRCTL register

[David Laight]

From: Song, Yoong Siang
> Sent: 14 April 2023 12:16
...
> >I have checked Foxville manual for SRRCTL (Split and Replication Receive
> >Control) register and below GENMASKs looks correct.
> >
> >> -#define IGC_SRRCTL_BSIZEPKT_SHIFT		10 /* Shift _right_ */
> >> -#define IGC_SRRCTL_BSIZEHDRSIZE_SHIFT		2  /* Shift _left_ */
> >> +#define IGC_SRRCTL_BSIZEPKT_MASK	GENMASK(6, 0)
> >> +#define IGC_SRRCTL_BSIZEPKT_SHIFT	10 /* Shift _right_ */
> >
> >Shift due to 1 KB resolution of BSIZEPKT (manual field BSIZEPACKET)
> 
> Ya, 1K = BIT(10), so need to shift right 10 bits.

I bet the code would be easier to read if it did 'value / 1024u'.
The object code will be (much) the same.

> >> +#define IGC_SRRCTL_BSIZEHDRSIZE_MASK	GENMASK(13, 8)
> >> +#define IGC_SRRCTL_BSIZEHDRSIZE_SHIFT	2  /* Shift _left_ */
> >
> >This shift is suspicious, but as you inherited it I guess it works.
> >I did the math, and it happens to work, knowing (from manual) value is in 64 bytes
> >resolution.
> 
> It is in 64 = BIT(6) resolution, so need to shift right 6 bits.
> But it start on 8th bit, so need to shift left 8 bits.
> Thus, total = shift left 2 bits.
> 
> I dint put the explanation into the header file because it is too lengthy
> and user can know from databook.
> 
> How do you feel on the necessary of explaining the shifting logic?

Not everyone trying to grok the code will have the manual.
Even writing (8 - 6) will help.
Or (I think) if the value is in bits 13-8 in units of 64 then just:
	((value >> 8) & 0x1f) * 64
gcc will do a single shift right and a mask 9at some point).
You might want some defines, but if they aren't used much
just comments that refer to the names in the manual/datasheet
can be enough.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)