| Message ID | 20230416-brcmfmac-noise-v1-1-f0624e408761@marcan.st (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | brcmfmac: Demote some kernel errors to info | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Sun, Apr 16, 2023 at 09:42:17PM +0900, Hector Martin wrote: > People are getting spooked by brcmfmac errors on their boot console. > There's no reason for these messages to be errors. > > Cc: stable@vger.kernel.org > Fixes: d6a5c562214f ("wifi: brcmfmac: add support for vendor-specific firmware api") > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c | 4 ++-- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c | 4 ++-- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c | 4 ++-- > 3 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c > index ac3a36fa3640..c83bc435b257 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c > @@ -12,13 +12,13 @@ > > static int brcmf_bca_attach(struct brcmf_pub *drvr) > { > - pr_err("%s: executing\n", __func__); > + pr_info("%s: executing\n", __func__); Why are these here at all? Please just remove these entirely, you can get this information normally with ftrace. Or, just delete these functions, why have empty ones at all? thanks, greg k-h
Greg KH <gregkh@linuxfoundation.org> writes: > On Sun, Apr 16, 2023 at 09:42:17PM +0900, Hector Martin wrote: > >> People are getting spooked by brcmfmac errors on their boot console. >> There's no reason for these messages to be errors. >> >> Cc: stable@vger.kernel.org >> Fixes: d6a5c562214f ("wifi: brcmfmac: add support for vendor-specific firmware api") >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c | 4 ++-- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c | 4 ++-- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c | 4 ++-- >> 3 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git >> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c >> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c >> index ac3a36fa3640..c83bc435b257 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c >> @@ -12,13 +12,13 @@ >> >> static int brcmf_bca_attach(struct brcmf_pub *drvr) >> { >> - pr_err("%s: executing\n", __func__); >> + pr_info("%s: executing\n", __func__); > > Why are these here at all? Please just remove these entirely, you can > get this information normally with ftrace. > > Or, just delete these functions, why have empty ones at all? Yeah, deleting these sound like the best idea.
On 16/04/2023 21.46, Greg KH wrote: > On Sun, Apr 16, 2023 at 09:42:17PM +0900, Hector Martin wrote: >> People are getting spooked by brcmfmac errors on their boot console. >> There's no reason for these messages to be errors. >> >> Cc: stable@vger.kernel.org >> Fixes: d6a5c562214f ("wifi: brcmfmac: add support for vendor-specific firmware api") >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c | 4 ++-- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c | 4 ++-- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c | 4 ++-- >> 3 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c >> index ac3a36fa3640..c83bc435b257 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c >> @@ -12,13 +12,13 @@ >> >> static int brcmf_bca_attach(struct brcmf_pub *drvr) >> { >> - pr_err("%s: executing\n", __func__); >> + pr_info("%s: executing\n", __func__); > > Why are these here at all? Please just remove these entirely, you can > get this information normally with ftrace. > > Or, just delete these functions, why have empty ones at all? This is a new WIP code path that Arend introduced which currently deliberately does nothing (but is intended to hold firmware vendor specific init in the future). So we can just drop the messages, but I don't think we want to remove the code entirely. - Hector
On Mon, Apr 17, 2023 at 04:54:33PM +0900, Hector Martin wrote: > On 16/04/2023 21.46, Greg KH wrote: > > On Sun, Apr 16, 2023 at 09:42:17PM +0900, Hector Martin wrote: > >> People are getting spooked by brcmfmac errors on their boot console. > >> There's no reason for these messages to be errors. > >> > >> Cc: stable@vger.kernel.org > >> Fixes: d6a5c562214f ("wifi: brcmfmac: add support for vendor-specific firmware api") > >> Signed-off-by: Hector Martin <marcan@marcan.st> > >> --- > >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c | 4 ++-- > >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c | 4 ++-- > >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c | 4 ++-- > >> 3 files changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c > >> index ac3a36fa3640..c83bc435b257 100644 > >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c > >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c > >> @@ -12,13 +12,13 @@ > >> > >> static int brcmf_bca_attach(struct brcmf_pub *drvr) > >> { > >> - pr_err("%s: executing\n", __func__); > >> + pr_info("%s: executing\n", __func__); > > > > Why are these here at all? Please just remove these entirely, you can > > get this information normally with ftrace. > > > > Or, just delete these functions, why have empty ones at all? > > This is a new WIP code path that Arend introduced which currently > deliberately does nothing (but is intended to hold firmware vendor > specific init in the future). So we can just drop the messages, but I > don't think we want to remove the code entirely. Why have empty functions that do nothing? If you want to put vendor-specific anything in here, add it when that is needed. We don't like having dead code laying around in the kernel if at all possible. thanks, greg k-h
On 17/04/2023 17.06, Greg KH wrote: > On Mon, Apr 17, 2023 at 04:54:33PM +0900, Hector Martin wrote: >> On 16/04/2023 21.46, Greg KH wrote: >>> On Sun, Apr 16, 2023 at 09:42:17PM +0900, Hector Martin wrote: >>>> People are getting spooked by brcmfmac errors on their boot console. >>>> There's no reason for these messages to be errors. >>>> >>>> Cc: stable@vger.kernel.org >>>> Fixes: d6a5c562214f ("wifi: brcmfmac: add support for vendor-specific firmware api") >>>> Signed-off-by: Hector Martin <marcan@marcan.st> >>>> --- >>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c | 4 ++-- >>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c | 4 ++-- >>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c | 4 ++-- >>>> 3 files changed, 6 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c >>>> index ac3a36fa3640..c83bc435b257 100644 >>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c >>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c >>>> @@ -12,13 +12,13 @@ >>>> >>>> static int brcmf_bca_attach(struct brcmf_pub *drvr) >>>> { >>>> - pr_err("%s: executing\n", __func__); >>>> + pr_info("%s: executing\n", __func__); >>> >>> Why are these here at all? Please just remove these entirely, you can >>> get this information normally with ftrace. >>> >>> Or, just delete these functions, why have empty ones at all? >> >> This is a new WIP code path that Arend introduced which currently >> deliberately does nothing (but is intended to hold firmware vendor >> specific init in the future). So we can just drop the messages, but I >> don't think we want to remove the code entirely. > > Why have empty functions that do nothing? If you want to put > vendor-specific anything in here, add it when that is needed. We don't > like having dead code laying around in the kernel if at all possible. That's a question for Arend. But I think we should not be blocking this fix on that. I'll send a v2 that just removes the messages. - Hector
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c index ac3a36fa3640..c83bc435b257 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c @@ -12,13 +12,13 @@ static int brcmf_bca_attach(struct brcmf_pub *drvr) { - pr_err("%s: executing\n", __func__); + pr_info("%s: executing\n", __func__); return 0; } static void brcmf_bca_detach(struct brcmf_pub *drvr) { - pr_err("%s: executing\n", __func__); + pr_info("%s: executing\n", __func__); } const struct brcmf_fwvid_ops brcmf_bca_ops = { diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c index b75652ba9359..e39d66b07831 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c @@ -12,13 +12,13 @@ static int brcmf_cyw_attach(struct brcmf_pub *drvr) { - pr_err("%s: executing\n", __func__); + pr_info("%s: executing\n", __func__); return 0; } static void brcmf_cyw_detach(struct brcmf_pub *drvr) { - pr_err("%s: executing\n", __func__); + pr_info("%s: executing\n", __func__); } const struct brcmf_fwvid_ops brcmf_cyw_ops = { diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c index 02de99818efa..133d274b4025 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c @@ -12,13 +12,13 @@ static int brcmf_wcc_attach(struct brcmf_pub *drvr) { - pr_err("%s: executing\n", __func__); + pr_info("%s: executing\n", __func__); return 0; } static void brcmf_wcc_detach(struct brcmf_pub *drvr) { - pr_err("%s: executing\n", __func__); + pr_info("%s: executing\n", __func__); } const struct brcmf_fwvid_ops brcmf_wcc_ops = {
People are getting spooked by brcmfmac errors on their boot console. There's no reason for these messages to be errors. Cc: stable@vger.kernel.org Fixes: d6a5c562214f ("wifi: brcmfmac: add support for vendor-specific firmware api") Signed-off-by: Hector Martin <marcan@marcan.st> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/bca/core.c | 4 ++-- drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c | 4 ++-- drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-)