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

[Jesper Dangaard Brouer]

On 14/04/2023 14.32, David Laight wrote:
> 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.

I agree. Code becomes more readable for humans and machine code will be 
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 didnt put the explanation into the header file because it is too lengthy
> > and user can know from databook.

Well, users usually don't have access to the databook (Programming
Interface) PDF.  Personally I have it, but I had to go though a lot of
red-tape to get it (under Red Hat NDA).


> > 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.

After Alexander Lobakin opened my eyes for GENMASK, FIELD_PREP and
FIELD_GET, I find that easier to read and work-with these kind of
register value manipulations, see[1] include/linux/bitfield.h.  It will
also detect if the assigned value exceeds the mask (like David code
handled via mask). (thx Alex)

 [1] 
https://elixir.bootlin.com/linux/v6.3-rc6/source/include/linux/bitfield.h#L14

So, instead of:
  srrctl |= IGC_RX_HDR_LEN << IGC_SRRCTL_BSIZEHDRSIZE_SHIFT;

I would write

  /* BSIZEHDR value in 64 bytes resolution */
  srrctl |= FIELD_PREP(IGC_SRRCTL_BSIZEHDRSIZE_MASK, (IGC_RX_HDR_LEN / 
64));

--Jesper