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

[Richard Fitzgerald <rf@opensource.cirrus.com>]

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.