On Thu, 14 Feb 2019 at 13:38, Arend Van Spriel
<arend.vanspriel@xxxxxxxxxxxx> wrote:
> On 2/13/2019 12:26 PM, Rafał Miłecki wrote:
> > From: Rafał Miłecki <rafal@xxxxxxxxxx>
> >
> > This recently added macro provides more meaningful error messages thanks
> > to identifying a specific wiphy. It's especially important on systems
> > with few cards supported by the same (brcmfmac) driver.
>
> Acked-by: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx>
> > Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
> > ---
> > I obviously couldn't hit & test all error cases. To perform some basic
> > testing I added:
> > bphy_err(wiphy, "test\n");
> > for every wiphy variable added by this patch & tested in on BCM4366.
> >
> > I got a lot of messages like:
> > [ 2842.424524] ieee80211 phy1: brcmf_netdev_start_xmit: test
> > [ 2842.434921] ieee80211 phy1: brcmf_msgbuf_txflow: test
> > [ 2842.440275] ieee80211 phy1: brcmf_msgbuf_process_msgtype: test
> > [ 2842.446141] ieee80211 phy1: brcmf_get_ifp: test
> > so it seems to work correctly.
>
> I was thinking about fact that module name is no longer prefixed, but I
> am not sure that is a bad thing here.
I think including function name should allow identifying error message
in 99,9% cases. If for some reason we want the "brcmfmac: " prefix, it
should only take a simple change to the bphy_err() macro.
> > .../broadcom/brcm80211/brcmfmac/bcdc.c | 26 +++---
> > .../broadcom/brcm80211/brcmfmac/common.c | 38 +++++----
> > .../broadcom/brcm80211/brcmfmac/core.c | 78 ++++++++++-------
> > .../broadcom/brcm80211/brcmfmac/feature.c | 6 +-
> > .../broadcom/brcm80211/brcmfmac/fweh.c | 22 +++--
> > .../broadcom/brcm80211/brcmfmac/fwil.c | 15 ++--
> > .../broadcom/brcm80211/brcmfmac/fwsignal.c | 36 +++++---
> > .../broadcom/brcm80211/brcmfmac/msgbuf.c | 65 ++++++++------
> > .../broadcom/brcm80211/brcmfmac/p2p.c | 85 +++++++++++--------
> > .../broadcom/brcm80211/brcmfmac/pno.c | 22 +++--
> > .../broadcom/brcm80211/brcmfmac/proto.c | 7 +-
> > 11 files changed, 240 insertions(+), 160 deletions(-)
>
> [...]
>
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> > index e772c0845638..2e0e2badfb80 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>
> [...]
>
> > @@ -141,7 +142,8 @@ void brcmf_configure_arp_nd_offload(struct brcmf_if *ifp, bool enable)
> >
> > static void _brcmf_set_multicast_list(struct work_struct *work)
> > {
> > - struct brcmf_if *ifp;
> > + struct brcmf_if *ifp = container_of(work, struct brcmf_if, multicast_work);
>
> checkpatch complains about the length here.
One little warning. I thought it may pass unnoticed ;) Let me fix that.
