| Message ID | 20220117142919.207370-2-marcan@marcan.st (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| 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 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? > goto fail; > }
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 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.
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.
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c index 8b149996fc00..f876b1d8d00d 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c @@ -1777,6 +1777,8 @@ static void brcmf_pcie_setup(struct device *dev, int ret, ret = brcmf_chip_get_raminfo(devinfo->ci); if (ret) { brcmf_err(bus, "Failed to get RAM info\n"); + release_firmware(fw); + brcmf_fw_nvram_free(nvram); goto fail; }
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: 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(+)