Re: brcmfmac: error messages while entering suspend

[Stefan Wahren <wahrenst@gmx.net>]

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?

Regards