[PATCH 0/2] Allow SoundWire devices to communicate during remove

[PATCH 0/2] Allow SoundWire devices to communicate during remove

[Charles Keepax]

Currently on Intel systems SoundWire drivers can't communicate with the
device during driver removal. This is primarily because the IRQs are
disabled before the driver remove callback is run. The result of this is
such transactions timeout causing a) a lot of errors in the log and b)
driver remove to take a very long time.

This issue affects cs42l43 and cs42l45, primarily due to both using
regmap IRQ.

 soundwire_intel soundwire_intel.link.0: IO transfer timed out, cmd 3 device 6 addr 5d len 1
 soundwire sdw-master-0-0: trf on Slave 6 failed:-110 write addr 5d count 0
 sdca_class sdw:0:0:01fa:4245:01: Failed to sync masks in 5d

As regmap IRQ is torn down it will mask the interrupts that are
removed. However, there are many valid reasons a driver might want
communicate with the device during removal, others would include
disabling jack detection, putting the device into the lowest possible
power state to save power, etc.

This patch set attempts to address this problem trying to locate the
reason interrupts are disabled, fixing that and then leaving the IRQs
enabled for the remove callback.

Charles Keepax (2):
  soundwire: bus: Don't unassign dev_num before unregistering device
  soundwire: intel_auxdevice: Don't disable IRQs before removing
    children

 drivers/soundwire/bus.c             |  4 ++--
 drivers/soundwire/intel.h           |  1 +
 drivers/soundwire/intel_auxdevice.c |  5 ++++-
 drivers/soundwire/intel_init.c      | 16 ++++++++++++++++
 include/linux/soundwire/sdw_intel.h |  1 +
 5 files changed, 24 insertions(+), 3 deletions(-)

-- 
2.47.3

[PATCH 1/2] soundwire: bus: Don't unassign dev_num before unregistering device

[Charles Keepax]

Don't mark dev_num as unassigned until after device_unregister()
has been called. The driver may want to communicate with the
device as part of the driver remove operation, so the dev_num
should remain assigned until that has completed.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index aeaae5a57c89d..b17718f4277ed 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -175,8 +175,9 @@ static int sdw_delete_slave(struct device *dev, void *data)
 
 	sdw_slave_debugfs_exit(slave);
 
-	mutex_lock(&bus->bus_lock);
+	device_unregister(dev);
 
+	mutex_lock(&bus->bus_lock);
 	if (slave->dev_num) { /* clear dev_num if assigned */
 		clear_bit(slave->dev_num, bus->assigned);
 		if (bus->ops && bus->ops->put_device_num)
@@ -185,7 +186,6 @@ static int sdw_delete_slave(struct device *dev, void *data)
 	list_del_init(&slave->node);
 	mutex_unlock(&bus->bus_lock);
 
-	device_unregister(dev);
 	return 0;
 }
 
-- 
2.47.3

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

[Charles Keepax]

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. 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@opensource.cirrus.com>
---
 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);
 }
 
 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 */
 	u32 shim_mask;
 	u32 shim_base;
-- 
2.47.3

Re: [PATCH 1/2] soundwire: bus: Don't unassign dev_num before unregistering device

[Pierre-Louis Bossart]

On 9/11/26 18:19, Charles Keepax wrote:
> Don't mark dev_num as unassigned until after device_unregister()
> has been called. The driver may want to communicate with the
> device as part of the driver remove operation, so the dev_num
> should remain assigned until that has completed.

That change looks fine.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>

But I am a bit worried about the dev_num handling now that I think of
it. This is *only* at the manager level. At the peripheral level, the
dev_num register has not been modified so the hardware still reports as
ATTACHED with the dev_num whose mask was cleared on the host side.
IOW we have a zombie peripheral on the link, and if we do a sequence
rmmod/insmod things could be fun...
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  drivers/soundwire/bus.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index aeaae5a57c89d..b17718f4277ed 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -175,8 +175,9 @@ static int sdw_delete_slave(struct device *dev, void *data)
>  
>  	sdw_slave_debugfs_exit(slave);
>  
> -	mutex_lock(&bus->bus_lock);
> +	device_unregister(dev);
>  
> +	mutex_lock(&bus->bus_lock);
>  	if (slave->dev_num) { /* clear dev_num if assigned */
>  		clear_bit(slave->dev_num, bus->assigned);
>  		if (bus->ops && bus->ops->put_device_num)
> @@ -185,7 +186,6 @@ static int sdw_delete_slave(struct device *dev, void *data)
>  	list_del_init(&slave->node);
>  	mutex_unlock(&bus->bus_lock);
>  
> -	device_unregister(dev);
>  	return 0;
>  }
>  

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@opensource.cirrus.com>
> ---
>  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;

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

