* [PATCH] staging: rtl8192e: Use min_t/max_t macros for variable comparison @ 2022-11-02 22:36 Deepak R Varma 2022-11-02 22:56 ` Philipp Hortmann 2022-11-03 8:24 ` David Laight 0 siblings, 2 replies; 7+ messages in thread From: Deepak R Varma @ 2022-11-02 22:36 UTC (permalink / raw) To: outreachy, Greg Kroah-Hartman, linux-staging, linux-kernel Simplify code by using min_t and max_t helper macros in place of lengthy if/else block oriented logical evaluation and value assignment. This issue is identified by coccicheck using the minmax.cocci file. Use the *_t variants of min/max macros to avoid compiler warnings about data typecast. Also, use u32 as type for min_t macro to avoid any truncation of data associated with enum constant HT_AGG_SIZE_32K. Signed-off-by: Deepak R Varma <drv@mailo.com> --- drivers/staging/rtl8192e/rtl819x_HTProc.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c b/drivers/staging/rtl8192e/rtl819x_HTProc.c index 62aa8e893c34..ccb86660ab48 100644 --- a/drivers/staging/rtl8192e/rtl819x_HTProc.c +++ b/drivers/staging/rtl8192e/rtl819x_HTProc.c @@ -587,17 +587,12 @@ void HTOnAssocRsp(struct rtllib_device *ieee) else pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_64K; } else { - if (pPeerHTCap->MaxRxAMPDUFactor < HT_AGG_SIZE_32K) - pHTInfo->CurrentAMPDUFactor = - pPeerHTCap->MaxRxAMPDUFactor; - else - pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_32K; + pHTInfo->CurrentAMPDUFactor = min_t(u32, pPeerHTCap->MaxRxAMPDUFactor, + HT_AGG_SIZE_32K); } } - if (pHTInfo->MPDU_Density > pPeerHTCap->MPDUDensity) - pHTInfo->current_mpdu_density = pHTInfo->MPDU_Density; - else - pHTInfo->current_mpdu_density = pPeerHTCap->MPDUDensity; + pHTInfo->current_mpdu_density = max_t(u8, pHTInfo->MPDU_Density, + pPeerHTCap->MPDUDensity); if (pHTInfo->iot_action & HT_IOT_ACT_TX_USE_AMSDU_8K) { pHTInfo->bCurrentAMPDUEnable = false; pHTInfo->ForcedAMSDUMode = HT_AGG_FORCE_ENABLE; -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: rtl8192e: Use min_t/max_t macros for variable comparison 2022-11-02 22:36 [PATCH] staging: rtl8192e: Use min_t/max_t macros for variable comparison Deepak R Varma @ 2022-11-02 22:56 ` Philipp Hortmann 2022-11-03 8:24 ` David Laight 1 sibling, 0 replies; 7+ messages in thread From: Philipp Hortmann @ 2022-11-02 22:56 UTC (permalink / raw) To: Deepak R Varma, outreachy, Greg Kroah-Hartman, linux-staging, linux-kernel On 11/2/22 23:36, Deepak R Varma wrote: > Simplify code by using min_t and max_t helper macros in place of lengthy > if/else block oriented logical evaluation and value assignment. This > issue is identified by coccicheck using the minmax.cocci file. > > Use the *_t variants of min/max macros to avoid compiler warnings about > data typecast. > Also, use u32 as type for min_t macro to avoid any truncation of data > associated with enum constant HT_AGG_SIZE_32K. > > Signed-off-by: Deepak R Varma <drv@mailo.com> > --- > drivers/staging/rtl8192e/rtl819x_HTProc.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c b/drivers/staging/rtl8192e/rtl819x_HTProc.c > index 62aa8e893c34..ccb86660ab48 100644 > --- a/drivers/staging/rtl8192e/rtl819x_HTProc.c > +++ b/drivers/staging/rtl8192e/rtl819x_HTProc.c > @@ -587,17 +587,12 @@ void HTOnAssocRsp(struct rtllib_device *ieee) > else > pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_64K; > } else { > - if (pPeerHTCap->MaxRxAMPDUFactor < HT_AGG_SIZE_32K) > - pHTInfo->CurrentAMPDUFactor = > - pPeerHTCap->MaxRxAMPDUFactor; > - else > - pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_32K; > + pHTInfo->CurrentAMPDUFactor = min_t(u32, pPeerHTCap->MaxRxAMPDUFactor, > + HT_AGG_SIZE_32K); > } > } > - if (pHTInfo->MPDU_Density > pPeerHTCap->MPDUDensity) > - pHTInfo->current_mpdu_density = pHTInfo->MPDU_Density; > - else > - pHTInfo->current_mpdu_density = pPeerHTCap->MPDUDensity; > + pHTInfo->current_mpdu_density = max_t(u8, pHTInfo->MPDU_Density, > + pPeerHTCap->MPDUDensity); > if (pHTInfo->iot_action & HT_IOT_ACT_TX_USE_AMSDU_8K) { > pHTInfo->bCurrentAMPDUEnable = false; > pHTInfo->ForcedAMSDUMode = HT_AGG_FORCE_ENABLE; > -- > 2.34.1 > > > > Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] staging: rtl8192e: Use min_t/max_t macros for variable comparison 2022-11-02 22:36 [PATCH] staging: rtl8192e: Use min_t/max_t macros for variable comparison Deepak R Varma 2022-11-02 22:56 ` Philipp Hortmann @ 2022-11-03 8:24 ` David Laight 2022-11-03 8:53 ` Dan Carpenter 1 sibling, 1 reply; 7+ messages in thread From: David Laight @ 2022-11-03 8:24 UTC (permalink / raw) To: 'Deepak R Varma', outreachy@lists.linux.dev, Greg Kroah-Hartman, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org From: Deepak R Varma > Sent: 02 November 2022 22:37 > > Simplify code by using min_t and max_t helper macros in place of lengthy > if/else block oriented logical evaluation and value assignment. This > issue is identified by coccicheck using the minmax.cocci file. > > Use the *_t variants of min/max macros to avoid compiler warnings about > data typecast. > Also, use u32 as type for min_t macro to avoid any truncation of data > associated with enum constant HT_AGG_SIZE_32K. > > Signed-off-by: Deepak R Varma <drv@mailo.com> > --- > drivers/staging/rtl8192e/rtl819x_HTProc.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c b/drivers/staging/rtl8192e/rtl819x_HTProc.c > index 62aa8e893c34..ccb86660ab48 100644 > --- a/drivers/staging/rtl8192e/rtl819x_HTProc.c > +++ b/drivers/staging/rtl8192e/rtl819x_HTProc.c > @@ -587,17 +587,12 @@ void HTOnAssocRsp(struct rtllib_device *ieee) > else > pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_64K; > } else { > - if (pPeerHTCap->MaxRxAMPDUFactor < HT_AGG_SIZE_32K) > - pHTInfo->CurrentAMPDUFactor = > - pPeerHTCap->MaxRxAMPDUFactor; > - else > - pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_32K; > + pHTInfo->CurrentAMPDUFactor = min_t(u32, pPeerHTCap->MaxRxAMPDUFactor, > + HT_AGG_SIZE_32K); For min() to fail there must be a signed v unsigned mismatch. Maybe that ought to be fixed. > } > } > - if (pHTInfo->MPDU_Density > pPeerHTCap->MPDUDensity) > - pHTInfo->current_mpdu_density = pHTInfo->MPDU_Density; > - else > - pHTInfo->current_mpdu_density = pPeerHTCap->MPDUDensity; > + pHTInfo->current_mpdu_density = max_t(u8, pHTInfo->MPDU_Density, > + pPeerHTCap->MPDUDensity); Using u8 with max_t() really doesn't make any sense. The value will get promoted to signed int prior to the comparison. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: rtl8192e: Use min_t/max_t macros for variable comparison 2022-11-03 8:24 ` David Laight @ 2022-11-03 8:53 ` Dan Carpenter 2022-11-03 9:18 ` Deepak R Varma 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2022-11-03 8:53 UTC (permalink / raw) To: David Laight Cc: 'Deepak R Varma', outreachy@lists.linux.dev, Greg Kroah-Hartman, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org On Thu, Nov 03, 2022 at 08:24:15AM +0000, David Laight wrote: > > --- a/drivers/staging/rtl8192e/rtl819x_HTProc.c > > +++ b/drivers/staging/rtl8192e/rtl819x_HTProc.c > > @@ -587,17 +587,12 @@ void HTOnAssocRsp(struct rtllib_device *ieee) > > else > > pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_64K; > > } else { > > - if (pPeerHTCap->MaxRxAMPDUFactor < HT_AGG_SIZE_32K) > > - pHTInfo->CurrentAMPDUFactor = > > - pPeerHTCap->MaxRxAMPDUFactor; > > - else > > - pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_32K; > > + pHTInfo->CurrentAMPDUFactor = min_t(u32, pPeerHTCap->MaxRxAMPDUFactor, > > + HT_AGG_SIZE_32K); > > For min() to fail there must be a signed v unsigned mismatch. > Maybe that ought to be fixed. > u32 is the right choice here. I'm having a hard time understanding your email. You might be saying we could declare HT_AGG_SIZE_32K as a u32 so then we could use min() instead of min_t()? HT_AGG_SIZE_32K is an enum. pPeerHTCap->MaxRxAMPDUFactor is a bitfield. u8 MaxRxAMPDUFactor:2; We will never be able to use min(). > > } > > } > > - if (pHTInfo->MPDU_Density > pPeerHTCap->MPDUDensity) > > - pHTInfo->current_mpdu_density = pHTInfo->MPDU_Density; > > - else > > - pHTInfo->current_mpdu_density = pPeerHTCap->MPDUDensity; > > + pHTInfo->current_mpdu_density = max_t(u8, pHTInfo->MPDU_Density, > > + pPeerHTCap->MPDUDensity); > > Using u8 with max_t() really doesn't make any sense. Using u8 looks wrong because you would worry that one of the types is larger than U8_MAX. But it's actually fine. The types are u8 vs another bitfield. I would probably have gone with u32 here as well. > The value will get promoted to signed int prior to the comparison. > That's sort of true-ish but I don't understand what you are saying? #confused regards, dan carpenter ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: rtl8192e: Use min_t/max_t macros for variable comparison 2022-11-03 8:53 ` Dan Carpenter @ 2022-11-03 9:18 ` Deepak R Varma 2022-11-03 10:09 ` Dan Carpenter 0 siblings, 1 reply; 7+ messages in thread From: Deepak R Varma @ 2022-11-03 9:18 UTC (permalink / raw) To: Dan Carpenter Cc: David Laight, outreachy@lists.linux.dev, Greg Kroah-Hartman, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org On Thu, Nov 03, 2022 at 11:53:45AM +0300, Dan Carpenter wrote: > On Thu, Nov 03, 2022 at 08:24:15AM +0000, David Laight wrote: > > > --- a/drivers/staging/rtl8192e/rtl819x_HTProc.c > > > +++ b/drivers/staging/rtl8192e/rtl819x_HTProc.c > > > @@ -587,17 +587,12 @@ void HTOnAssocRsp(struct rtllib_device *ieee) > > > else > > > pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_64K; > > > } else { > > > - if (pPeerHTCap->MaxRxAMPDUFactor < HT_AGG_SIZE_32K) > > > - pHTInfo->CurrentAMPDUFactor = > > > - pPeerHTCap->MaxRxAMPDUFactor; > > > - else > > > - pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_32K; > > > + pHTInfo->CurrentAMPDUFactor = min_t(u32, pPeerHTCap->MaxRxAMPDUFactor, > > > + HT_AGG_SIZE_32K); > > > > For min() to fail there must be a signed v unsigned mismatch. > > Maybe that ought to be fixed. > > > > u32 is the right choice here. > > I'm having a hard time understanding your email. You might be saying > we could declare HT_AGG_SIZE_32K as a u32 so then we could use min() > instead of min_t()? HT_AGG_SIZE_32K is an enum. > > pPeerHTCap->MaxRxAMPDUFactor is a bitfield. > > u8 MaxRxAMPDUFactor:2; > > We will never be able to use min(). I think we could do min((u32)a, (u32)b), but it is just unwrapped min_t if I understand David's comment. > > > > } > > > } > > > - if (pHTInfo->MPDU_Density > pPeerHTCap->MPDUDensity) > > > - pHTInfo->current_mpdu_density = pHTInfo->MPDU_Density; > > > - else > > > - pHTInfo->current_mpdu_density = pPeerHTCap->MPDUDensity; > > > + pHTInfo->current_mpdu_density = max_t(u8, pHTInfo->MPDU_Density, > > > + pPeerHTCap->MPDUDensity); > > > > Using u8 with max_t() really doesn't make any sense. > > Using u8 looks wrong because you would worry that one of the types is > larger than U8_MAX. But it's actually fine. The types are u8 vs another > bitfield. I would probably have gone with u32 here as well. I will take your advise and upgrade the type to u32 as a revision. > > > The value will get promoted to signed int prior to the comparison. > > > > That's sort of true-ish but I don't understand what you are saying? > #confused Yes, I too did not understand David's comment. I tried to dig dipper into max_t but it gets very complex. Can you please elaborate how you determined the promotion to signed int? > > regards, > dan carpenter > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: rtl8192e: Use min_t/max_t macros for variable comparison 2022-11-03 9:18 ` Deepak R Varma @ 2022-11-03 10:09 ` Dan Carpenter 2022-11-04 8:01 ` Deepak R Varma 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2022-11-03 10:09 UTC (permalink / raw) To: Deepak R Varma Cc: David Laight, outreachy@lists.linux.dev, Greg Kroah-Hartman, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org On Thu, Nov 03, 2022 at 02:48:35PM +0530, Deepak R Varma wrote: > On Thu, Nov 03, 2022 at 11:53:45AM +0300, Dan Carpenter wrote: > > On Thu, Nov 03, 2022 at 08:24:15AM +0000, David Laight wrote: > > > > --- a/drivers/staging/rtl8192e/rtl819x_HTProc.c > > > > +++ b/drivers/staging/rtl8192e/rtl819x_HTProc.c > > > > @@ -587,17 +587,12 @@ void HTOnAssocRsp(struct rtllib_device *ieee) > > > > else > > > > pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_64K; > > > > } else { > > > > - if (pPeerHTCap->MaxRxAMPDUFactor < HT_AGG_SIZE_32K) > > > > - pHTInfo->CurrentAMPDUFactor = > > > > - pPeerHTCap->MaxRxAMPDUFactor; > > > > - else > > > > - pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_32K; > > > > + pHTInfo->CurrentAMPDUFactor = min_t(u32, pPeerHTCap->MaxRxAMPDUFactor, > > > > + HT_AGG_SIZE_32K); > > > > > > For min() to fail there must be a signed v unsigned mismatch. > > > Maybe that ought to be fixed. > > > > > > > u32 is the right choice here. > > > > I'm having a hard time understanding your email. You might be saying > > we could declare HT_AGG_SIZE_32K as a u32 so then we could use min() > > instead of min_t()? HT_AGG_SIZE_32K is an enum. > > > > pPeerHTCap->MaxRxAMPDUFactor is a bitfield. > > > > u8 MaxRxAMPDUFactor:2; > > > > We will never be able to use min(). > > I think we could do min((u32)a, (u32)b), but it is just unwrapped min_t > if I understand David's comment. > No. Do not do that. I think it's a checkpatch warning. What you have is fine. > > > > > > } > > > > } > > > > - if (pHTInfo->MPDU_Density > pPeerHTCap->MPDUDensity) > > > > - pHTInfo->current_mpdu_density = pHTInfo->MPDU_Density; > > > > - else > > > > - pHTInfo->current_mpdu_density = pPeerHTCap->MPDUDensity; > > > > + pHTInfo->current_mpdu_density = max_t(u8, pHTInfo->MPDU_Density, > > > > + pPeerHTCap->MPDUDensity); > > > > > > Using u8 with max_t() really doesn't make any sense. > > > > Using u8 looks wrong because you would worry that one of the types is > > larger than U8_MAX. But it's actually fine. The types are u8 vs another > > bitfield. I would probably have gone with u32 here as well. > I will take your advise and upgrade the type to u32 as a revision. Sounds good. It's not something I would have asked you to redo the patch over, but it would have been my personal preference. regards, dan carpenter ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: rtl8192e: Use min_t/max_t macros for variable comparison 2022-11-03 10:09 ` Dan Carpenter @ 2022-11-04 8:01 ` Deepak R Varma 0 siblings, 0 replies; 7+ messages in thread From: Deepak R Varma @ 2022-11-04 8:01 UTC (permalink / raw) To: Dan Carpenter Cc: David Laight, outreachy@lists.linux.dev, Greg Kroah-Hartman, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org On Thu, Nov 03, 2022 at 01:09:20PM +0300, Dan Carpenter wrote: > On Thu, Nov 03, 2022 at 02:48:35PM +0530, Deepak R Varma wrote: > > On Thu, Nov 03, 2022 at 11:53:45AM +0300, Dan Carpenter wrote: > > > On Thu, Nov 03, 2022 at 08:24:15AM +0000, David Laight wrote: > > > > > --- a/drivers/staging/rtl8192e/rtl819x_HTProc.c > > > > > +++ b/drivers/staging/rtl8192e/rtl819x_HTProc.c > > > > > @@ -587,17 +587,12 @@ void HTOnAssocRsp(struct rtllib_device *ieee) > > > > > else > > > > > pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_64K; > > > > > } else { > > > > > - if (pPeerHTCap->MaxRxAMPDUFactor < HT_AGG_SIZE_32K) > > > > > - pHTInfo->CurrentAMPDUFactor = > > > > > - pPeerHTCap->MaxRxAMPDUFactor; > > > > > - else > > > > > - pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_32K; > > > > > + pHTInfo->CurrentAMPDUFactor = min_t(u32, pPeerHTCap->MaxRxAMPDUFactor, > > > > > + HT_AGG_SIZE_32K); > > > > > > > > For min() to fail there must be a signed v unsigned mismatch. > > > > Maybe that ought to be fixed. > > > > > > > > > > u32 is the right choice here. > > > > > > I'm having a hard time understanding your email. You might be saying > > > we could declare HT_AGG_SIZE_32K as a u32 so then we could use min() > > > instead of min_t()? HT_AGG_SIZE_32K is an enum. > > > > > > pPeerHTCap->MaxRxAMPDUFactor is a bitfield. > > > > > > u8 MaxRxAMPDUFactor:2; > > > > > > We will never be able to use min(). > > > > I think we could do min((u32)a, (u32)b), but it is just unwrapped min_t > > if I understand David's comment. > > > > No. Do not do that. I think it's a checkpatch warning. What you have > is fine. > > > > > > > > > } > > > > > } > > > > > - if (pHTInfo->MPDU_Density > pPeerHTCap->MPDUDensity) > > > > > - pHTInfo->current_mpdu_density = pHTInfo->MPDU_Density; > > > > > - else > > > > > - pHTInfo->current_mpdu_density = pPeerHTCap->MPDUDensity; > > > > > + pHTInfo->current_mpdu_density = max_t(u8, pHTInfo->MPDU_Density, > > > > > + pPeerHTCap->MPDUDensity); > > > > > > > > Using u8 with max_t() really doesn't make any sense. > > > > > > Using u8 looks wrong because you would worry that one of the types is > > > larger than U8_MAX. But it's actually fine. The types are u8 vs another > > > bitfield. I would probably have gone with u32 here as well. > > I will take your advise and upgrade the type to u32 as a revision. > > Sounds good. It's not something I would have asked you to redo the > patch over, but it would have been my personal preference. That is no problem. I am waiting on David to elaborate on his feedback and accordingly plan a consolidated revision. > > regards, > dan carpenter > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-04 8:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-02 22:36 [PATCH] staging: rtl8192e: Use min_t/max_t macros for variable comparison Deepak R Varma 2022-11-02 22:56 ` Philipp Hortmann 2022-11-03 8:24 ` David Laight 2022-11-03 8:53 ` Dan Carpenter 2022-11-03 9:18 ` Deepak R Varma 2022-11-03 10:09 ` Dan Carpenter 2022-11-04 8:01 ` Deepak R Varma
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox