[PATCH 0/2] Fix minor issue in SoundWire slave IRQ mapping

[PATCH 0/2] Fix minor issue in SoundWire slave IRQ mapping

[Charles Keepax]

Currently there would be problems if multiple devices on the same bus
attempted to use SoundWire IRQ handling rather than the IRQ callback
mechanism. So far only cs42l43 uses this system so this hasn't caused
any problems.

Thanks,
Charles

Charles Keepax (2):
  soundwire: bus: Simplify sdw_assign_device_num()
  soundwire: bus: Add internal slave ID and use for IRQs

 drivers/soundwire/bus.c       | 37 ++++++++++++++---------------------
 drivers/soundwire/bus_type.c  | 13 ++++++++++++
 drivers/soundwire/irq.c       |  4 ++--
 include/linux/soundwire/sdw.h |  5 +++++
 4 files changed, 35 insertions(+), 24 deletions(-)

-- 
2.39.5

[PATCH 1/2] soundwire: bus: Simplify sdw_assign_device_num()

[Charles Keepax]

Simplify the code in sdw_assign_device_num(). Remove the new_device
flag which can be simply handled inline and do a bit less shuffling of
dev_num in and out of various variables. This patch should cause no
functional changes.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/bus.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 39aecd34c641..3f1d8ff55022 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -751,41 +751,36 @@ static int sdw_get_device_num(struct sdw_slave *slave)
 static int sdw_assign_device_num(struct sdw_slave *slave)
 {
 	struct sdw_bus *bus = slave->bus;
-	int ret, dev_num;
-	bool new_device = false;
+	struct device *dev = bus->dev;
+	int ret;
 
 	/* check first if device number is assigned, if so reuse that */
 	if (!slave->dev_num) {
 		if (!slave->dev_num_sticky) {
+			int dev_num;
+
 			mutex_lock(&slave->bus->bus_lock);
 			dev_num = sdw_get_device_num(slave);
 			mutex_unlock(&slave->bus->bus_lock);
 			if (dev_num < 0) {
-				dev_err(bus->dev, "Get dev_num failed: %d\n",
-					dev_num);
+				dev_err(dev, "Get dev_num failed: %d\n", dev_num);
 				return dev_num;
 			}
-			slave->dev_num = dev_num;
+
 			slave->dev_num_sticky = dev_num;
-			new_device = true;
 		} else {
-			slave->dev_num = slave->dev_num_sticky;
+			dev_dbg(dev, "Slave already registered, reusing dev_num: %d\n",
+				slave->dev_num_sticky);
 		}
 	}
 
-	if (!new_device)
-		dev_dbg(bus->dev,
-			"Slave already registered, reusing dev_num:%d\n",
-			slave->dev_num);
-
 	/* Clear the slave->dev_num to transfer message on device 0 */
-	dev_num = slave->dev_num;
 	slave->dev_num = 0;
 
-	ret = sdw_write_no_pm(slave, SDW_SCP_DEVNUMBER, dev_num);
+	ret = sdw_write_no_pm(slave, SDW_SCP_DEVNUMBER, slave->dev_num_sticky);
 	if (ret < 0) {
-		dev_err(bus->dev, "Program device_num %d failed: %d\n",
-			dev_num, ret);
+		dev_err(dev, "Program device_num %d failed: %d\n",
+			slave->dev_num_sticky, ret);
 		return ret;
 	}
 
@@ -793,7 +788,7 @@ static int sdw_assign_device_num(struct sdw_slave *slave)
 	slave->dev_num = slave->dev_num_sticky;
 
 	if (bus->ops && bus->ops->new_peripheral_assigned)
-		bus->ops->new_peripheral_assigned(bus, slave, dev_num);
+		bus->ops->new_peripheral_assigned(bus, slave, slave->dev_num);
 
 	return 0;
 }
-- 
2.39.5

[PATCH 2/2] soundwire: bus: Add internal slave ID and use for IRQs

[Charles Keepax]

Currently the SoundWire IRQ code uses the dev_num to create an IRQ
mapping for each slave. However, there is an issue there, the dev_num
is only allocated when the slave enumerates on the bus and enumeration
may happen before or after probe of the slave driver. In the case
enumeration happens after probe of the slave driver then the IRQ
mapping will use dev_num before it is set. This could cause multiple
slaves to use zero as their IRQ mapping.

It is very desirable to have the IRQ mapped before the slave probe
is called, so drivers can do resource allocation in probe as normal. To
solve these issues add an internal ID created for each slave when it is
probed and use that for mapping the IRQ. In the case that
get_device_num() is not implemented this ID can also be reused for the
dev_num.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/bus.c       |  8 +++-----
 drivers/soundwire/bus_type.c  | 13 +++++++++++++
 drivers/soundwire/irq.c       |  4 ++--
 include/linux/soundwire/sdw.h |  5 +++++
 4 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 3f1d8ff55022..f5fa92f3deae 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -56,6 +56,8 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
 		return ret;
 	}
 
