[PATCH 0/7] soundwire: Fix driver removal
Removal of child drivers and the bus driver was broken and would result in a slew of various errors.
Most of these were caused by the code shutting down in the wrong order, shutting down the bus driver first. The bus driver should be shut down after the child drivers have been removed (compare with the I2C and SPI subsystem for example).
These patches fix that.
A secondary problem was over the cleanup of child drivers. The removal functions were not the opposite of the probe function, and the ownership of struct sdw_slave is tricky because it mixes two separate usages and currently has to be "owned" by the bus driver.
Tested with 4 peripherals on 1 bus and 8 peripherals on 2 buses.
Richard Fitzgerald (7): soundwire: bus: Do not forcibly disable child pm_runtime soundwire: intel_init: Separate shutdown and cleanup ASoC: SOF: Intel: Don't disable Soundwire interrupt before the bus has shut down soundwire: bus: Add remove callback to struct sdw_master_ops soundwire: intel: Don't disable interrupt until children are removed soundwire: intel: Don't disable pm_runtime until children are removed soundwire: bus: Fix premature removal of sdw_slave objects
drivers/soundwire/bus.c | 37 ++++++++++++++++++++++++----- drivers/soundwire/intel.c | 13 ++++++++-- drivers/soundwire/intel_init.c | 25 +++++++++++++++---- drivers/soundwire/slave.c | 21 ++++++++++++---- include/linux/soundwire/sdw.h | 3 ++- include/linux/soundwire/sdw_intel.h | 2 ++ sound/soc/sof/intel/hda.c | 16 ++++++++++--- 7 files changed, 96 insertions(+), 21 deletions(-)
Do not call pm_runtime_disable() of a child driver in sdw_delete_slave(). We really should never be trying to disable another driver's pm_runtime - it is up to the child driver to disable it or the core driver framework cleanup. The driver core will runtime-resume a driver before calling its remove() so we shouldn't break that.
The patch that introduced this is commit dff70572e9a3 ("soundwire: bus: disable pm_runtime in sdw_slave_delete") which says:
"prevent any race condition with the resume being executed after the bus and slave devices are removed"
The actual problem is that the bus driver is shutting itself down before the child drivers have been removed, which is the wrong way around (see for example I2C and SPI drivers). If this is fixed, the bus driver will still be operational when the driver framework runtime_resumes the child drivers to remove them. Then the bus driver will remove() and can shut down safely.
Also note that the child drivers are not necessarily idle when the bus driver is removed, so disabling their pm_runtime and stopping the bus might break more than only their remove().
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- drivers/soundwire/bus.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 0bcc2d161eb9..99429892221b 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -151,8 +151,6 @@ 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);
On 9/7/22 12:13, Richard Fitzgerald wrote:
Do not call pm_runtime_disable() of a child driver in sdw_delete_slave(). We really should never be trying to disable another driver's pm_runtime - it is up to the child driver to disable it or the core driver framework cleanup. The driver core will runtime-resume a driver before calling its remove() so we shouldn't break that.
The patch that introduced this is commit dff70572e9a3 ("soundwire: bus: disable pm_runtime in sdw_slave_delete") which says:
"prevent any race condition with the resume being executed after the bus and slave devices are removed"
The actual problem is that the bus driver is shutting itself down before the child drivers have been removed, which is the wrong way around (see for example I2C and SPI drivers). If this is fixed, the bus driver will still be operational when the driver framework runtime_resumes the child drivers to remove them. Then the bus driver will remove() and can shut down safely.
The description of the fix looks good, but "if this is fixed" is very confusing to me.
Don't you have a dependency issue here?
There should be first a patch to fix the bus issue and then remove this pm_runtime_disable second.
Also note that the child drivers are not necessarily idle when the bus driver is removed, so disabling their pm_runtime and stopping the bus might break more than only their remove().
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
drivers/soundwire/bus.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 0bcc2d161eb9..99429892221b 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -151,8 +151,6 @@ 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);
Move the freeing of context data out of sdw_intel_exit() into a new exported function sdw_intel_remove().
This splits shutdown and cleanup into separate stages, allowing the calling code to perform its own shutdown after the bus has shutdown but before the context has been deleted.
The struct sdw_intel_ctx pointer is passed to the calling code by sdw_intel_probe() and the calling code passes it back as an opaque token. When the caller is removed it must have the opportunity to teardown its use of this token after the bus driver has stopped but before the context memory has been freed. It should not be doing its teardown before calling sdw_intel_exit() because that will break any bus activity currently in progress and the removal of child drivers.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- drivers/soundwire/intel_init.c | 24 ++++++++++++++++++++---- include/linux/soundwire/sdw_intel.h | 2 ++ sound/soc/sof/intel/hda.c | 4 +++- 3 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index d091513919df..078e01f67830 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -292,6 +292,13 @@ static struct sdw_intel_ctx return NULL; }
+static void sdw_intel_remove_controller(struct sdw_intel_ctx *ctx) +{ + kfree(ctx->ids); + kfree(ctx->ldev); + kfree(ctx); +} + static int sdw_intel_startup_controller(struct sdw_intel_ctx *ctx) { @@ -360,6 +367,18 @@ struct sdw_intel_ctx } EXPORT_SYMBOL_NS(sdw_intel_probe, SOUNDWIRE_INTEL_INIT);
+/** + * sdw_intel_remove() - SoundWire Intel remove routine + * @ctx: SoundWire context allocated in the probe + * + * Free all the context created by sdw_intel_probe. + */ +void sdw_intel_remove(struct sdw_intel_ctx *ctx) +{ + return sdw_intel_remove_controller(ctx); +} +EXPORT_SYMBOL_NS(sdw_intel_remove, SOUNDWIRE_INTEL_INIT); + /** * sdw_intel_startup() - SoundWire Intel startup * @ctx: SoundWire context allocated in the probe @@ -376,14 +395,11 @@ EXPORT_SYMBOL_NS(sdw_intel_startup, SOUNDWIRE_INTEL_INIT); * sdw_intel_exit() - SoundWire Intel exit * @ctx: SoundWire context allocated in the probe * - * Delete the controller instances created and cleanup + * Stop the controller instances. */ void sdw_intel_exit(struct sdw_intel_ctx *ctx) { sdw_intel_cleanup(ctx); - kfree(ctx->ids); - kfree(ctx->ldev); - kfree(ctx); } EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index 2e9fd91572d4..7f7327cab712 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -282,6 +282,8 @@ void sdw_intel_process_wakeen_event(struct sdw_intel_ctx *ctx); struct sdw_intel_ctx * sdw_intel_probe(struct sdw_intel_res *res);
+void sdw_intel_remove(struct sdw_intel_ctx *ctx); + int sdw_intel_startup(struct sdw_intel_ctx *ctx);
void sdw_intel_exit(struct sdw_intel_ctx *ctx); diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 8639ea63a10d..ee67e21e739f 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -241,8 +241,10 @@ static int hda_sdw_exit(struct snd_sof_dev *sdev)
hda_sdw_int_enable(sdev, false);
- if (hdev->sdw) + if (hdev->sdw) { sdw_intel_exit(hdev->sdw); + sdw_intel_remove(hdev->sdw); + } hdev->sdw = NULL;
return 0;
Until the Soundwire child drivers have been removed and the bus driver has shut down any of them can still be actively doing something. And any of them may need bus transactions to shut down their hardware. So the Soundwire interrupt must not be disabled until the point that nothing can be using it.
Normally it is up to the driver using the interrupt to ensure that it doesn't break if there is an interrupt while it is shutting down. However, the design of the Intel drivers means that the Soundwire bus driver doesn't have control of its own interrupt - instead its interrupt handler is called externally by the code in hda.c. Therefore hda.c must shutdown the bus before disabling the interrupt and freeing the context memory.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/soc/sof/intel/hda.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index ee67e21e739f..34f5de052bc0 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -236,17 +236,25 @@ int hda_sdw_startup(struct snd_sof_dev *sdev) static int hda_sdw_exit(struct snd_sof_dev *sdev) { struct sof_intel_hda_dev *hdev; + void *tmp_sdw;
hdev = sdev->pdata->hw_pdata; - - hda_sdw_int_enable(sdev, false); - - if (hdev->sdw) { + if (hdev->sdw) sdw_intel_exit(hdev->sdw); - sdw_intel_remove(hdev->sdw); - } + + /* The bus has now stopped so the interrupt can be disabled */ + hda_sdw_int_enable(sdev, false); + + /* Wait for last run of irq handler to complete */ + synchronize_irq(sdev->ipc_irq); + + /* Stop using the pointer */ + tmp_sdw = hdev->sdw; hdev->sdw = NULL;
+ if (tmp_sdw) + sdw_intel_remove(tmp_sdw); + return 0; }
On Wed, Sep 07, 2022 at 11:13:58AM +0100, Richard Fitzgerald wrote:
Until the Soundwire child drivers have been removed and the bus driver has shut down any of them can still be actively doing something. And any of them may need bus transactions to shut down their hardware. So the Soundwire interrupt must not be disabled until the point that nothing can be using it.
Acked-by: Mark Brown broonie@kernel.org
During removal of a bus driver the bus must stay operational while child drivers are being removed, since (a) they might have been busy when the bus driver removal started and (b) the might need to access the bus to run their shutdown procedures. Only after that can the bus driver disable the bus.
Add a new remove callback to struct sdw_master_ops that the bus driver can implement to disable the bus after children are removed.
This is modeled on the ASoC component_remove, which indicates that the driver is no longer required.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- drivers/soundwire/bus.c | 5 +++++ include/linux/soundwire/sdw.h | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 99429892221b..1327a312be86 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -176,6 +176,11 @@ static int sdw_delete_slave(struct device *dev, void *data) void sdw_bus_master_delete(struct sdw_bus *bus) { device_for_each_child(bus->dev, NULL, sdw_delete_slave); + + /* Children have been removed so it is now safe for the bus to stop */ + if (bus->ops->remove) + bus->ops->remove(bus); + sdw_master_device_del(bus);
sdw_bus_debugfs_exit(bus); diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index a2b31d25ea27..aa492173d5eb 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -839,6 +839,7 @@ struct sdw_defer { * @set_bus_conf: Set the bus configuration * @pre_bank_switch: Callback for pre bank switch * @post_bank_switch: Callback for post bank switch + * @remove: Called when it is safe to stop the bus controller. */ struct sdw_master_ops { int (*read_prop)(struct sdw_bus *bus); @@ -855,7 +856,7 @@ struct sdw_master_ops { struct sdw_bus_params *params); int (*pre_bank_switch)(struct sdw_bus *bus); int (*post_bank_switch)(struct sdw_bus *bus); - + void (*remove)(struct sdw_bus *bus); };
/**
The cadence_master code needs the interrupt to complete message transfers. When the bus driver is being removed child drivers are removed, and their remove actions might need bus transactions.
Use the sdw_master_ops.remove callback to disable the interrupt handling only after the child drivers have been removed.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- drivers/soundwire/intel.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 01be62fa6c83..d5e723a9c80b 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1255,6 +1255,13 @@ static int intel_prop_read(struct sdw_bus *bus) return 0; }
+static void intel_bus_remove(struct sdw_bus *bus) +{ + struct sdw_cdns *cdns = bus_to_cdns(bus); + + sdw_cdns_enable_interrupt(cdns, false); +} + static struct sdw_master_ops sdw_intel_ops = { .read_prop = sdw_master_read_prop, .override_adr = sdw_dmi_override_adr, @@ -1264,6 +1271,7 @@ static struct sdw_master_ops sdw_intel_ops = { .set_bus_conf = cdns_bus_conf, .pre_bank_switch = intel_pre_bank_switch, .post_bank_switch = intel_post_bank_switch, + .remove = intel_bus_remove, };
static int intel_init(struct sdw_intel *sdw) @@ -1502,7 +1510,6 @@ static void intel_link_remove(struct auxiliary_device *auxdev) */ if (!bus->prop.hw_disabled) { intel_debugfs_exit(sdw); - sdw_cdns_enable_interrupt(cdns, false); snd_soc_unregister_component(dev); } sdw_bus_master_delete(bus);
On 9/7/22 12:14, Richard Fitzgerald wrote:
The cadence_master code needs the interrupt to complete message transfers. When the bus driver is being removed child drivers are removed, and their remove actions might need bus transactions.
Use the sdw_master_ops.remove callback to disable the interrupt handling only after the child drivers have been removed.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
drivers/soundwire/intel.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 01be62fa6c83..d5e723a9c80b 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1255,6 +1255,13 @@ static int intel_prop_read(struct sdw_bus *bus) return 0; }
+static void intel_bus_remove(struct sdw_bus *bus) +{
- struct sdw_cdns *cdns = bus_to_cdns(bus);
- sdw_cdns_enable_interrupt(cdns, false);
don't you need to check for any on-going transactions on the bus?
I wonder if there could be a corner case where there are no child devices but still a device physically attached to the bus. I am not sure if the 'no devices left' is a good-enough indication of no activity on the bus.
+}
static struct sdw_master_ops sdw_intel_ops = { .read_prop = sdw_master_read_prop, .override_adr = sdw_dmi_override_adr, @@ -1264,6 +1271,7 @@ static struct sdw_master_ops sdw_intel_ops = { .set_bus_conf = cdns_bus_conf, .pre_bank_switch = intel_pre_bank_switch, .post_bank_switch = intel_post_bank_switch,
- .remove = intel_bus_remove,
};
static int intel_init(struct sdw_intel *sdw) @@ -1502,7 +1510,6 @@ static void intel_link_remove(struct auxiliary_device *auxdev) */ if (!bus->prop.hw_disabled) { intel_debugfs_exit(sdw);
sdw_cdns_enable_interrupt(cdns, false);
On 12/09/2022 11:53, Pierre-Louis Bossart wrote:
On 9/7/22 12:14, Richard Fitzgerald wrote:
The cadence_master code needs the interrupt to complete message transfers. When the bus driver is being removed child drivers are removed, and their remove actions might need bus transactions.
Use the sdw_master_ops.remove callback to disable the interrupt handling only after the child drivers have been removed.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
drivers/soundwire/intel.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 01be62fa6c83..d5e723a9c80b 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1255,6 +1255,13 @@ static int intel_prop_read(struct sdw_bus *bus) return 0; }
+static void intel_bus_remove(struct sdw_bus *bus) +{
- struct sdw_cdns *cdns = bus_to_cdns(bus);
- sdw_cdns_enable_interrupt(cdns, false);
don't you need to check for any on-going transactions on the bus?
As all the child drivers have removed, I think the only other place that can generate bus transactions is the PING handler but sdw_cdns_enable_interrupt(false) calls cancel_work_sync() to cancel the cdns->work and it sets a flag so that it will not be re-queued.
I wonder if there could be a corner case where there are no child devices but still a device physically attached to the bus. I am not sure if the 'no devices left' is a good-enough indication of no activity on the bus.
As above - yes there could, but sdw_cdns_enable_interrupt(false) will cancel the work and stop it being re-queued.
+}
- static struct sdw_master_ops sdw_intel_ops = { .read_prop = sdw_master_read_prop, .override_adr = sdw_dmi_override_adr,
@@ -1264,6 +1271,7 @@ static struct sdw_master_ops sdw_intel_ops = { .set_bus_conf = cdns_bus_conf, .pre_bank_switch = intel_pre_bank_switch, .post_bank_switch = intel_post_bank_switch,
.remove = intel_bus_remove, };
static int intel_init(struct sdw_intel *sdw)
@@ -1502,7 +1510,6 @@ static void intel_link_remove(struct auxiliary_device *auxdev) */ if (!bus->prop.hw_disabled) { intel_debugfs_exit(sdw);
sdw_cdns_enable_interrupt(cdns, false);
On 9/12/22 17:36, Richard Fitzgerald wrote:
On 12/09/2022 11:53, Pierre-Louis Bossart wrote:
On 9/7/22 12:14, Richard Fitzgerald wrote:
The cadence_master code needs the interrupt to complete message transfers. When the bus driver is being removed child drivers are removed, and their remove actions might need bus transactions.
Use the sdw_master_ops.remove callback to disable the interrupt handling only after the child drivers have been removed.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
drivers/soundwire/intel.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 01be62fa6c83..d5e723a9c80b 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1255,6 +1255,13 @@ static int intel_prop_read(struct sdw_bus *bus) return 0; } +static void intel_bus_remove(struct sdw_bus *bus) +{ + struct sdw_cdns *cdns = bus_to_cdns(bus);
+ sdw_cdns_enable_interrupt(cdns, false);
don't you need to check for any on-going transactions on the bus?
As all the child drivers have removed, I think the only other place that can generate bus transactions is the PING handler but sdw_cdns_enable_interrupt(false) calls cancel_work_sync() to cancel the cdns->work and it sets a flag so that it will not be re-queued.
I wonder if there could be a corner case where there are no child devices but still a device physically attached to the bus. I am not sure if the 'no devices left' is a good-enough indication of no activity on the bus.
As above - yes there could, but sdw_cdns_enable_interrupt(false) will cancel the work and stop it being re-queued.
Ah yes, I forgot that part, thanks!
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
On 12/09/2022 18:12, Pierre-Louis Bossart wrote:
On 9/12/22 17:36, Richard Fitzgerald wrote:
On 12/09/2022 11:53, Pierre-Louis Bossart wrote:
On 9/7/22 12:14, Richard Fitzgerald wrote:
The cadence_master code needs the interrupt to complete message transfers. When the bus driver is being removed child drivers are removed, and their remove actions might need bus transactions.
Use the sdw_master_ops.remove callback to disable the interrupt handling only after the child drivers have been removed.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
drivers/soundwire/intel.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 01be62fa6c83..d5e723a9c80b 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1255,6 +1255,13 @@ static int intel_prop_read(struct sdw_bus *bus) return 0; } +static void intel_bus_remove(struct sdw_bus *bus) +{ + struct sdw_cdns *cdns = bus_to_cdns(bus);
+ sdw_cdns_enable_interrupt(cdns, false);
don't you need to check for any on-going transactions on the bus?
As all the child drivers have removed, I think the only other place that can generate bus transactions is the PING handler but sdw_cdns_enable_interrupt(false) calls cancel_work_sync() to cancel the cdns->work and it sets a flag so that it will not be re-queued.
I wonder if there could be a corner case where there are no child devices but still a device physically attached to the bus. I am not sure if the 'no devices left' is a good-enough indication of no activity on the bus.
As above - yes there could, but sdw_cdns_enable_interrupt(false) will cancel the work and stop it being re-queued.
Ah yes, I forgot that part, thanks!
... but I have noticed that there is a bug in sdw_cdns_enable_interrupt(). It doesn't ensure that the IRQ thread has seen the cdns->interrupt_enabled = false. I'll add a patch to fix that when I re-push this chain.
When the bus driver is removed the child drivers will be removed first. These may need to perform bus transactions to shut down, and the device driver core will runtime-resume the driver before calling its remove().
For this to work the pm_runtime of the bus driver must still be enabled. So do not disable pm_runtime until the bus driver has been unregistered.
Though this could be done by powering-up the bus driver and then disabling its pm_runtime with the bus still powered-up, there's no need to bypass the standard device framework behaviour.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- drivers/soundwire/intel.c | 4 +++- drivers/soundwire/intel_init.c | 1 - 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index d5e723a9c80b..3345310e979c 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1504,7 +1504,7 @@ static void intel_link_remove(struct auxiliary_device *auxdev) struct sdw_bus *bus = &cdns->bus;
/* - * Since pm_runtime is already disabled, we don't decrease + * Since pm_runtime will be disabled, we don't decrease * the refcount when the clock_stop_quirk is * SDW_INTEL_CLK_STOP_NOT_ALLOWED */ @@ -1513,6 +1513,8 @@ static void intel_link_remove(struct auxiliary_device *auxdev) snd_soc_unregister_component(dev); } sdw_bus_master_delete(bus); + + pm_runtime_disable(&auxdev->dev); }
int intel_link_process_wakeen_event(struct auxiliary_device *auxdev) diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 078e01f67830..ce26d2df088a 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -115,7 +115,6 @@ static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx)
ldev = ctx->ldev[i];
- pm_runtime_disable(&ldev->auxdev.dev); if (!ldev->link_res.clock_stop_quirks) pm_runtime_put_noidle(ldev->link_res.dev);
When the bus manager is removed sdw_bus_master_delete() should not be deleting the struct sdw_slave objects until the bus manager has been stopped. The first step of removing child drivers should only be calling device_unregister() on the child. The counterpart to sdw_drv_probe() is sdw_drv_remove(), not sdw_delete_slave().
The sdw_slave objects are created by the bus manager probe() from ACPI/DT information. They are not created when a child driver probes so should not be deleted by a child driver remove.
Change-Id: I25cc145df12fdc7c126f8f594a5f76eedce25488 Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- drivers/soundwire/bus.c | 30 ++++++++++++++++++++++++++---- drivers/soundwire/slave.c | 21 +++++++++++++++++---- 2 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 1327a312be86..5533eb589286 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -146,9 +146,8 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent, } EXPORT_SYMBOL(sdw_bus_master_add);
-static int sdw_delete_slave(struct device *dev, void *data) +static int sdw_delete_slave(struct sdw_slave *slave) { - struct sdw_slave *slave = dev_to_sdw_dev(dev); struct sdw_bus *bus = slave->bus;
sdw_slave_debugfs_exit(slave); @@ -163,7 +162,24 @@ static int sdw_delete_slave(struct device *dev, void *data) list_del_init(&slave->node); mutex_unlock(&bus->bus_lock);
+ mutex_destroy(&slave->sdw_dev_lock); + kfree(slave); + + return 0; +} + +static int sdw_remove_child(struct device *dev, void *data) +{ + /* + * Do not remove the struct sdw_slave yet. This is created by + * the bus manager probe() from ACPI information and used by the + * bus manager to hold status of each peripheral. Its lifetime + * is that of the bus manager. + */ + + /* This will call sdw_drv_remove() */ device_unregister(dev); + return 0; }
@@ -171,16 +187,22 @@ static int sdw_delete_slave(struct device *dev, void *data) * sdw_bus_master_delete() - delete the bus master instance * @bus: bus to be deleted * - * Remove the instance, delete the child devices. + * Remove the child devices, remove the master instance. */ void sdw_bus_master_delete(struct sdw_bus *bus) { - device_for_each_child(bus->dev, NULL, sdw_delete_slave); + struct sdw_slave *slave, *tmp; + + device_for_each_child(bus->dev, NULL, sdw_remove_child);
/* Children have been removed so it is now safe for the bus to stop */ if (bus->ops->remove) bus->ops->remove(bus);
+ /* Now the bus is stopped it is safe to free things */ + list_for_each_entry_safe(slave, tmp, &bus->slaves, node) + sdw_delete_slave(slave); + sdw_master_device_del(bus);
sdw_bus_debugfs_exit(bus); diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index c1c1a2ac293a..b6161d002b97 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -10,10 +10,23 @@
static void sdw_slave_release(struct device *dev) { - struct sdw_slave *slave = dev_to_sdw_dev(dev); - - mutex_destroy(&slave->sdw_dev_lock); - kfree(slave); + /* + * The release() callback should not be empty + * (see Documentation/core-api/kobject.rst) but the ownership + * of struct sdw_slave is muddled. It is used for two separate + * purposes: + * 1) by the bus driver to track its own state information for + * physical devices on the bus and found in ACPI/DT, whether + * or not there is a child driver for it; + * 2) to hold the child driver object. + * + * The struct sdw_slave cannot be freed when the child driver + * is released because it is holding info used by the bus + * driver. It is freed when the bus driver is removed. + * + * Until the ownership issue is untangled this cannot free + * the struct sdw_slave object containing the child dev. + */ }
struct device_type sdw_slave_type = {
On 9/7/22 12:14, Richard Fitzgerald wrote:
When the bus manager is removed sdw_bus_master_delete() should not be deleting the struct sdw_slave objects until the bus manager has been stopped. The first step of removing child drivers should only be calling device_unregister() on the child. The counterpart to sdw_drv_probe() is sdw_drv_remove(), not sdw_delete_slave().
The sdw_slave objects are created by the bus manager probe() from ACPI/DT information. They are not created when a child driver probes so should not be deleted by a child driver remove.
Change-Id: I25cc145df12fdc7c126f8f594a5f76eedce25488
spurious Change-Id
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
drivers/soundwire/bus.c | 30 ++++++++++++++++++++++++++---- drivers/soundwire/slave.c | 21 +++++++++++++++++---- 2 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 1327a312be86..5533eb589286 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -146,9 +146,8 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent, } EXPORT_SYMBOL(sdw_bus_master_add);
-static int sdw_delete_slave(struct device *dev, void *data) +static int sdw_delete_slave(struct sdw_slave *slave) {
struct sdw_slave *slave = dev_to_sdw_dev(dev); struct sdw_bus *bus = slave->bus;
sdw_slave_debugfs_exit(slave);
@@ -163,7 +162,24 @@ static int sdw_delete_slave(struct device *dev, void *data) list_del_init(&slave->node); mutex_unlock(&bus->bus_lock);
- mutex_destroy(&slave->sdw_dev_lock);
- kfree(slave);
- return 0;
+}
+static int sdw_remove_child(struct device *dev, void *data) +{
- /*
* Do not remove the struct sdw_slave yet. This is created by* the bus manager probe() from ACPI information and used by the* bus manager to hold status of each peripheral. Its lifetime* is that of the bus manager.*/- /* This will call sdw_drv_remove() */ device_unregister(dev);
- return 0;
}
@@ -171,16 +187,22 @@ static int sdw_delete_slave(struct device *dev, void *data)
- sdw_bus_master_delete() - delete the bus master instance
- @bus: bus to be deleted
- Remove the instance, delete the child devices.
*/
- Remove the child devices, remove the master instance.
void sdw_bus_master_delete(struct sdw_bus *bus) {
- device_for_each_child(bus->dev, NULL, sdw_delete_slave);
struct sdw_slave *slave, *tmp;
device_for_each_child(bus->dev, NULL, sdw_remove_child);
/* Children have been removed so it is now safe for the bus to stop */ if (bus->ops->remove) bus->ops->remove(bus);
/* Now the bus is stopped it is safe to free things */
list_for_each_entry_safe(slave, tmp, &bus->slaves, node)
sdw_delete_slave(slave);sdw_master_device_del(bus);
sdw_bus_debugfs_exit(bus);
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index c1c1a2ac293a..b6161d002b97 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -10,10 +10,23 @@
static void sdw_slave_release(struct device *dev) {
- struct sdw_slave *slave = dev_to_sdw_dev(dev);
- mutex_destroy(&slave->sdw_dev_lock);
- kfree(slave);
- /*
* The release() callback should not be empty* (see Documentation/core-api/kobject.rst) but the ownership* of struct sdw_slave is muddled. It is used for two separate* purposes:* 1) by the bus driver to track its own state information for* physical devices on the bus and found in ACPI/DT, whether* or not there is a child driver for it;* 2) to hold the child driver object.** The struct sdw_slave cannot be freed when the child driver* is released because it is holding info used by the bus* driver. It is freed when the bus driver is removed.** Until the ownership issue is untangled this cannot free* the struct sdw_slave object containing the child dev.*/}
struct device_type sdw_slave_type = {
participants (3)
-
Mark Brown -
Pierre-Louis Bossart -
Richard Fitzgerald