diff mbox

[RFC,2/2] brcmfmac: add brcmf_err_pub macro for better error messages

Message ID 20170118142703.9824-2-zajec5@gmail.com
State RFC
Headers show

Commit Message

Rafał Miłecki Jan. 18, 2017, 2:27 p.m. UTC
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! :)

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(-)

Comments

Arend Van Spriel Jan. 20, 2017, 10:26 a.m. UTC | #1
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);
>  }
>
Rafał Miłecki Jan. 20, 2017, 11:17 a.m. UTC | #2
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?
Arend Van Spriel Jan. 20, 2017, 11:37 a.m. UTC | #3
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
Rafał Miłecki Jan. 20, 2017, 12:52 p.m. UTC | #4
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 mbox

Patch

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);
 }