[PATCH v2 0/3] Allow SoundWire devices to communicate during remove

[PATCH v2 0/3] Allow SoundWire devices to communicate during remove

[Charles Keepax]

Currently on Intel systems SoundWire drivers can't communicate with the
device during driver removal. This is primarily because the IRQs are
disabled before the driver remove callback is run. The result of this is
such transactions timeout causing a) a lot of errors in the log and b)
driver remove to take a very long time.

This issue affects cs42l43 and cs42l45, primarily due to both using
regmap IRQ.

 soundwire_intel soundwire_intel.link.0: IO transfer timed out, cmd 3 device 6 addr 5d len 1
 soundwire sdw-master-0-0: trf on Slave 6 failed:-110 write addr 5d count 0
 sdca_class sdw:0:0:01fa:4245:01: Failed to sync masks in 5d

As regmap IRQ is torn down it will mask the interrupts that are
removed. However, there are many valid reasons a driver might want
communicate with the device during removal, others would include
disabling jack detection, putting the device into the lowest possible
power state to save power, etc.

This patch set attempts to address this problem trying to locate the
reason interrupts are disabled, fixing that and then leaving the IRQs
enabled for the remove callback.

Thanks,
Charles

Changes since v1:
 - Add a new helper to destroy the children on the bus separately, this
   allows the IRQs to be disabled for final cleanup.
 - Split device_unregister into device_del and put_device.

Changes since v1:
 - Add a new helper to destroy the children on the bus separately, this
   allows the IRQs to be disabled for final cleanup.
 - Split device_unregister into device_del and put_device.

Charles Keepax (3):
  soundwire: bus: Don't unassign dev_num before unregistering device
  soundwire: bus: Expose a helper to remove devices from the bus
  soundwire: intel_auxdevice: Don't disable IRQs before removing
    children

 drivers/soundwire/bus.c             | 45 +++++++++++++++++++++--------
 drivers/soundwire/intel.h           |  1 +
 drivers/soundwire/intel_auxdevice.c |  7 ++++-
 drivers/soundwire/intel_init.c      | 21 ++++++++++++++
 include/linux/soundwire/sdw.h       |  1 +
 include/linux/soundwire/sdw_intel.h |  1 +
 6 files changed, 63 insertions(+), 13 deletions(-)

-- 
2.47.3

[PATCH v2 1/3] soundwire: bus: Don't unassign dev_num before unregistering device

[Charles Keepax]

Don't mark dev_num as unassigned until after the driver remove has
been called. The driver may want to communicate with the device
as part of the driver remove operation, so the dev_num should
remain assigned until that has completed. This requires moving from
device_unregister() to manually calling device_del() and device_put()
to ensure that the slave struct isn't freed too early.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Split device_unregister into device_del and put_device

 drivers/soundwire/bus.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index aeaae5a57c89d..1488b6540844c 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -175,8 +175,9 @@ static int sdw_delete_slave(struct device *dev, void *data)
 
 	sdw_slave_debugfs_exit(slave);
 
-	mutex_lock(&bus->bus_lock);
+	device_del(dev);
 
+	mutex_lock(&bus->bus_lock);
 	if (slave->dev_num) { /* clear dev_num if assigned */
 		clear_bit(slave->dev_num, bus->assigned);
 		if (bus->ops && bus->ops->put_device_num)
@@ -185,7 +186,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
 	list_del_init(&slave->node);
 	mutex_unlock(&bus->bus_lock);
 
-	device_unregister(dev);
+	put_device(dev);
+
 	return 0;
 }
 
-- 
2.47.3

[PATCH v2 2/3] soundwire: bus: Expose a helper to remove devices from the bus

[Charles Keepax]

Some SoundWire controllers may have features such as IRQs that are
needed to support peripheral operation, it is desirable to continue
to support these whilst peripheral drivers are removed. However,
the current sdw_bus_master_delete() monolithically removes the
peripherals and destroys the controller giving no chance for the
controller driver to perform clean up after the slaves are removed,
but before it is itself destroyed.

Split out a separate helper function that only removes the
peripherals, this will allow drivers that require this to sequence
things appropriately. The functionality of the current helper is
left as is, if the peripherals have already been removed the call
to do this is a no-op which allows drivers that don't need this
functionality to remain untouched.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

