Re: [PATCH 4/3] rt2860sta: call release_firmware on module exit,else resume is broken

[Ben Hutchings]

On Sat, 2009-04-18 at 16:15 +0100, Darren Salt wrote:
> Resume breakage is as follows: it takes somewhat longer (firmware request
> timeout), as a result of which the firmware isn't reloaded; and any
> subsequent attempt to trying to bring down the interface results in a panic &
> lockup (SysRq failure on my 901).
> 
> Consequently, it's better to request the firmware once and hang onto it until
> the module is unloaded.

Greg hasn't accepted the previous patch, so there's not much point
submitting patches on top of it yet.  I suggest you post a combined
patch once David has done a release of linux-firmware including
rt2860.bin.

> Signed-off-by: Darren Salt <linux@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> 
> diff -pur a/drivers/staging/rt2860/2860_main_dev.c b/drivers/staging/rt2860/2860_main_dev.c
> --- a/drivers/staging/rt2860/2860_main_dev.c
> +++ b/drivers/staging/rt2860/2860_main_dev.c
> @@ -289,6 +289,7 @@ static INT __init rt2860_init_module(VOI
>  static VOID __exit rt2860_cleanup_module(VOID)
>  {
>      pci_unregister_driver(&rt2860_driver);
> +    NICForgetFirmware();
>  }
>  
>  module_init(rt2860_init_module);
> diff -pur a/drivers/staging/rt2860/common/rtmp_init.c b/drivers/staging/rt2860/common/rtmp_init.c
> --- a/drivers/staging/rt2860/common/rtmp_init.c
> +++ b/drivers/staging/rt2860/common/rtmp_init.c
> @@ -144,6 +144,8 @@ RTMP_REG_PAIR	STAMACRegTable[] =	{
>  #define FW_FILENAME "rt2860.bin"
>  MODULE_FIRMWARE(FW_FILENAME);
>  
> +const struct firmware *fw = NULL;
> +

Must be static.

>  #define FIRMWAREIMAGE_MAX_LENGTH	0x2000
>  #define FIRMWARE_MAJOR_VERSION	0
>  #define FIRMWARE_MINOR_VERSION	2
> @@ -2583,6 +2585,12 @@ VOID NICEraseFirmware(
>  
>  }/* End of NICEraseFirmware */
>  
> +VOID NICForgetFirmware(void)
> +{
> +	release_firmware(fw);
> +	fw = NULL;
> +}
> +
>  /*
>  	========================================================================
>  
> @@ -2603,11 +2611,11 @@ VOID NICEraseFirmware(
>  NDIS_STATUS NICLoadFirmware(
>  	IN PRTMP_ADAPTER pAd)
>  {
> -	const struct firmware *fw;
>  	int ret, i;
>  	NDIS_STATUS Status = NDIS_STATUS_SUCCESS;
>  
> -	ret = request_firmware(&fw, FW_FILENAME,
> +	ret = fw ? 0 :
> +	      request_firmware(&fw, FW_FILENAME,
>  			       &((POS_COOKIE)pAd->OS_Cookie)->pci_dev->dev);

This is not thread-safe unless NICLoadFirmware is guarded by a global
lock.  Is it always called under RTNL?  If so, we can add ASSERT_RTNL()
to ensure that this doesn't change.

>  	if (ret) {
>  		printk(KERN_ERR "rt2860sta: firmware file %s request failed (%d)\n",
> @@ -2659,7 +2667,7 @@ NDIS_STATUS NICLoadFirmware(
>  	RTMP_IO_WRITE32(pAd, H2M_BBP_AGENT, 0);
>  	RTMP_IO_WRITE32(pAd, H2M_MAILBOX_CSR, 0);
>  
> -	release_firmware(fw);
> +	/*release_firmware(fw);*/
[...]

Just remove it.
 
Ben.

Attachment: signature.asc
Description: This is a digitally signed message part