Re: [PATCH V7 4/9] wifi: mac80211: Add support for ACPI WBRF

[Mario Limonciello]

On 7/25/2023 13:57, Andrew Lunn wrote:
> > > > > @@ -1395,6 +1395,8 @@ int ieee80211_register_hw(struct
> > > ieee80211_hw *hw)
> > > > >     debugfs_hw_add(local);
> > > > >     rate_control_add_debugfs(local);
> > > > >
> > > > > +  ieee80211_check_wbrf_support(local);
> > > > > +
> > > > >     rtnl_lock();
> > > > >     wiphy_lock(hw->wiphy);
> > > >
> > > > > +void ieee80211_check_wbrf_support(struct ieee80211_local *local) {
> > > > > +  struct wiphy *wiphy = local->hw.wiphy;
> > > > > +  struct device *dev;
> > > > > +
> > > > > +  if (!wiphy)
> > > > > +          return;
> > > > > +
> > > > > +  dev = wiphy->dev.parent;
> > > > > +  if (!dev)
> > > > > +          return;
> > > > > +
> > > > > +  local->wbrf_supported = wbrf_supported_producer(dev);
> > > > > +  dev_dbg(dev, "WBRF is %s supported\n",
> > > > > +          local->wbrf_supported ? "" : "not"); }
> > > >
> > > > This seems wrong. wbrf_supported_producer() is about "Should this
> > > > device report the frequencies it is using?" The answer to that depends
> > > > on a combination of: Are there consumers registered with the core, and
> > > > is the policy set so WBRF should take actions. > The problem here is,
> > > > you have no idea of the probe order. It could be this device probes
> > > > before others, so wbrf_supported_producer() reports false, but a few
> > > > second later would report true, once other devices have probed.
> > > >
> > > > It should be an inexpensive call into the core, so can be made every
> > > > time the channel changes. All the core needs to do is check if the
> > > > list of consumers is empty, and if not, check a Boolean policy value.
> > > >
> > > >        Andrew
> > >
> > > No, it's not a combination of whether consumers are registered with the core.
> > > If a consumer probes later it needs to know the current in use frequencies too.
> > >
> > > The reason is because of this sequence of events:
> > > 1) Producer probes.
> > > 2) Producer selects a frequency.
> > > 3) Consumer probes.
> > > 4) Producer stays at same frequency.
> > >
> > > If the producer doesn't notify the frequency because a consumer isn't yet
> > > loaded then the consumer won't be able to get the current frequency.
> > Yes, exactly.
>
> So now we are back to, what is the point of wbrf_supported_producer()?
>
> I'm talking general case here, not your ACPI implementation. All i'm
> really interested in is the generic API, which is what an Intel CPU,
> combined with a Radieon GPU and a Qualcomm WiFi device will use. Or an
> AMD CPU combined with an nvidia GPU and a Mediatek Wifi, etc. The wbrf
> core should support an combination of produces and consumers in a
> generic way.
>
> If you assume devices can probe in any order, and come and go, it
> seems like the producers need to always report what frequencies they
> are using. Otherwise when a noise generator pops into existence, as
> you say, it has no idea what frequencies the producers are using.
As the series stands today if the probe order is reversed everything 
works fine.

1) Consumer probes
2) Producer probes
3) Producer selects a frequency
4) Consumer reacts to frequency.

> The exception is when policy says there is no need to actually do
> anything. If we can assume the policy is fixed, then
> wbrf_supported_producer() could just report the policy which the wbrf
> core should know about.
>
>      Andrew

This comes back to the point that was mentioned by Johannes - you need 
to have deep design understanding of the hardware to know whether or not 
you will have producers that a consumer need to react to.

For example the physical location GDDR6 memory and proximity to the 
hinge where the antenna was routed might play a big factor in whether 
you need something like this.

If all producers indicate their frequency and all consumers react to it 
you may have activated mitigations that are unnecessary. The hardware 
designer may have added extra shielding or done the layout such that 
they're not needed.

So I don't think we're ever going to be in a situation that the generic 
implementation should be turned on by default.  It's a "developer knob".

As mentioned in the Kconfig it's intended use is for identifying 
situations that may benefit from mitigation before support was 
introduced into the firmware.

If needed these can then be enabled using the AMD ACPI interface, a DT 
one if one is developed or maybe even an allow-list of SMBIOS strings.