On Wed, Apr 06, 2022 at 07:09:44PM -0500, Rebecca Mckeever wrote:
> Replace if else statement with min_t or max_t macros to increase
> readability and conform to Linux kernel coding style. The _t versions
> of the macros must be used to avoid applying typeof to the bit fields
> pPeerHTCap->MaxRxAMPDUFactor, and pPeerHTCap->MPDUDensity.
> Found with minmax coccinelle script.
>
> Signed-off-by: Rebecca Mckeever <remckee0@xxxxxxxxx>
> ---
> drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> index dba3f2db9f48..8f2cd669c5d4 100644
> --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> @@ -940,10 +940,8 @@ void HTOnAssocRsp(struct ieee80211_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(u8, pPeerHTCap->MaxRxAMPDUFactor,
> + HT_AGG_SIZE_32K);
The u8 cast doesn't cause a problem, but it makes me nervous so I have
to look up the types involved.
pPeerHTCap->MaxRxAMPDUFactor is 2 bits of a u8.
HT_AGG_SIZE_32K is an enum, which in real life terms of the compilers we
use is going to be a uint.
With min_t() you want to do the casts to a large enough unsigned type
so that it doesn't truncate. And so u8 works. But it would be less
scary if you cast it to a u32 or uint because a read will assume that
probably pPeerHTCap->MaxRxAMPDUFactor is a u32 and HT_AGG_SIZE_32K is an
int. These are false assumptions, but probably that's what most people
will assume.
The reader in this case will feel safe and not worry about truncation so
they won't have to look up the types.
regards,
dan carpenter
