Skip to content

Commit

Permalink
soundwire: use driver callbacks directly with proper locking
Browse files Browse the repository at this point in the history 
In the SoundWire probe, we store a pointer from the driver ops into
the 'slave' structure. This can lead to kernel oopses when unbinding
codec drivers, e.g. with the following sequence to remove machine
driver and codec driver.

/sbin/modprobe -r snd_soc_sof_sdw
/sbin/modprobe -r snd_soc_rt711

The full details can be found in the BugLink below, for reference the
two following examples show different cases of driver ops/callbacks
being invoked after the driver .remove().

kernel: BUG: kernel NULL pointer dereference, address: 0000000000000150
kernel: Workqueue: events cdns_update_slave_status_work [soundwire_cadence]
kernel: RIP: 0010:mutex_lock+0x19/0x30
kernel: Call Trace:
kernel:  ? sdw_handle_slave_status+0x426/0xe00 [soundwire_bus 94ff184bf398570c3f8ff7efe9e32529f532e4ae]
kernel:  ? newidle_balance+0x26a/0x400
kernel:  ? cdns_update_slave_status_work+0x1e9/0x200 [soundwire_cadence 1bcf98eebe5ba9833cd433323769ac923c9c6f82]

kernel: BUG: unable to handle page fault for address: ffffffffc07654c8
kernel: Workqueue: pm pm_runtime_work
kernel: RIP: 0010:sdw_bus_prep_clk_stop+0x6f/0x160 [soundwire_bus]
kernel: Call Trace:
kernel:  <TASK>
kernel:  sdw_cdns_clock_stop+0xb5/0x1b0 [soundwire_cadence 1bcf98eebe5ba9833cd433323769ac923c9c6f82]
kernel:  intel_suspend_runtime+0x5f/0x120 [soundwire_intel aca858f7c87048d3152a4a41bb68abb9b663a1dd]
kernel:  ? dpm_sysfs_remove+0x60/0x60

This was not detected earlier in Intel tests since the tests first
remove the parent PCI device and shut down the bus. The sequence
above is a corner case which keeps the bus operational but without a
driver bound.

This patch removes the use the 'slave' ops and uses proper locking to
make sure there are no live callbacks based on a dangling pointer
executed during or after the driver unbinding sequence. In one
specific case, indicated with comments in the code, the device lock is
already taken at a higher level when starting the resume operations.

The issue with the ops pointer has been there since December 2017, but
there were so many changes in the bus handling code that this patch
will not apply cleanly all the way to this initial commit. The changes
can be easily backported though.

Thanks to Dan Williams for his suggestions on an earlier version of
this patch.

BugLink: thesofproject#3531
Fixes: 56d4fe3 ("soundwire: Add MIPI DisCo property helpers")
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
  • Loading branch information
@plbossart
plbossart committed Apr 11, 2022
1 parent d87f8ed commit ee192d0
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 39 deletions.
78 changes: 62 additions & 16 deletions drivers/soundwire/bus.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <linux/pm_runtime.h>
#include <linux/soundwire/sdw_registers.h>
#include <linux/soundwire/sdw.h>
#include <linux/soundwire/sdw_type.h>
#include "bus.h"
#include "sysfs_local.h"

