| Message ID | 20170118142703.9824-2-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On 18-1-2017 15:27, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This macro accepts an extra argument of struct brcmf_pub pointer. It > allows referencing struct device and printing error using dev_err. This > is very useful on devices with more than one wireless card suppored by > brcmfmac. Thanks for dev_err it's possible to identify device that error > is related to. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > I could convert few more brcmf_err calls to this new brcmf_err_pub but I don't > want to spent too much time on this in case someone has a better idea. > > If you do, comment! :) I do not, but still comment ;-). Like the idea. Was on our list because it is damn convenient when testing on router with multiple devices. I would prefer to replace the existing brcmf_err() rather than introducing a new one and would like to do the same for brcmf_dbg(). However, not all call sites have a struct brcmf_pub instance available. Everywhere before completing brcmf_attach() that is. Regards, Arend > Example > Before: > [ 14.735640] brcmfmac: brcmf_c_preinit_dcmds: Firmware version = wl0: Sep 18 2015 03:30:01 version 7.35.177.56 (r587209) FWID 01-6cb8e269 > [ 15.155732] brcmfmac: brcmf_c_preinit_dcmds: Firmware version = wl0: Sep 18 2015 03:30:01 version 7.35.177.56 (r587209) FWID 01-6cb8e269 > [ 15.535682] brcmfmac: brcmf_c_preinit_dcmds: Firmware version = wl0: Sep 18 2015 03:30:01 version 7.35.177.56 (r587209) FWID 01-6cb8e269 > After: > [ 14.746754] brcmfmac 0000:01:00.0: brcmf_c_preinit_dcmds: Firmware version = wl0: Sep 18 2015 03:30:01 version 7.35.177.56 (r587209) FWID 01-6cb8e269 > [ 15.166854] brcmfmac 0001:03:00.0: brcmf_c_preinit_dcmds: Firmware version = wl0: Sep 18 2015 03:30:01 version 7.35.177.56 (r587209) FWID 01-6cb8e269 > [ 15.546807] brcmfmac 0001:04:00.0: brcmf_c_preinit_dcmds: Firmware version = wl0: Sep 18 2015 03:30:01 version 7.35.177.56 (r587209) FWID 01-6cb8e269 > --- > .../wireless/broadcom/brcm80211/brcmfmac/common.c | 19 ++++++------- > .../wireless/broadcom/brcm80211/brcmfmac/core.c | 31 +++++++++++----------- > .../wireless/broadcom/brcm80211/brcmfmac/debug.h | 17 ++++++++---- > .../broadcom/brcm80211/brcmfmac/tracepoint.c | 4 +-- > 4 files changed, 40 insertions(+), 31 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > index 5fb4a14..7a05de78 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > @@ -108,6 +108,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) > { > s8 eventmask[BRCMF_EVENTING_MASK_LEN]; > u8 buf[BRCMF_DCMD_SMLEN]; > + struct brcmf_pub *pub = ifp->drvr; > struct brcmf_rev_info_le revinfo; > struct brcmf_rev_info *ri; > char *ptr; > @@ -117,7 +118,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) > err = brcmf_fil_iovar_data_get(ifp, "cur_etheraddr", ifp->mac_addr, > sizeof(ifp->mac_addr)); > if (err < 0) { > - brcmf_err("Retreiving cur_etheraddr failed, %d\n", err); > + brcmf_err_pub(pub, "Retreiving cur_etheraddr failed, %d\n", err); > goto done; > } > memcpy(ifp->drvr->mac, ifp->mac_addr, sizeof(ifp->drvr->mac)); > @@ -126,7 +127,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) > &revinfo, sizeof(revinfo)); > ri = &ifp->drvr->revinfo; > if (err < 0) { > - brcmf_err("retrieving revision info failed, %d\n", err); > + brcmf_err_pub(pub, "retrieving revision info failed, %d\n", err); > } else { > ri->vendorid = le32_to_cpu(revinfo.vendorid); > ri->deviceid = le32_to_cpu(revinfo.deviceid); > @@ -153,7 +154,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) > strcpy(buf, "ver"); > err = brcmf_fil_iovar_data_get(ifp, "ver", buf, sizeof(buf)); > if (err < 0) { > - brcmf_err("Retreiving version information failed, %d\n", > + brcmf_err_pub(pub, "Retreiving version information failed, %d\n", > err); > goto done; > } > @@ -161,7 +162,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) > strsep(&ptr, "\n"); > > /* Print fw version info */ > - brcmf_err("Firmware version = %s\n", buf); > + brcmf_err_pub(pub, "Firmware version = %s\n", buf); > > /* locate firmware version number for ethtool */ > ptr = strrchr(buf, ' ') + 1; > @@ -170,7 +171,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) > /* set mpc */ > err = brcmf_fil_iovar_int_set(ifp, "mpc", 1); > if (err) { > - brcmf_err("failed setting mpc\n"); > + brcmf_err_pub(pub, "failed setting mpc\n"); > goto done; > } > > @@ -180,14 +181,14 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) > err = brcmf_fil_iovar_data_get(ifp, "event_msgs", eventmask, > BRCMF_EVENTING_MASK_LEN); > if (err) { > - brcmf_err("Get event_msgs error (%d)\n", err); > + brcmf_err_pub(pub, "Get event_msgs error (%d)\n", err); > goto done; > } > setbit(eventmask, BRCMF_E_IF); > err = brcmf_fil_iovar_data_set(ifp, "event_msgs", eventmask, > BRCMF_EVENTING_MASK_LEN); > if (err) { > - brcmf_err("Set event_msgs error (%d)\n", err); > + brcmf_err_pub(pub, "Set event_msgs error (%d)\n", err); > goto done; > } > > @@ -195,7 +196,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) > err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_SCAN_CHANNEL_TIME, > BRCMF_DEFAULT_SCAN_CHANNEL_TIME); > if (err) { > - brcmf_err("BRCMF_C_SET_SCAN_CHANNEL_TIME error (%d)\n", > + brcmf_err_pub(pub, "BRCMF_C_SET_SCAN_CHANNEL_TIME error (%d)\n", > err); > goto done; > } > @@ -204,7 +205,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) > err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_SCAN_UNASSOC_TIME, > BRCMF_DEFAULT_SCAN_UNASSOC_TIME); > if (err) { > - brcmf_err("BRCMF_C_SET_SCAN_UNASSOC_TIME error (%d)\n", > + brcmf_err_pub(pub, "BRCMF_C_SET_SCAN_UNASSOC_TIME error (%d)\n", > err); > goto done; > } > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c > index b73a55b..a42007b 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c > @@ -59,7 +59,7 @@ struct brcmf_if *brcmf_get_ifp(struct brcmf_pub *drvr, int ifidx) > s32 bsscfgidx; > > if (ifidx < 0 || ifidx >= BRCMF_MAX_IFS) { > - brcmf_err("ifidx %d out of range\n", ifidx); > + brcmf_err_pub(drvr, "ifidx %d out of range\n", ifidx); > return NULL; > } > > @@ -204,7 +204,8 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb, > > /* Can the device send data? */ > if (drvr->bus_if->state != BRCMF_BUS_UP) { > - brcmf_err("xmit rejected state=%d\n", drvr->bus_if->state); > + brcmf_err_pub(drvr, "xmit rejected state=%d\n", > + drvr->bus_if->state); > netif_stop_queue(ndev); > dev_kfree_skb(skb); > ret = -ENODEV; > @@ -222,7 +223,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb, > dev_kfree_skb(skb); > skb = skb2; > if (skb == NULL) { > - brcmf_err("%s: skb_realloc_headroom failed\n", > + brcmf_err_pub(drvr, "%s: skb_realloc_headroom failed\n", > brcmf_ifname(ifp)); > ret = -ENOMEM; > goto done; > @@ -466,7 +467,7 @@ static int brcmf_netdev_open(struct net_device *ndev) > > /* If bus is not ready, can't continue */ > if (bus_if->state != BRCMF_BUS_UP) { > - brcmf_err("failed bus is not ready\n"); > + brcmf_err_pub(drvr, "failed bus is not ready\n"); > return -EAGAIN; > } > > @@ -480,7 +481,7 @@ static int brcmf_netdev_open(struct net_device *ndev) > ndev->features &= ~NETIF_F_IP_CSUM; > > if (brcmf_cfg80211_up(ndev)) { > - brcmf_err("failed to bring up cfg80211\n"); > + brcmf_err_pub(drvr, "failed to bring up cfg80211\n"); > return -EIO; > } > > @@ -525,7 +526,7 @@ int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked) > else > err = register_netdev(ndev); > if (err != 0) { > - brcmf_err("couldn't register the net device\n"); > + brcmf_err_pub(drvr, "couldn't register the net device\n"); > goto fail; > } > > @@ -643,7 +644,7 @@ struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bsscfgidx, s32 ifidx, > */ > if (ifp) { > if (ifidx) { > - brcmf_err("ERROR: netdev:%s already exists\n", > + brcmf_err_pub(drvr, "ERROR: netdev:%s already exists\n", > ifp->ndev->name); > netif_stop_queue(ifp->ndev); > brcmf_net_detach(ifp->ndev, false); > @@ -702,7 +703,7 @@ static void brcmf_del_if(struct brcmf_pub *drvr, s32 bsscfgidx, > ifp = drvr->iflist[bsscfgidx]; > drvr->iflist[bsscfgidx] = NULL; > if (!ifp) { > - brcmf_err("Null interface, bsscfgidx=%d\n", bsscfgidx); > + brcmf_err_pub(drvr, "Null interface, bsscfgidx=%d\n", bsscfgidx); > return; > } > brcmf_dbg(TRACE, "Enter, bsscfgidx=%d, ifidx=%d\n", bsscfgidx, > @@ -787,7 +788,7 @@ static int brcmf_inetaddr_changed(struct notifier_block *nb, > ret = brcmf_fil_iovar_data_get(ifp, "arp_hostip", addr_table, > sizeof(addr_table)); > if (ret) { > - brcmf_err("fail to get arp ip table err:%d\n", ret); > + brcmf_err_pub(drvr, "fail to get arp ip table err:%d\n", ret); > return NOTIFY_OK; > } > > @@ -804,7 +805,7 @@ static int brcmf_inetaddr_changed(struct notifier_block *nb, > ret = brcmf_fil_iovar_data_set(ifp, "arp_hostip", > &ifa->ifa_address, sizeof(ifa->ifa_address)); > if (ret) > - brcmf_err("add arp ip err %d\n", ret); > + brcmf_err_pub(drvr, "add arp ip err %d\n", ret); > } > break; > case NETDEV_DOWN: > @@ -816,7 +817,7 @@ static int brcmf_inetaddr_changed(struct notifier_block *nb, > ret = brcmf_fil_iovar_data_set(ifp, "arp_hostip_clear", > NULL, 0); > if (ret) { > - brcmf_err("fail to clear arp ip table err:%d\n", > + brcmf_err_pub(drvr, "fail to clear arp ip table err:%d\n", > ret); > return NOTIFY_OK; > } > @@ -827,7 +828,7 @@ static int brcmf_inetaddr_changed(struct notifier_block *nb, > &addr_table[i], > sizeof(addr_table[i])); > if (ret) > - brcmf_err("add arp ip err %d\n", > + brcmf_err_pub(drvr, "add arp ip err %d\n", > ret); > } > } > @@ -923,7 +924,7 @@ int brcmf_attach(struct device *dev, struct brcmf_mp_device *settings) > /* Attach and link in the protocol */ > ret = brcmf_proto_attach(drvr); > if (ret != 0) { > - brcmf_err("brcmf_prot_attach failed\n"); > + brcmf_err_pub(drvr, "brcmf_prot_attach failed\n"); > goto fail; > } > > @@ -1045,7 +1046,7 @@ int brcmf_bus_started(struct device *dev) > return 0; > > fail: > - brcmf_err("failed: %d\n", ret); > + brcmf_err_pub(drvr, "failed: %d\n", ret); > if (drvr->config) { > brcmf_cfg80211_detach(drvr->config); > drvr->config = NULL; > @@ -1152,7 +1153,7 @@ int brcmf_netdev_wait_pend8021x(struct brcmf_if *ifp) > MAX_WAIT_FOR_8021X_TX); > > if (!err) > - brcmf_err("Timed out waiting for no pending 802.1x packets\n"); > + brcmf_err_pub(ifp->drvr, "Timed out waiting for no pending 802.1x packets\n"); > > return !err; > } > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h > index 1fe4aa9..aab8c71 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h > @@ -50,16 +50,23 @@ > * debugging is not selected. When debugging the driver error > * messages are as important as other tracing or even more so. > */ > -#define brcmf_err(fmt, ...) \ > +#define __brcmf_err(dev, fmt, ...) \ > do { \ > if (IS_ENABLED(CONFIG_BRCMDBG) || net_ratelimit()) \ > - pr_err("%s: " fmt, __func__, ##__VA_ARGS__); \ > + dev_err(dev, "%s: " fmt, __func__, \ > + ##__VA_ARGS__); \ > } while (0) > +#define brcmf_err(fmt, ...) \ > + __brcmf_err(NULL, fmt, ##__VA_ARGS__) > +#define brcmf_err_pub(pub, fmt, ...) \ > + __brcmf_err(pub->bus_if->dev, fmt, ##__VA_ARGS__) > #else > -__printf(2, 3) > -void __brcmf_err(const char *func, const char *fmt, ...); > +__printf(3, 4) > +void __brcmf_err(struct brcmf_pub *pub, const char *func, const char *fmt, ...); > #define brcmf_err(fmt, ...) \ > - __brcmf_err(__func__, fmt, ##__VA_ARGS__) > + __brcmf_err(NULL, __func__, fmt, ##__VA_ARGS__) > +#define brcmf_err_pub(fmt, ...) \ > + __brcmf_err(pub, __func__, fmt, ##__VA_ARGS__) > #endif > > #if defined(DEBUG) || defined(CONFIG_BRCM_TRACING) > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c > index fe67559..3e8d60b 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c > @@ -21,7 +21,7 @@ > #include "tracepoint.h" > #include "debug.h" > > -void __brcmf_err(const char *func, const char *fmt, ...) > +void __brcmf_err(struct brcmf_pub *pub, const char *func, const char *fmt, ...) > { > struct va_format vaf = { > .fmt = fmt, > @@ -30,7 +30,7 @@ void __brcmf_err(const char *func, const char *fmt, ...) > > va_start(args, fmt); > vaf.va = &args; > - pr_err("%s: %pV", func, &vaf); > + dev_err(pub ? pub->bus_if->dev, NULL, "%s: %pV", func, &vaf); > trace_brcmf_err(func, &vaf); > va_end(args); > } >
On 01/20/2017 11:26 AM, Arend Van Spriel wrote: > On 18-1-2017 15:27, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> This macro accepts an extra argument of struct brcmf_pub pointer. It >> allows referencing struct device and printing error using dev_err. This >> is very useful on devices with more than one wireless card suppored by >> brcmfmac. Thanks for dev_err it's possible to identify device that error >> is related to. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> I could convert few more brcmf_err calls to this new brcmf_err_pub but I don't >> want to spent too much time on this in case someone has a better idea. >> >> If you do, comment! :) > > I do not, but still comment ;-). Like the idea. Was on our list because > it is damn convenient when testing on router with multiple devices. I > would prefer to replace the existing brcmf_err() rather than introducing > a new one and would like to do the same for brcmf_dbg(). However, not > all call sites have a struct brcmf_pub instance available. Everywhere > before completing brcmf_attach() that is. I decided to try new macro because: 1) I was too lazy to convert all calls at once 2) I could stick to brcmf_err when struct brcmf_pub isn't available yet In theory we could pass NULL to dev_err, I'd result in something like: [ 14.746754] (NULL device *): brcmf_c_preinit_dcmds: Firmware version = wl0: Sep 18 2015 03:30:01 version 7.35.177.56 (r587209) FWID 01-6cb8e269 Unfortunately AFAIK it's not possible to handle brcmf_err(NULL "Foo\n"); calls while keeping brcmf_err a macro. I was thinking about something like: #define brcmf_err(pub, fmt, ...) \ do { \ if (IS_ENABLED(CONFIG_BRCMDBG) || net_ratelimit()) \ dev_err(pub ? pub->bus_if->dev : NULL, \ "%s: " fmt, __func__, \ ##__VA_ARGS__); \ } while (0) but this wouldn't compile because for brcmf_err(NULL "Foo\n"); it would roll out to: dev_err(NULL ? NULL->bus_if->dev : NULL, "Foo\n"); I *can* support brcmf_err(NULL "Foo\n"); calls if I change brcmf_err from macro to a function (inline probably). Do you think it's a good idea?
On 20-1-2017 12:17, Rafał Miłecki wrote: > On 01/20/2017 11:26 AM, Arend Van Spriel wrote: >> On 18-1-2017 15:27, Rafał Miłecki wrote: >>> From: Rafał Miłecki <rafal@milecki.pl> >>> >>> This macro accepts an extra argument of struct brcmf_pub pointer. It >>> allows referencing struct device and printing error using dev_err. This >>> is very useful on devices with more than one wireless card suppored by >>> brcmfmac. Thanks for dev_err it's possible to identify device that error >>> is related to. >>> >>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>> --- >>> I could convert few more brcmf_err calls to this new brcmf_err_pub >>> but I don't >>> want to spent too much time on this in case someone has a better idea. >>> >>> If you do, comment! :) >> >> I do not, but still comment ;-). Like the idea. Was on our list because >> it is damn convenient when testing on router with multiple devices. I >> would prefer to replace the existing brcmf_err() rather than introducing >> a new one and would like to do the same for brcmf_dbg(). However, not >> all call sites have a struct brcmf_pub instance available. Everywhere >> before completing brcmf_attach() that is. > > I decided to try new macro because: > 1) I was too lazy to convert all calls at once > 2) I could stick to brcmf_err when struct brcmf_pub isn't available yet > > In theory we could pass NULL to dev_err, I'd result in something like: > [ 14.746754] (NULL device *): brcmf_c_preinit_dcmds: Firmware version > = wl0: Sep 18 2015 03:30:01 version 7.35.177.56 (r587209) FWID 01-6cb8e269 > > Unfortunately AFAIK it's not possible to handle > brcmf_err(NULL "Foo\n"); > calls while keeping brcmf_err a macro. > > I was thinking about something like: > #define brcmf_err(pub, fmt, ...) \ > do { \ > if (IS_ENABLED(CONFIG_BRCMDBG) || net_ratelimit()) \ > dev_err(pub ? pub->bus_if->dev : NULL, \ > "%s: " fmt, __func__, \ > ##__VA_ARGS__); \ > } while (0) > > but this wouldn't compile because for brcmf_err(NULL "Foo\n"); it would > roll > out to: > dev_err(NULL ? NULL->bus_if->dev : NULL, "Foo\n"); > > I *can* support brcmf_err(NULL "Foo\n"); calls if I change brcmf_err from > macro to a function (inline probably). Do you think it's a good idea? I was wondering if brcmf_err(ptr, "bla\n") could determine the pointer type and accept 'struct device *' and 'struct brcmf_pub *'. I think we always have a struct device pointer. Only in brcmf_module_init() we do not. Regards, Arend
On 20 January 2017 at 12:37, Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > On 20-1-2017 12:17, Rafał Miłecki wrote: >> On 01/20/2017 11:26 AM, Arend Van Spriel wrote: >>> On 18-1-2017 15:27, Rafał Miłecki wrote: >>>> From: Rafał Miłecki <rafal@milecki.pl> >>>> >>>> This macro accepts an extra argument of struct brcmf_pub pointer. It >>>> allows referencing struct device and printing error using dev_err. This >>>> is very useful on devices with more than one wireless card suppored by >>>> brcmfmac. Thanks for dev_err it's possible to identify device that error >>>> is related to. >>>> >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>>> --- >>>> I could convert few more brcmf_err calls to this new brcmf_err_pub >>>> but I don't >>>> want to spent too much time on this in case someone has a better idea. >>>> >>>> If you do, comment! :) >>> >>> I do not, but still comment ;-). Like the idea. Was on our list because >>> it is damn convenient when testing on router with multiple devices. I >>> would prefer to replace the existing brcmf_err() rather than introducing >>> a new one and would like to do the same for brcmf_dbg(). However, not >>> all call sites have a struct brcmf_pub instance available. Everywhere >>> before completing brcmf_attach() that is. >> >> I decided to try new macro because: >> 1) I was too lazy to convert all calls at once >> 2) I could stick to brcmf_err when struct brcmf_pub isn't available yet >> >> In theory we could pass NULL to dev_err, I'd result in something like: >> [ 14.746754] (NULL device *): brcmf_c_preinit_dcmds: Firmware version >> = wl0: Sep 18 2015 03:30:01 version 7.35.177.56 (r587209) FWID 01-6cb8e269 >> >> Unfortunately AFAIK it's not possible to handle >> brcmf_err(NULL "Foo\n"); >> calls while keeping brcmf_err a macro. >> >> I was thinking about something like: >> #define brcmf_err(pub, fmt, ...) \ >> do { \ >> if (IS_ENABLED(CONFIG_BRCMDBG) || net_ratelimit()) \ >> dev_err(pub ? pub->bus_if->dev : NULL, \ >> "%s: " fmt, __func__, \ >> ##__VA_ARGS__); \ >> } while (0) >> >> but this wouldn't compile because for brcmf_err(NULL "Foo\n"); it would >> roll >> out to: >> dev_err(NULL ? NULL->bus_if->dev : NULL, "Foo\n"); >> >> I *can* support brcmf_err(NULL "Foo\n"); calls if I change brcmf_err from >> macro to a function (inline probably). Do you think it's a good idea? > > I was wondering if brcmf_err(ptr, "bla\n") could determine the pointer > type and accept 'struct device *' and 'struct brcmf_pub *'. I think we > always have a struct device pointer. Only in brcmf_module_init() we do not. Is this possible with a macro? Could you point me to some example for something like this? Also is this acceptable for upstream code?
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c index 5fb4a14..7a05de78 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c @@ -108,6 +108,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) { s8 eventmask[BRCMF_EVENTING_MASK_LEN]; u8 buf[BRCMF_DCMD_SMLEN]; + struct brcmf_pub *pub = ifp->drvr; struct brcmf_rev_info_le revinfo; struct brcmf_rev_info *ri; char *ptr; @@ -117,7 +118,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) err = brcmf_fil_iovar_data_get(ifp, "cur_etheraddr", ifp->mac_addr, sizeof(ifp->mac_addr)); if (err < 0) { - brcmf_err("Retreiving cur_etheraddr failed, %d\n", err); + brcmf_err_pub(pub, "Retreiving cur_etheraddr failed, %d\n", err); goto done; } memcpy(ifp->drvr->mac, ifp->mac_addr, sizeof(ifp->drvr->mac)); @@ -126,7 +127,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) &revinfo, sizeof(revinfo)); ri = &ifp->drvr->revinfo; if (err < 0) { - brcmf_err("retrieving revision info failed, %d\n", err); + brcmf_err_pub(pub, "retrieving revision info failed, %d\n", err); } else { ri->vendorid = le32_to_cpu(revinfo.vendorid); ri->deviceid = le32_to_cpu(revinfo.deviceid); @@ -153,7 +154,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) strcpy(buf, "ver"); err = brcmf_fil_iovar_data_get(ifp, "ver", buf, sizeof(buf)); if (err < 0) { - brcmf_err("Retreiving version information failed, %d\n", + brcmf_err_pub(pub, "Retreiving version information failed, %d\n", err); goto done; } @@ -161,7 +162,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) strsep(&ptr, "\n"); /* Print fw version info */ - brcmf_err("Firmware version = %s\n", buf); + brcmf_err_pub(pub, "Firmware version = %s\n", buf); /* locate firmware version number for ethtool */ ptr = strrchr(buf, ' ') + 1; @@ -170,7 +171,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) /* set mpc */ err = brcmf_fil_iovar_int_set(ifp, "mpc", 1); if (err) { - brcmf_err("failed setting mpc\n"); + brcmf_err_pub(pub, "failed setting mpc\n"); goto done; } @@ -180,14 +181,14 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) err = brcmf_fil_iovar_data_get(ifp, "event_msgs", eventmask, BRCMF_EVENTING_MASK_LEN); if (err) { - brcmf_err("Get event_msgs error (%d)\n", err); + brcmf_err_pub(pub, "Get event_msgs error (%d)\n", err); goto done; } setbit(eventmask, BRCMF_E_IF); err = brcmf_fil_iovar_data_set(ifp, "event_msgs", eventmask, BRCMF_EVENTING_MASK_LEN); if (err) { - brcmf_err("Set event_msgs error (%d)\n", err); + brcmf_err_pub(pub, "Set event_msgs error (%d)\n", err); goto done; } @@ -195,7 +196,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_SCAN_CHANNEL_TIME, BRCMF_DEFAULT_SCAN_CHANNEL_TIME); if (err) { - brcmf_err("BRCMF_C_SET_SCAN_CHANNEL_TIME error (%d)\n", + brcmf_err_pub(pub, "BRCMF_C_SET_SCAN_CHANNEL_TIME error (%d)\n", err); goto done; } @@ -204,7 +205,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_SCAN_UNASSOC_TIME, BRCMF_DEFAULT_SCAN_UNASSOC_TIME); if (err) { - brcmf_err("BRCMF_C_SET_SCAN_UNASSOC_TIME error (%d)\n", + brcmf_err_pub(pub, "BRCMF_C_SET_SCAN_UNASSOC_TIME error (%d)\n", err); goto done; } diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c index b73a55b..a42007b 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c @@ -59,7 +59,7 @@ struct brcmf_if *brcmf_get_ifp(struct brcmf_pub *drvr, int ifidx) s32 bsscfgidx; if (ifidx < 0 || ifidx >= BRCMF_MAX_IFS) { - brcmf_err("ifidx %d out of range\n", ifidx); + brcmf_err_pub(drvr, "ifidx %d out of range\n", ifidx); return NULL; } @@ -204,7 +204,8 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb, /* Can the device send data? */ if (drvr->bus_if->state != BRCMF_BUS_UP) { - brcmf_err("xmit rejected state=%d\n", drvr->bus_if->state); + brcmf_err_pub(drvr, "xmit rejected state=%d\n", + drvr->bus_if->state); netif_stop_queue(ndev); dev_kfree_skb(skb); ret = -ENODEV; @@ -222,7 +223,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb, dev_kfree_skb(skb); skb = skb2; if (skb == NULL) { - brcmf_err("%s: skb_realloc_headroom failed\n", + brcmf_err_pub(drvr, "%s: skb_realloc_headroom failed\n", brcmf_ifname(ifp)); ret = -ENOMEM; goto done; @@ -466,7 +467,7 @@ static int brcmf_netdev_open(struct net_device *ndev) /* If bus is not ready, can't continue */ if (bus_if->state != BRCMF_BUS_UP) { - brcmf_err("failed bus is not ready\n"); + brcmf_err_pub(drvr, "failed bus is not ready\n"); return -EAGAIN; } @@ -480,7 +481,7 @@ static int brcmf_netdev_open(struct net_device *ndev) ndev->features &= ~NETIF_F_IP_CSUM; if (brcmf_cfg80211_up(ndev)) { - brcmf_err("failed to bring up cfg80211\n"); + brcmf_err_pub(drvr, "failed to bring up cfg80211\n"); return -EIO; } @@ -525,7 +526,7 @@ int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked) else err = register_netdev(ndev); if (err != 0) { - brcmf_err("couldn't register the net device\n"); + brcmf_err_pub(drvr, "couldn't register the net device\n"); goto fail; } @@ -643,7 +644,7 @@ struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bsscfgidx, s32 ifidx, */ if (ifp) { if (ifidx) { - brcmf_err("ERROR: netdev:%s already exists\n", + brcmf_err_pub(drvr, "ERROR: netdev:%s already exists\n", ifp->ndev->name); netif_stop_queue(ifp->ndev); brcmf_net_detach(ifp->ndev, false); @@ -702,7 +703,7 @@ static void brcmf_del_if(struct brcmf_pub *drvr, s32 bsscfgidx, ifp = drvr->iflist[bsscfgidx]; drvr->iflist[bsscfgidx] = NULL; if (!ifp) { - brcmf_err("Null interface, bsscfgidx=%d\n", bsscfgidx); + brcmf_err_pub(drvr, "Null interface, bsscfgidx=%d\n", bsscfgidx); return; } brcmf_dbg(TRACE, "Enter, bsscfgidx=%d, ifidx=%d\n", bsscfgidx, @@ -787,7 +788,7 @@ static int brcmf_inetaddr_changed(struct notifier_block *nb, ret = brcmf_fil_iovar_data_get(ifp, "arp_hostip", addr_table, sizeof(addr_table)); if (ret) { - brcmf_err("fail to get arp ip table err:%d\n", ret); + brcmf_err_pub(drvr, "fail to get arp ip table err:%d\n", ret); return NOTIFY_OK; } @@ -804,7 +805,7 @@ static int brcmf_inetaddr_changed(struct notifier_block *nb, ret = brcmf_fil_iovar_data_set(ifp, "arp_hostip", &ifa->ifa_address, sizeof(ifa->ifa_address)); if (ret) - brcmf_err("add arp ip err %d\n", ret); + brcmf_err_pub(drvr, "add arp ip err %d\n", ret); } break; case NETDEV_DOWN: @@ -816,7 +817,7 @@ static int brcmf_inetaddr_changed(struct notifier_block *nb, ret = brcmf_fil_iovar_data_set(ifp, "arp_hostip_clear", NULL, 0); if (ret) { - brcmf_err("fail to clear arp ip table err:%d\n", + brcmf_err_pub(drvr, "fail to clear arp ip table err:%d\n", ret); return NOTIFY_OK; } @@ -827,7 +828,7 @@ static int brcmf_inetaddr_changed(struct notifier_block *nb, &addr_table[i], sizeof(addr_table[i])); if (ret) - brcmf_err("add arp ip err %d\n", + brcmf_err_pub(drvr, "add arp ip err %d\n", ret); } } @@ -923,7 +924,7 @@ int brcmf_attach(struct device *dev, struct brcmf_mp_device *settings) /* Attach and link in the protocol */ ret = brcmf_proto_attach(drvr); if (ret != 0) { - brcmf_err("brcmf_prot_attach failed\n"); + brcmf_err_pub(drvr, "brcmf_prot_attach failed\n"); goto fail; } @@ -1045,7 +1046,7 @@ int brcmf_bus_started(struct device *dev) return 0; fail: - brcmf_err("failed: %d\n", ret); + brcmf_err_pub(drvr, "failed: %d\n", ret); if (drvr->config) { brcmf_cfg80211_detach(drvr->config); drvr->config = NULL; @@ -1152,7 +1153,7 @@ int brcmf_netdev_wait_pend8021x(struct brcmf_if *ifp) MAX_WAIT_FOR_8021X_TX); if (!err) - brcmf_err("Timed out waiting for no pending 802.1x packets\n"); + brcmf_err_pub(ifp->drvr, "Timed out waiting for no pending 802.1x packets\n"); return !err; } diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h index 1fe4aa9..aab8c71 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h @@ -50,16 +50,23 @@ * debugging is not selected. When debugging the driver error * messages are as important as other tracing or even more so. */ -#define brcmf_err(fmt, ...) \ +#define __brcmf_err(dev, fmt, ...) \ do { \ if (IS_ENABLED(CONFIG_BRCMDBG) || net_ratelimit()) \ - pr_err("%s: " fmt, __func__, ##__VA_ARGS__); \ + dev_err(dev, "%s: " fmt, __func__, \ + ##__VA_ARGS__); \ } while (0) +#define brcmf_err(fmt, ...) \ + __brcmf_err(NULL, fmt, ##__VA_ARGS__) +#define brcmf_err_pub(pub, fmt, ...) \ + __brcmf_err(pub->bus_if->dev, fmt, ##__VA_ARGS__) #else -__printf(2, 3) -void __brcmf_err(const char *func, const char *fmt, ...); +__printf(3, 4) +void __brcmf_err(struct brcmf_pub *pub, const char *func, const char *fmt, ...); #define brcmf_err(fmt, ...) \ - __brcmf_err(__func__, fmt, ##__VA_ARGS__) + __brcmf_err(NULL, __func__, fmt, ##__VA_ARGS__) +#define brcmf_err_pub(fmt, ...) \ + __brcmf_err(pub, __func__, fmt, ##__VA_ARGS__) #endif #if defined(DEBUG) || defined(CONFIG_BRCM_TRACING) diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c index fe67559..3e8d60b 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c @@ -21,7 +21,7 @@ #include "tracepoint.h" #include "debug.h" -void __brcmf_err(const char *func, const char *fmt, ...) +void __brcmf_err(struct brcmf_pub *pub, const char *func, const char *fmt, ...) { struct va_format vaf = { .fmt = fmt, @@ -30,7 +30,7 @@ void __brcmf_err(const char *func, const char *fmt, ...) va_start(args, fmt); vaf.va = &args; - pr_err("%s: %pV", func, &vaf); + dev_err(pub ? pub->bus_if->dev, NULL, "%s: %pV", func, &vaf); trace_brcmf_err(func, &vaf); va_end(args); }