[Charles Keepax]

On Mon, Sep 14, 2026 at 08:27:23PM +0200, Pierre-Louis Bossart wrote:
> 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()

What was the reason for deciding to do that, that is not in itself
a reason? A drivers remove callback should be able to access
registers on the device. The host here requires interrupts to
do so. This was the only functional reason in the code I could
find that things were done in this order.

> > --- 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?

I mean it definitely works and fixes the problems on driver
remove. I will check to see if the call is redundant at this
stage, or if there are any potential dangers I am missing.

> > 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?

I mean we could reuse the lock but in my experience locks having
a clearly defined purpose is less error prone, than having catch
all locks. The shim lock claims to protect the SHIM registers,
this one protects the list of links. But if you feel strongly I
am happy to try reuse the shim lock for this?

Ultimately, I am not super attached to this way of solving the
problem but we do need to come up with some solution to allow
drivers to access their device in driver remove. This causes
devices to take 1-2 minutes to remove the driver and fills the log
with loads of error messages. I am more than happy to entertain
other ways of making that happen if you have ideas you prefer?

Thanks,
Charles

Re: [PATCH 1/2] soundwire: bus: Don't unassign dev_num before unregistering device

[Charles Keepax]

On Mon, Sep 14, 2026 at 08:21:34PM +0200, Pierre-Louis Bossart wrote:
> On 9/11/26 18:19, Charles Keepax wrote:
> > Don't mark dev_num as unassigned until after device_unregister()
> > has been called. The driver may want to communicate with the
> > device as part of the driver remove operation, so the dev_num
> > should remain assigned until that has completed.
> 
> That change looks fine.
> 
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>
> 
> But I am a bit worried about the dev_num handling now that I think of
> it. This is *only* at the manager level. At the peripheral level, the
> dev_num register has not been modified so the hardware still reports as
> ATTACHED with the dev_num whose mask was cleared on the host side.
> IOW we have a zombie peripheral on the link, and if we do a sequence
> rmmod/insmod things could be fun...

Hmm... yeah would probably be nicer to update the slave too. I
will think that through and see if I can find time to have a look
at doing an extra patch for it.

Thanks,
Charles

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

[Charles Keepax]

On Tue, Sep 15, 2026 at 10:13:28AM +0100, Charles Keepax wrote:
> On Mon, Sep 14, 2026 at 08:27:23PM +0200, Pierre-Louis Bossart wrote:
> > On 9/11/26 18:19, Charles Keepax wrote:
> > 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?
> 
> I mean it definitely works and fixes the problems on driver
> remove. I will check to see if the call is redundant at this
> stage, or if there are any potential dangers I am missing.

Hmm... ok so the call definitely isn't redundant nothing
else masks all the IRQs. However, I think you might be onto
something with the race, I think if we get a slave interrupt
in this window it could cause problems (queues cdns->work which
then uses things from the bus). I think we might need to export
cdns_enable_slave_interrupt() so we can separately disable the
slave interrupts whilst leaving the other interrupts functional.

Thanks,
Charles

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

[Pierre-Louis Bossart]

>>> 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()
> 
> What was the reason for deciding to do that, that is not in itself
> a reason? A drivers remove callback should be able to access
> registers on the device. The host here requires interrupts to
> do so. This was the only functional reason in the code I could
> find that things were done in this order.

like I said it was thought to be simpler this way, we never thought
about the need to do anything in remove().
IOW this is a missed requirement that led to a simple solution, not a
deliberate decision to cripple the remove() step.

>>> --- 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?
> 
> I mean it definitely works and fixes the problems on driver
> remove. I will check to see if the call is redundant at this
> stage, or if there are any potential dangers I am missing.

ok

