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
