[AMD Official Use Only - General]
Personally I would like to treat the wbrf core as a water pool. Any stream can flow in. Also any needed can drain water from it at any time.
The way to allow producers to report only when there is consumer existing does not work. Since the consumer might come after the producer.
Just considering the scenario below:
Wifi core/driver started --> wifi driver reports its frequency in-use --> proper action taken by wbrf core --> amdgpu driver(consumer) started
What should be the proper action taken by wbrf core then? Stop the producer to report its frequency in-use? That might lead consumer to never have a chance to know that.
The wbrf_supported_producer and wbrf_supported_consumer APIs seem unnecessary for the generic implementation.
But to support AMD ACPI implementation(or future device tree implementation), they are needed. The wbrf core needs to check whether the necessary AML codes are there.
It needs those information to judge whether a producer can report(will be accepted) or a consumer can retrieve needed information.
> > +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.
This is to align with the AMD ACPI implementation.
But I can make this AMD ACPI specific and bring a more generic version here.
Evan
> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Thursday, July 13, 2023 7:12 AM
> To: Quan, Evan <Evan.Quan@xxxxxxx>
> Cc: rafael@xxxxxxxxxx; lenb@xxxxxxxxxx; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>; Pan, Xinhui <Xinhui.Pan@xxxxxxx>;
> airlied@xxxxxxxxx; daniel@xxxxxxxx; johannes@xxxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; Limonciello, Mario <Mario.Limonciello@xxxxxxx>;
> mdaenzer@xxxxxxxxxx; maarten.lankhorst@xxxxxxxxxxxxxxx;
> tzimmermann@xxxxxxx; hdegoede@xxxxxxxxxx; jingyuwang_vip@xxxxxxx;
> Lazar, Lijo <Lijo.Lazar@xxxxxxx>; jim.cromie@xxxxxxxxx;
> bellosilicio@xxxxxxxxx; andrealmeid@xxxxxxxxxx; trix@xxxxxxxxxx;
> jsg@xxxxxxxxx; arnd@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> acpi@xxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx
> Subject: 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(&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
