2022-07-13 08:11:58

by Martin Kaiser

Subject: [PATCH] staging: r888eu: use dynamic allocation for efuse buffer

Use kmalloc to allocate the efuse buffer in ReadAdapterInfo8188EU and
free it on exit. This is better than using a 512 byte array on the stack.

It's ok to drop the __aligned(4) qualifier. kmalloc aligns to
ARCH_KMALLOC_MINALIGN, this is at least 8 bytes.

Suggested-by: Dan Carpenter <[email protected]>
Suggested-by: Larry Finger <[email protected]>
Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/hal/usb_halinit.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_halinit.c b/drivers/staging/r8188eu/hal/usb_halinit.c
index 8902dda7b8d8..421fe7c40390 100644
--- a/drivers/staging/r8188eu/hal/usb_halinit.c
+++ b/drivers/staging/r8188eu/hal/usb_halinit.c
@@ -926,7 +926,7 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)
{
struct eeprom_priv *eeprom = &Adapter->eeprompriv;
struct led_priv *ledpriv = &Adapter->ledpriv;
- u8 efuse_buf[EFUSE_MAP_LEN_88E] __aligned(4);
+ u8 *efuse_buf;
u8 eeValue;
int res;

@@ -937,7 +937,10 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)

eeprom->bautoload_fail_flag = !(eeValue & EEPROM_EN);

- memset(efuse_buf, 0xFF, sizeof(efuse_buf));
+ efuse_buf = kmalloc(EFUSE_MAP_LEN_88E, GFP_KERNEL);
+ if (!efuse_buf)
+ return;
+ memset(efuse_buf, 0xFF, EFUSE_MAP_LEN_88E);

if (!(eeValue & BOOT_FROM_EEPROM) && !eeprom->bautoload_fail_flag) {
rtl8188e_EfusePowerSwitch(Adapter, true);
@@ -957,6 +960,7 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)
Hal_ReadThermalMeter_88E(Adapter, efuse_buf, eeprom->bautoload_fail_flag);

ledpriv->bRegUseLed = true;
+ kfree(efuse_buf);
}

static void ResumeTxBeacon(struct adapter *adapt)
--
2.30.2

2022-07-13 10:43:21

by Pavel Skripkin

Subject: Re: [PATCH] staging: r888eu: use dynamic allocation for efuse buffer

Hi Martin,

Martin Kaiser <[email protected]> says:
> Use kmalloc to allocate the efuse buffer in ReadAdapterInfo8188EU and
> free it on exit. This is better than using a 512 byte array on the stack.
>
> It's ok to drop the __aligned(4) qualifier. kmalloc aligns to
> ARCH_KMALLOC_MINALIGN, this is at least 8 bytes.
>
> Suggested-by: Dan Carpenter <[email protected]>
> Suggested-by: Larry Finger <[email protected]>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_halinit.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_halinit.c b/drivers/staging/r8188eu/hal/usb_halinit.c
> index 8902dda7b8d8..421fe7c40390 100644
> --- a/drivers/staging/r8188eu/hal/usb_halinit.c
> +++ b/drivers/staging/r8188eu/hal/usb_halinit.c
> @@ -926,7 +926,7 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)
> {
> struct eeprom_priv *eeprom = &Adapter->eeprompriv;
> struct led_priv *ledpriv = &Adapter->ledpriv;
> - u8 efuse_buf[EFUSE_MAP_LEN_88E] __aligned(4);
> + u8 *efuse_buf;
> u8 eeValue;
> int res;
>
> @@ -937,7 +937,10 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)
>
> eeprom->bautoload_fail_flag = !(eeValue & EEPROM_EN);
>
> - memset(efuse_buf, 0xFF, sizeof(efuse_buf));
> + efuse_buf = kmalloc(EFUSE_MAP_LEN_88E, GFP_KERNEL);
> + if (!efuse_buf)
> + return;

I think, it worth returning an error to caller. Functions right after
the allocation do initialization, so leaving fields as-is seems to be
dangerous





Thanks,
--Pavel Skripkin

2022-07-13 13:31:39

by Larry Finger

Subject: Re: [PATCH] staging: r888eu: use dynamic allocation for efuse buffer

