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

[Mario Limonciello]

On 7/5/23 21:58, Quan, Evan wrote:
> [AMD Official Use Only - General]
>
> Hi Andrew,
>
> I discussed with Mario about your proposal/concerns here.
> We believe some changes below might address your concerns.
> - place/move the wbrf_supported_producer check inside acpi_amd_wbrf_add_exclusion and acpi_amd_wbrf_add_exclusion
> - place the wbrf_supported_consumer check inside acpi_amd_wbrf_retrieve_exclusions
> So that the wbrf_supported_producer and wbrf_supported_consumer can be dropped.
> We made some prototypes and even performed some tests which showed technically it is absolutely practicable.
>
> However, we found several issues with that.
> - The overhead caused by the extra _producer/_consumer check on every calling of wbrf_add/remove/retrieve_ecxclusion.
>    Especially when you consider there might be multiple producers and consumers in the system at the same time. And some of
>    them might do in-use band/frequency switching frequently.

One more piece of overhead that is in this same theme that Evan didn't 
mention is the case of a system "without" AMD's ACPI WBRF but the kernel 
was configured with it enabled.  Think like a distro kernel.

Moving it into add/remove exclusion would mean that every single time 
frequency changed by a producer the _DSM would attempt to be evaluated 
and fail.  To avoid that extra call overhead after the first time would 
mean needing to keep a variable somewhere, and at that point what did 
you save?

> - Some extra costs caused by the "know it only at the last minute". For example, to support WBRF, amdgpu driver needs some preparations: install the notification hander,
>    setup the delay workqueue(to handle possible events flooding) and even notify firmware engine to be ready. However, only on the 1st notification receiving,
>    it is realized(reported by wbrf_supported_consumer check) the WBRF feature is actually not supported. All those extra costs can be actually avoided if we can know the WBRF is not supported at first.
>    This could happen to other consumers and producers too.
>
> After a careful consideration, we think the changes do not benefit us much. It does not deserve us to spend extra efforts.
> Thus we would like to stick with original implementations. That is to have wbrf_supported_producer and wbrf_supported_consumer interfaces exposed.
> Then other drivers/subsystems can do necessary wbrf support check in advance and coordinate their actions accordingly.
> Please let us know your thoughts.
>
> BR,
> Evan
> > -----Original Message-----
> > From: Andrew Lunn <andrew@xxxxxxx>
> > Sent: Tuesday, July 4, 2023 9:07 PM
> > 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 V5 1/9] drivers core: Add support for Wifi band RF
> > mitigations
> >
> > > > What is the purpose of this stage? Why would it not be supported for
> > > > this device?
> > > This is needed for wbrf support via ACPI mechanism. If BIOS(AML code)
> > > does not support the wbrf adding/removing for some device, it should
> > speak that out so that the device can be aware of that.
> >
> > How much overhead is this adding? How deep do you need to go to find the
> > BIOS does not support it? And how often is this called?
> >
> > Where do we want to add complexity? In the generic API? Or maybe a little
> > deeper in the ACPI specific code?
> >
> >         Andrew