[PATCH 0/7] soundwire: Fix driver removal

[PATCH 0/7] soundwire: Fix driver removal

[Richard Fitzgerald]

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(-)

-- 
2.30.2

[PATCH 1/7] soundwire: bus: Do not forcibly disable child pm_runtime

[Richard Fitzgerald]

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);
-- 
2.30.2

[PATCH 2/7] soundwire: intel_init: Separate shutdown and cleanup

[Richard Fitzgerald]

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;
-- 
2.30.2

[PATCH 3/7] ASoC: SOF: Intel: Don't disable Soundwire interrupt before the bus has shut down

[Richard Fitzgerald]

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;
 }
 
-- 
2.30.2

[PATCH 4/7] soundwire: bus: Add remove callback to struct sdw_master_ops

[Richard Fitzgerald]

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);
 };
 
 /**
-- 
2.30.2

[PATCH 5/7] soundwire: intel: Don't disable interrupt until children are removed

[Richard Fitzgerald]

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);
-- 
2.30.2

[PATCH 6/7] soundwire: intel: Don't disable pm_runtime until children are removed

[Richard Fitzgerald]

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);
 
-- 
2.30.2

[PATCH 7/7] soundwire: bus: Fix premature removal of sdw_slave objects

[Richard Fitzgerald]

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 = {
-- 
2.30.2

Re: [PATCH 3/7] ASoC: SOF: Intel: Don't disable Soundwire interrupt before the bus has shut down

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 422 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/7] soundwire: bus: Do not forcibly disable child pm_runtime

[Pierre-Louis Bossart]

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);

Re: [PATCH 5/7] soundwire: intel: Don't disable interrupt until children are removed

[Pierre-Louis Bossart]

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);
>  		snd_soc_unregister_component(dev);
>  	}
>  	sdw_bus_master_delete(bus);

Re: [PATCH 7/7] soundwire: bus: Fix premature removal of sdw_slave objects

[Pierre-Louis Bossart]

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 = {

Re: [PATCH 5/7] soundwire: intel: Don't disable interrupt until children are removed

[Richard Fitzgerald]

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);
>>   		snd_soc_unregister_component(dev);
>>   	}
>>   	sdw_bus_master_delete(bus);

Re: [PATCH 5/7] soundwire: intel: Don't disable interrupt until children are removed

[Pierre-Louis Bossart]

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>

Re: [PATCH 5/7] soundwire: intel: Don't disable interrupt until children are removed

[Richard Fitzgerald]

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.