re: mwifiex: add firmware dump feature for PCIe

re: mwifiex: add firmware dump feature for PCIe

[Dan Carpenter]

Hello Amitkumar Karwar,

The patch e050c76fcf49: "mwifiex: add firmware dump feature for PCIe"
from Apr 17, 2014, leads to the following static checker warning:

	drivers/net/wireless/mwifiex/pcie.c:2252 mwifiex_pcie_fw_dump_work()
	error: we previously assumed 'adapter' could be null (see line 2251)

drivers/net/wireless/mwifiex/pcie.c
  2228  /* This function dump firmware memory to file */
  2229  static void mwifiex_pcie_fw_dump_work(struct work_struct *work)
  2230  {
  2231          struct mwifiex_adapter *adapter =
  2232                          container_of(work, struct mwifiex_adapter, iface_work);
  2233          unsigned int reg, reg_start, reg_end;
  2234          u8 *dbg_ptr;
  2235          struct timeval t;
  2236          u8 dump_num = 0, idx, i, read_reg, doneflag = 0;
  2237          enum rdwr_status stat;
  2238          u32 memory_size;
  2239          u8 filename[MAX_FULL_NAME_LEN];
  2240          mm_segment_t fs;
  2241          loff_t pos;
  2242          u8 *end_ptr;
  2243          u8 *name_prefix = "/var/log/fw_dump_";
  2244          struct memory_type_mapping mem_type_mapping_tbl[] = {
  2245                  {"ITCM", NULL, NULL, 0xF0},
  2246                  {"DTCM", NULL, NULL, 0xF1},
  2247                  {"SQRAM", NULL, NULL, 0xF2},
  2248                  {"IRAM", NULL, NULL, 0xF3},
  2249          };
  2250  
  2251          if (!adapter) {
                     ^^^^^^^
Check.

  2252                  dev_err(adapter->dev, "Could not dump firmwware info\n");
                                ^^^^^^^^^^^^
Dereference.

  2253                  return;
  2254          }

The main question is why are we writing to /var and /tmp anyway instead
of putting this in debugfs or sysfs?

regards,
dan carpenter

RE: mwifiex: add firmware dump feature for PCIe

[Bing Zhao]

Hi Dan,

> Hello Amitkumar Karwar,
> 
> The patch e050c76fcf49: "mwifiex: add firmware dump feature for PCIe"
> from Apr 17, 2014, leads to the following static checker warning:
> 
> 	drivers/net/wireless/mwifiex/pcie.c:2252 mwifiex_pcie_fw_dump_work()
> 	error: we previously assumed 'adapter' could be null (see line 2251)

Thanks for reporting this error.

> 
> drivers/net/wireless/mwifiex/pcie.c
>   2228  /* This function dump firmware memory to file */
>   2229  static void mwifiex_pcie_fw_dump_work(struct work_struct *work)
>   2230  {
>   2231          struct mwifiex_adapter *adapter =
>   2232                          container_of(work, struct mwifiex_adapter, iface_work);
>   2233          unsigned int reg, reg_start, reg_end;
>   2234          u8 *dbg_ptr;
>   2235          struct timeval t;
>   2236          u8 dump_num = 0, idx, i, read_reg, doneflag = 0;
>   2237          enum rdwr_status stat;
>   2238          u32 memory_size;
>   2239          u8 filename[MAX_FULL_NAME_LEN];
>   2240          mm_segment_t fs;
>   2241          loff_t pos;
>   2242          u8 *end_ptr;
>   2243          u8 *name_prefix = "/var/log/fw_dump_";
>   2244          struct memory_type_mapping mem_type_mapping_tbl[] = {
>   2245                  {"ITCM", NULL, NULL, 0xF0},
>   2246                  {"DTCM", NULL, NULL, 0xF1},
>   2247                  {"SQRAM", NULL, NULL, 0xF2},
>   2248                  {"IRAM", NULL, NULL, 0xF3},
>   2249          };
>   2250
>   2251          if (!adapter) {
>                      ^^^^^^^
> Check.
> 
>   2252                  dev_err(adapter->dev, "Could not dump firmwware info\n");
>                                 ^^^^^^^^^^^^
> Dereference.

You are right, adapter is NULL here. I will send a patch to fix it.

> 
>   2253                  return;
>   2254          }
> 
> The main question is why are we writing to /var and /tmp anyway instead
> of putting this in debugfs or sysfs?

AFAIK, the debugfs or sysfs cannot store/hold the files we retrieve from firmware at the scene. We write fw_dump files to rootfs so that the files are stored even if the system reboots due to hung_task timeout. 

Thanks,
Bing

Re: mwifiex: add firmware dump feature for PCIe

[Dan Carpenter]

On Wed, Apr 23, 2014 at 02:37:09PM -0700, Bing Zhao wrote:
> 
> AFAIK, the debugfs or sysfs cannot store/hold the files we retrieve
> from firmware at the scene. We write fw_dump files to rootfs so that
> the files are stored even if the system reboots due to hung_task
> timeout. 

Ah.  Gotcha.

regards,
dan carpenter

Re: mwifiex: add firmware dump feature for PCIe

[Kalle Valo]

Bing Zhao <bzhao@marvell.com> writes:

>> The main question is why are we writing to /var and /tmp anyway instead
>> of putting this in debugfs or sysfs?

IMHO no sane wifi driver should _ever_ write anything to a file system.
That's just evil.

> AFAIK, the debugfs or sysfs cannot store/hold the files we retrieve
> from firmware at the scene. We write fw_dump files to rootfs so that
> the files are stored even if the system reboots due to hung_task
> timeout.

Can't you have a user space tool reading the sysfs file and store the
file permanently? That way kernel is not involved in file system access
at all.

-- 
Kalle Valo