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

[Mario Limonciello <mario.limonciello@amd.com>]

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