| Message ID | 20220117142919.207370-1-marcan@marcan.st |
|---|---|
| Headers | show |
| Series | misc brcmfmac fixes (M1/T2 series spin-off) | expand |
On 1/17/2022 3:29 PM, Hector Martin wrote: > This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that > the CLM blob is released in the device remove path. > > Fixes: 82f93cf46d60 ("brcmfmac: get chip's default RAM info during PCIe setup") Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 2 ++ > 1 file changed, 2 insertions(+)
On 1/17/2022 3:29 PM, Hector Martin wrote: > This allows us to get console messages if the firmware crashed during > early init, or if an operation failed and we're about to shut down. We do have a module parameter that forces probing to succeed regardless any failure so we can create memory dump of the wifi device, read the console, etc. Still it can be useful to add these console read calls. Thanks. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 4 ++++ > 1 file changed, 4 insertions(+)
19.01.2022 20:49, Andy Shevchenko пишет: > On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <marcan@marcan.st> wrote: >> >> This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that >> the CLM blob is released in the device remove path. > > ... > >> if (ret) { > >> brcmf_err(bus, "Failed to get RAM info\n"); >> + release_firmware(fw); >> + brcmf_fw_nvram_free(nvram); > > Can we first undo the things and only after print a message? Having message first usually is more preferred because at minimum you'll get the message if "undoing the things" crashes, i.e. will be more obvious what happened.
On 1/19/2022 6:49 PM, Andy Shevchenko wrote: > On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <marcan@marcan.st> wrote: >> >> This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that >> the CLM blob is released in the device remove path. > > ... > >> if (ret) { > >> brcmf_err(bus, "Failed to get RAM info\n"); >> + release_firmware(fw); >> + brcmf_fw_nvram_free(nvram); > > Can we first undo the things and only after print a message? What would be your motivation? When reading logs I am used to seeing an error message followed by cleanup related messages. Following your suggestion you could see cleanup related messages, the error print as above, followed by more cleanup related messages. The cleanup routine would preferably be silent, but I tend to flip on extra debug message levels. Regards, Arend >> goto fail; >> } > >
20.01.2022 00:31, Andy Shevchenko пишет: > On Wed, Jan 19, 2022 at 11:22 PM Dmitry Osipenko <digetx@gmail.com> wrote: >> >> 19.01.2022 20:49, Andy Shevchenko пишет: >>> On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <marcan@marcan.st> wrote: >>>> >>>> This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that >>>> the CLM blob is released in the device remove path. >>> >>> ... >>> >>>> if (ret) { >>> >>>> brcmf_err(bus, "Failed to get RAM info\n"); >>>> + release_firmware(fw); >>>> + brcmf_fw_nvram_free(nvram); >>> >>> Can we first undo the things and only after print a message? >> >> Having message first usually is more preferred because at minimum you'll >> get the message if "undoing the things" crashes, i.e. will be more >> obvious what happened. > > If "undo the things" crashes, I would rather like to see that crash > report, while serial UART at 9600 will continue flushing the message > and then hang without any pointers to what the heck happened. Not > here, but in general, messages are also good to be out of the locks. The hang is actually a better example. It's the most annoying when there is a silent hang and no error messages.