From: Sean Wang <[email protected]>
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 <[email protected]>
---
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;
+
if (!local->use_chanctx) {
for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
const struct ieee80211_iface_combination *comb;
--
2.25.1
On Tue, 2022-08-16 at 08:03 +0800, [email protected] wrote:
> From: Sean Wang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]> wrote:
>
> On Tue, 2022-08-16 at 08:03 +0800, [email protected] wrote:
> > From: Sean Wang <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]> 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