linux-kernel - Re: [PATCH] soundwire: intel_auxdevice: Don't disable IRQs before removing children

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z2FRYfHoHX4Dv+C2@opensource.cirrus.com>
Date: Tue, 17 Dec 2024 10:24:33 +0000
From: Charles Keepax <ckeepax@...nsource.cirrus.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.dev>
CC: <vkoul@...nel.org>, <peter.ujfalusi@...ux.intel.com>,
        <yung-chuan.liao@...ux.intel.com>, <sanyog.r.kale@...el.com>,
        <linux-sound@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <patches@...nsource.cirrus.com>
Subject: Re: [PATCH] soundwire: intel_auxdevice: Don't disable IRQs before
 removing children

On Mon, Dec 16, 2024 at 11:35:23AM -0600, Pierre-Louis Bossart wrote:
> On 12/12/24 5:02 AM, 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. Obviously, devices should be able to access
> > their own registers during disable to park the device suitably.
> 
> What does 'park the device suitably' mean really? That sounds
> scary. What happens if the manager abruptly ceases to operate and
> yanks the power? Does the device catch on fire? On a related note,
> the manager should *never* expect to find the device in a 'suitable'
> state but always proceed with complete re-initialization.
> 
> It would be good to expand on the issue, introducing new locks
> is one of those "no good deed goes unpunished" situations.
> 

I would agree that one should never make hardware that needs parked
to avoid damage, but people do stupid things. Also, it doesn't
have to be as catastrophic as that, it is usually a case of wanting
to move the device into its lowest power state, turn off regulators
on the device etc.

> > 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 itself. 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
> > before or after any additions/removals from the list.
> 
> that sounds very detailed but the initial reason for all this is still
> unclear.

If you want, I can add the exact reason I am adding this change to
the commit message, which is that regmap IRQ sensibly masks IRQs
as they are removed, so when the cs42l43 removes one sees a bunch
of failed transations, as the bus has broken. But to be honest I
feel like it is overly specific, one could construct any number of
similar situations, the real problem here is it is completely normal
and reasonable to want to communicate with a device in remove and
we should support that.

Thanks,
Charles