>>> 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?
> 
> I mean we could reuse the lock but in my experience locks having
> a clearly defined purpose is less error prone, than having catch
> all locks. The shim lock claims to protect the SHIM registers,
> this one protects the list of links. But if you feel strongly I
> am happy to try reuse the shim lock for this?

I am not asking for you to share, just that we need to have a discussion
on whether this new lock is redundant or not, and wording that describes
why using existing locks isn't desired/needed.

this shim_lock is a protection for very low-level access to registers,
and completely tied to the way the IP registers were clustered together,
it's indeed a different conceptual level you're after. So even if there
is some redundancy or overlap, it's probably simpler to use a different
lock for different things,

> Ultimately, I am not super attached to this way of solving the
> problem but we do need to come up with some solution to allow
> drivers to access their device in driver remove. This causes
> devices to take 1-2 minutes to remove the driver and fills the log
> with loads of error messages. I am more than happy to entertain
> other ways of making that happen if you have ideas you prefer?

I don't have any objections with the requirement to let the codec do
whatever it needs on remove(), this was a miss in the design and it
needs to be addressed.
I am just nervous about the interrupt part and possible race conditions.
Maybe the last stage of the remove() should be to mask all interrupts on
the codec side with a common helper shared by all codec drivers?

BTW we did have scripts to remove/insert in loops to try and find
issues, not sure if you're aware of them and if they still work, see:

https://github.com/thesofproject/sof-test/blob/main/tools/kmod/

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

[Charles Keepax]

On Tue, Sep 15, 2026 at 02:22:03PM +0200, Pierre-Louis Bossart wrote:
> 
> >>> 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()
> > 
> > What was the reason for deciding to do that, that is not in itself
> > a reason? A drivers remove callback should be able to access
> > registers on the device. The host here requires interrupts to
> > do so. This was the only functional reason in the code I could
> > find that things were done in this order.
> 
> like I said it was thought to be simpler this way, we never thought
> about the need to do anything in remove().
> IOW this is a missed requirement that led to a simple solution, not a
> deliberate decision to cripple the remove() step.

Ok I will update to reflect this being more what we need to
update to add this and remove the apparent reason wording.

> >>> --- 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?
> > 
> > I mean we could reuse the lock but in my experience locks having
> > a clearly defined purpose is less error prone, than having catch
> > all locks. The shim lock claims to protect the SHIM registers,
> > this one protects the list of links. But if you feel strongly I
> > am happy to try reuse the shim lock for this?
> 
> I am not asking for you to share, just that we need to have a discussion
> on whether this new lock is redundant or not, and wording that describes
> why using existing locks isn't desired/needed.
> 
> this shim_lock is a protection for very low-level access to registers,
> and completely tied to the way the IP registers were clustered together,
> it's indeed a different conceptual level you're after. So even if there
> is some redundancy or overlap, it's probably simpler to use a different
> lock for different things,

So a quick test appears to suggest using the shim_lock would
work. The original description is mostly focused on the low level
registers, which does feel like a different purpose. That said
though it does basically work the same its a lock linked to the
controller and the initial description does make reference to
protecting the shim_mask field as well so I guess it is already
protecting some state in the same struct.

Happy to go with you and Intel's preferred options here.

> 
> > Ultimately, I am not super attached to this way of solving the
> > problem but we do need to come up with some solution to allow
> > drivers to access their device in driver remove. This causes
> > devices to take 1-2 minutes to remove the driver and fills the log
> > with loads of error messages. I am more than happy to entertain
> > other ways of making that happen if you have ideas you prefer?
> 
> I don't have any objections with the requirement to let the codec do
> whatever it needs on remove(), this was a miss in the design and it
> needs to be addressed.
> I am just nervous about the interrupt part and possible race conditions.
> Maybe the last stage of the remove() should be to mask all interrupts on
> the codec side with a common helper shared by all codec drivers?

Yeah that is fair, it is a major reshuffling. But we can carry
on that discussion on the other branch.

> BTW we did have scripts to remove/insert in loops to try and find
> issues, not sure if you're aware of them and if they still work, see:
> 
> https://github.com/thesofproject/sof-test/blob/main/tools/kmod/

Yeah I believe that causing problems is one of the main things
people have been complaining about.

Thanks,
Charles