Re: brcmfmac: error messages while entering suspend

[Arend Van Spriel <arend.vanspriel@broadcom.com>]

On November 4, 2024 9:06:04 PM Stefan Wahren <wahrenst@gmx.net> wrote:

> Hi,
>
> Am 04.11.24 um 13:18 schrieb Johannes Berg:
>> On Mon, 2024-11-04 at 12:59 +0100, Stefan Wahren wrote:
>>>>> [  384.292071] ieee80211 phy0: brcmf_fil_cmd_data: bus is down. we have
>>>>> nothing to do.
>>>>> [  384.292079] ieee80211 phy0: brcmf_cfg80211_get_tx_power: error (-5)
>>>>>
>>>>> These errors are not new and I assume they have always been there. I'm
>>>>> not an expert here, so I want to know is the problem here that the SDIO
>>>>> interface is shutdown before brcmfmac is suspended or lies the issue
>>>>> within brcmfmac suspend itself?
>>>> Upon suspend we execute the remove path and cleaning the interfaces.
>>>> We notify cfg80211 about the removal, which in turn will notify
>>>> userspace, but is tries to obtain the tx power from brcmfmac.
>> I guess "it tries to obtain" is some sort of event path that wants to
>> include the TX power in an event. That doesn't seem to make all that
>> much sense on removal events though, so perhaps we could remove the
>> get_channel and get_tx_power calls for NL80211_CMD_DEL_INTERFACE.
> Not sure if I get it right, but the follow patch make the errors go away:
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 1ac8a196f376..52120cce2f7e 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -4006,23 +4006,25 @@ static int nl80211_send_iface(struct sk_buff
> *msg, u32 portid, u32 seq, int flag
>          nla_put_u32(msg, NL80211_ATTR_VIF_RADIO_MASK, wdev->radio_mask))
>          goto nla_put_failure;
>
> -    if (rdev->ops->get_channel && !wdev->valid_links) {
> -        struct cfg80211_chan_def chandef = {};
> -        int ret;
> +    if (cmd != NL80211_CMD_DEL_INTERFACE) {
> +        if (rdev->ops->get_channel && !wdev->valid_links) {
> +            struct cfg80211_chan_def chandef = {};
> +            int ret;
>
> -        ret = rdev_get_channel(rdev, wdev, 0, &chandef);
> -        if (ret == 0 && nl80211_send_chandef(msg, &chandef))
> -            goto nla_put_failure;
> -    }
> +            ret = rdev_get_channel(rdev, wdev, 0, &chandef);
> +            if (ret == 0 && nl80211_send_chandef(msg, &chandef))
> +                goto nla_put_failure;
> +        }
>
> -    if (rdev->ops->get_tx_power) {
> -        int dbm, ret;
> +        if (rdev->ops->get_tx_power) {
> +            int dbm, ret;
>
> -        ret = rdev_get_tx_power(rdev, wdev, &dbm);
> -        if (ret == 0 &&
> -            nla_put_u32(msg, NL80211_ATTR_WIPHY_TX_POWER_LEVEL,
> -                DBM_TO_MBM(dbm)))
> -            goto nla_put_failure;
> +            ret = rdev_get_tx_power(rdev, wdev, &dbm);
> +            if (ret == 0 &&
> +                nla_put_u32(msg, NL80211_ATTR_WIPHY_TX_POWER_LEVEL,
> +                    DBM_TO_MBM(dbm)))
> +                goto nla_put_failure;
> +        }
>      }
>
>      switch (wdev->iftype) {
>
> But this change doesn't consider get_txq_stats and the further calls
> rdev_get_channel for the valid_links.
>
> Do we actually need nl80211_send_iface() for NL80211_CMD_DEL_INTERFACE?

In term of user-space API the answer would (probably) be yes. At least the 
primitive should be send. What attributes should be included is debatable. 
The primitive is probably used by user-space to cleanup or disable 
functionality using the given interface. So only attributes identifying the 
interface are likely sufficient (wdev-id, if-index).

Regards,
Arend