Skip to contentSkip to conversation

i3c: Support IBI-based system wakeup

RFC PATCH Superseded Adrian Hunter 3 participants 17 messages Jul 1 – Jul 2

Subsystems (2)

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

cover i3c: Support IBI-based system wakeup

Hi

[ Re-sent to correct email addresses of linux-pci and linux-pm mailing lists ]

Intel LPSS I3C controllers support up to two I3C busses and can wake the system from an In-Band Interrupt (IBI) via a PCI PME. Today that wakeup capability lives only at the PCI function, with no way to express which I3C device is actually responsible for waking the system, and no way for user space to enable or disable wakeup on a per-device basis.

This series pushes the wakeup capability down to the individual I3C devices and then aggregates the resulting wakeup state back up to the PCI device.

An IBI-capable I3C device on a bus whose controller can wake the system is marked as wakeup capable, so it can be managed through the standard device wakeup framework (e.g. via power/wakeup in sysfs). When such a device is enabled for wakeup, a wakeup event is reported each time it queues an IBI.

At suspend time, the mipi-i3c-hci PCI driver aggregates the wakeup configuration of the I3C devices across its HCI instances (up to two I3C busses) and enables PCI wakeup (PME) only when at least one attached I3C device is enabled as a wakeup source. This keeps the PCI wakeup state in sync with the actual requirements of the devices on the busses.

The series is organised as follows:

- Patches 1-4 add the generic I3C core support: an ibi_wakeup flag for controllers, marking IBI-capable devices as wakeup capable, reporting wakeup events on IBIs, a helper to query whether any device on a bus has wakeup enabled, and a fix to reject IBI requests from devices that do not advertise IBI capability.

- Patches 5-7 wire this up for the mipi-i3c-hci driver: propagate the aggregated I3C wakeup requirements to the PCI function, factor out i3c_hci_sysdev() for the shared device lookup, and advertise IBI wakeup capability when the underlying system device can wake the system.

Note, since the PCI wakeup state is now derived from the wakeup configuration of the attached I3C devices, the PCI device's power/wakeup sysfs attribute no longer provides independent wakeup control.

Adrian Hunter (7):
i3c: master: Support IBI-based wakeup capability
i3c: master: Report wakeup events for IBIs
i3c: master: Add helper to query bus wakeup requirements
i3c: master: Reject IBI requests from non-IBI-capable devices
i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI
i3c: mipi-i3c-hci: Factor out i3c_hci_sysdev()
i3c: mipi-i3c-hci: Advertise IBI wakeup capability

 drivers/i3c/device.c                               |  8 +++--
 drivers/i3c/master.c                               | 41 ++++++++++++++++++++++
 drivers/i3c/master/mipi-i3c-hci/core.c             | 15 ++++++++
 drivers/i3c/master/mipi-i3c-hci/dma.c              | 15 +-------
 drivers/i3c/master/mipi-i3c-hci/hci.h              |  2 ++
 drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c | 23 ++++++++++--
 include/linux/i3c/master.h                         |  3 ++
 7 files changed, 88 insertions(+), 19 deletions(-)

Regards
Adrian

#1 lore raw

RFC PATCH 1/7 i3c: master: Support IBI-based wakeup capability

An I3C controller acts as a bus controller for one or more I3C devices. If the controller can wake the system in response to an In-Band Interrupt (IBI), then any device on that bus that is capable of generating IBIs can potentially be used as a wakeup source.

Add an ibi_wakeup flag to struct i3c_master_controller so controller drivers can advertise support for IBI-based wakeup.

If set, mark IBI-capable I3C devices as wakeup capable when they are registered, allowing wakeup management through the standard device wakeup framework.

---
 drivers/i3c/master.c       | 8 ++++++++
 include/linux/i3c/master.h | 2 ++
 2 files changed, 10 insertions(+)
drivers/i3c/master.c +8 -0
index f1be38a640ca..a47e2924bb70 100644
@@ -17,6 +17,7 @@
 #include <linux/list.h>
 #include <linux/of.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_wakeup.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
@@ -1930,6 +1931,13 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
 		if (desc->boardinfo)
 			desc->dev->dev.of_node = desc->boardinfo->of_node;
 
