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
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
Bing Zhao <[email protected]> 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
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