New since v1.

 drivers/soundwire/bus.c       | 43 +++++++++++++++++++++++++----------
 include/linux/soundwire/sdw.h |  1 +
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 1488b6540844c..4c4e69a58fbef 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -169,7 +169,6 @@ EXPORT_SYMBOL(sdw_bus_master_add);
 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);
 
@@ -177,18 +176,37 @@ static int sdw_delete_slave(struct device *dev, void *data)
 
 	device_del(dev);
 
-	mutex_lock(&bus->bus_lock);
-	if (slave->dev_num) { /* clear dev_num if assigned */
-		clear_bit(slave->dev_num, bus->assigned);
-		if (bus->ops && bus->ops->put_device_num)
-			bus->ops->put_device_num(bus, slave);
-	}
-	list_del_init(&slave->node);
-	mutex_unlock(&bus->bus_lock);
+	return 0;
+}
 
-	put_device(dev);
+/**
+ * sdw_bus_slaves_delete() - delete all peripherals on a bus
+ * @bus: bus with peripherals to be deleted
+ *
+ * Delete the child devices.
+ */
+void sdw_bus_slaves_delete(struct sdw_bus *bus)
+{
+	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
+}
+EXPORT_SYMBOL(sdw_bus_slaves_delete);
 
-	return 0;
+static void sdw_bus_slaves_put(struct sdw_bus *bus)
+{
+	struct sdw_slave *slave, *tmp;
+
+	list_for_each_entry_safe(slave, tmp, &bus->slaves, node) {
+		mutex_lock(&bus->bus_lock);
+		if (slave->dev_num) { /* clear dev_num if assigned */
+			clear_bit(slave->dev_num, bus->assigned);
+			if (bus->ops && bus->ops->put_device_num)
+				bus->ops->put_device_num(bus, slave);
+		}
+		list_del_init(&slave->node);
+		mutex_unlock(&bus->bus_lock);
+
+		put_device(&slave->dev);
+	}
 }
 
 /**
@@ -199,7 +217,8 @@ 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);
+	sdw_bus_slaves_delete(bus);
+	sdw_bus_slaves_put(bus);
 
 	sdw_irq_delete(bus);
 
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index f710e5932b4b2..df9ee56493b17 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -899,6 +899,7 @@ struct sdw_master_ops {
 
 int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
 		       struct fwnode_handle *fwnode);
+void sdw_bus_slaves_delete(struct sdw_bus *bus);
 void sdw_bus_master_delete(struct sdw_bus *bus);
 
 void sdw_show_ping_status(struct sdw_bus *bus, bool sync_delay);
-- 
2.47.3

[PATCH v2 3/3] soundwire: intel_auxdevice: Don't disable IRQs before removing children

[Charles Keepax]

Currently the auxiliary device for the link disables IRQs before it
calls sdw_bus_master_delete(). This has the side effect that none
of the devices on the link can access their own registers whilst
their remove functions run, because the IRQs are required for bus
transactions to function.

There appear to be two things that currently block leaving the
IRQs enabled during peripheral removal. Firstly, the IRQ handler
iterates through a linked list of all the links, once a link is
removed the memory pointed at by this linked list is freed, but
not removed from the linked_list. Secondly, the potential that
an IRQ runs after the controller itself has been destroyed.

For the first problem add a list_del() for the linked list item,
note whilst the list itself is contained in the intel_init portion
of the code, the list remove needs to be attached to the auxiliary
device for the link, since that owns the memory that the list points
at. Locking is also required to ensure the IRQ handler runs either
before or after any additions/removals from the list. A new lock is
added for this, the shim_lock is used to gate access to the shared
registers so doesn't feel a super obvious fit for managing the list.

For the second problem utilise the newly added helper that allows
destroying the peripherals separately, allowing the auxiliary device
to disable IRQs before running its own cleanup.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Use new sdw_bus_slaves_delete() helper, which allows us to disable
   IRQs before destroying the master completely.

 drivers/soundwire/intel.h           |  1 +
 drivers/soundwire/intel_auxdevice.c |  7 ++++++-
 drivers/soundwire/intel_init.c      | 21 +++++++++++++++++++++
 include/linux/soundwire/sdw_intel.h |  1 +
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index 7a2e7e73ad632..ec82ac8d54adb 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -47,6 +47,7 @@ struct sdw_intel_link_res {
 	u32 link_mask;
 	struct sdw_cdns *cdns;
 	struct list_head list;
+	struct mutex *link_lock; /* lock protecting list */
 	struct hdac_bus *hbus;
 };
 
diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
index 901a71262094f..1439cef43462d 100644
--- a/drivers/soundwire/intel_auxdevice.c
+++ b/drivers/soundwire/intel_auxdevice.c
@@ -508,8 +508,13 @@ static void intel_link_remove(struct auxiliary_device *auxdev)
 	if (!bus->prop.hw_disabled) {
 		sdw_intel_debugfs_exit(sdw);
 		cancel_delayed_work_sync(&cdns->attach_dwork);
-		sdw_cdns_enable_interrupt(cdns, false);
 	}
+
+	sdw_bus_slaves_delete(bus);
+
+	if (!bus->prop.hw_disabled)
+		sdw_cdns_enable_interrupt(cdns, false);
+
 	sdw_bus_master_delete(bus);
 }
 
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index ad48d67fa9358..a7437cd420288 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -28,6 +28,15 @@ static void intel_link_dev_release(struct device *dev)
 	kfree(ldev);
 }
 
+static void intel_link_list_del(void *data)
+{
+	struct sdw_intel_link_res *link = data;
+
+	mutex_lock(link->link_lock);
+	list_del(&link->list);
+	mutex_unlock(link->link_lock);
+}
+
 /* alloc, init and add link devices */
 static struct sdw_intel_link_dev *intel_link_dev_register(struct sdw_intel_res *res,
 							  struct sdw_intel_ctx *ctx,
@@ -79,6 +88,7 @@ static struct sdw_intel_link_dev *intel_link_dev_register(struct sdw_intel_res *
 		link->shim_lock = res->eml_lock;
 		link->mic_privacy = res->mic_privacy;
 	}
+	link->link_lock = &ctx->link_lock;
 
 	link->ops = res->ops;
 	link->dev = res->dev;
@@ -145,8 +155,10 @@ irqreturn_t sdw_intel_thread(int irq, void *dev_id)
 	struct sdw_intel_ctx *ctx = dev_id;
 	struct sdw_intel_link_res *link;
 
+	mutex_lock(&ctx->link_lock);
 	list_for_each_entry(link, &ctx->link_list, list)
 		sdw_cdns_irq(irq, link->cdns);
+	mutex_unlock(&ctx->link_lock);
 
 	return IRQ_HANDLED;
 }
@@ -165,6 +177,7 @@ static struct sdw_intel_ctx
 	u32 link_mask;
 	int num_slaves = 0;
 	int count;
+	int ret;
 	int i;
 
 	if (!res)
@@ -210,6 +223,7 @@ static struct sdw_intel_ctx
 	ctx->link_mask = res->link_mask;
 	ctx->handle = res->handle;
 	mutex_init(&ctx->shim_lock);
+	mutex_init(&ctx->link_lock);
 
 	link_mask = ctx->link_mask;
 
@@ -246,7 +260,14 @@ static struct sdw_intel_ctx
 			i++;
 			goto err;
 		}
+
+		mutex_lock(&ctx->link_lock);
 		list_add_tail(&link->list, &ctx->link_list);
+		mutex_unlock(&ctx->link_lock);
+		ret = devm_add_action_or_reset(&ldev->auxdev.dev, intel_link_list_del, link);
+		if (ret)
+			goto err;
+
 		bus = &link->cdns->bus;
 		/* Calculate number of slaves */
 		list_for_each(node, &bus->slaves)
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h
index 9710f2dc04e29..7495d35ed2fc3 100644
--- a/include/linux/soundwire/sdw_intel.h
+++ b/include/linux/soundwire/sdw_intel.h
@@ -307,6 +307,7 @@ struct sdw_intel_ctx {
 	acpi_handle handle;
 	struct sdw_intel_link_dev **ldev;
 	struct list_head link_list;
+	struct mutex link_lock; /* lock protecting link_list */
 	struct mutex shim_lock; /* lock for access to shared SHIM registers */
 	u32 shim_mask;
 	u32 shim_base;
-- 
2.47.3