+		/*
+		 * In the case of IBI wakeup, any IBI-capable device can
+		 * wakeup.
+		 */
+		if (master->ibi_wakeup && (desc->info.bcr & I3C_BCR_IBI_REQ_CAP))
+			device_set_wakeup_capable(&desc->dev->dev, true);
+
 		ret = device_register(&desc->dev->dev);
 		if (ret) {
 			dev_err(&master->dev,
include/linux/i3c/master.h +2 -0
index 4d2a68793324..af3c35a8e7b6 100644
@@ -511,6 +511,7 @@ struct i3c_master_controller_ops {
  * @hotjoin: true if the master support hotjoin
  * @rpm_allowed: true if Runtime PM allowed
  * @rpm_ibi_allowed: true if IBI and Hot-Join allowed while runtime suspended
+ * @ibi_wakeup: IBI can wakeup the system
  * @shutting_down: set to true when master begins shutdown or unregister
  * @boardinfo.i3c: list of I3C  boardinfo objects
  * @boardinfo.i2c: list of I2C boardinfo objects
@@ -545,6 +546,7 @@ struct i3c_master_controller {
 	unsigned int hotjoin: 1;
 	unsigned int rpm_allowed: 1;
 	unsigned int rpm_ibi_allowed: 1;
+	unsigned int ibi_wakeup: 1;
 	bool shutting_down;
 	struct {
 		struct list_head i3c;
Signature · 1 line
2.53.0
#2 lore raw

Re: [PATCH RFC RESEND 1/7] i3c: master: Support IBI-based wakeup capability

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free and double-free of I3C device due to dangling pointer on device_register() failure.
- [High] Reference count underflow of OF node due to missing of_node_get() during device registration. --

commit 9efcfa1df1912edf83596e1ebd38559fc710aaca
Author: Adrian Hunter <[email protected]>

i3c: master: Support IBI-based wakeup capability

This patch adds an ibi_wakeup flag to the I3C master controller. It allows IBI-capable I3C devices to be marked as wakeup capable during registration if the controller supports waking the system via In-Band Interrupts.

Quoted 3 lines
drivers/i3c/master.c +0 -0

[ ... ]

drivers/i3c/master.c i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) · line 1932

Quoted 3 lines
drivers/i3c/master.c +0 -0
@@ -1930,6 +1931,13 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
 		if (desc->boardinfo)
 			desc->dev->dev.of_node = desc->boardinfo->of_node;

[Severity: High]
This isn't a bug introduced by this patch, but does this assignment cause an OF node reference count underflow?

The pointer is assigned without taking a new reference via of_node_get(). However, when the device is freed, i3c_device_release() unconditionally calls of_node_put(i3cdev->dev.of_node).

If device_register() fails just below this, or when the device is eventually unregistered, the release function will drop a reference that was never acquired for the device itself.

Quoted 12 lines
drivers/i3c/master.c +7 -0
+		/*
+		 * In the case of IBI wakeup, any IBI-capable device can
+		 * wakeup.
+		 */
+		if (master->ibi_wakeup && (desc->info.bcr & I3C_BCR_IBI_REQ_CAP))
+			device_set_wakeup_capable(&desc->dev->dev, true);
+
 		ret = device_register(&desc->dev->dev);
 		if (ret) {
 			dev_err(&master->dev,
 				"Failed to add I3C device (err = %d)\n", ret);
Quoted 1 line

put_device(&desc->dev->dev);

[Severity: High]
This is a pre-existing issue, but does this error path leave a dangling pointer that causes a use-after-free and double-free later?

If device_register() fails, put_device() is called which frees desc->dev via i3c_device_release(), but desc->dev is never set to NULL.

Later, during controller teardown, i3c_master_unregister_i3c_devs() iterates over the bus and checks if (!i3cdev->dev). Since the pointer wasn't cleared, it proceeds to dereference it and free it again:

i3c_master_unregister_i3c_devs() {
    ...
    i3c_bus_for_each_i3cdev(&master->bus, i3cdev) {
        if (!i3cdev->dev)
            continue;

        i3cdev->dev->desc = NULL;
        ...
        else
            put_device(&i3cdev->dev->dev);
}

Should desc->dev be set to NULL after the put_device() call here?

Quoted 1 line

}

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Reviewed-by on v1 1/7
#3 lore raw

Re: [PATCH RFC RESEND 1/7] i3c: master: Support IBI-based wakeup capability

In reply to Adrian Hunter
Quoted 8 lines · Adrian Hunter

An I3C controller acts as a bus controller for one or more I3C devices. If the controller can wake the system in response to an In-Band Interrupt (IBI), then any device on that bus that is capable of generating IBIs can potentially be used as a wakeup source.

Add an ibi_wakeup flag to struct i3c_master_controller so controller drivers can advertise support for IBI-based wakeup.

Quoted 6 lines

If set, mark IBI-capable I3C devices as wakeup capable when they are registered, allowing wakeup management through the standard device wakeup framework.

---

Quoted 53 lines
 drivers/i3c/master.c       | 8 ++++++++
 include/linux/i3c/master.h | 2 ++
 2 files changed, 10 insertions(+)
drivers/i3c/master.c +8 -0
index f1be38a640ca..a47e2924bb70 100644
@@ -17,6 +17,7 @@
 #include <linux/list.h>
 #include <linux/of.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_wakeup.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
@@ -1930,6 +1931,13 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
 		if (desc->boardinfo)
 			desc->dev->dev.of_node = desc->boardinfo->of_node;
 
+		/*
+		 * In the case of IBI wakeup, any IBI-capable device can
+		 * wakeup.
+		 */
+		if (master->ibi_wakeup && (desc->info.bcr & I3C_BCR_IBI_REQ_CAP))
+			device_set_wakeup_capable(&desc->dev->dev, true);
+
 		ret = device_register(&desc->dev->dev);
 		if (ret) {
 			dev_err(&master->dev,
include/linux/i3c/master.h +2 -1
index 4d2a68793324..af3c35a8e7b6 100644
@@ -511,6 +511,7 @@ struct i3c_master_controller_ops {
  * @hotjoin: true if the master support hotjoin
  * @rpm_allowed: true if Runtime PM allowed
  * @rpm_ibi_allowed: true if IBI and Hot-Join allowed while runtime suspended
+ * @ibi_wakeup: IBI can wakeup the system
  * @shutting_down: set to true when master begins shutdown or unregister
  * @boardinfo.i3c: list of I3C  boardinfo objects
  * @boardinfo.i2c: list of I2C boardinfo objects
@@ -545,6 +546,7 @@ struct i3c_master_controller {
 	unsigned int hotjoin: 1;
 	unsigned int rpm_allowed: 1;
 	unsigned int rpm_ibi_allowed: 1;
+	unsigned int ibi_wakeup: 1;
 	bool shutting_down;
 	struct {
 		struct list_head i3c;
--

2.53.0

#4 lore raw

RFC PATCH 2/7 i3c: master: Report wakeup events for IBIs

In reply to Adrian Hunter

An I3C device configured as a wakeup source can wake the system by generating an In-Band Interrupt (IBI).

When an IBI is queued for processing, record a wakeup event for the device if wakeup is enabled. Use a 100 ms processing interval to give the I3C device driver time to process the IBI.

---
 drivers/i3c/master.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/i3c/master.c +6 -0
index a47e2924bb70..a36f9b617cbf 100644
@@ -2994,6 +2994,9 @@ static void i3c_master_unregister_i3c_devs(struct i3c_master_controller *master)
 	}
 }
 
+/* Approximate time for IBI handler to run */
+#define I3C_WAKEUP_PROCESSING_TIME_MS 100
+
 /**
  * i3c_master_queue_ibi() - Queue an IBI
  * @dev: the device this IBI is coming from
@@ -3007,6 +3010,9 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot)
 	if (!dev->ibi || !slot)
 		return;
 
+	if (device_may_wakeup(&dev->dev->dev))
+		pm_wakeup_event(&dev->dev->dev, I3C_WAKEUP_PROCESSING_TIME_MS);
+
 	atomic_inc(&dev->ibi->pending_ibis);
 	queue_work(dev->ibi->wq, &slot->work);
 }
Signature · 1 line
2.53.0
#5 lore raw

Re: [PATCH RFC RESEND 2/7] i3c: master: Report wakeup events for IBIs

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unconditional NULL pointer dereference of dev->dev in i3c_master_queue_ibi during device teardown

Pre-existing issues:
- [High] Lockless TOCTOU race leading to Use-After-Free of dev->ibi during concurrent IBI teardown --

commit ffdd9b066a2bdadecaab205d187dab1a5ecff5ae
Author: Adrian Hunter <[email protected]>

i3c: master: Report wakeup events for IBIs

An I3C device configured as a wakeup source can wake the system by generating an In-Band Interrupt (IBI).

When an IBI is queued for processing, record a wakeup event for the device if wakeup is enabled. Use a 100 ms processing interval to give the I3C device driver time to process the IBI.

Quoted 4 lines
drivers/i3c/master.c +0 -0
index a47e2924bb701..a36f9b617cbf7 100644

[ ... ]

drivers/i3c/master.c void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot) · line 3015

Quoted 7 lines
drivers/i3c/master.c +3 -0
@@ -3007,6 +3010,9 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot)
 	if (!dev->ibi || !slot)
 		return;
 
+	if (device_may_wakeup(&dev->dev->dev))
+		pm_wakeup_event(&dev->dev->dev, I3C_WAKEUP_PROCESSING_TIME_MS);
+

[Severity: High]
Does this code introduce a NULL pointer dereference if an interrupt arrives during device teardown?

During I3C master controller unregistration, i3c_master_unregister_i3c_devs() clears the device description and sets i3cdev->dev to NULL:

	i3cdev->dev->desc = NULL;
	if (device_is_registered(&i3cdev->dev->dev))
		device_unregister(&i3cdev->dev->dev);
	else
		put_device(&i3cdev->dev->dev);
	i3cdev->dev = NULL;

Since clearing dev->desc prevents the driver's remove callback from disabling the hardware IBI, the IBI may still fire. If i3c_master_queue_ibi() executes at this point, the newly added device_may_wakeup(&dev->dev->dev) will dereference the NULL dev->dev pointer.

drivers/i3c/master.c void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot) · line 3016

Quoted 1 line
drivers/i3c/master.c +0 -0
 	atomic_inc(&dev->ibi->pending_ibis);

[Severity: High]
This is a pre-existing issue, but does this lockless check open a use-after-free race window?

If a hardware IBI interrupt is processed concurrently on another CPU, it can evaluate if (!dev->ibi) as false, get preempted, and then access dev->ibi->pending_ibis and dev->ibi->wq after they have been freed by i3c_dev_free_ibi_locked():

	kfree(dev->ibi);
	dev->ibi = NULL;

This is possible for hardware controllers like dw-i3c-master that do not implicitly synchronize with executing hardware IRQ handlers during their disable op.

Quoted 2 lines
drivers/i3c/master.c +0 -0
 	queue_work(dev->ibi->wq, &slot->work);
 }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#6 lore raw

Re: [PATCH RFC RESEND 2/7] i3c: master: Report wakeup events for IBIs

Quoted 40 lines · On 01/07/2026 23:33, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unconditional NULL pointer dereference of dev->dev in i3c_master_queue_ibi during device teardown

Pre-existing issues:
- [High] Lockless TOCTOU race leading to Use-After-Free of dev->ibi during concurrent IBI teardown --

commit ffdd9b066a2bdadecaab205d187dab1a5ecff5ae
Author: Adrian Hunter <[email protected]>

i3c: master: Report wakeup events for IBIs

An I3C device configured as a wakeup source can wake the system by generating an In-Band Interrupt (IBI).

When an IBI is queued for processing, record a wakeup event for the device if wakeup is enabled. Use a 100 ms processing interval to give the I3C device driver time to process the IBI.

Quoted 4 lines
drivers/i3c/master.c +0 -0
index a47e2924bb701..a36f9b617cbf7 100644

[ ... ]

drivers/i3c/master.c void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot) · line 3015

Quoted 7 lines
drivers/i3c/master.c +3 -0
@@ -3007,6 +3010,9 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot)
 	if (!dev->ibi || !slot)
 		return;
 
+	if (device_may_wakeup(&dev->dev->dev))
+		pm_wakeup_event(&dev->dev->dev, I3C_WAKEUP_PROCESSING_TIME_MS);
+

[Severity: High]
Does this code introduce a NULL pointer dereference if an interrupt arrives during device teardown?

Quoted 6 lines

During I3C master controller unregistration, i3c_master_unregister_i3c_devs() clears the device description and sets i3cdev->dev to NULL:

	i3cdev->dev->desc = NULL;
	if (device_is_registered(&i3cdev->dev->dev))
		device_unregister(&i3cdev->dev->dev);

I3C drivers are required to disable and free IBIs when they are removed, and they do do that. There is a corner case, if the IBI disable fails, but that is out of scope for this patch set.

Quoted 2 lines
	else
		put_device(&i3cdev->dev->dev);

dev->ibi will be NULL for devices that are not registered, so i3c_master_queue_ibi() would never get to i3c_master_queue_ibi() even if somehow the controller driver tried to queue an IBI.

Quoted 18 lines
	i3cdev->dev = NULL;

Since clearing dev->desc prevents the driver's remove callback from disabling the hardware IBI, the IBI may still fire. If i3c_master_queue_ibi() executes at this point, the newly added device_may_wakeup(&dev->dev->dev) will dereference the NULL dev->dev pointer.

drivers/i3c/master.c void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot) · line 3016

Quoted 1 line
drivers/i3c/master.c +0 -0
 	atomic_inc(&dev->ibi->pending_ibis);

[Severity: High]
This is a pre-existing issue, but does this lockless check open a use-after-free race window?

If a hardware IBI interrupt is processed concurrently on another CPU, it can evaluate if (!dev->ibi) as false, get preempted, and then access dev->ibi->pending_ibis and dev->ibi->wq after they have been freed by i3c_dev_free_ibi_locked():

Quoted 6 lines
	kfree(dev->ibi);
	dev->ibi = NULL;

This is possible for hardware controllers like dw-i3c-master that do not implicitly synchronize with executing hardware IRQ handlers during their disable op.

Yes, some drivers have gaps.

For dw, an IBI would have to arrive immediately before the DISEC CCC and end up being processed during the same invocation of dw_i3c_master_irq_handler(). The DISEC would complete() first which would potentially allow i3c_dev_disable_ibi_locked() to atomic_read(&dev->ibi->pending_ibis) before the dw driver gets to atomic_inc(&dev->ibi->pending_ibis) via i3c_master_queue_ibi().

Out of scope for this patch set.

Quoted 4 lines
Quoted 3 lines
drivers/i3c/master.c +0 -0
 	queue_work(dev->ibi->wq, &slot->work);
 }
#7 lore raw

RFC PATCH 3/7 i3c: master: Add helper to query bus wakeup requirements

In reply to Adrian Hunter

Add i3c_master_any_wakeup_enabled(), which iterates over the devices on an I3C bus and reports whether any of them are enabled for system wakeup.

Controller drivers can use this helper to determine whether wakeup support must remain available while the system is suspended.

---
 drivers/i3c/master.c       | 27 +++++++++++++++++++++++++++
 include/linux/i3c/master.h |  1 +
 2 files changed, 28 insertions(+)
drivers/i3c/master.c +27 -0
index a36f9b617cbf..979457e635bd 100644
@@ -1957,6 +1957,33 @@ static void i3c_master_reg_work_fn(struct work_struct *work)
 	i3c_bus_normaluse_unlock(&master->bus);
 }
 
+/**
+ * i3c_master_any_wakeup_enabled() - check if any device can wake the system
+ * @master: I3C master controller
+ *
+ * Iterate over devices on the bus and return true if any device has
+ * system wakeup enabled.
+ *
+ * Return: true if any device may wake the system, false otherwise.
+ */
+bool i3c_master_any_wakeup_enabled(struct i3c_master_controller *master)
+{
+	struct i3c_dev_desc *desc;
+	bool wakeup = false;
+
+	i3c_bus_normaluse_lock(&master->bus);
+	i3c_bus_for_each_i3cdev(&master->bus, desc) {
+		if (desc->dev && device_may_wakeup(&desc->dev->dev)) {
+			wakeup = true;
+			break;
+		}
+	}
+	i3c_bus_normaluse_unlock(&master->bus);
+
+	return wakeup;
+}
+EXPORT_SYMBOL_GPL(i3c_master_any_wakeup_enabled);
+
 /**
  * i3c_master_dma_map_single() - Map buffer for single DMA transfer
  * @dev: device object of a device doing DMA
include/linux/i3c/master.h +1 -0
index af3c35a8e7b6..b9f2d84ac276 100644
@@ -746,6 +746,7 @@ void i3c_generic_ibi_recycle_slot(struct i3c_generic_ibi_pool *pool,
 				  struct i3c_ibi_slot *slot);
 
 void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot);
+bool i3c_master_any_wakeup_enabled(struct i3c_master_controller *master);
 
 struct i3c_ibi_slot *i3c_master_get_free_ibi_slot(struct i3c_dev_desc *dev);
 
Signature · 1 line
2.53.0
#8 lore raw

Re: [PATCH RFC RESEND 3/7] i3c: master: Add helper to query bus wakeup requirements

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Concurrent execution between i3c_master_any_wakeup_enabled() and the device registration worker causes data races on desc->dev because both paths use a read-side lock.

Pre-existing issues:
- [High] The i3c_master_any_wakeup_enabled() helper can dereference a dangling desc->dev pointer if a previous I3C device registration failed. --

commit 3ed97d097ea9f23fefcb772811737adab713eb66
Author: Adrian Hunter <[email protected]>

i3c: master: Add helper to query bus wakeup requirements

This commit adds i3c_master_any_wakeup_enabled(), which iterates over the devices on an I3C bus and reports whether any of them are enabled for system wakeup. Controller drivers can use this helper to determine whether wakeup support must remain available while the system is suspended.

Quoted 4 lines
drivers/i3c/master.c +0 -0
index a36f9b617cbf7..979457e635bd9 100644

[ ... ]

drivers/i3c/master.c static void i3c_master_reg_work_fn(struct work_struct *work) · line 1974

Quoted 6 lines
drivers/i3c/master.c +6 -0
+bool i3c_master_any_wakeup_enabled(struct i3c_master_controller *master)
+{
+	struct i3c_dev_desc *desc;
+	bool wakeup = false;
+
+	i3c_bus_normaluse_lock(&master->bus);

[Severity: High]
Does this code risk a data race with the device registration worker?

Looking at i3c_master_reg_work_fn(), it allocates and initializes desc->dev while holding this same read-side lock (i3c_bus_normaluse_lock). If the PCI controller runtime suspends and calls this helper while the registration work is executing, they can run concurrently since both only hold a read-side lock.

Can this lead to a use-before-initialization if device_may_wakeup() is called while the memory is still being initialized?

drivers/i3c/master.c static void i3c_master_reg_work_fn(struct work_struct *work) · line 1976

Quoted 2 lines
drivers/i3c/master.c +2 -0
+	i3c_bus_for_each_i3cdev(&master->bus, desc) {
+		if (desc->dev && device_may_wakeup(&desc->dev->dev)) {

[Severity: High]
This is a pre-existing issue, but could this check trigger a use-after-free if a previous device registration failed?

In i3c_master_register_new_i3c_devs(), if device_register() fails, the error path drops the reference with put_device(&desc->dev->dev). This frees the underlying memory, but desc->dev is never set to NULL.

If that happens, won't this loop observe a dangling non-NULL desc->dev pointer and unconditionally dereference it during suspend?

Quoted 9 lines
drivers/i3c/master.c +9 -0
+			wakeup = true;
+			break;
+		}
+	}
+	i3c_bus_normaluse_unlock(&master->bus);
+
+	return wakeup;
+}
+EXPORT_SYMBOL_GPL(i3c_master_any_wakeup_enabled);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
#9 lore raw

RFC PATCH 4/7 i3c: master: Reject IBI requests from non-IBI-capable devices

In reply to Adrian Hunter

i3c_device_request_ibi() does not verify that a device advertises IBI support before attempting to set up IBI handling.

Add a check for I3C_BCR_IBI_REQ_CAP and fail with -EOPNOTSUPP when IBI support is not reported by the device. This keeps IBI setup consistent with other IBI-related functionality, such as exposing wakeup capability only for IBI-capable devices.

---
 drivers/i3c/device.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/i3c/device.c +5 -3
index 101eaa77de68..307ca6dc04ca 100644
@@ -205,12 +205,14 @@ int i3c_device_request_ibi(struct i3c_device *dev,
 		return ret;
 
 	i3c_bus_normaluse_lock(dev->bus);
-	if (dev->desc) {
+	if (!dev->desc) {
+		ret = -ENOENT;
+	} else if (!(dev->desc->info.bcr & I3C_BCR_IBI_REQ_CAP)) {
+		ret = -EOPNOTSUPP;
+	} else {
 		mutex_lock(&dev->desc->ibi_lock);
 		ret = i3c_dev_request_ibi_locked(dev->desc, req);
 		mutex_unlock(&dev->desc->ibi_lock);
-	} else {
-		ret = -ENOENT;
 	}
 	i3c_bus_normaluse_unlock(dev->bus);
 
Signature · 1 line
2.53.0
#10 lore raw

Re: [PATCH RFC RESEND 4/7] i3c: master: Reject IBI requests from non-IBI-capable devices

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Lockless access to dev->ibi from interrupt context races with non-atomic pointer clearing, causing a potential Use-After-Free.
- [High] Recursive read lock acquisition of bus->lock leading to potential AB-BA deadlock with writers.
- [High] Memory and resource leak of the IBI workqueue on the error path of i3c_dev_request_ibi_locked.
--

commit f69cfae3be2eea35641a7d0435a4171201d362ea
Author: Adrian Hunter <[email protected]>

i3c: master: Reject IBI requests from non-IBI-capable devices

i3c_device_request_ibi() does not verify that a device advertises IBI support before attempting to set up IBI handling. Add a check for I3C_BCR_IBI_REQ_CAP and fail with -EOPNOTSUPP when IBI support is not reported by the device.

drivers/i3c/device.c int i3c_device_request_ibi(struct i3c_device *dev, · line 207

Quoted 8 lines
drivers/i3c/device.c +0 -0
index 101eaa77de686..307ca6dc04ca0 100644
@@ -205,12 +205,14 @@ int i3c_device_request_ibi(struct i3c_device *dev,
 		return ret;
 
 	i3c_bus_normaluse_lock(dev->bus);

[Severity: High]
This is a pre-existing issue, but can this cause a deadlock with writers if a writer arrives between two down_read() calls?

In drivers/i3c/master.c:i3c_master_reg_work_fn(), the first read lock is acquired before triggering synchronous device registration:

	i3c_bus_normaluse_lock(&master->bus);
	if (!master->shutting_down)
		i3c_master_register_new_i3c_devs(master);

This synchronous probe path then calls i3c_device_request_ibi(), which recursively attempts to acquire the same lock. If a writer such as i3c_bus_maintenance_lock() (which takes down_write) arrives between the two down_read() calls, the second down_read() blocks to prevent writer starvation, while the writer blocks waiting for the first reader.

drivers/i3c/device.c int i3c_device_request_ibi(struct i3c_device *dev, · line 214

Quoted 8 lines
drivers/i3c/device.c +5 -1
-	if (dev->desc) {
+	if (!dev->desc) {
+		ret = -ENOENT;
+	} else if (!(dev->desc->info.bcr & I3C_BCR_IBI_REQ_CAP)) {
+		ret = -EOPNOTSUPP;
+	} else {
 		mutex_lock(&dev->desc->ibi_lock);
 		ret = i3c_dev_request_ibi_locked(dev->desc, req);

[Severity: High]
This is a pre-existing issue, but does this code leak the workqueue on the error path?

Looking at drivers/i3c/master.c:i3c_dev_request_ibi_locked():

	ibi->wq = alloc_ordered_workqueue(dev_name(i3cdev_to_dev(dev->dev)), WQ_MEM_RECLAIM);
	...
	ret = master->ops->request_ibi(dev, req);
	if (ret) {
		kfree(ibi);
		dev->ibi = NULL;
	}

If master->ops->request_ibi() fails, the ibi struct is freed but there is no call to destroy_workqueue(ibi->wq), permanently leaking the memory and associated kernel thread resources.

[Severity: High]
This is a pre-existing issue, but could this lead to a use-after-free if an interrupt handler accesses dev->ibi concurrently?

In drivers/i3c/master.c:i3c_master_queue_ibi(), the hardware interrupt handler reads dev->ibi locklessly to queue incoming IBIs:

	if (!dev->ibi || !slot)
		return;
	...
	atomic_inc(&dev->ibi->pending_ibis);
	queue_work(dev->ibi->wq, &slot->work);

However, in the error path of i3c_dev_request_ibi_locked() shown above, and in i3c_dev_free_ibi_locked(), the memory is freed before the pointer is cleared:

	kfree(ibi);
	dev->ibi = NULL;

Because there is no synchronization, a concurrent interrupt on another CPU can read the pointer, get delayed, and then dereference it after it has been freed.

Quoted 6 lines
drivers/i3c/device.c +0 -2
 		mutex_unlock(&dev->desc->ibi_lock);
-	} else {
-		ret = -ENOENT;
 	}
 	i3c_bus_normaluse_unlock(dev->bus);
 
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
#11 lore raw

RFC PATCH 5/7 i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI

In reply to Adrian Hunter

Keep the PCI wakeup state aligned with the wakeup requirements of the devices served by the controller(s).

The PCI function is the wakeup source for HCI instances exposed beneath it. However, wakeup is only needed when at least one attached I3C device is enabled as a wakeup source.

During suspend, check whether any HCI instance has a wakeup-enabled I3C device and enable wakeup for the PCI function only in that case. Otherwise leave PCI wakeup disabled.

Note, the suspend callback is used for both system and runtime suspend. Although this change may update the PCI wakeup state during runtime suspend, it does so only when the required wakeup state changes. Moreover, PCI wakeup-capable devices already have PME wakeup armed for runtime suspend, so changing the wakeup-enabled state does not affect runtime PM wakeup behavior.

Note also, since the PCI wakeup state is derived from the wakeup configuration of the attached I3C devices, the PCI device power/wakeup sysfs attribute no longer provides independent wakeup control.

---
 .../master/mipi-i3c-hci/mipi-i3c-hci-pci.c    | 23 +++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)
drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c +21 -2
index 5a9e2a43eff8..2b3bf6fa74f2 100644
@@ -265,6 +265,8 @@ static bool mipi_i3c_hci_pci_is_operational(struct device *dev, bool update)
 struct mipi_i3c_hci_pci_pm_data {
 	struct device *dev[INST_MAX];
 	int dev_cnt;
+	bool can_wakeup;
+	bool may_wakeup;
 };
 
 static bool mipi_i3c_hci_pci_is_mfd(struct device *dev)
@@ -272,6 +274,13 @@ static bool mipi_i3c_hci_pci_is_mfd(struct device *dev)
 	return dev_is_platform(dev) && mfd_get_cell(to_platform_device(dev));
 }
 
+static bool mipi_i3c_hci_pci_any_wakeup_enabled(struct device *dev)
+{
+	struct i3c_hci *hci = dev_get_drvdata(dev);
+
+	return i3c_master_any_wakeup_enabled(&hci->master);
+}
+
 static int mipi_i3c_hci_pci_suspend_instance(struct device *dev, void *data)
 {
 	struct mipi_i3c_hci_pci_pm_data *pm_data = data;
@@ -287,6 +296,9 @@ static int mipi_i3c_hci_pci_suspend_instance(struct device *dev, void *data)
 
 	pm_data->dev[pm_data->dev_cnt++] = dev;
 
+	if (pm_data->can_wakeup && mipi_i3c_hci_pci_any_wakeup_enabled(dev))
+		pm_data->may_wakeup = true;
+
 	return 0;
 }
 
@@ -317,12 +329,19 @@ static int mipi_i3c_hci_pci_suspend(struct device *dev)
 	if (!hci->info->control_instance_pm)
 		return 0;
 
+	pm_data.can_wakeup = device_can_wakeup(dev);
+
 	ret = device_for_each_child_reverse(dev, &pm_data, mipi_i3c_hci_pci_suspend_instance);
-	if (ret)
+	if (ret) {
 		for (int i = 0; i < pm_data.dev_cnt; i++)
 			i3c_hci_rpm_resume(pm_data.dev[i]);
+		return ret;
+	}
 
-	return ret;
+	if (device_may_wakeup(dev) != pm_data.may_wakeup)
+		device_set_wakeup_enable(dev, pm_data.may_wakeup);
+
+	return 0;
 }
 
 static int mipi_i3c_hci_pci_resume(struct device *dev)
Signature · 1 line
2.53.0
#12 lore raw

Re: [PATCH RFC RESEND 5/7] i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 18 lines

Keep the PCI wakeup state aligned with the wakeup requirements of the devices served by the controller(s).

The PCI function is the wakeup source for HCI instances exposed beneath it. However, wakeup is only needed when at least one attached I3C device is enabled as a wakeup source.

During suspend, check whether any HCI instance has a wakeup-enabled I3C device and enable wakeup for the PCI function only in that case. Otherwise leave PCI wakeup disabled.

Note, the suspend callback is used for both system and runtime suspend. Although this change may update the PCI wakeup state during runtime suspend, it does so only when the required wakeup state changes. Moreover, PCI wakeup-capable devices already have PME wakeup armed for runtime suspend, so changing the wakeup-enabled state does not affect runtime PM wakeup behavior.

Quoted 5 lines

Note also, since the PCI wakeup state is derived from the wakeup configuration of the attached I3C devices, the PCI device power/wakeup sysfs attribute no longer provides independent wakeup control.

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
#13 lore raw

RFC PATCH 6/7 i3c: mipi-i3c-hci: Factor out i3c_hci_sysdev()

In reply to Adrian Hunter

The MIPI I3C HCI driver needs to identify the underlying system device used for DMA mapping and PM operations. The logic for determining that device is currently embedded in the DMA implementation.

Factor this code out into i3c_hci_sysdev() so it can be shared by other parts of the driver and keep the device-selection logic in one place.

---
 drivers/i3c/master/mipi-i3c-hci/core.c | 12 ++++++++++++
 drivers/i3c/master/mipi-i3c-hci/dma.c  | 15 +--------------
 drivers/i3c/master/mipi-i3c-hci/hci.h  |  2 ++
 3 files changed, 15 insertions(+), 14 deletions(-)
drivers/i3c/master/mipi-i3c-hci/core.c +12 -0
index e80aa1f5722e..4e17b1480362 100644
@@ -15,6 +15,7 @@
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
+#include <linux/pci.h>
 #include <linux/platform_data/mipi-i3c-hci.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -117,6 +118,17 @@ static inline struct i3c_hci *to_i3c_hci(struct i3c_master_controller *m)
 	return container_of(m, struct i3c_hci, master);
 }
 
+/*
+ * Determine the device that does PM / DMA and has IOMMU setup done for it in
+ * case of enabled IOMMU (for use with the DMA API).
+ * Such device is either "mipi-i3c-hci" platform device (OF/ACPI enumeration)
+ * parent or grandparent (PCI enumeration).
+ */
+struct device *i3c_hci_sysdev(struct device *dev)
+{
+	return dev->parent && dev_is_pci(dev->parent) ? dev->parent : dev;
+}
+
 static void i3c_hci_set_master_dyn_addr(struct i3c_hci *hci)
 {
 	reg_write(MASTER_DEVICE_ADDR,
drivers/i3c/master/mipi-i3c-hci/dma.c +1 -14
index 0672ed1132f8..7c2b20474130 100644
@@ -15,7 +15,6 @@
 #include <linux/errno.h>
 #include <linux/i3c/master.h>
 #include <linux/io.h>
-#include <linux/pci.h>
 
 #include "hci.h"
 #include "cmd.h"
@@ -301,23 +300,11 @@ static int hci_dma_init(struct i3c_hci *hci)
 {
 	struct hci_rings_data *rings;
 	struct hci_rh_data *rh;
-	struct device *sysdev;
 	u32 regval;
 	unsigned int i, nr_rings, xfers_sz, resps_sz;
 	unsigned int ibi_status_ring_sz, ibi_data_ring_sz;
 	int ret;
 
-	/*
-	 * Set pointer to a physical device that does DMA and has IOMMU setup
-	 * done for it in case of enabled IOMMU and use it with the DMA API.
-	 * Here such device is either
-	 * "mipi-i3c-hci" platform device (OF/ACPI enumeration) parent or
-	 * grandparent (PCI enumeration).
-	 */
-	sysdev = hci->master.dev.parent;
-	if (sysdev->parent && dev_is_pci(sysdev->parent))
-		sysdev = sysdev->parent;
-
 	regval = rhs_reg_read(CONTROL);
 	nr_rings = FIELD_GET(MAX_HEADER_COUNT_CAP, regval);
 	dev_dbg(&hci->master.dev, "%d DMA rings available\n", nr_rings);
@@ -332,7 +319,7 @@ static int hci_dma_init(struct i3c_hci *hci)
 		return -ENOMEM;
 	hci->io_data = rings;
 	rings->total = nr_rings;
-	rings->sysdev = sysdev;
+	rings->sysdev = i3c_hci_sysdev(hci->master.dev.parent);
 
 	for (i = 0; i < rings->total; i++) {
 		u32 offset = rhs_reg_read(RHn_OFFSET(i));
drivers/i3c/master/mipi-i3c-hci/hci.h +2 -0
index b3d9803b1968..b8d2a3d680f8 100644
@@ -184,6 +184,8 @@ void amd_set_resp_buf_thld(struct i3c_hci *hci);
 void i3c_hci_sync_irq_inactive(struct i3c_hci *hci);
 int i3c_hci_process_xfer(struct i3c_hci *hci, struct hci_xfer *xfer, int n);
 
+struct device *i3c_hci_sysdev(struct device *dev);
+
 #define DEFAULT_AUTOSUSPEND_DELAY_MS 1000
 
 int i3c_hci_rpm_suspend(struct device *dev);
Signature · 1 line
2.53.0
#14 lore raw

Re: [PATCH RFC RESEND 6/7] i3c: mipi-i3c-hci: Factor out i3c_hci_sysdev()

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 8 lines

The MIPI I3C HCI driver needs to identify the underlying system device used for DMA mapping and PM operations. The logic for determining that device is currently embedded in the DMA implementation.

Factor this code out into i3c_hci_sysdev() so it can be shared by other parts of the driver and keep the device-selection logic in one place.

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
#15 lore raw

RFC PATCH 7/7 i3c: mipi-i3c-hci: Advertise IBI wakeup capability

In reply to Adrian Hunter

Set master->ibi_wakeup during probe when the associated system device advertises wakeup capability, allowing the I3C core to mark IBI-capable I3C devices as wakeup capable.

Tweak the comment for i3c_hci_sysdev() to mention the new usage.

---
 drivers/i3c/master/mipi-i3c-hci/core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/i3c/master/mipi-i3c-hci/core.c +6 -3
index 4e17b1480362..55724a986f90 100644
@@ -120,9 +120,9 @@ static inline struct i3c_hci *to_i3c_hci(struct i3c_master_controller *m)
 
 /*
  * Determine the device that does PM / DMA and has IOMMU setup done for it in
- * case of enabled IOMMU (for use with the DMA API).
- * Such device is either "mipi-i3c-hci" platform device (OF/ACPI enumeration)
- * parent or grandparent (PCI enumeration).
+ * case of enabled IOMMU (for use with the DMA API). It is also used to check
+ * for wakeup capability. Such device is either "mipi-i3c-hci" platform device
+ * (OF/ACPI enumeration) parent or grandparent (PCI enumeration).
  */
 struct device *i3c_hci_sysdev(struct device *dev)
 {
@@ -1175,6 +1175,9 @@ static int i3c_hci_probe(struct platform_device *pdev)
 	if (hci->quirks & HCI_QUIRK_RPM_IBI_ALLOWED)
 		hci->master.rpm_ibi_allowed = true;
 
+	if (device_can_wakeup(i3c_hci_sysdev(&pdev->dev)))
+		hci->master.ibi_wakeup = true;
+
 	return i3c_master_register(&hci->master, &pdev->dev, &i3c_hci_ops, false);
 }
 
Signature · 1 line
2.53.0
#16 lore raw

Re: [PATCH RFC RESEND 7/7] i3c: mipi-i3c-hci: Advertise IBI wakeup capability

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 7 lines

Set master->ibi_wakeup during probe when the associated system device advertises wakeup capability, allowing the I3C core to mark IBI-capable I3C devices as wakeup capable.

Tweak the comment for i3c_hci_sysdev() to mention the new usage.

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7