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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z2KbELGGjeH3NQdY@opensource.cirrus.com>
Date: Wed, 18 Dec 2024 09:51:12 +0000
From: Charles Keepax <ckeepax@...nsource.cirrus.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.dev>
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 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