Re: [PATCH V6 1/9] drivers core: Add support for Wifi band RF mitigations

[Mario Limonciello]

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