Re: [PATCH net] net: ethernet: ti: cpsw_ale: Fix cpsw_ale_get_field()/cpsw_ale_set_field()

[Patil, Tanmay]

On 7/12/2023 7:20 PM, Roger Quadros wrote:
> On 12/07/2023 14:06, Siddharth Vadapalli wrote:
> > From: Tanmay Patil <t-patil@xxxxxx>
> >
> > CPSW ALE has 75 bit ALE entries which are stored within three 32 bit words.
> > The cpsw_ale_get_field() and cpsw_ale_set_field() functions assume that the
> > field will be strictly contained within one word. However, this is not
> > guaranteed to be the case and it is possible for ALE field entries to span
> > across up to two words at the most.
> >
> > Fix the methods to handle getting/setting fields spanning up to two words.
> >
> > Fixes: db82173f23c5 ("netdev: driver: ethernet: add cpsw address lookup engine support")
> > Signed-off-by: Tanmay Patil <t-patil@xxxxxx>
> > [s-vadapalli@xxxxxx: rephrased commit message and added Fixes tag]
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
> > ---
> >   drivers/net/ethernet/ti/cpsw_ale.c | 24 +++++++++++++++++++-----
> >   1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
> > index 0c5e783e574c..64bf22cd860c 100644
> > --- a/drivers/net/ethernet/ti/cpsw_ale.c
> > +++ b/drivers/net/ethernet/ti/cpsw_ale.c
> > @@ -106,23 +106,37 @@ struct cpsw_ale_dev_id {
> >   
> >   static inline int cpsw_ale_get_field(u32 *ale_entry, u32 start, u32 bits)
> >   {
> > -	int idx;
> > +	int idx, idx2;
> > +	u32 hi_val = 0;
> >   
> >   	idx    = start / 32;
> > +	idx2 = (start + bits - 1) / 32;
> > +	/* Check if bits to be fetched exceed a word */
> > +	if (idx != idx2) {
> > +		idx2 = 2 - idx2; /* flip */
> > +		hi_val = ale_entry[idx2] << ((idx2 * 32) - start);
> > +	}
> >   	start -= idx * 32;
> >   	idx    = 2 - idx; /* flip */
> > -	return (ale_entry[idx] >> start) & BITMASK(bits);
> > +	return (hi_val + (ale_entry[idx] >> start)) & BITMASK(bits);
>
> Should this be bit-wise OR instead of ADD?

As the hi_val has been declared and left shifted in this function, we 
are sure that the trailing bits are all '0'. Hence we can directly add them.

> >   }
> >   
> >   static inline void cpsw_ale_set_field(u32 *ale_entry, u32 start, u32 bits,
> >   				      u32 value)
> >   {
> > -	int idx;
> > +	int idx, idx2;
> >   
> >   	value &= BITMASK(bits);
> > -	idx    = start / 32;
> > +	idx = start / 32;
> > +	idx2 = (start + bits - 1) / 32;
> > +	/* Check if bits to be set exceed a word */
> > +	if (idx != idx2) {
> > +		idx2 = 2 - idx2; /* flip */
> > +		ale_entry[idx2] &= ~(BITMASK(bits + start - (idx2 * 32)));
> > +		ale_entry[idx2] |= (value >> ((idx2 * 32) - start));
> > +	}
> >   	start -= idx * 32;
> > -	idx    = 2 - idx; /* flip */
> > +	idx = 2 - idx; /* flip */
> >   	ale_entry[idx] &= ~(BITMASK(bits) << start);
> >   	ale_entry[idx] |=  (value << start);
> >   }