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

[Pierre-Louis Bossart]

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@xxxxxxxxxx>
> 
> ---
> 
> Cc: Patrick Lai <quic_plai@xxxxxxxxxxx>
> ---
>  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);