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

From: Pierre-Louis Bossart
Date: Wed Dec 18 2024 - 15:51:45 EST



On 12/18/24 4:51 AM, Charles Keepax wrote:
On Tue, Dec 17, 2024 at 05:49:17PM -0600, Pierre-Louis Bossart wrote:
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:
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 -

The clock should only stop toggling after the drivers have been
removed, anything else is a bug.

I am pretty sure the SDCA spec even mandates that
the state changes to at least PS_2.

This code applies to more than just SDCA devices.

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.


Not really sure I follow this bit, none of this has anything to do
with when one restarts interacting with the device. It is about
leaving the device in a nice state when you stop interacting with
it.

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.

I don't see much confusion here, the two locks are at different
levels in the stack. If is fairly normal for a framework to have
a lock and drivers to have individual locks under that. And the
comment with the lock states it is protecting the list.

That said I am not attached to this way of solving the problem
either, all I am attached to is allowing devices to communicate in
their remove functions. I think perhaps the important questions
here are do you object to my assertion that a device should be
able to communicate in its remove function? Or do you object to
the way I have solved that problem? I am certainly open to other
solutions, if you have any suggestions?

I agree that the device should be reachable during the remove(), but I believe the scope of expected interaction should be limited to a strict minimum. To be clearer, so far not a single device had a requirement for any sort of interaction on remove. You would need to clarify which codec driver needs this.

I don't see how the 'link_lock' and 'bus_lock' are at different levels of the stack, the 'master' device and the 'auxiliary' device are both quite thin and I don't quite see what's different between the two.