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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

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.