Re: [PATCH 5/6] soudnwire: master: protect concurrecnt check for bus->md

Pierre-Louis Bossart
2023-04-20T17:15:55Z

typos in commit title...

On 4/20/23 05:16, Krzysztof Kozlowski wrote:
> The Soundwire master controllers might want to check for bus->md

Apologies for being pedantic but 'manager' and 'controller' are
different concepts in SoundWire, see DisCo spec.
It's not a 1:1 mapping, a controller can rely on M managers

> initialization to avoid race between early interrupt and finish of
> sdw_bus_master_add()/sdw_master_device_add(). Such early interrupt can
> happen if Soundwire devices are not powered off during their probe.
>
> Add a store release barrier, so the Soundwire controllers can safely
> check it in concurrent (e.g. in interrupt) way.

Can you elaborate on the race condition? I am not following what breaks,
and what entity generates the 'early interrupt'.

I am specifically concerned about adding this in common code without any
matching smp_load_acquire() - which is only added in the following patch
for the Qualcomm manager only, but not added for Intel/AMD managers. Is
this not a problem?

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Cc: Patrick Lai <quic_plai@quicinc.com>
> ---
> drivers/soundwire/master.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
> index 9b05c9e25ebe..d5bf13e7e602 100644
> --- a/drivers/soundwire/master.c
> +++ b/drivers/soundwire/master.c
> @@ -161,7 +161,12 @@ int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
> /* add shortcuts to improve code readability/compactness */
> md->bus = bus;
> bus->dev = &md->dev;
> - bus->md = md;
> + /*
> + * Make sure the contents of md is stored before storing bus->md.
> + * Paired with new slave attached and slave status interrupts
> + * on the Soundwire master side.
> + */
> + smp_store_release(&bus->md, md);
>
> pm_runtime_set_autosuspend_delay(&bus->md->dev, SDW_MASTER_SUSPEND_DELAY_MS);
> pm_runtime_use_autosuspend(&bus->md->dev);