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