[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d4047df7-6e55-1fd5-a61b-9aea3b4da3c5@amd.com>
Date: Wed, 12 Jul 2023 18:28:42 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Andrew Lunn <andrew@...n.ch>, Evan Quan <evan.quan@....com>
Cc: rafael@...nel.org, lenb@...nel.org, Alexander.Deucher@....com,
Christian.Koenig@....com, Xinhui.Pan@....com, airlied@...il.com,
daniel@...ll.ch, johannes@...solutions.net, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
mdaenzer@...hat.com, maarten.lankhorst@...ux.intel.com,
tzimmermann@...e.de, hdegoede@...hat.com, jingyuwang_vip@....com,
Lijo.Lazar@....com, jim.cromie@...il.com, bellosilicio@...il.com,
andrealmeid@...lia.com, trix@...hat.com, jsg@....id.au,
arnd@...db.de, linux-kernel@...r.kernel.org,
linux-acpi@...r.kernel.org, amd-gfx@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, linux-wireless@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH V6 1/9] drivers core: Add support for Wifi band RF
mitigations
On 7/12/23 18:12, Andrew Lunn wrote:
>> +/**
>> + * 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?
>
Potential producers are supposed to be the ones asking the question.
> Rather than "Determine if the device can report frequencies" should it be
> "Determine if the device should report frequencies"
Agree.
>
> 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.
Which is why this function exists.
With the AMD ACPI implementation the platform will dictate this information.
If a future device tree implementation is added it would work similarly.
>
>> +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.
The point is a lot clearer when you look at the description for the
Kconfig included in patch 2.
+ Ideally it is the hardware designer/provider who should provide a
+ solution for the possible RF interference issue. Since they know
+ well whether there could be RF interference issue with their
+ platforms.
+
+ Say Y to enable this generic WBRF solution for diagnosing potential
+ interference issues on systems without the ACPI mechanism and
+ developing solutions.
WBRF_AMD_ACPI and WBRF_GENERIC are mutually exclusive. I don't expect
the average user to enable WBRF_GENERIC, but there isn't anything to
stop them from doing so.
It may also aide in developing a WBRF_DEVICE_TREE or similar.
>
>> +/**
>> + * 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.
Again, look at patch 2 and the purpose of WBRF_GENERIC. I suppose an
argument can be made to bring WBRF_GENERIC into patch 1.
>
>> + * 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(&wbrf_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
Powered by blists - more mailing lists