On Thu, Mar 24, 2011 at 03:44:30PM +0100, Roland Vossen wrote:
> Code cleanup. Asserts have been replaced by less lethal WARN_ON's.
> These WARN_ONs are a sanity check for the messages that Mac80211
> gives to the driver, and vice versa.
>
The brcm80211 ASSERT() macro is compiled out unless you have
BCMDBG defined. There is no way to enable that through
"make menuconfig". So up to now basically everyone except the broadcom
devs have run with all the ASSERT()s compiled out. We could just delete
them all and no one would notice. I've obviously said before that I'd
prefer to delete them where possible.
> Signed-off-by: Roland Vossen <rvossen@xxxxxxxxxxxx>
> Reviewed-by: Arend van Spriel <arend@xxxxxxxxxxxx>
> ---
> drivers/staging/brcm80211/brcmsmac/wl_mac80211.c | 55 ++++++-----
> drivers/staging/brcm80211/brcmsmac/wlc_main.c | 104 ++++++++++++---------
> 2 files changed, 89 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/staging/brcm80211/brcmsmac/wl_mac80211.c b/drivers/staging/brcm80211/brcmsmac/wl_mac80211.c
> index 774b4e9..1ae8e22 100644
> --- a/drivers/staging/brcm80211/brcmsmac/wl_mac80211.c
> +++ b/drivers/staging/brcm80211/brcmsmac/wl_mac80211.c
> @@ -184,11 +184,9 @@ static int wl_ops_start(struct ieee80211_hw *hw)
>
> static void wl_ops_stop(struct ieee80211_hw *hw)
> {
> -#ifdef BRCMDBG
> struct wl_info *wl = hw->priv;
Blank line after declarations.
> - ASSERT(wl);
> -#endif /*BRCMDBG*/
> - ieee80211_stop_queues(hw);
> + if (likely(!WARN_ON(wl == NULL)))
WARN_ON() includes it's own likely/unlikely so the hint isn't needed
here.
Really this one is utterly pointless like I mentioned earlier. We used
to use "wl" but now that we don't use the "wl" variable any more in this
function and we don't care if it's NULL or not.
This function should just be:
static void wl_ops_stop(struct ieee80211_hw *hw)
{
ieee80211_stop_queues(hw);
}
Or better yet, we can just delete the whole function.
> + ieee80211_stop_queues(hw);
> }
>
> static int
> @@ -276,7 +274,7 @@ static int wl_ops_config(struct ieee80211_hw *hw, u32 changed)
> goto config_out;
> }
> wlc_iovar_getint(wl->wlc, "bcn_li_bcn", &new_int);
> - ASSERT(new_int == conf->listen_interval);
> + WARN_ON(new_int != conf->listen_interval);
> }
> if (changed & IEEE80211_CONF_CHANGE_MONITOR)
> WL_ERROR("%s: change monitor mode: %s (implement)\n", __func__,
> @@ -614,13 +612,12 @@ wl_ops_ampdu_action(struct ieee80211_hw *hw,
> struct ieee80211_sta *sta, u16 tid, u16 *ssn,
> u8 buf_size)
> {
> -#if defined(BCMDBG)
> struct scb *scb = (struct scb *)sta->drv_priv;
> -#endif
> struct wl_info *wl = hw->priv;
> int status;
>
> - ASSERT(scb->magic == SCB_MAGIC);
> + if (unlikely(WARN_ON(scb->magic != SCB_MAGIC)))
> + return -EIDRM;
> switch (action) {
> case IEEE80211_AMPDU_RX_START:
> WL_NONE("%s: action = IEEE80211_AMPDU_RX_START\n", __func__);
> @@ -724,7 +721,7 @@ static int wl_set_hint(struct wl_info *wl, char *abbrev)
> static struct wl_info *wl_attach(u16 vendor, u16 device, unsigned long regs,
> uint bustype, void *btparam, uint irq)
> {
> - struct wl_info *wl;
> + struct wl_info *wl = NULL;
> int unit, err;
>
> unsigned long base_addr;
> @@ -741,8 +738,10 @@ static struct wl_info *wl_attach(u16 vendor, u16 device, unsigned long regs,
>
> /* allocate private info */
> hw = pci_get_drvdata(btparam); /* btparam == pdev */
> - wl = hw->priv;
> - ASSERT(wl);
> + if (likely(hw != NULL))
> + wl = hw->priv;
This isn't a fast path. The likely() here is not needed. It makes the
code less readable for no reason.
> + if (unlikely(WARN_ON(hw == NULL || wl == NULL)))
> + goto fail1;
>
> atomic_set(&wl->callbacks, 0);
>
> @@ -792,8 +791,10 @@ static struct wl_info *wl_attach(u16 vendor, u16 device, unsigned long regs,
> wl->pub = wlc_pub(wl->wlc);
>
> wl->pub->ieee_hw = hw;
> - ASSERT(wl->pub->ieee_hw);
> - ASSERT(wl->pub->ieee_hw->priv == wl);
> +
> + if (unlikely(wl->pub->ieee_hw->priv != wl)) {
> + goto fail;
> + }
The curly braces are not needed. Checkpatch should warn about this, but
it missed it. It does catch it if you run checkpatch -f on the patched
file... I'm not sure what's the story with that.
Also why was the ASSERT even here in the first place? We already know
this is is true because we did the assignments ourselves. What are we
trying to check here? Let's just delete this.
>
>
> if (wlc_iovar_setint(wl->wlc, "mpc", 0)) {
> @@ -817,7 +818,7 @@ static struct wl_info *wl_attach(u16 vendor, u16 device, unsigned long regs,
> }
>
> memcpy(perm, &wl->pub->cur_etheraddr, ETH_ALEN);
> - ASSERT(is_valid_ether_addr(perm));
> + WARN_ON(!is_valid_ether_addr(perm));
> SET_IEEE80211_PERM_ADDR(hw, perm);
>
> err = ieee80211_register_hw(hw);
> @@ -1031,7 +1032,7 @@ static int ieee_hw_rate_init(struct ieee80211_hw *hw)
> }
> WL_NONE("%s: phylist = %c\n", __func__, phy_list[0]);
>
> - if (phy_list[0] == 'n' || phy_list[0] == 'c') {
> + if (likely(!WARN_ON(phy_list[0] != 'n' && phy_list[0] != 'c'))) {
This is sort of ugly even after we remove the duplicate likely(). Why
don't you just replace the BUG_ON() with a WARN_ON(1);
> if (phy_list[0] == 'c') {
> /* Single stream */
> wl_band_2GHz_nphy.ht_cap.mcs.rx_mask[1] = 0;
> @@ -1039,7 +1040,6 @@ static int ieee_hw_rate_init(struct ieee80211_hw *hw)
> }
> hw->wiphy->bands[IEEE80211_BAND_2GHZ] = &wl_band_2GHz_nphy;
> } else {
> - BUG();
> return -1;
> }
>
> @@ -1099,13 +1099,13 @@ static int ieee_hw_init(struct ieee80211_hw *hw)
> static int __devinit
> wl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> - int rc;
> + int rc = -ENODEV;
> struct wl_info *wl;
> struct ieee80211_hw *hw;
> u32 val;
>
> - ASSERT(pdev);
> -
> + if (unlikely(pdev == NULL))
> + goto err_1;
> WL_TRACE("%s: bus %d slot %d func %d irq %d\n",
> __func__, pdev->bus->number, PCI_SLOT(pdev->devfn),
> PCI_FUNC(pdev->devfn), pdev->irq);
> @@ -1121,7 +1121,8 @@ wl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> WL_ERROR("%s: Cannot enable device %d-%d_%d\n",
> __func__, pdev->bus->number, PCI_SLOT(pdev->devfn),
> PCI_FUNC(pdev->devfn));
> - return -ENODEV;
> + rc = -ENODEV;
Just keep the error code from pci_enable_device() instead of overwriting
it with -ENODEV.
> + goto err_1;
> }
> pci_set_master(pdev);
>
> @@ -1152,8 +1153,8 @@ wl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> }
> return 0;
> err_1:
> - WL_ERROR("%s: err_1: Major hoarkage\n", __func__);
> - return 0;
> + WL_ERROR("err_1: Major hoarkage\n");
> + return rc;
> }
>
> static int wl_suspend(struct pci_dev *pdev, pm_message_t state)
> @@ -1342,7 +1343,8 @@ static void wl_free(struct wl_info *wl)
> {
> struct wl_timer *t, *next;
>
> - ASSERT(wl);
> + if (unlikely(WARN_ON(wl == NULL)))
> + return;
This is never called with a NULL pointer so we could just delete it. On
the other hand, free() functions often accept NULL pointers so it might
be better to use:
if (!wl)
return;
But probably it's better to just delete the check. If people start
randomly introducing calls to wl_free() and we start merging them
without any review then we're already screwed and this WARN_ON()
won't help us.
> /* free ucode data */
> if (wl->fw.fw_cnt)
> wl_ucode_data_free();
> @@ -1521,7 +1523,7 @@ static irqreturn_t BCMFASTPATH wl_isr(int irq, void *dev_id)
>
> /* ...and call the second level interrupt handler */
> /* schedule dpc */
> - ASSERT(wl->resched == false);
> + WARN_ON_ONCE(wl->resched == true);
I assume this one triggers quite a bit on your devel systems?
> tasklet_schedule(&wl->tasklet);
> }
> }
> @@ -1810,7 +1812,10 @@ int wl_ucode_init_uint(struct wl_info *wl, u32 *data, u32 idx)
> entry++, hdr++) {
> if (hdr->idx == idx) {
> pdata = wl->fw.fw_bin[i]->data + hdr->offset;
> - ASSERT(hdr->len == 4);
> + if (hdr->len != 4) {
> + WL_ERROR("ERROR: fw hdr len\n");
> + return -1;
-1 is -EPERM. -EINVAL would be more appropriate here.
> + }
> *data = *((u32 *) pdata);
> return 0;
> }
> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_main.c b/drivers/staging/brcm80211/brcmsmac/wlc_main.c
> index 98a5466..e3aa15b 100644
> --- a/drivers/staging/brcm80211/brcmsmac/wlc_main.c
> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_main.c
> @@ -1179,7 +1179,7 @@ void wlc_protection_upd(struct wlc_info *wlc, uint idx, int val)
> break;
>
> default:
> - ASSERT(0);
> + WARN_ON(true);
> break;
> }
>
> @@ -1367,7 +1367,8 @@ void wlc_wme_setparams(struct wlc_info *wlc, u16 aci, void *arg, bool suspend)
> u16 *shm_entry;
> struct ieee80211_tx_queue_params *params = arg;
>
> - ASSERT(wlc);
> + if (unlikely(WARN_ON(wlc == NULL || aci >= AC_COUNT)))
> + return;
wlc isn't going to be NULL here.
[snip]
> @@ -5173,27 +5185,30 @@ wlc_sendpkt_mac80211(struct wlc_info *wlc, struct sk_buff *sdu,
> struct scb *scb = &global_scb;
> struct ieee80211_hdr *d11_header = (struct ieee80211_hdr *)(sdu->data);
>
> - ASSERT(sdu);
> -
> + if (unlikely(WARN_ON(sdu == NULL)))
> + goto wlc_sendpkt_fail;
Just return directly.
[snipped... This stuff will all need to be redone to remove all the likely and
unlikely hints]
> @@ -6959,10 +6975,8 @@ wlc_recvctl(struct wlc_info *wlc, d11rxhdr_t *rxh, struct sk_buff *p)
> skb_pull(p, D11_PHY_HDR_LEN);
> __skb_trim(p, len_mpdu);
>
> - ASSERT(!(p->next));
> - ASSERT(!(p->prev));
> -
> - ASSERT(IS_ALIGNED((unsigned long)skb->data, 2));
This breaks the compile.
CC [M] drivers/staging/brcm80211/brcmsmac/wlc_main.o
drivers/staging/brcm80211/brcmsmac/wlc_main.c: In function âprep_mac80211_statusâ:
drivers/staging/brcm80211/brcmsmac/wlc_main.c:6846: warning: unused variable âtsf_hâ
drivers/staging/brcm80211/brcmsmac/wlc_main.c:6846: warning: unused variable âtsf_lâ
drivers/staging/brcm80211/brcmsmac/wlc_main.c: In function âwlc_recvctlâ:
drivers/staging/brcm80211/brcmsmac/wlc_main.c:6979: error: âskbâ undeclared (first use in this function)
drivers/staging/brcm80211/brcmsmac/wlc_main.c:6979: error: (Each undeclared identifier is reported only once
drivers/staging/brcm80211/brcmsmac/wlc_main.c:6979: error: for each function it appears in.)
drivers/staging/brcm80211/brcmsmac/wlc_main.c:6979: warning: type defaults to âintâ in declaration of âtype nameâ
make[1]: *** [drivers/staging/brcm80211/brcmsmac/wlc_main.o] Error 1
make: *** [drivers/staging/brcm80211/brcmsmac/wlc_main.o] Error 2
> + WARN_ON(p->next != NULL || p->prev != NULL);
If you break it up, it's more readable and if you ever trigger the
warning then you'll know which one is non-NULL.
WARN_ON(p->next);
WARN_ON(p->prev);
> + WARN_ON(!IS_ALIGNED((unsigned long)skb->data, 2));
>
> memcpy(IEEE80211_SKB_RXCB(p), &rx_status, sizeof(rx_status));
> ieee80211_rx_irqsafe(wlc->pub->ieee_hw, p);
There was a lot of code to review so I didn't review this entire patch.
Obviously it will have to be redone to fix the compile and remove the
likely/unlikely hints. I'll take another look at it when version 2 is
ready.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
