On 12/19/24 04:27, Charles Keepax wrote:
On Wed, Dec 18, 2024 at 04:40:22PM -0500, Pierre-Louis Bossart wrote:
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.
Ok I can add that to the commit message.
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.
Not sure I follow that, like for example I assume I could just make
the code use the bus_lock instead of adding a lock but to me that
feels like a layer violation, you have a driver directly taking
framework locks. But I guess I don't totally object if everyone
else prefers that.
To be clear, I am not sure if the bus_lock is enough, I was only asking
what the differences are.
In terms of other solutions, I could look at redoing more of the
IRQ handling. If rather than using a hard coded linked list, the
IRQs were properly registered as IRQs with the IRQ framework then
we could simply add and remove them using the normal APIs, which
would be a lot cleaner. Of course, fundamentally that is pretty
equivalent but it will then just use a lock from the IRQ framework.
You would have to review this with Bard, the IRQs for the SoundWire IP
are "interesting", and things started to work reliably only when we
ended-up with with a single IRQ that would then check if every manager
IP reported something. It's not really possible to view SoundWire
interrupts in isolation and deal with the IRQ framework, it's really
best to deal with the PCI interrupt and work from there.
Having looked at the code in more details, I think there are other issues,
see e.g. this part of the code called from snd_bus_master_delete().
static int sdw_delete_slave(struct device *dev, void *data)
{
struct sdw_slave *slave = dev_to_sdw_dev(dev);
struct sdw_bus *bus = slave->bus;
pm_runtime_disable(dev);
sdw_slave_debugfs_exit(slave);
mutex_lock(&bus->bus_lock);
if (slave->dev_num) { /* clear dev_num if assigned */
clear_bit(slave->dev_num, bus->assigned);
if (bus->ops && bus->ops->put_device_num)
bus->ops->put_device_num(bus, slave);
}
So at this point an interaction with the device is not longer possible, even
if the Cadence interrupts are kept active, since there's no valid device
number to use...
list_del_init(&slave->node);
mutex_unlock(&bus->bus_lock);
... but this is where the .remove() will take place.
device_unregister(dev);
return 0;
}
What am I missing?
Hmm... yes that is a good spot, I will investigate that further.
Certainly I do see these operations happening in the wrong order
but it doesn't seem to cause the register transactions to fail.
Most likely it is best to reverse these two, it makes sense
to not clear the device number until we are finished with the
device, but would be good to understand what is going on first.
Yes, I think reversing the two parts should be enough. It makes no sense
to clear a device number since that's not a desired SoundWire operation.
But we absolutely want to release the IDAs and make sure there are no
leaks during bind/unbind tests.
[Index of Archives]
[Pulseaudio]
[Linux Audio Users]
[ALSA Devel]
[Fedora Desktop]
[Fedora SELinux]
[Big List of Linux Books]
[Yosemite News]
[KDE Users]