+	ida_init(&bus->slave_ida);
+
 	ret = sdw_master_device_add(bus, parent, fwnode);
 	if (ret < 0) {
 		dev_err(parent, "Failed to add master device at link %d\n",
@@ -731,11 +733,7 @@ static int sdw_get_device_num(struct sdw_slave *slave)
 		if (bit < 0)
 			goto err;
 	} else {
-		bit = find_first_zero_bit(bus->assigned, SDW_MAX_DEVICES);
-		if (bit == SDW_MAX_DEVICES) {
-			bit = -ENODEV;
-			goto err;
-		}
+		bit = slave->index;
 	}
 
 	/*
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index e98d5db81b1c..71aa307a5ac4 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -105,9 +105,19 @@ static int sdw_drv_probe(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = ida_alloc_range(&slave->bus->slave_ida, SDW_ENUM_DEV_NUM + 1,
+			      SDW_MAX_DEVICES, GFP_KERNEL);
+	if (ret < 0) {
+		dev_err(dev, "Failed to allocated ID: %d\n", ret);
+		return ret;
+	}
+	slave->index = ret;
+
 	ret = drv->probe(slave, id);
 	if (ret) {
 		dev_pm_domain_detach(dev, false);
+		ida_free(&slave->bus->slave_ida, slave->index);
+		slave->index = SDW_ENUM_DEV_NUM;
 		return ret;
 	}
 
@@ -174,6 +184,9 @@ static int sdw_drv_remove(struct device *dev)
 
 	dev_pm_domain_detach(dev, false);
 
+	ida_free(&slave->bus->slave_ida, slave->index);
+	slave->index = SDW_ENUM_DEV_NUM;
+
 	return ret;
 }
 
diff --git a/drivers/soundwire/irq.c b/drivers/soundwire/irq.c
index c237e6d0766b..98829290d134 100644
--- a/drivers/soundwire/irq.c
+++ b/drivers/soundwire/irq.c
@@ -50,12 +50,12 @@ static void sdw_irq_dispose_mapping(void *data)
 {
 	struct sdw_slave *slave = data;
 
-	irq_dispose_mapping(irq_find_mapping(slave->bus->domain, slave->dev_num));
+	irq_dispose_mapping(slave->irq);
 }
 
 void sdw_irq_create_mapping(struct sdw_slave *slave)
 {
-	slave->irq = irq_create_mapping(slave->bus->domain, slave->dev_num);
+	slave->irq = irq_create_mapping(slave->bus->domain, slave->index);
 	if (!slave->irq)
 		dev_warn(&slave->dev, "Failed to map IRQ\n");
 
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 2362f621d94c..7dbbd38c66bd 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -8,6 +8,7 @@
 #include <linux/bug.h>
 #include <linux/completion.h>
 #include <linux/device.h>
+#include <linux/idr.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/lockdep_types.h>
@@ -630,6 +631,7 @@ struct sdw_slave_ops {
  * struct sdw_slave - SoundWire Slave
  * @id: MIPI device ID
  * @dev: Linux device
+ * @index: internal ID for this slave
  * @irq: IRQ number
  * @status: Status reported by the Slave
  * @bus: Bus handle
@@ -661,6 +663,7 @@ struct sdw_slave_ops {
 struct sdw_slave {
 	struct sdw_slave_id id;
 	struct device dev;
+	int index;
 	int irq;
 	enum sdw_slave_status status;
 	struct sdw_bus *bus;
@@ -968,6 +971,7 @@ struct sdw_stream_runtime {
  * @md: Master device
  * @bus_lock_key: bus lock key associated to @bus_lock
  * @bus_lock: bus lock
+ * @slave_ida: IDA for allocating internal slave IDs
  * @slaves: list of Slaves on this bus
  * @msg_lock_key: message lock key associated to @msg_lock
  * @msg_lock: message lock
@@ -1010,6 +1014,7 @@ struct sdw_bus {
 	struct sdw_master_device *md;
 	struct lock_class_key bus_lock_key;
 	struct mutex bus_lock;
+	struct ida slave_ida;
 	struct list_head slaves;
 	struct lock_class_key msg_lock_key;
 	struct mutex msg_lock;
-- 
2.39.5

Re: [PATCH 2/2] soundwire: bus: Add internal slave ID and use for IRQs

[Pierre-Louis Bossart]

On 4/22/25 12:42, Charles Keepax wrote:
> Currently the SoundWire IRQ code uses the dev_num to create an IRQ
> mapping for each slave. However, there is an issue there, the dev_num
> is only allocated when the slave enumerates on the bus and enumeration
> may happen before or after probe of the slave driver. In the case
> enumeration happens after probe of the slave driver then the IRQ
> mapping will use dev_num before it is set. This could cause multiple
> slaves to use zero as their IRQ mapping.
> 
> It is very desirable to have the IRQ mapped before the slave probe
> is called, so drivers can do resource allocation in probe as normal. To
> solve these issues add an internal ID created for each slave when it is
> probed and use that for mapping the IRQ. In the case that
> get_device_num() is not implemented this ID can also be reused for the
> dev_num.

Humm, I am missing something. Does this work in the case of spurious 'ghost' ACPI devices, which is quite common for Intel DSDT?

In this case, each 'ghost' device would be assigned a dev_num during the driver probe, but that dev_num would never be used for enumeration, would it?

Let's assume for the sake of the argument there are 16 ghost devices and only one real device that gets enumerated. How can we guarantee that the real device is enumerated with a dev_num <= 11 (the max number of devices supported on the bus)?

I see the patch add a limit during probe, so now that means the number of ACPI devices MUST be lower than 11. That doesn't sound right to me and could cause some configurations to fail. It's perfectly ok to have ghost devices and no limits on how many our BIOS colleagues decide to list.

Using a dedicated IDA for IRQ mapping looks like a good idea to me, but I don't think you can really use the same IDA for dev_num

Re: [PATCH 2/2] soundwire: bus: Add internal slave ID and use for IRQs

[Charles Keepax]

On Tue, Apr 22, 2025 at 01:50:13PM +0200, Pierre-Louis Bossart wrote:
> On 4/22/25 12:42, Charles Keepax wrote:
> I see the patch add a limit during probe, so now that
> means the number of ACPI devices MUST be lower than 11. That
> doesn't sound right to me and could cause some configurations
> to fail. It's perfectly ok to have ghost devices and no limits
> on how many our BIOS colleagues decide to list.

Yeah it will limit the ACPI to 11 devices. I can't say I am a
huge fan of the "ghost" devices, like its ACPI it is for
describing the hardware, so it should describe the hardware.

That said my thinking on this was I have not seen a system with
more than 4 real devices on a single bus, and not more than a
couple ghosts in the ACPI. So this didn't look like a big issue.

> Using a dedicated IDA for IRQ mapping looks like a good
> idea to me, but I don't think you can really use the same IDA
> for dev_num

If you are really concerned about the ghost devices I could back
out the part that reuses the ID for the dev_num. However I do
need to know the maximum number of devices when the IRQ domain is
allocated. So I can't really see a way to avoid picking a maximum
number of devices for the bus. What number of real + ghosts would
you be comfortable with? Although I guess if not using it for the
dev_num it is then fairly easy to expand if needed so perhaps I
just back out the dev_num bit, but stick with 11 for now and we
can expand if necessary?

Thanks,
Charles

Re: [PATCH 2/2] soundwire: bus: Add internal slave ID and use for IRQs

[Pierre-Louis Bossart]

On 4/22/25 14:11, Charles Keepax wrote:
> On Tue, Apr 22, 2025 at 01:50:13PM +0200, Pierre-Louis Bossart wrote:
>> On 4/22/25 12:42, Charles Keepax wrote:
>> I see the patch add a limit during probe, so now that
>> means the number of ACPI devices MUST be lower than 11. That
>> doesn't sound right to me and could cause some configurations
>> to fail. It's perfectly ok to have ghost devices and no limits
>> on how many our BIOS colleagues decide to list.
> 
> Yeah it will limit the ACPI to 11 devices. I can't say I am a
> huge fan of the "ghost" devices, like its ACPI it is for
> describing the hardware, so it should describe the hardware.
> 
> That said my thinking on this was I have not seen a system with
> more than 4 real devices on a single bus, and not more than a
> couple ghosts in the ACPI. So this didn't look like a big issue.
> 
>> Using a dedicated IDA for IRQ mapping looks like a good
>> idea to me, but I don't think you can really use the same IDA
>> for dev_num
> 
> If you are really concerned about the ghost devices I could back
> out the part that reuses the ID for the dev_num. However I do
> need to know the maximum number of devices when the IRQ domain is
> allocated. So I can't really see a way to avoid picking a maximum
> number of devices for the bus. What number of real + ghosts would
> you be comfortable with? Although I guess if not using it for the
> dev_num it is then fairly easy to expand if needed so perhaps I
> just back out the dev_num bit, but stick with 11 for now and we
> can expand if necessary?

A maximum of 16 devices total is probably ok. That's 10 more than the worst-case configuration we've seen so far, and I can't think of a case where more than 10 ghost devices would be listed.