Expand Down Expand Up @@ -846,12 +847,19 @@ static int sdw_slave_clk_stop_callback(struct sdw_slave *slave,
enum sdw_clk_stop_mode mode,
enum sdw_clk_stop_type type)
{
int ret;
struct device *dev = &slave->dev;
struct sdw_driver *drv;

if (slave->ops && slave->ops->clk_stop) {
ret = slave->ops->clk_stop(slave, mode, type);
if (ret < 0)
return ret;
/*
* this function can only be called from a pm_runtime
* sequence where the device is already locked
*/
device_lock_assert(dev);

if (dev->driver) {
drv = drv_to_sdw_driver(dev->driver);
if (drv->ops && drv->ops->clk_stop)
return drv->ops->clk_stop(slave, mode, type);
}

return 0;
Expand Down Expand Up @@ -1616,14 +1624,25 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
}

/* 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);
if (slave_notify) {
struct device *dev = &slave->dev;
struct sdw_driver *drv;

device_lock(dev);

if (dev->driver) {
drv = drv_to_sdw_driver(dev->driver);
if (drv->ops && drv->ops->interrupt_callback) {
slave_intr.sdca_cascade = sdca_cascade;
slave_intr.control_port = clear;
memcpy(slave_intr.port, &port_status,
sizeof(slave_intr.port));

drv->ops->interrupt_callback(slave, &slave_intr);
}
}

device_unlock(dev);
}

/* Ack interrupt */
Expand Down Expand Up @@ -1697,8 +1716,12 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
static int sdw_update_slave_status(struct sdw_slave *slave,
enum sdw_slave_status status)
{
struct device *dev = &slave->dev;
struct sdw_driver *drv;
unsigned long time;

device_lock_assert(dev);

if (!slave->probed) {
/*
* the slave status update is typically handled in an
Expand All @@ -1716,10 +1739,13 @@ static int sdw_update_slave_status(struct sdw_slave *slave,
}
}

if (!slave->ops || !slave->ops->update_status)
return 0;
if (dev->driver) {
drv = drv_to_sdw_driver(dev->driver);
if (drv->ops && drv->ops->update_status)
return drv->ops->update_status(slave, status);
}

return slave->ops->update_status(slave, status);
return 0;
}

/**
Expand Down Expand Up @@ -1828,7 +1854,10 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
break;
}

device_lock(&slave->dev);
ret = sdw_update_slave_status(slave, status[i]);
device_unlock(&slave->dev);

if (ret < 0)
dev_err(&slave->dev,
"Update Slave status failed:%d\n", ret);
Expand Down Expand Up @@ -1860,6 +1889,7 @@ EXPORT_SYMBOL(sdw_handle_slave_status);
void sdw_clear_slave_status(struct sdw_bus *bus, u32 request)
{
struct sdw_slave *slave;
bool lock;
int i;

/* Check all non-zero devices */
Expand All @@ -1878,7 +1908,23 @@ void sdw_clear_slave_status(struct sdw_bus *bus, u32 request)
if (slave->status != SDW_SLAVE_UNATTACHED) {
sdw_modify_slave_status(slave, SDW_SLAVE_UNATTACHED);
slave->first_interrupt_done = false;

lock = device_trylock(&slave->dev);

/*
* this bus/manager-level function can only be called from
* a resume sequence. If the peripheral device (child of the
* manager device) is locked, this indicates a resume operation
* initiated by the device core to deal with .remove() or .shutdown()
* at the peripheral level. With the parent-child order enforced
* by PM frameworks on resume, the peripheral resume has not started
* yet, so it's safe to assume the lock will not be released while
* the update_status callback is invoked.
*/
sdw_update_slave_status(slave, SDW_SLAVE_UNATTACHED);

if (lock)
device_unlock(&slave->dev);
}

/* keep track of request, used in pm_runtime resume */
Expand Down
6 changes: 2 additions & 4 deletions drivers/soundwire/bus_type.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,6 @@ static int sdw_drv_probe(struct device *dev)
if (!id)
return -ENODEV;

slave->ops = drv->ops;

/*
* attach to power domain but don't turn on (last arg)
*/
Expand All @@ -118,8 +116,8 @@ static int sdw_drv_probe(struct device *dev)
}

/* device is probed so let's read the properties now */
if (slave->ops && slave->ops->read_prop)
slave->ops->read_prop(slave);
if (drv->ops && drv->ops->read_prop)
drv->ops->read_prop(slave);

/* init the sysfs as we have properties now */
ret = sdw_slave_sysfs_init(slave);
Expand Down
57 changes: 38 additions & 19 deletions drivers/soundwire/stream.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <linux/slab.h>
#include <linux/soundwire/sdw_registers.h>
#include <linux/soundwire/sdw.h>
#include <linux/soundwire/sdw_type.h>
#include <sound/soc.h>
#include "bus.h"

Expand Down Expand Up @@ -401,20 +402,26 @@ static int sdw_do_port_prep(struct sdw_slave_runtime *s_rt,
struct sdw_prepare_ch prep_ch,
enum sdw_port_prep_ops cmd)
{
const struct sdw_slave_ops *ops = s_rt->slave->ops;
int ret;
struct device *dev = &s_rt->slave->dev;
struct sdw_driver *drv;
int ret = 0;

if (ops->port_prep) {
ret = ops->port_prep(s_rt->slave, &prep_ch, cmd);
if (ret < 0) {
dev_err(&s_rt->slave->dev,
"Slave Port Prep cmd %d failed: %d\n",
cmd, ret);
return ret;
device_lock(dev);

if (dev->driver) {
drv = drv_to_sdw_driver(dev->driver);
if (drv->ops && drv->ops->port_prep) {
ret = drv->ops->port_prep(s_rt->slave, &prep_ch, cmd);
if (ret < 0)
dev_err(&s_rt->slave->dev,
"Slave Port Prep cmd %d failed: %d\n",
cmd, ret);
}
}

return 0;
device_unlock(dev);

return ret;
}

static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
Expand Down Expand Up @@ -578,7 +585,7 @@ static int sdw_notify_config(struct sdw_master_runtime *m_rt)
struct sdw_slave_runtime *s_rt;
struct sdw_bus *bus = m_rt->bus;
struct sdw_slave *slave;
int ret = 0;
int ret;

if (bus->ops->set_bus_conf) {
ret = bus->ops->set_bus_conf(bus, &bus->params);
Expand All @@ -587,19 +594,31 @@ static int sdw_notify_config(struct sdw_master_runtime *m_rt)
}

list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
slave = s_rt->slave;
struct sdw_driver *drv;
struct device *dev;

if (slave->ops->bus_config) {
ret = slave->ops->bus_config(slave, &bus->params);
if (ret < 0) {
dev_err(bus->dev, "Notify Slave: %d failed\n",
slave->dev_num);
return ret;
slave = s_rt->slave;
dev = &slave->dev;

device_lock(dev);

if (dev->driver) {
drv = drv_to_sdw_driver(dev->driver);
if (drv->ops && drv->ops->bus_config) {
ret = drv->ops->bus_config(slave, &bus->params);
if (ret < 0) {
device_unlock(dev);
dev_err(bus->dev, "Notify Slave: %d failed\n",
slave->dev_num);
return ret;
}
}
}

device_unlock(dev);
}

return ret;
return 0;
}

/**
Expand Down

0 comments on commit ee192d0

Please sign in to comment.