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

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

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?

Thanks,
Charles