Hi,
On 5/25/23 23:01, Maximilian Luz wrote:
> Currently, event completion work-items are restricted to be run strictly
> in non-parallel fashion by the respective workqueue. However, this has
> lead to some problems:
>
> In some instances, the event notifier function called inside this
> completion workqueue takes a non-negligible amount of time to execute.
> One such example is the battery event handling code (surface_battery.c),
> which can result in a full battery information refresh, involving
> further synchronous communication with the EC inside the event handler.
> This is made worse if the communication fails spuriously, generally
> incurring a multi-second timeout.
>
> Since the event completions are run strictly non-parallel, this blocks
> other events from being propagated to the respective subsystems. This
> becomes especially noticeable for keyboard and touchpad input, which
> also funnel their events through this system. Here, users have reported
> occasional multi-second "freezes".
>
> Note, however, that the event handling system was never intended to run
> purely sequentially. Instead, we have one work struct per EC/SAM
> subsystem, processing the event queue for that subsystem. These work
> structs were intended to run in parallel, allowing sequential processing
> of work items for each subsystem but parallel processing of work items
> across subsystems.
>
> The only restriction to this is the way the workqueue is created.
> Therefore, replace create_workqueue() with alloc_workqueue() and do not
> restrict the maximum number of parallel work items to be executed on
> that queue, resolving any cross-subsystem blockage.
>
> Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
> Link: https://github.com/linux-surface/linux-surface/issues/1026
> Signed-off-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>
Thank you for your patch, I've applied this patch to my fixes
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes
Note it will show up in my fixes branch once I've pushed my
local branch there, which might take a while.
I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.
Regards,
Hans
> ---
> drivers/platform/surface/aggregator/controller.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
> index 535581c0471c5..7fc602e01487d 100644
> --- a/drivers/platform/surface/aggregator/controller.c
> +++ b/drivers/platform/surface/aggregator/controller.c
> @@ -825,7 +825,7 @@ static int ssam_cplt_init(struct ssam_cplt *cplt, struct device *dev)
>
> cplt->dev = dev;
>
> - cplt->wq = create_workqueue(SSAM_CPLT_WQ_NAME);
> + cplt->wq = alloc_workqueue(SSAM_CPLT_WQ_NAME, WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
> if (!cplt->wq)
> return -ENOMEM;
>
