On November 4, 2024 9:06:04 PM Stefan Wahren <wahrenst@xxxxxxx> 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
