| Message ID | 473fc7b169f288b7815a7736cf33ac0ec1599a09.1660606893.git.objelf@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| 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
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
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; + if (!local->use_chanctx) { for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) { const struct ieee80211_iface_combination *comb;