On 23/03/2022 19:45, Pierre-Louis Bossart wrote:
Hi,
I could use feedback/guidance on a possible conceptual bug in the
SoundWire probe and bus handling.
When we probe a driver, the code does this:
static int sdw_drv_probe(struct device *dev)
{
struct sdw_slave *slave = dev_to_sdw_dev(dev);
struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
const struct sdw_device_id *id;
const char *name;
int ret;
/*
* fw description is mandatory to bind
*/
if (!dev->fwnode)
return -ENODEV;
if (!IS_ENABLED(CONFIG_ACPI) && !dev->of_node)
return -ENODEV;
id = sdw_get_device_id(slave, drv);
if (!id)
return -ENODEV;
slave->ops = drv->ops;
may be add try_module_get(dev->driver->owner) here, which should prevent
the module from unloading in the first place.
--srini
The last line is the problematic one. If at some point, the user does an
rmmod and unbinds the SoundWire codec driver, the .remove will be called
and the 'drv' will no longer be valid, but we will still have a
reference to drv->ops and use that pointer in the bus code, e.g.
/* Update the Slave driver */
if (slave_notify && slave->ops &&
slave->ops->interrupt_callback) {
slave_intr.sdca_cascade = sdca_cascade;
slave_intr.control_port = clear;
memcpy(slave_intr.port, &port_status,
sizeof(slave_intr.port));
slave->ops->interrupt_callback(slave,
&slave_intr);
}
I noodled with a potential fix in
https://github.com/thesofproject/linux/pull/3534/commits/82d64fb0fd39b532263f060a8ec86e47e9ab305b
where I force-reset this slave->ops pointer, but it is likely to be very
racy.
We probably need to avoid such references, or have a clean mechanism to
unbind, e.g. with all commands and interrupts stopped while the codec
driver .remove routine is handled.
Initial error reports at https://github.com/thesofproject/linux/issues/3531
Suggestions and comments welcome, thanks!
-Pierre
