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

[Charles Keepax <ckeepax@opensource.cirrus.com>]

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