staging: r888eu: use dynamic allocation for efuse buffer

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

Posted by Martin Kaiser 3 years, 9 months ago

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 <dan.carpenter@oracle.com>
Suggested-by: Larry Finger <Larry.Finger@lwfinger.net>
Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 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

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

Posted by Michael Straube 3 years, 9 months ago

Hi Martin,

just a typo in the subject line: r888eu

regards,
Michael

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

Posted by Michael Straube 3 years, 9 months ago

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

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

Posted by Larry Finger 3 years, 9 months ago

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 <dan.carpenter@oracle.com>
> Suggested-by: Larry Finger <Larry.Finger@lwfinger.net>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>   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 <Larry.Finger@lwfinger.net>

Thanks,

Larry

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

Posted by Pavel Skripkin 3 years, 9 months ago

Hi Martin,

Martin Kaiser <martin@kaiser.cx> 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 <dan.carpenter@oracle.com>
> Suggested-by: Larry Finger <Larry.Finger@lwfinger.net>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>   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

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

Posted by Martin Kaiser 3 years, 9 months ago

Hi Pavel,

Thus wrote Pavel Skripkin (paskripkin@gmail.com):

> Hi Martin,

> Martin Kaiser <martin@kaiser.cx> 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 <dan.carpenter@oracle.com>
> > Suggested-by: Larry Finger <Larry.Finger@lwfinger.net>
> > Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> > ---
> >   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