mirror of
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git
synced 2024-08-23 17:30:02 +00:00
![Pierre-Louis Bossart](/assets/img/avatar_default.png)
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. While trying to solve this kernel oopses, it became clear that the existing SoundWire bus does not deal well with the unbind case. Commit528be501b7
("soundwire: sdw_slave: add probe_complete structure and new fields") added a 'probed' status variable and a 'probe_complete' struct completion. This status is however not reset on remove and likewise the 'probe complete' is not re-initialized, so the bind/unbind/bind test cases would fail. The timeout used before the 'update_status' callback was also a bad idea in hindsight, there should really be no timing assumption as to if and when a driver is bound to a device. An initial draft was based on device_lock() and device_unlock() was tested. This proved too complicated, with deadlocks created during the suspend-resume sequences, which also use the same device_lock/unlock() as the bind/unbind sequences. On a CometLake device, a bad DSDT/BIOS caused spurious resumes and the use of device_lock() caused hangs during suspend. After multiple weeks or testing and painful reverse-engineering of deadlocks on different devices, we looked for alternatives that did not interfere with the device core. A bus notifier was used successfully to keep track of DRIVER_BOUND and DRIVER_UNBIND events. This solved the bind-unbind-bind case in tests, but it can still be defeated with a theoretical corner case where the memory is freed by a .remove while the callback is in use. The notifier only helps make sure the driver callbacks are valid, but not that the memory allocated in probe remains valid while the callbacks are invoked. This patch suggests the introduction of a new 'sdw_dev_lock' mutex protecting probe/remove and all driver callbacks. Since this mutex is 'local' to SoundWire only, it does not interfere with existing locks and does not create deadlocks. In addition, this patch removes the 'probe_complete' completion, instead we directly invoke the 'update_status' from the probe routine. That removes any sort of timing dependency and a much better support for the device/driver model, the driver could be bound before the bus started, or eons after the bus started and the hardware would be properly initialized in all cases. BugLink: https://github.com/thesofproject/linux/issues/3531 Fixes:56d4fe31af
("soundwire: Add MIPI DisCo property helpers") Fixes:528be501b7
("soundwire: sdw_slave: add probe_complete structure and new fields") Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Reviewed-by: Rander Wang <rander.wang@intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com> Link: https://lore.kernel.org/r/20220621225641.221170-2-pierre-louis.bossart@linux.intel.com Signed-off-by: Vinod Koul <vkoul@kernel.org>
249 lines
5.7 KiB
C
249 lines
5.7 KiB
C
// SPDX-License-Identifier: GPL-2.0
|
|
// Copyright(c) 2015-17 Intel Corporation.
|
|
|
|
#include <linux/module.h>
|
|
#include <linux/mod_devicetable.h>
|
|
#include <linux/pm_domain.h>
|
|
#include <linux/soundwire/sdw.h>
|
|
#include <linux/soundwire/sdw_type.h>
|
|
#include "bus.h"
|
|
#include "sysfs_local.h"
|
|
|
|
/**
|
|
* sdw_get_device_id - find the matching SoundWire device id
|
|
* @slave: SoundWire Slave Device
|
|
* @drv: SoundWire Slave Driver
|
|
*
|
|
* The match is done by comparing the mfg_id and part_id from the
|
|
* struct sdw_device_id.
|
|
*/
|
|
static const struct sdw_device_id *
|
|
sdw_get_device_id(struct sdw_slave *slave, struct sdw_driver *drv)
|
|
{
|
|
const struct sdw_device_id *id;
|
|
|
|
for (id = drv->id_table; id && id->mfg_id; id++)
|
|
if (slave->id.mfg_id == id->mfg_id &&
|
|
slave->id.part_id == id->part_id &&
|
|
(!id->sdw_version ||
|
|
slave->id.sdw_version == id->sdw_version) &&
|
|
(!id->class_id ||
|
|
slave->id.class_id == id->class_id))
|
|
return id;
|
|
|
|
return NULL;
|
|
}
|
|
|
|
static int sdw_bus_match(struct device *dev, struct device_driver *ddrv)
|
|
{
|
|
struct sdw_slave *slave;
|
|
struct sdw_driver *drv;
|
|
int ret = 0;
|
|
|
|
if (is_sdw_slave(dev)) {
|
|
slave = dev_to_sdw_dev(dev);
|
|
drv = drv_to_sdw_driver(ddrv);
|
|
|
|
ret = !!sdw_get_device_id(slave, drv);
|
|
}
|
|
return ret;
|
|
}
|
|
|
|
int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
|
|
{
|
|
/* modalias is sdw:m<mfg_id>p<part_id>v<version>c<class_id> */
|
|
|
|
return snprintf(buf, size, "sdw:m%04Xp%04Xv%02Xc%02X\n",
|
|
slave->id.mfg_id, slave->id.part_id,
|
|
slave->id.sdw_version, slave->id.class_id);
|
|
}
|
|
|
|
int sdw_slave_uevent(struct device *dev, struct kobj_uevent_env *env)
|
|
{
|
|
struct sdw_slave *slave = dev_to_sdw_dev(dev);
|
|
char modalias[32];
|
|
|
|
sdw_slave_modalias(slave, modalias, sizeof(modalias));
|
|
|
|
if (add_uevent_var(env, "MODALIAS=%s", modalias))
|
|
return -ENOMEM;
|
|
|
|
return 0;
|
|
}
|
|
|
|
struct bus_type sdw_bus_type = {
|
|
.name = "soundwire",
|
|
.match = sdw_bus_match,
|
|
};
|
|
EXPORT_SYMBOL_GPL(sdw_bus_type);
|
|
|
|
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;
|
|
|
|
/*
|
|
* attach to power domain but don't turn on (last arg)
|
|
*/
|
|
ret = dev_pm_domain_attach(dev, false);
|
|
if (ret)
|
|
return ret;
|
|
|
|
mutex_lock(&slave->sdw_dev_lock);
|
|
|
|
ret = drv->probe(slave, id);
|
|
if (ret) {
|
|
name = drv->name;
|
|
if (!name)
|
|
name = drv->driver.name;
|
|
mutex_unlock(&slave->sdw_dev_lock);
|
|
|
|
dev_err(dev, "Probe of %s failed: %d\n", name, ret);
|
|
dev_pm_domain_detach(dev, false);
|
|
return ret;
|
|
}
|
|
|
|
/* device is probed so let's read the properties now */
|
|
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);
|
|
if (ret < 0)
|
|
dev_warn(dev, "Slave sysfs init failed:%d\n", ret);
|
|
|
|
/*
|
|
* Check for valid clk_stop_timeout, use DisCo worst case value of
|
|
* 300ms
|
|
*
|
|
* TODO: check the timeouts and driver removal case
|
|
*/
|
|
if (slave->prop.clk_stop_timeout == 0)
|
|
slave->prop.clk_stop_timeout = 300;
|
|
|
|
slave->bus->clk_stop_timeout = max_t(u32, slave->bus->clk_stop_timeout,
|
|
slave->prop.clk_stop_timeout);
|
|
|
|
slave->probed = true;
|
|
|
|
/*
|
|
* if the probe happened after the bus was started, notify the codec driver
|
|
* of the current hardware status to e.g. start the initialization.
|
|
* Errors are only logged as warnings to avoid failing the probe.
|
|
*/
|
|
if (drv->ops && drv->ops->update_status) {
|
|
ret = drv->ops->update_status(slave, slave->status);
|
|
if (ret < 0)
|
|
dev_warn(dev, "%s: update_status failed with status %d\n", __func__, ret);
|
|
}
|
|
|
|
mutex_unlock(&slave->sdw_dev_lock);
|
|
|
|
dev_dbg(dev, "probe complete\n");
|
|
|
|
return 0;
|
|
}
|
|
|
|
static int sdw_drv_remove(struct device *dev)
|
|
{
|
|
struct sdw_slave *slave = dev_to_sdw_dev(dev);
|
|
struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
|
|
int ret = 0;
|
|
|
|
mutex_lock(&slave->sdw_dev_lock);
|
|
|
|
slave->probed = false;
|
|
|
|
if (drv->remove)
|
|
ret = drv->remove(slave);
|
|
|
|
mutex_unlock(&slave->sdw_dev_lock);
|
|
|
|
dev_pm_domain_detach(dev, false);
|
|
|
|
return ret;
|
|
}
|
|
|
|
static void sdw_drv_shutdown(struct device *dev)
|
|
{
|
|
struct sdw_slave *slave = dev_to_sdw_dev(dev);
|
|
struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
|
|
|
|
if (drv->shutdown)
|
|
drv->shutdown(slave);
|
|
}
|
|
|
|
/**
|
|
* __sdw_register_driver() - register a SoundWire Slave driver
|
|
* @drv: driver to register
|
|
* @owner: owning module/driver
|
|
*
|
|
* Return: zero on success, else a negative error code.
|
|
*/
|
|
int __sdw_register_driver(struct sdw_driver *drv, struct module *owner)
|
|
{
|
|
const char *name;
|
|
|
|
drv->driver.bus = &sdw_bus_type;
|
|
|
|
if (!drv->probe) {
|
|
name = drv->name;
|
|
if (!name)
|
|
name = drv->driver.name;
|
|
|
|
pr_err("driver %s didn't provide SDW probe routine\n", name);
|
|
return -EINVAL;
|
|
}
|
|
|
|
drv->driver.owner = owner;
|
|
drv->driver.probe = sdw_drv_probe;
|
|
drv->driver.remove = sdw_drv_remove;
|
|
drv->driver.shutdown = sdw_drv_shutdown;
|
|
|
|
return driver_register(&drv->driver);
|
|
}
|
|
EXPORT_SYMBOL_GPL(__sdw_register_driver);
|
|
|
|
/**
|
|
* sdw_unregister_driver() - unregisters the SoundWire Slave driver
|
|
* @drv: driver to unregister
|
|
*/
|
|
void sdw_unregister_driver(struct sdw_driver *drv)
|
|
{
|
|
driver_unregister(&drv->driver);
|
|
}
|
|
EXPORT_SYMBOL_GPL(sdw_unregister_driver);
|
|
|
|
static int __init sdw_bus_init(void)
|
|
{
|
|
sdw_debugfs_init();
|
|
return bus_register(&sdw_bus_type);
|
|
}
|
|
|
|
static void __exit sdw_bus_exit(void)
|
|
{
|
|
sdw_debugfs_exit();
|
|
bus_unregister(&sdw_bus_type);
|
|
}
|
|
|
|
postcore_initcall(sdw_bus_init);
|
|
module_exit(sdw_bus_exit);
|
|
|
|
MODULE_DESCRIPTION("SoundWire bus");
|
|
MODULE_LICENSE("GPL v2");
|