Re: [PATCH V7 4/9] wifi: mac80211: Add support for ACPI WBRF
On Mon, Aug 14, 2023 at 09:50:49AM +, Quan, Evan wrote: > [AMD Official Use Only - General] > > Hi Andrew, > > I sent out a new V8 series last week. > A kernel parameter `wbrf` was introduced there to decide the policy. > Please help to check whether that makes sense to you. > Please share your insights there. netdev has a pretty strong policy of not adding new kernel parameters. It is a really painful interface to use, and there are generally better configuration interfaces within netdev. However, as far as i can see, it is outside of netdev, so this policy does not necessarily apply. Andrew
Re: [PATCH V7 4/9] wifi: mac80211: Add support for ACPI WBRF
> 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. Yes, this is the policy is keep referring to. I would expect that there is something somewhere in ACPI which says for this machine, the policy is Yes/No. It could well be that AMD based machine has a different ACPI extension to indicate this policy to what Intel machine has. As far as i understand it, you have not submitted this yet for formal approval, this is all vendor specific, so Intel could do it completely differently. Hence i would expect a generic API to tell the core what the policy is, and your glue code can call into ACPI to find out that information, and then tell the core. > 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. And the policy will indicate No, nothing needs to be done. The core can then tell produces and consumes not to bother telling the core anything. > 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". Wrong. You should have a generic core, which your AMD CPU DDR device plugs into. The Intel CPU DDR device can plug into, the nvidea GPU can plug into, your Radeon GPU can plug into, the intel ARC can plug into, the generic WiFi core plugs into, etc. > 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. Notice i've not mentioned DT for a while. I just want a generic core, which AMD, Intel, nvidea, Ampare, Graviton, Qualcomm, Marvell, ..., etc can use. We should be solving this problem once, for everybody, not adding a solution for just one vendor. Andrew
Re: [PATCH V7 4/9] wifi: mac80211: Add support for ACPI WBRF
> > >> @@ -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.
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
Re: [PATCH V7 4/9] wifi: mac80211: Add support for ACPI WBRF
> @@ -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
Re: [PATCH V6 1/9] drivers core: Add support for Wifi band RF mitigations
> The wbrf_supported_producer and wbrf_supported_consumer APIs seem > unnecessary for the generic implementation. I'm happy with these, once the description is corrected. As i said in another comment, 'can' should be replaced with 'should'. The device itself knows if it can, only the core knows if it should, based on the policy of if actions need to be taken, and there are both providers and consumers registered with the core. Andrew
Re: [PATCH V6 1/9] drivers core: Add support for Wifi band RF mitigations
> +/**
> + * wbrf_supported_producer - Determine if the device can report frequencies
> + *
> + * @dev: device pointer
> + *
> + * WBRF is used to mitigate devices that cause harmonic interference.
> + * This function will determine if this device needs to report such
> frequencies.
How is the WBRF core supposed to answer this question? That it knows
there is at least one device which has registered with WBRF saying it
can change its behaviour to avoid causing interference?
Rather than "Determine if the device can report frequencies" should it be
"Determine if the device should report frequencies"
A WiFi device can always report frequencies, since it knows what
frequency is it currently using. However, it is pointless making such
reports if there is no device which can actually make use of the
information.
> +bool wbrf_supported_producer(struct device *dev)
> +{
> + return true;
> +}
I found the default implementation of true being odd. It makes me
wounder, what is the point of this call. I would expect this to see if
a linked list is empty or not.
> +/**
> + * wbrf_supported_consumer - Determine if the device can react to frequencies
This again seems odd. A device should know if it can react to
frequencies or not. WBRF core should not need to tell it. What makes
more sense to me is that this call is about a device telling the WBRF
core it is able to react to frequencies. The WBRF core then can give a
good answer to wbrf_supported_producer(), yes, i know of some other
device who might be able to do something to avoid causing interference
to you, so please do tell me about frequencies you want to use.
What is missing here in this API is policy information. The WBRF core
knows it has zero or more devices which can report what frequencies
they are using, and it has zero or more devices which maybe can do
something. But then you need policy to say this particular board needs
any registered devices to actually do something because of poor
shielding. Should this policy be as simple as a bool, or should it
actually say the board has shielding issues for a list of frequencies?
I think the answer to what will depend on the cost of taking action
when no action is actually required.
> + * wbrf_register_notifier - Register for notifications of frequency changes
> + *
> + * @nb: driver notifier block
> + *
> + * WBRF is used to mitigate devices that cause harmonic interference.
> + * This function will allow consumers to register for frequency
> notifications.
> + */
> +int wbrf_register_notifier(struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_register(_chain_head, nb);
> +}
What are the timing requirements for the handler? Should the handler
block until the device has finished doing what it needs to do and the
frequency response has settled? We don't want the WiFi device doing a
SNR measurement until we know local noise is at a minimum. I think it
would be good to document things like this here.
> +struct wbrf_ranges_out {
> + u32 num_of_ranges;
> + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES];
> +} __packed;
Seems odd using packed here. It is the only structure which is
packed. I would also move the u32 after the struct so it is naturally
aligned on 64 bit systems.
Andrew
Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
> > What is the purpose of this stage? Why would it not be supported for this > > device? > This is needed for wbrf support via ACPI mechanism. If BIOS(AML code) does > not support the wbrf adding/removing for some device, > it should speak that out so that the device can be aware of that. How much overhead is this adding? How deep do you need to go to find the BIOS does not support it? And how often is this called? Where do we want to add complexity? In the generic API? Or maybe a little deeper in the ACPI specific code? Andrew
Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
> Drivers/subsystems contributing frequencies:
>
> 1) During probe, check `wbrf_supported_producer` to see if WBRF supported
>for the device.
What is the purpose of this stage? Why would it not be supported for
this device?
> +#ifdef CONFIG_WBRF
> +bool wbrf_supported_producer(struct device *dev);
> +int wbrf_add_exclusion(struct device *adev,
> +struct wbrf_ranges_in *in);
> +int wbrf_remove_exclusion(struct device *dev,
> + struct wbrf_ranges_in *in);
> +int wbrf_retrieve_exclusions(struct device *dev,
> + struct wbrf_ranges_out *out);
> +bool wbrf_supported_consumer(struct device *dev);
> +
> +int wbrf_register_notifier(struct notifier_block *nb);
> +int wbrf_unregister_notifier(struct notifier_block *nb);
> +#else
> +static inline bool wbrf_supported_producer(struct device *dev) { return
> false; }
> +static inline int wbrf_add_exclusion(struct device *adev,
> + struct wbrf_ranges_in *in) { return
> -ENODEV; }
> +static inline int wbrf_remove_exclusion(struct device *dev,
> + struct wbrf_ranges_in *in) { return
> -ENODEV; }
The normal aim of stubs is that so long as it is not expected to be
fatal if the functionality is missing, the caller should not care if
it is missing. So i would expect these to return 0, indicating
everything worked as expected.
> +static inline int wbrf_retrieve_exclusions(struct device *dev,
> +struct wbrf_ranges_out *out) {
> return -ENODEV; }
This is more complex. Ideally you want to return an empty set, so
there is nothing to do. So i think the stub probably wants to do a
memset and then return 0.
> +static inline bool wbrf_supported_consumer(struct device *dev) { return
> false; }
> +static inline int wbrf_register_notifier(struct notifier_block *nb) { return
> -ENODEV; }
> +static inline int wbrf_unregister_notifier(struct notifier_block *nb) {
> return -ENODEV; }
And these can just return 0.
Andrew
Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
> Right now there are stubs for non CONFIG_WBRF as well as other patches are
> using #ifdef CONFIG_WBRF or having their own stubs. Like mac80211 patch
> looks for #ifdef CONFIG_WBRF.
>
> I think we should pick one or the other.
>
> Having other subsystems #ifdef CONFIG_WBRF will make the series easier to
> land through multiple trees; so I have a slight leaning in that direction.
#ifdef in C files is generally not liked because it makes build
testing harder. There are more permutations to build. It is better to use
if (IS_ENABLED(CONFIG_WBTR)) {
}
so that the code is compiled, and them throw away because
IS_ENABLED(CONFIG_WBTR) evaluates to false.
However, if the stubs are done correctly, the driver should not
care. I doubt this is used in any sort of hot path where every
instruction counts.
Andrew
Re: [PATCH V5 2/9] driver core: add ACPI based WBRF mechanism introduced by AMD
> + argv4 = kzalloc(sizeof(*argv4) * (2 * num_of_ranges + 2 + 1),
> GFP_KERNEL);
> + if (!argv4)
> + return -ENOMEM;
> +
> + argv4[arg_idx].package.type = ACPI_TYPE_PACKAGE;
> + argv4[arg_idx].package.count = 2 + 2 * num_of_ranges;
> + argv4[arg_idx++].package.elements = [1];
> + argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
> + argv4[arg_idx++].integer.value = num_of_ranges;
> + argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
> + argv4[arg_idx++].integer.value = action;
There is a lot of magic numbers in that kzalloc. It is being used as
an array, kcalloc() would be a good start to make it more readable.
Can some #define's be used to explain what the other numbers mean?
> + /*
> + * Bit 0 indicates whether there's support for any functions other than
> + * function 0.
> + */
Please make use of the BIT macro to give the different bits
informative names.
> + if ((mask & 0x1) && (mask & funcs) == funcs)
> + return true;
> +
> + return false;
> +}
> +
> +int acpi_amd_wbrf_retrieve_exclusions(struct device *dev,
> + struct wbrf_ranges_out *out)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> + union acpi_object *obj;
> +
> + if (!adev)
> + return -ENODEV;
> +
> + obj = acpi_evaluate_wbrf(adev->handle,
> + WBRF_REVISION,
> + WBRF_RETRIEVE);
> + if (!obj)
> + return -EINVAL;
> +
> + WARN(obj->buffer.length != sizeof(*out),
> + "Unexpected buffer length");
> + memcpy(out, obj->buffer.pointer, obj->buffer.length);
You WARN, and then overwrite whatever i passed the end of out? Please
at least use min(obj->buffer.length, sizeof(*out)), but better still:
if (obj->buffer.length != sizeof(*out)) {
dev_err(dev, "BIOS FUBAR, ignoring wrong sized WBRT information");
return -EINVAL;
}
> +#if defined(CONFIG_WBRF_GENERIC)
> static struct exclusion_range_pool wbrf_pool;
>
> static int _wbrf_add_exclusion_ranges(struct wbrf_ranges_in *in)
> @@ -89,6 +92,7 @@ static int _wbrf_retrieve_exclusion_ranges(struct
> wbrf_ranges_out *out)
>
> return 0;
> }
> +#endif
I was expecting you would keep these tables, and then call into the
BIOS as well. Having this table in debugfs seems like a useful thing
to have for debugging the BIOS.
> +#ifdef CONFIG_WBRF_AMD_ACPI
> +#else
> +static inline bool
> +acpi_amd_wbrf_supported_consumer(struct device *dev) { return false; }
> +static inline bool
> +acpi_amd_wbrf_supported_producer(struct device *dev) {return false; }
> +static inline int
> +acpi_amd_wbrf_remove_exclusion(struct device *dev,
> +struct wbrf_ranges_in *in) { return -ENODEV; }
> +static inline int
> +acpi_amd_wbrf_add_exclusion(struct device *dev,
> + struct wbrf_ranges_in *in) { return -ENODEV; }
> +static inline int
> +acpi_amd_wbrf_retrieve_exclusions(struct device *dev,
> + struct wbrf_ranges_out *out) { return
> -ENODEV; }
Do you actually need these stub versions?
Andrew
Re: [PATCH V5 4/9] wifi: mac80211: Add support for ACPI WBRF
> +static void get_chan_freq_boundary(u32 center_freq,
> +u32 bandwidth,
> +u64 *start,
> +u64 *end)
> +{
> + bandwidth = MHZ_TO_KHZ(bandwidth);
> + center_freq = MHZ_TO_KHZ(center_freq);
> +
> + *start = center_freq - bandwidth / 2;
> + *end = center_freq + bandwidth / 2;
> +
> + /* Frequency in HZ is expected */
> + *start = KHZ_TO_HZ(*start);
> + *end = KHZ_TO_HZ(*end);
> +}
This seems pretty generic, so maybe it should be moved into the shared
code? It can then become a NOP when the functionality if disabled.
Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> > Do only ACPI based systems have: > > > >interference of relatively high-powered harmonics of the (G-)DDR > >memory clocks with local radio module frequency bands used by > >Wifi 6/6e/7." > > > > Could Device Tree based systems not experience this problem? > > They could, of course, but they'd need some other driver to change > _something_ in the system? I don't even know what this is doing > precisely under the hood in the ACPI BIOS If you don't know what it is actually doing, it suggests the API is not very well defined. Is there even enough details that ARM64 ACPI BIOS could implement this? > > > +/** > > > + * APIs needed by drivers/subsystems for contributing frequencies: > > > + * During probe, check `wbrf_supported_producer` to see if WBRF is > > > supported. > > > + * If adding frequencies, then call `wbrf_add_exclusion` with the > > > + * start and end points specified for the frequency ranges added. > > > + * If removing frequencies, then call `wbrf_remove_exclusion` with > > > + * start and end points specified for the frequency ranges added. > > > + */ > > > +bool wbrf_supported_producer(struct acpi_device *adev); > > > +int wbrf_add_exclusion(struct acpi_device *adev, > > > +struct wbrf_ranges_in *in); > > > +int wbrf_remove_exclusion(struct acpi_device *adev, > > > + struct wbrf_ranges_in *in); > > > > Could struct device be used here, to make the API agnostic to where > > the information is coming from? That would then allow somebody in the > > future to implement a device tree based information provider. > > That does make sense, and it wouldn't even be that much harder if we > assume in a given platform there's only one provider That seems like a very reasonable assumption. It is theoretically possible to build an ACPI + DT hybrid, but i've never seen it actually done. If an ARM64 ACPI BIOS could implement this, then i would guess the low level bits would be solved, i guess jumping into the EL1 firmware. Putting DT on top instead should not be too hard. Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, Jun 21, 2023 at 01:50:34PM -0500, Limonciello, Mario wrote:
> So if we go down this path of CONFIG_WBRF and CONFIG_WBRF_ACPI, another
> question would be where should the new "wbrf.c" be stored? The ACPI only
> version most certainly made sense in drivers/acpi/wbrf.c, but a generic
> version that only has an ACPI implementation right now not so much.
>
> On 6/21/2023 1:30 PM, Andrew Lunn wrote:
> > > And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't
> > > set.
> > Why? How is ACPI special that it does not need notifiers?
> ACPI core does has notifiers that are used, but they don't work the same.
> If you look at patch 4, you'll see amdgpu registers and unregisters using
> both
>
> acpi_install_notify_handler()
> and
> acpi_remove_notify_handler()
>
> If we supported both ACPI notifications and non-ACPI notifications
> all consumers would have to have support to register and use both types.
I took a quick look at this:
#define CPM_GPU_NOTIFY_COMMAND 0x55
+static void amdgpu_acpi_wbrf_event(acpi_handle handle, u32 event, void *data)
+{
+ struct amdgpu_device *adev = (struct amdgpu_device *)data;
+
+ if (event == CPM_GPU_NOTIFY_COMMAND &&
+ adev->wbrf_event_handler)
+ adev->wbrf_event_handler(adev);
+}
handle is ignored, All you need is the void * data to link back to
your private data.
I find it interesting that CPM_GPU_NOTIFY_COMMAND is defined here. So
nothing else can use it. Should it maybe be called
CPM_AMDGPU_NOTIFY_COMMAND?
Overall, looking at this, i don't see anything which could not be made
abstract:
static void amdgpu_wbrf_event(u32 event, void *data)
{
struct amdgpu_device *adev = (struct amdgpu_device *)data;
if (event == WBRF_GPU_NOTIFY_COMMAND &&
adev->wbrf_event_handler)
adev->wbrf_event_handler(adev);
}
int amdgpu_register_wbrf_notify_handler(struct amdgpu_device *adev,
wbrf_notify_handler handler)
{
struct device *dev = adev->dev);
adev->wbrf_event_handler = handler;
return wbrf_install_notify_handler(dev, amdgpu_wbrf_event, adev);
}
Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> I think there is enough details for this to happen. It's done > so that either the AML can natively behave as a consumer or a > driver can behave as a consumer. > > > > > +/** > > > > > + * APIs needed by drivers/subsystems for contributing frequencies: > > > > > + * During probe, check `wbrf_supported_producer` to see if WBRF is > > > > > supported. > > > > > + * If adding frequencies, then call `wbrf_add_exclusion` with the > > > > > + * start and end points specified for the frequency ranges added. > > > > > + * If removing frequencies, then call `wbrf_remove_exclusion` with > > > > > + * start and end points specified for the frequency ranges added. > > > > > + */ > > > > > +bool wbrf_supported_producer(struct acpi_device *adev); > > > > > +int wbrf_add_exclusion(struct acpi_device *adev, > > > > > +struct wbrf_ranges_in *in); > > > > > +int wbrf_remove_exclusion(struct acpi_device *adev, > > > > > + struct wbrf_ranges_in *in); > > > > Could struct device be used here, to make the API agnostic to where > > > > the information is coming from? That would then allow somebody in the > > > > future to implement a device tree based information provider. > > > That does make sense, and it wouldn't even be that much harder if we > > > assume in a given platform there's only one provider > > That seems like a very reasonable assumption. It is theoretically > > possible to build an ACPI + DT hybrid, but i've never seen it actually > > done. > > > > If an ARM64 ACPI BIOS could implement this, then i would guess the low > > level bits would be solved, i guess jumping into the EL1 > > firmware. Putting DT on top instead should not be too hard. > > > > Andrew > > To make life easier I'll ask whether we can include snippets of > the matching ASL for this first implementation as part of the > public ACPI spec that matches this code when we release it. So it sounds like you are pretty open about this, there should be enough information for independent implementations. So please do make the APIs between the providers and the consumers abstract, struct device, not an ACPI object. Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> ACPI core does has notifiers that are used, but they don't work the same. > If you look at patch 4, you'll see amdgpu registers and unregisters using > both > > acpi_install_notify_handler() > and > acpi_remove_notify_handler() > > If we supported both ACPI notifications and non-ACPI notifications > all consumers would have to have support to register and use both types. Why would you want to support ACPI notifications and non-ACPI notifications? All you need is wbrf notification. The new wbrf.c should implement wbrf_install_notify_handler() and wbrf_remove_notify_handler(). As to where to put wbrf.c? I guess either drivers/base/ or drivers/wbrf/. Maybe ask GregKH? Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, Jun 21, 2023 at 01:45:56PM +0800, Evan Quan wrote: > From: Mario Limonciello > > Due to electrical and mechanical constraints in certain platform designs > there may be likely interference of relatively high-powered harmonics of > the (G-)DDR memory clocks with local radio module frequency bands used > by Wifi 6/6e/7. > > To mitigate this, AMD has introduced an ACPI based mechanism that > devices can use to notify active use of particular frequencies so > that devices can make relative internal adjustments as necessary > to avoid this resonance. Do only ACPI based systems have: interference of relatively high-powered harmonics of the (G-)DDR memory clocks with local radio module frequency bands used by Wifi 6/6e/7." Could Device Tree based systems not experience this problem? > +/** > + * APIs needed by drivers/subsystems for contributing frequencies: > + * During probe, check `wbrf_supported_producer` to see if WBRF is supported. > + * If adding frequencies, then call `wbrf_add_exclusion` with the > + * start and end points specified for the frequency ranges added. > + * If removing frequencies, then call `wbrf_remove_exclusion` with > + * start and end points specified for the frequency ranges added. > + */ > +bool wbrf_supported_producer(struct acpi_device *adev); > +int wbrf_add_exclusion(struct acpi_device *adev, > +struct wbrf_ranges_in *in); > +int wbrf_remove_exclusion(struct acpi_device *adev, > + struct wbrf_ranges_in *in); Could struct device be used here, to make the API agnostic to where the information is coming from? That would then allow somebody in the future to implement a device tree based information provider. Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, Jun 21, 2023 at 11:15:00AM -0500, Limonciello, Mario wrote: > > On 6/21/2023 10:39 AM, Johannes Berg wrote: > > On Wed, 2023-06-21 at 17:36 +0200, Andrew Lunn wrote: > > > On Wed, Jun 21, 2023 at 01:45:56PM +0800, Evan Quan wrote: > > > > From: Mario Limonciello > > > > > > > > Due to electrical and mechanical constraints in certain platform designs > > > > there may be likely interference of relatively high-powered harmonics of > > > > the (G-)DDR memory clocks with local radio module frequency bands used > > > > by Wifi 6/6e/7. > > > > > > > > To mitigate this, AMD has introduced an ACPI based mechanism that > > > > devices can use to notify active use of particular frequencies so > > > > that devices can make relative internal adjustments as necessary > > > > to avoid this resonance. > > > Do only ACPI based systems have: > > > > > > interference of relatively high-powered harmonics of the (G-)DDR > > > memory clocks with local radio module frequency bands used by > > > Wifi 6/6e/7." > > > > > > Could Device Tree based systems not experience this problem? > > They could, of course, but they'd need some other driver to change > > _something_ in the system? I don't even know what this is doing > > precisely under the hood in the ACPI BIOS, perhaps it adjusts the DDR > > memory clock frequency in response to WiFi using a frequency that will > > cause interference with harmonics. > > The way that WBRF has been architected, it's intended to be able > to scale to any type of device pair that has harmonic issues. So you set out to make something generic... > In the first use (Wifi 6e + specific AMD dGPUs) that matches this > series BIOS has the following purposes: > > 1) The existence of _DSM indicates that the system may not have > adequate shielding and should be using these mitigations. > > 2) Notification mechanism of frequency use. > > For the first problematic devices we *could* have done notifications > entirely in native Linux kernel code with notifier chains. > However that still means you need a hint from the platform that the > functionality is needed like a _DSD bit. > > It's also done this way so that AML could do some of the notifications > directly to applicable devices in the future without needing "consumer" > driver participation. And then tie is very closely to ACPI. Now, you are AMD, i get that ACPI is what you have. But i think as kernel Maintainers, we need to consider that ACPI is not the only thing used. Do we want the APIs to be agnostic? I think APIs used by drivers should be agnostic. Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> Think a little more about what a non-ACPI implementation > would look like: > > 1) Would producers and consumers still need you to set CONFIG_ACPI_WBRF? I would expect there to be an CONFIG_WBRF, and then under that a CONFIG_WBRF_ACPI, CONFIG_WBRF_DT, CONFIG_WBRF_FOOBAR, each indicating they are mutual exclusive to the other. > 2) How would you indicate you need WBRF support? As far as i understand it, you have something in ACPI which indicates it? Could that not also be a DT property? > 3) How would notifications from one device to another work? Linux notified chains? This is something which happens a lot in networking. A physical interface goes down and needs to tell team/bonding interface stack on top of it that its status has changed. It just calls a notification chain. > I don't think those are trivial problems that can be solved by > just making the pointer 'struct device' particularly as with the > ACPI implementation consumers are expecting the notification from > ACPI. Do consumers really care who the notification is from? Isn't the event and its content what matters, not who sent it? But I agree, i don't expect it is trivial. But it is going to get harder and harder to make abstract as more and more users are added. So we need to consider, do you want to do that work now, or later? Do we want to merge something we know is limited and not using the generic kernel abstractions? Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> Honestly I'm not sure though we need this complexity right now? I mean, > it'd be really easy to replace the calls in mac80211 with some other > more generalised calls in the future? > > You need some really deep platform/hardware level knowledge and > involvement to do this, so I don't think it's something that someone > will come up with very easily for a DT-based platform... What is this API about? It is a struct device says, i'm badly designed and make a mess of the following frequency bands. Optionally, if you ask me nicely, i might be able to tweak what i'm doing to avoid interfering with you. And it is about a struct device say, i'm using this particular frequency. If you can reduce the noise you make, i would be thankful. The one generating the noise could be anything. The PWM driving my laptop display back light?, What is being interfered with? The 3.5mm audio jack? How much deep system knowledge is needed to call pwm_set_state() to move the base frequency up above 20Khz so only my dog will hear it? But at the cost of a loss of efficiency and my battery going flatter faster? Is the DDR memory really the only badly designed component, when you think of the range of systems Linux is used on from PHC to tiny embedded systems? Ideally we want any sort of receiver with a low noise amplifier to just unconditionally use this API to let rest of the system know about it. And ideally we want anything which is a source of noise to declare itself. What happens after that should be up to the struct device causing the interference. Mario did say: The way that WBRF has been architected, it's intended to be able to scale to any type of device pair that has harmonic issues. Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't set. Why? How is ACPI special that it does not need notifiers? > I don't see why it couldn't be a DT/ACPI hybrid solution for ARM64. As said somewhere else, nobody does hybrid. In fact, turn it around. Why not implement all this in DT, and make X86 hybrid? That will make arm, powerpc, risc-v and mips much simpler :-) Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> I think what you're asking for is another layer of indirection
> like CONFIG_WBRF in addition to CONFIG_ACPI_WBRF.
>
> Producers would call functions like wbrf_supported_producer()
> where the source file is not guarded behind CONFIG_ACPI_WBRF,
> but instead by CONFIG_WBRF and locally use CONFIG_ACPI_WBRF within
> it. So a producer could look like this:
>
> bool wbrf_supported_producer(struct device *dev)
> {
> #ifdef CONFIG_ACPI_WBRF
> struct acpi_device *adev = ACPI_COMPANION(dev);
>
> if (adev)
> return check_acpi_wbrf(adev->handle,
> WBRF_REVISION,
> 1ULL << WBRF_RECORD);
> #endif
> return -ENODEV;
>
> }
> EXPORT_SYMBOL_GPL(wbrf_supported_producer);
>
> And then adding/removing could look something like this
>
> int wbrf_add_exclusion(struct device *dev,
> struct wbrf_ranges_in *in)
> {
> #ifdef CONFIG_ACPI_WBRF
> struct acpi_device *adev = ACPI_COMPANION(dev);
>
> if (adev)
> return wbrf_record(adev, WBRF_RECORD_ADD, in);
> #endif
> return -ENODEV;
> }
> EXPORT_SYMBOL_GPL(wbrf_add_exclusion);
>
> int wbrf_remove_exclusion(struct device *dev,
> struct wbrf_ranges_in *in)
> {
> #ifdef CONFIG_ACPI_WBRF
> struct acpi_device *adev = ACPI_COMPANION(dev);
>
> if (adev)
> return wbrf_record(adev, WBRF_RECORD_REMOVE, in);
> #endif
> return -ENODEV;
> }
> EXPORT_SYMBOL_GPL(wbrf_remove_exclusion);
Yes, this looks a lot better.
But what about notifications?
Andrew
