Message-ID: <7479a25e-6729-4aa0-b67e-7781bf8232da@linux.dev>
Date: Wed, 18 Dec 2024 15:45:35 -0500
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.dev>
To: Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc: Richard Fitzgerald <rf@...nsource.cirrus.com>, 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 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.

Powered by blists - more mailing lists