Re: [PATCH 2/2] soundwire: intel_auxdevice: Don't disable IRQs before removing children

[Pierre-Louis Bossart]

On 9/11/26 18:19, Charles Keepax wrote:
> Currently the auxiliary device for the link disables IRQs before
> it calls sdw_bus_master_delete(). This has the side effect that
> none of the devices on the link can access their own registers
> whilst their remove functions run, because the IRQs are required
> for bus transactions to function.
> 
> It would appear the reason for the disabling of the IRQs is that
> the IRQ handler iterates through a linked list of all the links,
> once a link is removed the memory pointed at by this linked list
> is freed, but not removed from the linked_list. 

That wasn't the reason, even if you have a single link we all thought it
made more sense to disable peripheral interrupts on the host before
calling sdw_bus_master_delete()

> Add a list_del()> for the linked list item, note whilst the list
itself is contained
> in the intel_init portion of the code, the list remove needs
> to be attached to the auxiliary device for the link, since
> that owns the memory that the list points at. Locking is also
> required to ensure the IRQ handler runs either before or after
> any additions/removals from the list.
> 
> Signed-off-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/soundwire/intel.h           |  1 +
>  drivers/soundwire/intel_auxdevice.c |  5 ++++-
>  drivers/soundwire/intel_init.c      | 16 ++++++++++++++++
>  include/linux/soundwire/sdw_intel.h |  1 +
>  4 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> index 7a2e7e73ad632..ec82ac8d54adb 100644
> --- a/drivers/soundwire/intel.h
> +++ b/drivers/soundwire/intel.h
> @@ -47,6 +47,7 @@ struct sdw_intel_link_res {
>  	u32 link_mask;
>  	struct sdw_cdns *cdns;
>  	struct list_head list;
> +	struct mutex *link_lock; /* lock protecting list */
>  	struct hdac_bus *hbus;
>  };
>  
> diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
> index 901a71262094f..1743793de2bd0 100644
> --- a/drivers/soundwire/intel_auxdevice.c
> +++ b/drivers/soundwire/intel_auxdevice.c
> @@ -508,9 +508,12 @@ static void intel_link_remove(struct auxiliary_device *auxdev)
>  	if (!bus->prop.hw_disabled) {
>  		sdw_intel_debugfs_exit(sdw);
>  		cancel_delayed_work_sync(&cdns->attach_dwork);
> -		sdw_cdns_enable_interrupt(cdns, false);
>  	}
> +
>  	sdw_bus_master_delete(bus);
> +
> +	if (!bus->prop.hw_disabled)
> +		sdw_cdns_enable_interrupt(cdns, false);
>  }

Sorry, that sequence looks really weird to me.

See the code in

void sdw_bus_master_delete(struct sdw_bus *bus)
{
	device_for_each_child(bus->dev, NULL, sdw_delete_slave);

	sdw_irq_delete(bus);

	sdw_master_device_del(bus);

After doing all this, one would mask the interrupts on the host side with


	if (!bus->prop.hw_disabled)
		sdw_cdns_enable_interrupt(cdns, false);

but that host is long gone.

Does this even work?

The last sdw_cdns_enable_interrupt(cdns, false) looks either very racy
or useless, no?
>  
>  int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
> diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
> index ad48d67fa9358..117c2e42b9bac 100644
> --- a/drivers/soundwire/intel_init.c
> +++ b/drivers/soundwire/intel_init.c
> @@ -28,6 +28,15 @@ static void intel_link_dev_release(struct device *dev)
>  	kfree(ldev);
>  }
>  
> +static void intel_link_list_del(void *data)
> +{
> +	struct sdw_intel_link_res *link = data;
> +
> +	mutex_lock(link->link_lock);
> +	list_del(&link->list);
> +	mutex_unlock(link->link_lock);
> +}
> +
>  /* alloc, init and add link devices */
>  static struct sdw_intel_link_dev *intel_link_dev_register(struct sdw_intel_res *res,
>  							  struct sdw_intel_ctx *ctx,
> @@ -79,6 +88,7 @@ static struct sdw_intel_link_dev *intel_link_dev_register(struct sdw_intel_res *
>  		link->shim_lock = res->eml_lock;
>  		link->mic_privacy = res->mic_privacy;
>  	}
> +	link->link_lock = &ctx->link_lock;
>  
>  	link->ops = res->ops;
>  	link->dev = res->dev;
> @@ -145,8 +155,10 @@ irqreturn_t sdw_intel_thread(int irq, void *dev_id)
>  	struct sdw_intel_ctx *ctx = dev_id;
>  	struct sdw_intel_link_res *link;
>  
> +	mutex_lock(&ctx->link_lock);
>  	list_for_each_entry(link, &ctx->link_list, list)
>  		sdw_cdns_irq(irq, link->cdns);
> +	mutex_unlock(&ctx->link_lock);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -210,6 +222,7 @@ static struct sdw_intel_ctx
>  	ctx->link_mask = res->link_mask;
>  	ctx->handle = res->handle;
>  	mutex_init(&ctx->shim_lock);
> +	mutex_init(&ctx->link_lock);
>  
>  	link_mask = ctx->link_mask;
>  
> @@ -246,7 +259,10 @@ static struct sdw_intel_ctx
>  			i++;
>  			goto err;
>  		}
> +		mutex_lock(&ctx->link_lock);
>  		list_add_tail(&link->list, &ctx->link_list);
> +		mutex_unlock(&ctx->link_lock);
> +		devm_add_action_or_reset(&ldev->auxdev.dev, intel_link_list_del, link);
>  		bus = &link->cdns->bus;
>  		/* Calculate number of slaves */
>  		list_for_each(node, &bus->slaves)
> diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h
> index 9710f2dc04e29..7495d35ed2fc3 100644
> --- a/include/linux/soundwire/sdw_intel.h
> +++ b/include/linux/soundwire/sdw_intel.h
> @@ -307,6 +307,7 @@ struct sdw_intel_ctx {
>  	acpi_handle handle;
>  	struct sdw_intel_link_dev **ldev;
>  	struct list_head link_list;
> +	struct mutex link_lock; /* lock protecting link_list */
>  	struct mutex shim_lock; /* lock for access to shared SHIM registers */

IIRC shim_lock was used to prevent access to common registers shared
between links. How many locks do we need?

>  	u32 shim_mask;
>  	u32 shim_base;