| Message ID | 20230712110657.1282499-1-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b685f1a58956fa36cc01123f253351b25bfacfda |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ethernet: ti: cpsw_ale: Fix cpsw_ale_get_field()/cpsw_ale_set_field() | expand |
On 12/07/2023 14:06, Siddharth Vadapalli wrote: > From: Tanmay Patil <t-patil@ti.com> > > 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@ti.com> > [s-vadapalli@ti.com: rephrased commit message and added Fixes tag] > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > 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? > } > > 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); > }
On 7/12/2023 7:20 PM, Roger Quadros wrote: > > > On 12/07/2023 14:06, Siddharth Vadapalli wrote: >> From: Tanmay Patil <t-patil@ti.com> >> >> 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@ti.com> >> [s-vadapalli@ti.com: rephrased commit message and added Fixes tag] >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> --- >> 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); >> } >
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Wed, 12 Jul 2023 16:36:57 +0530 you wrote: > From: Tanmay Patil <t-patil@ti.com> > > 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. > > [...] Here is the summary with links: - [net] net: ethernet: ti: cpsw_ale: Fix cpsw_ale_get_field()/cpsw_ale_set_field() https://git.kernel.org/netdev/net/c/b685f1a58956 You are awesome, thank you!
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); } 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); }