On 7/13/22 02:58, Martin Kaiser wrote:
> Use kmalloc to allocate the efuse buffer in ReadAdapterInfo8188EU and
> free it on exit. This is better than using a 512 byte array on the stack.
>
> It's ok to drop the __aligned(4) qualifier. kmalloc aligns to
> ARCH_KMALLOC_MINALIGN, this is at least 8 bytes.
>
> Suggested-by: Dan Carpenter <[email protected]>
> Suggested-by: Larry Finger <[email protected]>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_halinit.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_halinit.c b/drivers/staging/r8188eu/hal/usb_halinit.c
> index 8902dda7b8d8..421fe7c40390 100644
> --- a/drivers/staging/r8188eu/hal/usb_halinit.c
> +++ b/drivers/staging/r8188eu/hal/usb_halinit.c
> @@ -926,7 +926,7 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)
> {
> struct eeprom_priv *eeprom = &Adapter->eeprompriv;
> struct led_priv *ledpriv = &Adapter->ledpriv;
> - u8 efuse_buf[EFUSE_MAP_LEN_88E] __aligned(4);
> + u8 *efuse_buf;
> u8 eeValue;
> int res;
>
> @@ -937,7 +937,10 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)
>
> eeprom->bautoload_fail_flag = !(eeValue & EEPROM_EN);
>
> - memset(efuse_buf, 0xFF, sizeof(efuse_buf));
> + efuse_buf = kmalloc(EFUSE_MAP_LEN_88E, GFP_KERNEL);
> + if (!efuse_buf)
> + return;
> + memset(efuse_buf, 0xFF, EFUSE_MAP_LEN_88E);
>
> if (!(eeValue & BOOT_FROM_EEPROM) && !eeprom->bautoload_fail_flag) {
> rtl8188e_EfusePowerSwitch(Adapter, true);
> @@ -957,6 +960,7 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)
> Hal_ReadThermalMeter_88E(Adapter, efuse_buf, eeprom->bautoload_fail_flag);
>
> ledpriv->bRegUseLed = true;
> + kfree(efuse_buf);
> }
>
> static void ResumeTxBeacon(struct adapter *adapt)

Acked-by: Larry Finger <[email protected]>

Thanks,

Larry

2022-07-15 06:12:22

by Michael Straube

Subject: Re: [PATCH] staging: r888eu: use dynamic allocation for efuse buffer

Hi Martin,

just a typo in the subject line: r888eu

regards,
Michael

2022-07-15 06:12:31

by Michael Straube

Subject: Re: [PATCH] staging: r888eu: use dynamic allocation for efuse buffer

On 7/15/22 08:02, Michael Straube wrote:
> Hi Martin,
>
> just a typo in the subject line: r888eu
>

Sorry, I did not notice that this patch is already applied.
So nevermind ...

Michael

2022-07-15 20:16:02

by Martin Kaiser

Subject: Re: [PATCH] staging: r888eu: use dynamic allocation for efuse buffer

Hi Pavel,

Thus wrote Pavel Skripkin ([email protected]):

> Hi Martin,

> Martin Kaiser <[email protected]> says:
> > Use kmalloc to allocate the efuse buffer in ReadAdapterInfo8188EU and
> > free it on exit. This is better than using a 512 byte array on the stack.

> > It's ok to drop the __aligned(4) qualifier. kmalloc aligns to
> > ARCH_KMALLOC_MINALIGN, this is at least 8 bytes.

> > Suggested-by: Dan Carpenter <[email protected]>
> > Suggested-by: Larry Finger <[email protected]>
> > Signed-off-by: Martin Kaiser <[email protected]>
> > ---
> > drivers/staging/r8188eu/hal/usb_halinit.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)

> > diff --git a/drivers/staging/r8188eu/hal/usb_halinit.c b/drivers/staging/r8188eu/hal/usb_halinit.c
> > index 8902dda7b8d8..421fe7c40390 100644
> > --- a/drivers/staging/r8188eu/hal/usb_halinit.c
> > +++ b/drivers/staging/r8188eu/hal/usb_halinit.c
> > @@ -926,7 +926,7 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)
> > {
> > struct eeprom_priv *eeprom = &Adapter->eeprompriv;
> > struct led_priv *ledpriv = &Adapter->ledpriv;
> > - u8 efuse_buf[EFUSE_MAP_LEN_88E] __aligned(4);
> > + u8 *efuse_buf;
> > u8 eeValue;
> > int res;
> > @@ -937,7 +937,10 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)
> > eeprom->bautoload_fail_flag = !(eeValue & EEPROM_EN);
> > - memset(efuse_buf, 0xFF, sizeof(efuse_buf));
> > + efuse_buf = kmalloc(EFUSE_MAP_LEN_88E, GFP_KERNEL);
> > + if (!efuse_buf)
> > + return;

> I think, it worth returning an error to caller. Functions right after the
> allocation do initialization, so leaving fields as-is seems to be dangerous

yes, that makes sense. We could refuse to load the driver in this case.

Larry and Greg already accepted the patch as is, I'll add error handling
in a separate patch.

Thanks,
Martin