linux-kernel - Re: [PATCH v3 0/5] soundwire: Fixes for spurious and missing UNATTACH

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Sep 2022 16:00:10 +0200
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>, vkoul@...nel.org,
        yung-chuan.liao@...ux.intel.com, sanyog.r.kale@...el.com
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH v3 0/5] soundwire: Fixes for spurious and missing UNATTACH



On 9/14/22 14:09, Richard Fitzgerald wrote:
> The bus and cadence code has several bugs that cause UNATTACH notifications
> to either be sent spuriously or to be missed.
> 
> These can be seen occasionally with a single peripheral on the bus, but are
> much more frequent with multiple peripherals, where several peripherals
> could change state and report in consecutive PINGs.
> 
> The root of all of these bugs seems to be a code design flaw that assumed
> every PING status change would be handled separately. However, PINGs are
> handled by a workqueue function and there is no guarantee when that function
> will be scheduled to run or how much CPU time it will receive. PINGs will
> continue while the work function is handling a snapshot of a previous PING
> so the code must take account that (a) status could change during the
> work function and (b) there can be a backlog of changes before the IRQ work
> function runs again.
> 
> Tested with 4 peripherals on 1 bus, and 8 peripherals on 2 buses.

I added my Reviewed-by tags for the last patches, there's only one typo
which could be dealt with a follow-up patch if that's easier

Thanks again for this contribution, much appreciated.

> CHANGES SINCE V2:
> #4 Add a comment explaining why INTMASK isn't cleared when going around
>    the update_status loop.
> 
> #5 Leave the existing error handling in sdw_program_device_num(),
>    instead of suppressing the error return.
>    Add a comment in sdw_handle_slave_status() explaining why the error
>    is ignored.
>    Re-word the explanation of why sdw_handle_slave_status() must only return
>    early if it programmed a device ID.
> 
> Richard Fitzgerald (4):
>   soundwire: bus: Don't lose unattach notifications
>   soundwire: bus: Don't re-enumerate before status is UNATTACHED
>   soundwire: cadence: Fix lost ATTACHED interrupts when enumerating
>   soundwire: bus: Don't exit early if no device IDs were programmed
> 
> Simon Trimmer (1):
>   soundwire: cadence: fix updating slave status when a bus has multiple
>     peripherals
> 
>  drivers/soundwire/bus.c            | 44 +++++++++++++---
>  drivers/soundwire/cadence_master.c | 80 ++++++++++++++++--------------
>  2 files changed, 80 insertions(+), 44 deletions(-)
>