Message-ID: <e6dd9fc3-de55-47d2-a1b5-10884418a66d@linux.dev>
Date: Tue, 17 Dec 2024 17:49:17 -0600
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.dev>
To: Richard Fitzgerald <rf@...nsource.cirrus.com>,
 Charles Keepax <ckeepax@...nsource.cirrus.com>, vkoul@...nel.org
Cc: 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 12/17/24 4:49 AM, Richard Fitzgerald wrote:
> On 16/12/2024 5:35 pm, 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.
>>
>>> 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.
>>
> For example, if the bus driver module is unloaded, the kernel will call
> remove() on all the child drivers. The bus should remain functional
> while the child drivers deal with this unexpected unload. This could
> for example be writing some registers to put the peripheral into a
> low-power state. On ACPI systems the drivers don't have control of
> regulators so can't just pull power from the peripheral.

Answering to the two replies at once:

If the bus driver is unloaded, then the SoundWire clock will stop toggling. That's a rather large piece of information for the device to change states - I am pretty sure the SDCA spec even mandates that the state changes to at least PS_2.
But to some extent one could argue that a remove() should be more aggressive than a suspend() and the driver could use PS_4 as the lower power state - there is no real requirement to restart interaction with the device with a simple procedure.

The other problem I have with the notion of 'link_lock' is that we already have a notion of 'bus_lock'. And in everything we did so far the terms manager, link and bus are interoperable. So adding a new concept that looks very similar to the existing one shouldn't be done with an explanation of what lock is used for what.

Powered by blists - more mailing lists