| Message ID | cover.1660606893.git.objelf@gmail.com |
|---|---|
| Headers | show |
| Series | wifi: mt76: mt7921: introduce chanctx support | expand |
On Tue, 2022-08-16 at 08:03 +0800, sean.wang@mediatek.com wrote: > From: Sean Wang <sean.wang@mediatek.com> > > MT7921 device can be supported with the channel context depending on > the newer firmware so that we need a way to enable the chanctx related > methods until hw is being registered. > > Signed-off-by: Sean Wang <sean.wang@mediatek.com> > --- > net/mac80211/main.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/net/mac80211/main.c b/net/mac80211/main.c > index 5b1c47ed0cc0..98d05ed1a081 100644 > --- a/net/mac80211/main.c > +++ b/net/mac80211/main.c > @@ -1011,6 +1011,14 @@ int ieee80211_register_hw(struct ieee80211_hw *hw) > return -EINVAL; > #endif > > + /* check all or no channel context operations exist */ > + i = !!local->ops->add_chanctx + !!local->ops->remove_chanctx + > + !!local->ops->change_chanctx + !!local->ops->assign_vif_chanctx + > + !!local->ops->unassign_vif_chanctx; > + if (WARN_ON(i != 0 && i != 5)) > + return -EINVAL; > + local->use_chanctx = i == 5; > + Not sure I understand this - this just *adds* code, based on the description I would've expected you to *move* code? In any case, I'm not sure I see how this makes sense - ops is supposed to be const, and you're supposed to pass it to alloc_hw already, so how would it change?! Also, conceptually, I'm not sure why it's needed to alloc_hw before loading firmware, we also have a lot of things depend on the firmware capabilities in iwlwifi/mvm, and so we alloc/register HW after loading firmware. johannes
On Aug 16, Sean Wang wrote: > From: Sean Wang <sean.wang@mediatek.com> > > add mt76_connac_mcu_uni_set_chctx to set up the channel context per BSS > in the firmware > > Signed-off-by: Sean Wang <sean.wang@mediatek.com> > --- > .../wireless/mediatek/mt76/mt76_connac_mcu.c | 83 +++++++++++++++++++ > .../wireless/mediatek/mt76/mt76_connac_mcu.h | 3 + > 2 files changed, 86 insertions(+) > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c > index 0afcadce87fc..3d5c70765d4f 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c > +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c > @@ -1311,6 +1311,89 @@ mt76_connac_mcu_uni_bss_he_tlv(struct mt76_phy *phy, struct ieee80211_vif *vif, > he->max_nss_mcs[CMD_HE_MCS_BW8080] = cap->he_mcs_nss_supp.tx_mcs_80p80; > } > > +int mt76_connac_mcu_uni_set_chctx(struct mt76_phy *phy, struct mt76_vif *mvif, > + struct ieee80211_chanctx_conf *ctx) > +{ > + struct cfg80211_chan_def *chandef = ctx ? &ctx->def : &phy->chandef; > + int freq1 = chandef->center_freq1, freq2 = chandef->center_freq2; > + enum nl80211_band band = chandef->chan->band; > + struct mt76_dev *mdev = phy->dev; > + nit: remove new-line here. > + struct { > + struct { > + u8 bss_idx; > + u8 pad[3]; > + } __packed hdr; > + struct rlm_tlv { > + __le16 tag; > + __le16 len; > + u8 control_channel; > + u8 center_chan; > + u8 center_chan2; > + u8 bw; > + u8 tx_streams; > + u8 rx_streams; > + u8 short_st; > + u8 ht_op_info; > + u8 sco; > + u8 band; > + u8 pad[2]; > + } __packed rlm; > + } __packed rlm_req = { > + .hdr = { > + .bss_idx = mvif->idx, > + }, > + .rlm = { > + .tag = cpu_to_le16(UNI_BSS_INFO_RLM), > + .len = cpu_to_le16(sizeof(struct rlm_tlv)), > + .control_channel = chandef->chan->hw_value, > + .center_chan = ieee80211_frequency_to_channel(freq1), > + .center_chan2 = ieee80211_frequency_to_channel(freq2), > + .tx_streams = hweight8(phy->antenna_mask), > + .ht_op_info = 4, /* set HT 40M allowed */ > + .rx_streams = phy->chainmask, > + .short_st = true, > + .band = band, > + }, > + }; > + > + switch (chandef->width) { > + case NL80211_CHAN_WIDTH_40: > + rlm_req.rlm.bw = CMD_CBW_40MHZ; > + break; > + case NL80211_CHAN_WIDTH_80: > + rlm_req.rlm.bw = CMD_CBW_80MHZ; > + break; > + case NL80211_CHAN_WIDTH_80P80: > + rlm_req.rlm.bw = CMD_CBW_8080MHZ; > + break; > + case NL80211_CHAN_WIDTH_160: > + rlm_req.rlm.bw = CMD_CBW_160MHZ; > + break; > + case NL80211_CHAN_WIDTH_5: > + rlm_req.rlm.bw = CMD_CBW_5MHZ; > + break; > + case NL80211_CHAN_WIDTH_10: > + rlm_req.rlm.bw = CMD_CBW_10MHZ; > + break; > + case NL80211_CHAN_WIDTH_20_NOHT: > + case NL80211_CHAN_WIDTH_20: > + default: > + rlm_req.rlm.bw = CMD_CBW_20MHZ; > + rlm_req.rlm.ht_op_info = 0; > + break; > + } > + > + if (rlm_req.rlm.control_channel < rlm_req.rlm.center_chan) > + rlm_req.rlm.sco = 1; /* SCA */ > + else if (rlm_req.rlm.control_channel > rlm_req.rlm.center_chan) > + rlm_req.rlm.sco = 3; /* SCB */ > + > + return mt76_mcu_send_msg(mdev, MCU_UNI_CMD(BSS_INFO_UPDATE), &rlm_req, > + sizeof(rlm_req), true); > +} > +EXPORT_SYMBOL_GPL(mt76_connac_mcu_uni_set_chctx); > + > int mt76_connac_mcu_uni_add_bss(struct mt76_phy *phy, > struct ieee80211_vif *vif, > struct mt76_wcid *wcid, > diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h > index f1d7c05bd794..bf60b00d6020 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h > +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h > @@ -1727,6 +1727,9 @@ int mt76_connac_mcu_uni_add_dev(struct mt76_phy *phy, > int mt76_connac_mcu_sta_ba(struct mt76_dev *dev, struct mt76_vif *mvif, > struct ieee80211_ampdu_params *params, > int cmd, bool enable, bool tx); > +int mt76_connac_mcu_uni_set_chctx(struct mt76_phy *phy, > + struct mt76_vif *vif, > + struct ieee80211_chanctx_conf *ctx); > int mt76_connac_mcu_uni_add_bss(struct mt76_phy *phy, > struct ieee80211_vif *vif, > struct mt76_wcid *wcid, > -- > 2.25.1 >
On Aug 16, Sean Wang wrote: > From: Quan Zhou <quan.zhou@mediatek.com> > > Add unified ROC cmd/event which is only supported by the newer fw. > > Co-developed-by: Sean Wang <sean.wang@mediatek.com> > Signed-off-by: Sean Wang <sean.wang@mediatek.com> > Co-developed-by: Deren Wu <deren.wu@mediatek.com> > Signed-off-by: Deren Wu <deren.wu@mediatek.com> > Co-developed-by: Kaikai Hu <kaikai.hu@mediatek.com> > Signed-off-by: Kaikai Hu <kaikai.hu@mediatek.com> > Signed-off-by: Quan Zhou <quan.zhou@mediatek.com> > --- > .../wireless/mediatek/mt76/mt76_connac_mcu.h | 10 +- > .../net/wireless/mediatek/mt76/mt7921/mcu.c | 119 ++++++++++++++++++ > .../wireless/mediatek/mt76/mt7921/mt7921.h | 19 +++ > 3 files changed, 147 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h > index 1fcc3e8c5380..f3c1e1dc574a 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h > +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h > @@ -114,11 +114,15 @@ struct mt76_connac2_mcu_rxd { > > u8 eid; > u8 seq; > - u8 rsv[2]; > + u8 option; > + u8 __rsv; > + nit: remove new line here and use 'u8 rsv' to be consistent with the rest of the code. > > u8 ext_eid; > u8 rsv1[2]; > u8 s2d_index; > + > + u8 tlv[0]; > }; > > struct mt76_connac2_patch_hdr { > @@ -938,6 +942,9 @@ enum { > DEV_INFO_MAX_NUM > }; > > +#define MCU_UNI_CMD_EVENT BIT(1) > +#define MCU_UNI_CMD_UNSOLICITED_EVENT BIT(2) > + > /* event table */ > enum { > MCU_EVENT_TARGET_ADDRESS_LEN = 0x01, > @@ -1144,6 +1151,7 @@ enum { > MCU_UNI_CMD_OFFLOAD = 0x06, > MCU_UNI_CMD_HIF_CTRL = 0x07, > MCU_UNI_CMD_SNIFFER = 0x24, > + MCU_UNI_CMD_ROC = 0x27, > }; > > enum { > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c > index da12d0ae0835..76c8afc00c24 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c > @@ -275,6 +275,23 @@ mt7921_mcu_rx_unsolicited_event(struct mt7921_dev *dev, struct sk_buff *skb) > dev_kfree_skb(skb); > } > > +static void > +mt7921_mcu_uni_rx_unsolicited_event(struct mt7921_dev *dev, > + struct sk_buff *skb) > +{ > + struct mt76_connac2_mcu_rxd *rxd; > + > + rxd = (struct mt76_connac2_mcu_rxd *)skb->data; > + > + switch (rxd->eid) { > + case MCU_UNI_EVENT_ROC: > + break; > + default: > + break; > + } > + dev_kfree_skb(skb); > +} > + > void mt7921_mcu_rx_event(struct mt7921_dev *dev, struct sk_buff *skb) > { > struct mt76_connac2_mcu_rxd *rxd; > @@ -284,6 +301,11 @@ void mt7921_mcu_rx_event(struct mt7921_dev *dev, struct sk_buff *skb) > > rxd = (struct mt76_connac2_mcu_rxd *)skb->data; > > + if (rxd->option & MCU_UNI_CMD_UNSOLICITED_EVENT) { > + mt7921_mcu_uni_rx_unsolicited_event(dev, skb); > + return; > + } > + > if (rxd->eid == 0x6) { > mt76_mcu_rx_event(&dev->mt76, skb); > return; > @@ -521,6 +543,103 @@ int mt7921_mcu_set_tx(struct mt7921_dev *dev, struct ieee80211_vif *vif) > &req_mu, sizeof(req_mu), false); > } > > +int mt7921_mcu_set_roc(struct mt7921_phy *phy, struct mt7921_vif *vif, > + struct ieee80211_channel *chan, int duration, > + enum mt7921_roc_req type, u8 token_id) > +{ > + int center_ch = ieee80211_frequency_to_channel(chan->center_freq); > + struct mt7921_dev *dev = phy->dev; > + struct { > + struct { > + u8 rsv[4]; > + } __packed hdr; > + struct roc_acquire_tlv { > + __le16 tag; > + __le16 len; > + u8 bss_idx; > + u8 tokenid; > + u8 control_channel; > + u8 sco; > + u8 band; > + u8 bw; > + u8 center_chan; > + u8 center_chan2; > + u8 bw_from_ap; > + u8 center_chan_from_ap; > + u8 center_chan2_from_ap; > + u8 reqtype; > + __le32 maxinterval; > + u8 dbdcband; > + u8 rsv[3]; > + } __packed roc; > + } __packed req = { > + .roc = { > + .tag = cpu_to_le16(UNI_ROC_ACQUIRE), > + .len = cpu_to_le16(sizeof(struct roc_acquire_tlv)), > + .tokenid = token_id, > + .reqtype = type, > + .maxinterval = cpu_to_le32(duration), > + .bss_idx = vif->mt76.idx, > + .control_channel = chan->hw_value, > + .bw = CMD_CBW_20MHZ, > + .bw_from_ap = CMD_CBW_20MHZ, > + .center_chan = center_ch, > + .center_chan_from_ap = center_ch, > + .dbdcband = 0xff, /* auto */ > + }, > + }; > + > + if (chan->hw_value < center_ch) > + req.roc.sco = 1; /* SCA */ > + else if (chan->hw_value > center_ch) > + req.roc.sco = 3; /* SCB */ > + > + switch (chan->band) { > + case NL80211_BAND_6GHZ: > + req.roc.band = 3; > + break; > + case NL80211_BAND_5GHZ: > + req.roc.band = 2; > + break; > + default: > + req.roc.band = 1; > + break; > + } > + > + return mt76_mcu_send_msg(&dev->mt76, MCU_UNI_CMD(ROC), > + &req, sizeof(req), false); > +} > + > +int mt7921_mcu_abort_roc(struct mt7921_phy *phy, struct mt7921_vif *vif, > + u8 token_id) > +{ > + struct mt7921_dev *dev = phy->dev; > + struct { > + struct { > + u8 rsv[4]; > + } __packed hdr; > + struct roc_abort_tlv { > + __le16 tag; > + __le16 len; > + u8 bss_idx; > + u8 tokenid; > + u8 dbdcband; > + u8 rsv[5]; > + } __packed abort; > + } __packed req = { > + .abort = { > + .tag = cpu_to_le16(UNI_ROC_ABORT), > + .len = cpu_to_le16(sizeof(struct roc_abort_tlv)), > + .tokenid = token_id, > + .bss_idx = vif->mt76.idx, > + .dbdcband = 0xff, /* auto*/ > + }, > + }; > + > + return mt76_mcu_send_msg(&dev->mt76, MCU_UNI_CMD(ROC), > + &req, sizeof(req), false); > +} > + > int mt7921_mcu_set_chan_info(struct mt7921_phy *phy, int cmd) > { > struct mt7921_dev *dev = phy->dev; > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h b/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h > index c161031ac62a..c9044d546e94 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h > @@ -51,6 +51,20 @@ > #define MT7921_SDIO_HDR_TX_BYTES GENMASK(15, 0) > #define MT7921_SDIO_HDR_PKT_TYPE GENMASK(17, 16) > > +#define MCU_UNI_EVENT_ROC 0x27 > + > +enum { > + UNI_ROC_ACQUIRE, > + UNI_ROC_ABORT, > + UNI_ROC_NUM > +}; > + > +enum mt7921_roc_req { > + MT7921_ROC_REQ_JOIN, > + MT7921_ROC_REQ_ROC, > + MT7921_ROC_REQ_NUM > +}; > + > enum mt7921_sdio_pkt_type { > MT7921_SDIO_TXD, > MT7921_SDIO_DATA, > @@ -479,4 +493,9 @@ mt7921_init_acpi_sar_power(struct mt7921_phy *phy, bool set_default) > #endif > int mt7921_set_tx_sar_pwr(struct ieee80211_hw *hw, > const struct cfg80211_sar_specs *sar); > +int mt7921_mcu_set_roc(struct mt7921_phy *phy, struct mt7921_vif *vif, > + struct ieee80211_channel *chan, int duration, > + enum mt7921_roc_req type, u8 token_id); > +int mt7921_mcu_abort_roc(struct mt7921_phy *phy, struct mt7921_vif *vif, > + u8 token_id); > #endif > -- > 2.25.1 >
Hi Johannes, On Tue, Aug 16, 2022 at 3:22 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Tue, 2022-08-16 at 08:03 +0800, sean.wang@mediatek.com wrote: > > From: Sean Wang <sean.wang@mediatek.com> > > > > MT7921 device can be supported with the channel context depending on > > the newer firmware so that we need a way to enable the chanctx related > > methods until hw is being registered. > > > > Signed-off-by: Sean Wang <sean.wang@mediatek.com> > > --- > > net/mac80211/main.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/net/mac80211/main.c b/net/mac80211/main.c > > index 5b1c47ed0cc0..98d05ed1a081 100644 > > --- a/net/mac80211/main.c > > +++ b/net/mac80211/main.c > > @@ -1011,6 +1011,14 @@ int ieee80211_register_hw(struct ieee80211_hw *hw) > > return -EINVAL; > > #endif > > > > + /* check all or no channel context operations exist */ > > + i = !!local->ops->add_chanctx + !!local->ops->remove_chanctx + > > + !!local->ops->change_chanctx + !!local->ops->assign_vif_chanctx + > > + !!local->ops->unassign_vif_chanctx; > > + if (WARN_ON(i != 0 && i != 5)) > > + return -EINVAL; > > + local->use_chanctx = i == 5; > > + > > Not sure I understand this - this just *adds* code, based on the > description I would've expected you to *move* code? It can be done and looks better by *move* code instead of *adds* code. I will change it in the next version. Thanks for your input. > > In any case, I'm not sure I see how this makes sense - ops is supposed > to be const, and you're supposed to pass it to alloc_hw already, so how > would it change?! > > Also, conceptually, I'm not sure why it's needed to alloc_hw before > loading firmware, we also have a lot of things depend on the firmware > capabilities in iwlwifi/mvm, and so we alloc/register HW after loading > firmware. > Based on mt76 driver logic, alloc_hw would be needed before loading firmware because alloc_hw creates an instance of "struct mt76_dev*" the firmware loading relies on, and so the firmware capabilities cannot be decided before we alloc_hw in mt76 driver. sean > johannes >
On Wed, 2022-08-17 at 01:28 -0700, Sean Wang wrote: > Based on mt76 driver logic, alloc_hw would be needed before loading firmware > because alloc_hw creates an instance of "struct mt76_dev*" the > firmware loading relies on, > and so the firmware capabilities cannot be decided before we alloc_hw > in mt76 driver. > I don't really see why you couldn't change that though? There's no fundamental reason you need to load the firmware before registering with mac80211? And fundamentally, I'm not even sure how you are achieving a change of the ops - you're meant to point those to a *const* ops, so you need two versions of the ops, one with and one without chanctx, and point to the correct one at allocation ... johannes
Hi Johannes, On Wed, Aug 17, 2022 at 1:30 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Wed, 2022-08-17 at 01:28 -0700, Sean Wang wrote: > > Based on mt76 driver logic, alloc_hw would be needed before loading firmware > > because alloc_hw creates an instance of "struct mt76_dev*" the > > firmware loading relies on, > > and so the firmware capabilities cannot be decided before we alloc_hw > > in mt76 driver. > > > > I don't really see why you couldn't change that though? There's no > fundamental reason you need to load the firmware before registering with > mac80211? > It could be changed but it would break the consistency of the current mt76 driver. mt76 driver does the things in the following order - ieee80211_alloc_hw (where an instance of "struct mt76_dev *" would be created) - register bus operation into mt76 core (depending on struct mt76_dev to provide an abstraction layer for mt76 core to access bus) - read chip identifier (depending on bus operation) - load the firmware capabilities (depending on chip identifier) - initialize the hardware .... -ieee80211_register_hw If firmware capability is needed to load before ieee80211_alloc_hw, we need to create kind of similar functions to read chip identifiers and load firmware. I know It may not a strong reason not to change, but if we can support read firmware capabilities after alloc_hw, it provides flexibility to the vendor driver and helps mt7921 look more consistent and save a few changes across different mt7921 bus drivers (mt7921 now supports SDIO, USB, PCIe type driver). > And fundamentally, I'm not even sure how you are achieving a change of I kmemdup the const ops and ieee80211_alloc_hw with the duplicated ops the duplicated ops would be updated by the actual firmware capabilities before ieee80211_register_hw. > the ops - you're meant to point those to a *const* ops, so you need two > versions of the ops, one with and one without chanctx, and point to the > correct one at allocation ... > If you don't like the reason and the way I proposed in the patch, please let me know. I still can move on to the way you suggested here. > johannes
Hi Sean, > It could be changed but it would break the consistency of the current > mt76 driver. I'm not really convinced ... > mt76 driver does the things in the following order > - ieee80211_alloc_hw (where an instance of "struct mt76_dev *" would be created) > - register bus operation into mt76 core (depending on struct mt76_dev > to provide an abstraction layer for mt76 core to access bus) > - read chip identifier (depending on bus operation) > - load the firmware capabilities (depending on chip identifier) > - initialize the hardware > .... > -ieee80211_register_hw > > If firmware capability is needed to load before ieee80211_alloc_hw, we > need to create kind of similar functions to read chip identifiers and > load firmware. > I know It may not a strong reason not to change, but if we can support > read firmware capabilities after alloc_hw, it provides flexibility to > the vendor driver > and helps mt7921 look more consistent and save a few changes across > different mt7921 bus drivers (mt7921 now supports SDIO, USB, PCIe type > driver). But you're loading _firmware_, so to determine the capabilities all you should need is the actual file, no? I mean, you don't even have to load it into the device. Like iwlwifi, you could have an indication (or many flags, or TLVs, or whatnot) in the file that says what it's capable of. > I kmemdup the const ops and ieee80211_alloc_hw with the duplicated ops > the duplicated ops would be updated by the actual firmware > capabilities before ieee80211_register_hw. Well ... yeah ok that works, but it's pretty wasteful, and also makes this a nice security attack target - there's a reason ops structs are supposed to be const, that's because they can then be really read-only and you can't have function pointer changes. Some of the CFI stuff is meant to help avoid that, but still ... So anyway. I'm not really sure I buy this - even you while doing this already kind of introduced a bug because you didn't change this code: if (!use_chanctx || ops->remain_on_channel) wiphy->flags |= WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL; I guess you didn't notice because you have remain_on_channel in the first place, but that's not the only code there assuming that we have the ops in place and they don't change. If we really, really need to allow changing the ops, then we should probably make a much larger change to not even pass the ops until register, though I'm not really sure it's worth it just to have mt7921 avoid loading the firmware from disk before allocation? johannes
Hi Johannes, On Thu, Aug 18, 2022 at 3:49 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > Hi Sean, > > > It could be changed but it would break the consistency of the current > > mt76 driver. > > I'm not really convinced ... > > > mt76 driver does the things in the following order > > - ieee80211_alloc_hw (where an instance of "struct mt76_dev *" would be created) > > - register bus operation into mt76 core (depending on struct mt76_dev > > to provide an abstraction layer for mt76 core to access bus) > > - read chip identifier (depending on bus operation) > > - load the firmware capabilities (depending on chip identifier) > > - initialize the hardware > > .... > > -ieee80211_register_hw > > > > If firmware capability is needed to load before ieee80211_alloc_hw, we > > need to create kind of similar functions to read chip identifiers and > > load firmware. > > I know It may not a strong reason not to change, but if we can support > > read firmware capabilities after alloc_hw, it provides flexibility to > > the vendor driver > > and helps mt7921 look more consistent and save a few changes across > > different mt7921 bus drivers (mt7921 now supports SDIO, USB, PCIe type > > driver). > > But you're loading _firmware_, so to determine the capabilities all you > should need is the actual file, no? I mean, you don't even have to load > it into the device. Like iwlwifi, you could have an indication (or many > flags, or TLVs, or whatnot) in the file that says what it's capable of. > > > I kmemdup the const ops and ieee80211_alloc_hw with the duplicated ops > > the duplicated ops would be updated by the actual firmware > > capabilities before ieee80211_register_hw. > > Well ... yeah ok that works, but it's pretty wasteful, and also makes > this a nice security attack target - there's a reason ops structs are > supposed to be const, that's because they can then be really read-only > and you can't have function pointer changes. Some of the CFI stuff is > meant to help avoid that, but still ... > > So anyway. I'm not really sure I buy this - even you while doing this > already kind of introduced a bug because you didn't change this code: > > if (!use_chanctx || ops->remain_on_channel) > wiphy->flags |= WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL; > > I guess you didn't notice because you have remain_on_channel in the > first place, but that's not the only code there assuming that we have > the ops in place and they don't change. > > If we really, really need to allow changing the ops, then we should > probably make a much larger change to not even pass the ops until > register, though I'm not really sure it's worth it just to have mt7921 > avoid loading the firmware from disk before allocation? > > johannes Thanks for your input. I thought I'd try to write a patch to follow up on the idea you mentioned here. sean
Hi Sean, > > If we really, really need to allow changing the ops, then we should > > probably make a much larger change to not even pass the ops until > > register, though I'm not really sure it's worth it just to have mt7921 > > avoid loading the firmware from disk before allocation? > Thanks for your input. I thought I'd try to write a patch to follow up > on the idea you mentioned here. > I think you will introduce a bug into mt7921 when you do this, and I'm curious if you will find it ;-) Seriously though, this approach also seems fragile, and I don't even know if other bugs would be introduced. And splitting into three functions (alloc -> set_ops -> register) also feels a bit awkward. Is there really no chance you could add bits to the firmware _file_ format so you can query the capabilities before you actually _run_ the firmware? I guess you could even validate it at runtime again (and just fail is somebody messed up the file), but it would make things a lot simpler, I'm sure. johannes
On Fri, 2022-08-19 at 19:16 +0200, Johannes Berg wrote: > Hi Sean, > > > > If we really, really need to allow changing the ops, then we should > > > probably make a much larger change to not even pass the ops until > > > register, though I'm not really sure it's worth it just to have mt7921 > > > avoid loading the firmware from disk before allocation? > > > Thanks for your input. I thought I'd try to write a patch to follow up > > on the idea you mentioned here. > > > > I think you will introduce a bug into mt7921 when you do this, and I'm > curious if you will find it ;-) > Actually, no, clearing WIPHY_FLAG_IBSS_RSN in mt7921_init_wiphy() actually does nothing, I thought it was required. Anyway, the point is that now we set some defaults in alloc_hw(), some based on the ops, and then the driver could override those before it does register_hw(). With the proposed change, that's no longer possible, and I'm not sure I (or you) would want to audit each and every driver - WIPHY_FLAG_IBSS_RSN was something that showed up easily in grep though. So I still think you're better off doing it in the firmware file :-) johannes
From: Sean Wang <objelf@gmail.com> The patchset introduces chanctx support to enable the capability on the newer firmware to manage the channel context scheduling on multiple roles running on the device including Station, AP, and P2P GC/GO mode (will be extended based on the patchset in the future) to help users share the network with others on a single device. The things to be noted is that 1) The newer firmware is able to support the channel chanctx up to 2 interfaces simultaneously running on the different channels. 2) Before the driver is going sent out the management frames, the driver has to get the privilege from the firmware to acquire the current channel context for the frame until the frame handshake is completed and then get the privilege back to the firmware. 3) The patchset has to rely on ("mt76: mt7921: fix the firmware version report") ("mt76: mt7921: add mt7921_mutex_acquire at mt7921_[start, stop]_ap") Quan Zhou (1): wifi: mt76: mt7921: add unified ROC cmd/event support Sean Wang (8): wifi: mac80211: allow enabling chanctx until hw registration wifi: mt76: connac: add mt76_connac_mcu_uni_set_chctx wifi: mt76: connac: rely on mt76_connac_mcu_uni_set_chctx wifi: mt76: mt7921: add chanctx parameter to mt76_connac_mcu_uni_add_bss signature wifi: mt76: mt7921: drop ieee80211_[start, stop]_queues in driver wifi: mt76: connac: accept hw scan request at a time wifi: mt76: mt7921: introduce remain_on_channel support wifi: mt76: mt7921: introduce chanctx support .../net/wireless/mediatek/mt76/mt7615/mcu.c | 2 +- .../wireless/mediatek/mt76/mt76_connac_mcu.c | 152 ++++++------ .../wireless/mediatek/mt76/mt76_connac_mcu.h | 16 +- .../net/wireless/mediatek/mt76/mt7921/init.c | 74 +++++- .../net/wireless/mediatek/mt76/mt7921/mac.c | 2 +- .../net/wireless/mediatek/mt76/mt7921/main.c | 225 +++++++++++++++++- .../net/wireless/mediatek/mt76/mt7921/mcu.c | 151 +++++++++++- .../wireless/mediatek/mt76/mt7921/mt7921.h | 54 +++++ .../net/wireless/mediatek/mt76/mt7921/pci.c | 13 +- .../net/wireless/mediatek/mt76/mt7921/sdio.c | 11 +- .../net/wireless/mediatek/mt76/mt7921/usb.c | 1 + net/mac80211/main.c | 8 + 12 files changed, 620 insertions(+), 89 deletions(-)