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

[Richard Patel]

On Fri, Sep 25, 2026 at 04:42:16PM +0100, 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.
> 
> There appear to be two things that currently block leaving the
> IRQs enabled during peripheral removal. Firstly, 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. Secondly, the potential that
> an IRQ runs after the controller itself has been destroyed.
> 
> For the first problem 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. A new lock is
> added for this, the shim_lock is used to gate access to the shared
> registers so doesn't feel a super obvious fit for managing the list.
> 
> For the second problem utilise the newly added helper that allows
> destroying the peripherals separately, allowing the auxiliary device
> to disable IRQs before running its own cleanup.

I ran into a use-after-free on Samsung Galaxy Book6 (Panther Lake)
the other day.  I was going to send a patch adding RCU, then I saw
your patch already added a mutex:

  sof-audio-pci-intel-ptl 0000:00:1f.3: SOF firmware and/or topology file not found.
  Oops: general protection fault, kernel NULL pointer dereference 0x3c0: 0000 [#1] SMP NOPTI
  RIP: 0010:sdw_cdns_irq+0x9/0x1f0 [soundwire_cadence]
  Call Trace:
   sdw_intel_thread+0x2d/0x50 [soundwire_intel]
   hda_dsp_interrupt_thread+0x97/0x320 [snd_sof_intel_hda_generic]
   irq_thread_fn+0x23/0x60
   irq_thread+0xc7/0x190

I would add 'Fixes: 4a98a6b2fa75 ("soundwire: intel/cadence: merge Soundwire interrupt handlers/threads")' maybe

> @@ -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;
>  }

I don't understand the code very well, but isn't there a second UAF
with the ctx object getting freed?  kfree(ctx) in sdw_intel_exit()
runs well before the IRQ is unregistered.

  -- Richard