[PATCH 0/3] i3c: master: Small fixes for dev_nack_retry_count

[PATCH 0/3] i3c: master: Small fixes for dev_nack_retry_count

[Adrian Hunter]

Hi

Here are some small fixes for dev_nack_retry_count.


Adrian Hunter (3):
      i3c: master: Update dev_nack_retry_count under maintenance lock
      i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()
      i3c: master: Use unsigned int for dev_nack_retry_count consistently

 drivers/i3c/master.c               | 26 ++++++++++++++++++--------
 drivers/i3c/master/dw-i3c-master.c |  4 ++--
 include/linux/i3c/master.h         |  2 +-
 3 files changed, 21 insertions(+), 11 deletions(-)


Regards
Adrian

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH 0/3] i3c: master: Small fixes for dev_nack_retry_count

[Adrian Hunter]

Hi

Here are some small fixes for dev_nack_retry_count.


Adrian Hunter (3):
      i3c: master: Update dev_nack_retry_count under maintenance lock
      i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()
      i3c: master: Use unsigned int for dev_nack_retry_count consistently

 drivers/i3c/master.c               | 26 ++++++++++++++++++--------
 drivers/i3c/master/dw-i3c-master.c |  4 ++--
 include/linux/i3c/master.h         |  2 +-
 3 files changed, 21 insertions(+), 11 deletions(-)


Regards
Adrian

[PATCH 1/3] i3c: master: Update dev_nack_retry_count under maintenance lock

[Adrian Hunter]

Protect master->dev_nack_retry_count against concurrent sysfs updates
by updating it while holding the bus maintenance lock.

Consequently, combine adjacent return statements into one.

For consistency, read dev_nack_retry_count while holding the bus normaluse
lock.

Fixes: b58f47eb39268 ("i3c: add sysfs entry and attribute for Device NACK Retry count")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/i3c/master.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 367e0d6d4a64..66cf5d3bd987 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -744,7 +744,14 @@ static DEVICE_ATTR_RW(hotjoin);
 static ssize_t dev_nack_retry_count_show(struct device *dev,
 					 struct device_attribute *attr, char *buf)
 {
-	return sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
+	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
+	ssize_t ret;
+
+	i3c_bus_normaluse_lock(i3cbus);
+	ret = sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
+	i3c_bus_normaluse_unlock(i3cbus);
+
+	return ret;
 }
 
 static ssize_t dev_nack_retry_count_store(struct device *dev,
@@ -762,14 +769,11 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
 
 	i3c_bus_maintenance_lock(i3cbus);
 	ret = master->ops->set_dev_nack_retry(master, val);
+	if (!ret)
+		master->dev_nack_retry_count = val;
 	i3c_bus_maintenance_unlock(i3cbus);
 
-	if (ret)
-		return ret;
-
-	master->dev_nack_retry_count = val;
-
-	return count;
+	return ret ?: count;
 }
 
 static DEVICE_ATTR_RW(dev_nack_retry_count);
-- 
2.51.0


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH 1/3] i3c: master: Update dev_nack_retry_count under maintenance lock

[Adrian Hunter]

Protect master->dev_nack_retry_count against concurrent sysfs updates
by updating it while holding the bus maintenance lock.

Consequently, combine adjacent return statements into one.

For consistency, read dev_nack_retry_count while holding the bus normaluse
lock.

Fixes: b58f47eb39268 ("i3c: add sysfs entry and attribute for Device NACK Retry count")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/i3c/master.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 367e0d6d4a64..66cf5d3bd987 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -744,7 +744,14 @@ static DEVICE_ATTR_RW(hotjoin);
 static ssize_t dev_nack_retry_count_show(struct device *dev,
 					 struct device_attribute *attr, char *buf)
 {
-	return sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
+	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
+	ssize_t ret;
+
+	i3c_bus_normaluse_lock(i3cbus);
+	ret = sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
+	i3c_bus_normaluse_unlock(i3cbus);
+
+	return ret;
 }
 
 static ssize_t dev_nack_retry_count_store(struct device *dev,
@@ -762,14 +769,11 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
 
 	i3c_bus_maintenance_lock(i3cbus);
 	ret = master->ops->set_dev_nack_retry(master, val);
+	if (!ret)
+		master->dev_nack_retry_count = val;
 	i3c_bus_maintenance_unlock(i3cbus);
 
-	if (ret)
-		return ret;
-
-	master->dev_nack_retry_count = val;
-
-	return count;
+	return ret ?: count;
 }
 
 static DEVICE_ATTR_RW(dev_nack_retry_count);
-- 
2.51.0

[PATCH 2/3] i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()

[Adrian Hunter]

Ensure the device is runtime resumed while updating the retry
configuration to avoid accessing the controller while suspended.

Call i3c_master_rpm_get() before accessing the controller in
dev_nack_retry_count_store() and release it with
i3c_master_rpm_put() afterwards.

Fixes: 990c149c61ee4 ("i3c: master: Introduce optional Runtime PM support")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/i3c/master.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 66cf5d3bd987..903ac01ab413 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -767,12 +767,18 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	ret = i3c_master_rpm_get(master);
+	if (ret)
+		return ret;
+
 	i3c_bus_maintenance_lock(i3cbus);
 	ret = master->ops->set_dev_nack_retry(master, val);
 	if (!ret)
 		master->dev_nack_retry_count = val;
 	i3c_bus_maintenance_unlock(i3cbus);
 
+	i3c_master_rpm_put(master);
+
 	return ret ?: count;
 }
 
-- 
2.51.0


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH 2/3] i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()

[Adrian Hunter]

Ensure the device is runtime resumed while updating the retry
configuration to avoid accessing the controller while suspended.

Call i3c_master_rpm_get() before accessing the controller in
dev_nack_retry_count_store() and release it with
i3c_master_rpm_put() afterwards.

Fixes: 990c149c61ee4 ("i3c: master: Introduce optional Runtime PM support")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/i3c/master.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 66cf5d3bd987..903ac01ab413 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -767,12 +767,18 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	ret = i3c_master_rpm_get(master);
+	if (ret)
+		return ret;
+
 	i3c_bus_maintenance_lock(i3cbus);
 	ret = master->ops->set_dev_nack_retry(master, val);
 	if (!ret)
 		master->dev_nack_retry_count = val;
 	i3c_bus_maintenance_unlock(i3cbus);
 
+	i3c_master_rpm_put(master);
+
 	return ret ?: count;
 }
 
-- 
2.51.0

[PATCH 3/3] i3c: master: Use unsigned int for dev_nack_retry_count consistently

[Adrian Hunter]

Use unsigned int for dev_nack_retry_count across the core and
controller drivers to match the type of master->dev_nack_retry_count.

Update the sysfs store path to use kstrtouint() and adjust the
 ->set_dev_nack_retry() callback prototype and callers accordingly.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/i3c/master.c               | 4 ++--
 drivers/i3c/master/dw-i3c-master.c | 4 ++--
 include/linux/i3c/master.h         | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 903ac01ab413..58d20624ed05 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -760,10 +760,10 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
 {
 	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
 	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
-	unsigned long val;
+	unsigned int val;
 	int ret;
 
-	ret = kstrtoul(buf, 0, &val);
+	ret = kstrtouint(buf, 0, &val);
 	if (ret)
 		return ret;
 
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index c7030d0cd8a6..e4f6d6f29ed5 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -1481,7 +1481,7 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
 }
 
 static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
-					    unsigned long dev_nack_retry_cnt)
+					    unsigned int dev_nack_retry_cnt)
 {
 	struct dw_i3c_master *master = to_dw_i3c_master(m);
 	u32 reg;
@@ -1489,7 +1489,7 @@ static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
 
 	if (dev_nack_retry_cnt > DW_I3C_DEV_NACK_RETRY_CNT_MAX) {
 		dev_err(&master->base.dev,
-			"Value %ld exceeds maximum %d\n",
+			"Value %u exceeds maximum %d\n",
 			dev_nack_retry_cnt, DW_I3C_DEV_NACK_RETRY_CNT_MAX);
 		return -ERANGE;
 	}
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index f73cede87d36..6f0379bdbb6d 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -494,7 +494,7 @@ struct i3c_master_controller_ops {
 	int (*disable_hotjoin)(struct i3c_master_controller *master);
 	int (*set_speed)(struct i3c_master_controller *master, enum i3c_open_drain_speed speed);
 	int (*set_dev_nack_retry)(struct i3c_master_controller *master,
-				  unsigned long dev_nack_retry_cnt);
+				  unsigned int dev_nack_retry_cnt);
 };
 
 /**
-- 
2.51.0


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH 3/3] i3c: master: Use unsigned int for dev_nack_retry_count consistently

[Adrian Hunter]

Use unsigned int for dev_nack_retry_count across the core and
controller drivers to match the type of master->dev_nack_retry_count.

Update the sysfs store path to use kstrtouint() and adjust the
 ->set_dev_nack_retry() callback prototype and callers accordingly.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/i3c/master.c               | 4 ++--
 drivers/i3c/master/dw-i3c-master.c | 4 ++--
 include/linux/i3c/master.h         | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 903ac01ab413..58d20624ed05 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -760,10 +760,10 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
 {
 	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
 	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
-	unsigned long val;
+	unsigned int val;
 	int ret;
 
-	ret = kstrtoul(buf, 0, &val);
+	ret = kstrtouint(buf, 0, &val);
 	if (ret)
 		return ret;
 
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index c7030d0cd8a6..e4f6d6f29ed5 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -1481,7 +1481,7 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
 }
 
 static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
-					    unsigned long dev_nack_retry_cnt)
+					    unsigned int dev_nack_retry_cnt)
 {
 	struct dw_i3c_master *master = to_dw_i3c_master(m);
 	u32 reg;
@@ -1489,7 +1489,7 @@ static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
 
 	if (dev_nack_retry_cnt > DW_I3C_DEV_NACK_RETRY_CNT_MAX) {
 		dev_err(&master->base.dev,
-			"Value %ld exceeds maximum %d\n",
+			"Value %u exceeds maximum %d\n",
 			dev_nack_retry_cnt, DW_I3C_DEV_NACK_RETRY_CNT_MAX);
 		return -ERANGE;
 	}
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index f73cede87d36..6f0379bdbb6d 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -494,7 +494,7 @@ struct i3c_master_controller_ops {
 	int (*disable_hotjoin)(struct i3c_master_controller *master);
 	int (*set_speed)(struct i3c_master_controller *master, enum i3c_open_drain_speed speed);
 	int (*set_dev_nack_retry)(struct i3c_master_controller *master,
-				  unsigned long dev_nack_retry_cnt);
+				  unsigned int dev_nack_retry_cnt);
 };
 
 /**
-- 
2.51.0

Re: [PATCH 2/3] i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()

[sashiko-bot]

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

Pre-existing issues:
- [High] Hardware configuration, including the NACK retry count, is lost across runtime suspend/resume cycles.
--

commit c41eb9cead24507098d7a57db0101da0e5c68c34
Author: Adrian Hunter <adrian.hunter@intel.com>

i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()

Ensure the device is runtime resumed while updating the retry
configuration to avoid accessing the controller while suspended.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 05ea9e3c4d46f..a24944047e30c 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -767,12 +767,18 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
>  	i3c_bus_maintenance_lock(i3cbus);
>  	ret = master->ops->set_dev_nack_retry(master, val);
>  	if (!ret)
>  		master->dev_nack_retry_count = val;
>  	i3c_bus_maintenance_unlock(i3cbus);
>  
> +	i3c_master_rpm_put(master);

[Severity: High]
This isn't a bug introduced by this patch, but does putting the device to
sleep here immediately cause the newly written retry count to be lost?

When the dw-i3c-master device enters runtime suspend, the hardware is reset
via reset_control_assert() in dw_i3c_master_runtime_suspend().

Upon resuming, dw_i3c_master_restore_addrs() reads the newly reset hardware
register (which returns 0), updates only the address bits, and writes it
back. This permanently discards other configuration fields, including the
DEV_ADDR_TABLE_DEV_NACK_RETRY_CNT configured here, as well as other fields
like SIR_REJECT and IBI_MDB.

Because dev_nack_retry_count_store() now correctly drops the runtime PM
reference, the controller can suspend immediately after the configuration is
written. Will this silently revert the retry count back to its reset state?

> +
>  	return ret ?: count;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616113752.196140-1-adrian.hunter@intel.com?part=2

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 1/3] i3c: master: Update dev_nack_retry_count under maintenance lock

[sashiko-bot]

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

Pre-existing issues:
- [High] Preexisting Use-After-Free (UAF) due to devres freeing the I3C master controller while its embedded `struct device` is still referenced.
--

commit d8d2b455dc4bd3d672311fc14258b4a9d0b83e8b
Author: Adrian Hunter <adrian.hunter@intel.com>
Subject: i3c: master: Update dev_nack_retry_count under maintenance lock

This commit protects master->dev_nack_retry_count against concurrent sysfs
updates by updating it while holding the bus maintenance lock. It also
combines adjacent return statements and updates the read path to hold the bus
normaluse lock.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 372d911ecbadc..05ea9e3c4d46f 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -744,7 +744,14 @@ static DEVICE_ATTR_RW(hotjoin);
>  static ssize_t dev_nack_retry_count_show(struct device *dev,
>  					 struct device_attribute *attr, char *buf)
>  {
> -	return sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
> +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> +	ssize_t ret;
> +
> +	i3c_bus_normaluse_lock(i3cbus);
> +	ret = sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying sysfs
attributes here, is there a pre-existing use-after-free risk related to how
the i3c_master_controller is allocated and freed?

Many I3C master drivers (e.g., dw-i3c-master.c) allocate their driver context
(which embeds struct i3c_master_controller) using devm_kzalloc(). During
unbind/removal, device_unregister(&master->dev) is called, which drops the
initial reference. If a user-space process holds a sysfs file open (like this
dev_nack_retry_count attribute), or if an attacker blocks in a sysfs read or
write using userfaultfd, the kobject reference count remains elevated.

The driver's remove function then returns, and the devres framework
automatically frees the driver context memory. Later, when the final reference
is dropped, the driver core calls i3c_masterdev_release():

drivers/i3c/master.c:i3c_masterdev_release() {
        struct i3c_master_controller *master = dev_to_i3cmaster(dev);
        struct i3c_bus *bus = dev_to_i3cbus(dev);

        if (master->wq)
                destroy_workqueue(master->wq);
        ...
}

Will this result in a use-after-free since the release function accesses the
master struct after its memory has already been freed by devres? Should the
memory for structures embedding a struct device be freed within the release
function instead of being tied to the driver unbind lifecycle via devres?

> +	i3c_bus_normaluse_unlock(i3cbus);
> +
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616113752.196140-1-adrian.hunter@intel.com?part=1

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 1/3] i3c: master: Update dev_nack_retry_count under maintenance lock

[Adrian Hunter]

On 16/06/2026 14:51, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Preexisting Use-After-Free (UAF) due to devres freeing the I3C master controller while its embedded `struct device` is still referenced.
> --
> 
> commit d8d2b455dc4bd3d672311fc14258b4a9d0b83e8b
> Author: Adrian Hunter <adrian.hunter@intel.com>
> Subject: i3c: master: Update dev_nack_retry_count under maintenance lock
> 
> This commit protects master->dev_nack_retry_count against concurrent sysfs
> updates by updating it while holding the bus maintenance lock. It also
> combines adjacent return statements and updates the read path to hold the bus
> normaluse lock.
> 
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index 372d911ecbadc..05ea9e3c4d46f 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
>> @@ -744,7 +744,14 @@ static DEVICE_ATTR_RW(hotjoin);
>>  static ssize_t dev_nack_retry_count_show(struct device *dev,
>>  					 struct device_attribute *attr, char *buf)
>>  {
>> -	return sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
>> +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
>> +	ssize_t ret;
>> +
>> +	i3c_bus_normaluse_lock(i3cbus);
>> +	ret = sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but since we are modifying sysfs
> attributes here, is there a pre-existing use-after-free risk related to how
> the i3c_master_controller is allocated and freed?
> 
> Many I3C master drivers (e.g., dw-i3c-master.c) allocate their driver context
> (which embeds struct i3c_master_controller) using devm_kzalloc(). During
> unbind/removal, device_unregister(&master->dev) is called, which drops the
> initial reference. If a user-space process holds a sysfs file open (like this
> dev_nack_retry_count attribute), or if an attacker blocks in a sysfs read or
> write using userfaultfd, the kobject reference count remains elevated.
> 
> The driver's remove function then returns, and the devres framework
> automatically frees the driver context memory. Later, when the final reference
> is dropped, the driver core calls i3c_masterdev_release():
> 
> drivers/i3c/master.c:i3c_masterdev_release() {
>         struct i3c_master_controller *master = dev_to_i3cmaster(dev);
>         struct i3c_bus *bus = dev_to_i3cbus(dev);
> 
>         if (master->wq)
>                 destroy_workqueue(master->wq);
>         ...
> }
> 
> Will this result in a use-after-free since the release function accesses the
> master struct after its memory has already been freed by devres?

No.  Open sysfs files do not block device release.  On the other hand,
driver remove is blocked if a driver sysfs callback is in progress.


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 2/3] i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()

[Adrian Hunter]

On 16/06/2026 14:51, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Hardware configuration, including the NACK retry count, is lost across runtime suspend/resume cycles.
> --
> 
> commit c41eb9cead24507098d7a57db0101da0e5c68c34
> Author: Adrian Hunter <adrian.hunter@intel.com>
> 
> i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()
> 
> Ensure the device is runtime resumed while updating the retry
> configuration to avoid accessing the controller while suspended.
> 
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index 05ea9e3c4d46f..a24944047e30c 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
> 
> [ ... ]
> 
>> @@ -767,12 +767,18 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
>>  	i3c_bus_maintenance_lock(i3cbus);
>>  	ret = master->ops->set_dev_nack_retry(master, val);
>>  	if (!ret)
>>  		master->dev_nack_retry_count = val;
>>  	i3c_bus_maintenance_unlock(i3cbus);
>>  
>> +	i3c_master_rpm_put(master);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does putting the device to
> sleep here immediately cause the newly written retry count to be lost?
> 
> When the dw-i3c-master device enters runtime suspend, the hardware is reset
> via reset_control_assert() in dw_i3c_master_runtime_suspend().
> 
> Upon resuming, dw_i3c_master_restore_addrs() reads the newly reset hardware
> register (which returns 0), updates only the address bits, and writes it
> back. This permanently discards other configuration fields, including the
> DEV_ADDR_TABLE_DEV_NACK_RETRY_CNT configured here, as well as other fields
> like SIR_REJECT and IBI_MDB.
> 
> Because dev_nack_retry_count_store() now correctly drops the runtime PM
> reference, the controller can suspend immediately after the configuration is
> written. Will this silently revert the retry count back to its reset state?

+ Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

Looks like dw_i3c_master_restore_addrs() needs to do more to restore
dev_nack_retry_count values.


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 1/3] i3c: master: Update dev_nack_retry_count under maintenance lock

[Frank Li]

On Tue, Jun 16, 2026 at 02:37:50PM +0300, Adrian Hunter wrote:
> Protect master->dev_nack_retry_count against concurrent sysfs updates
> by updating it while holding the bus maintenance lock.
>
> Consequently, combine adjacent return statements into one.
>
> For consistency, read dev_nack_retry_count while holding the bus normaluse
> lock.
>
> Fixes: b58f47eb39268 ("i3c: add sysfs entry and attribute for Device NACK Retry count")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/i3c/master.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 367e0d6d4a64..66cf5d3bd987 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -744,7 +744,14 @@ static DEVICE_ATTR_RW(hotjoin);
>  static ssize_t dev_nack_retry_count_show(struct device *dev,
>  					 struct device_attribute *attr, char *buf)
>  {
> -	return sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
> +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> +	ssize_t ret;
> +
> +	i3c_bus_normaluse_lock(i3cbus);
> +	ret = sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
> +	i3c_bus_normaluse_unlock(i3cbus);
> +
> +	return ret;
>  }
>
>  static ssize_t dev_nack_retry_count_store(struct device *dev,
> @@ -762,14 +769,11 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
>
>  	i3c_bus_maintenance_lock(i3cbus);
>  	ret = master->ops->set_dev_nack_retry(master, val);
> +	if (!ret)
> +		master->dev_nack_retry_count = val;
>  	i3c_bus_maintenance_unlock(i3cbus);
>
> -	if (ret)
> -		return ret;
> -
> -	master->dev_nack_retry_count = val;
> -
> -	return count;
> +	return ret ?: count;
>  }
>
>  static DEVICE_ATTR_RW(dev_nack_retry_count);
> --
> 2.51.0
>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 1/3] i3c: master: Update dev_nack_retry_count under maintenance lock

[Frank Li]

On Tue, Jun 16, 2026 at 02:37:50PM +0300, Adrian Hunter wrote:
> Protect master->dev_nack_retry_count against concurrent sysfs updates
> by updating it while holding the bus maintenance lock.
>
> Consequently, combine adjacent return statements into one.
>
> For consistency, read dev_nack_retry_count while holding the bus normaluse
> lock.
>
> Fixes: b58f47eb39268 ("i3c: add sysfs entry and attribute for Device NACK Retry count")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/i3c/master.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 367e0d6d4a64..66cf5d3bd987 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -744,7 +744,14 @@ static DEVICE_ATTR_RW(hotjoin);
>  static ssize_t dev_nack_retry_count_show(struct device *dev,
>  					 struct device_attribute *attr, char *buf)
>  {
> -	return sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
> +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> +	ssize_t ret;
> +
> +	i3c_bus_normaluse_lock(i3cbus);
> +	ret = sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
> +	i3c_bus_normaluse_unlock(i3cbus);
> +
> +	return ret;
>  }
>
>  static ssize_t dev_nack_retry_count_store(struct device *dev,
> @@ -762,14 +769,11 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
>
>  	i3c_bus_maintenance_lock(i3cbus);
>  	ret = master->ops->set_dev_nack_retry(master, val);
> +	if (!ret)
> +		master->dev_nack_retry_count = val;
>  	i3c_bus_maintenance_unlock(i3cbus);
>
> -	if (ret)
> -		return ret;
> -
> -	master->dev_nack_retry_count = val;
> -
> -	return count;
> +	return ret ?: count;
>  }
>
>  static DEVICE_ATTR_RW(dev_nack_retry_count);
> --
> 2.51.0
>

Re: [PATCH 2/3] i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()

[Frank Li]

On Tue, Jun 16, 2026 at 02:37:51PM +0300, Adrian Hunter wrote:
> Ensure the device is runtime resumed while updating the retry
> configuration to avoid accessing the controller while suspended.
>
> Call i3c_master_rpm_get() before accessing the controller in
> dev_nack_retry_count_store() and release it with
> i3c_master_rpm_put() afterwards.
>
> Fixes: 990c149c61ee4 ("i3c: master: Introduce optional Runtime PM support")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/i3c/master.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 66cf5d3bd987..903ac01ab413 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -767,12 +767,18 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
>  	if (ret)
>  		return ret;
>
> +	ret = i3c_master_rpm_get(master);
> +	if (ret)
> +		return ret;
> +
>  	i3c_bus_maintenance_lock(i3cbus);
>  	ret = master->ops->set_dev_nack_retry(master, val);
>  	if (!ret)
>  		master->dev_nack_retry_count = val;
>  	i3c_bus_maintenance_unlock(i3cbus);
>
> +	i3c_master_rpm_put(master);
> +
>  	return ret ?: count;
>  }
>
> --
> 2.51.0
>

Re: [PATCH 2/3] i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()

[Frank Li]

On Tue, Jun 16, 2026 at 02:37:51PM +0300, Adrian Hunter wrote:
> Ensure the device is runtime resumed while updating the retry
> configuration to avoid accessing the controller while suspended.
>
> Call i3c_master_rpm_get() before accessing the controller in
> dev_nack_retry_count_store() and release it with
> i3c_master_rpm_put() afterwards.
>
> Fixes: 990c149c61ee4 ("i3c: master: Introduce optional Runtime PM support")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/i3c/master.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 66cf5d3bd987..903ac01ab413 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -767,12 +767,18 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
>  	if (ret)
>  		return ret;
>
> +	ret = i3c_master_rpm_get(master);
> +	if (ret)
> +		return ret;
> +
>  	i3c_bus_maintenance_lock(i3cbus);
>  	ret = master->ops->set_dev_nack_retry(master, val);
>  	if (!ret)
>  		master->dev_nack_retry_count = val;
>  	i3c_bus_maintenance_unlock(i3cbus);
>
> +	i3c_master_rpm_put(master);
> +
>  	return ret ?: count;
>  }
>
> --
> 2.51.0
>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 3/3] i3c: master: Use unsigned int for dev_nack_retry_count consistently

[Frank Li]

On Tue, Jun 16, 2026 at 02:37:52PM +0300, Adrian Hunter wrote:
> Use unsigned int for dev_nack_retry_count across the core and
> controller drivers to match the type of master->dev_nack_retry_count.
>
> Update the sysfs store path to use kstrtouint() and adjust the
>  ->set_dev_nack_retry() callback prototype and callers accordingly.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
Reviewed-by: Frank Li <Frank.Li@nxp.com>
>  drivers/i3c/master.c               | 4 ++--
>  drivers/i3c/master/dw-i3c-master.c | 4 ++--
>  include/linux/i3c/master.h         | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 903ac01ab413..58d20624ed05 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -760,10 +760,10 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
>  {
>  	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
>  	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> -	unsigned long val;
> +	unsigned int val;
>  	int ret;
>
> -	ret = kstrtoul(buf, 0, &val);
> +	ret = kstrtouint(buf, 0, &val);
>  	if (ret)
>  		return ret;
>
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index c7030d0cd8a6..e4f6d6f29ed5 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -1481,7 +1481,7 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
>  }
>
>  static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
> -					    unsigned long dev_nack_retry_cnt)
> +					    unsigned int dev_nack_retry_cnt)
>  {
>  	struct dw_i3c_master *master = to_dw_i3c_master(m);
>  	u32 reg;
> @@ -1489,7 +1489,7 @@ static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
>
>  	if (dev_nack_retry_cnt > DW_I3C_DEV_NACK_RETRY_CNT_MAX) {
>  		dev_err(&master->base.dev,
> -			"Value %ld exceeds maximum %d\n",
> +			"Value %u exceeds maximum %d\n",
>  			dev_nack_retry_cnt, DW_I3C_DEV_NACK_RETRY_CNT_MAX);
>  		return -ERANGE;
>  	}
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index f73cede87d36..6f0379bdbb6d 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -494,7 +494,7 @@ struct i3c_master_controller_ops {
>  	int (*disable_hotjoin)(struct i3c_master_controller *master);
>  	int (*set_speed)(struct i3c_master_controller *master, enum i3c_open_drain_speed speed);
>  	int (*set_dev_nack_retry)(struct i3c_master_controller *master,
> -				  unsigned long dev_nack_retry_cnt);
> +				  unsigned int dev_nack_retry_cnt);
>  };
>
>  /**
> --
> 2.51.0
>

Re: [PATCH 3/3] i3c: master: Use unsigned int for dev_nack_retry_count consistently

[Frank Li]

On Tue, Jun 16, 2026 at 02:37:52PM +0300, Adrian Hunter wrote:
> Use unsigned int for dev_nack_retry_count across the core and
> controller drivers to match the type of master->dev_nack_retry_count.
>
> Update the sysfs store path to use kstrtouint() and adjust the
>  ->set_dev_nack_retry() callback prototype and callers accordingly.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
Reviewed-by: Frank Li <Frank.Li@nxp.com>
>  drivers/i3c/master.c               | 4 ++--
>  drivers/i3c/master/dw-i3c-master.c | 4 ++--
>  include/linux/i3c/master.h         | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 903ac01ab413..58d20624ed05 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -760,10 +760,10 @@ static ssize_t dev_nack_retry_count_store(struct device *dev,
>  {
>  	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
>  	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> -	unsigned long val;
> +	unsigned int val;
>  	int ret;
>
> -	ret = kstrtoul(buf, 0, &val);
> +	ret = kstrtouint(buf, 0, &val);
>  	if (ret)
>  		return ret;
>
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index c7030d0cd8a6..e4f6d6f29ed5 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -1481,7 +1481,7 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
>  }
>
>  static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
> -					    unsigned long dev_nack_retry_cnt)
> +					    unsigned int dev_nack_retry_cnt)
>  {
>  	struct dw_i3c_master *master = to_dw_i3c_master(m);
>  	u32 reg;
> @@ -1489,7 +1489,7 @@ static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
>
>  	if (dev_nack_retry_cnt > DW_I3C_DEV_NACK_RETRY_CNT_MAX) {
>  		dev_err(&master->base.dev,
> -			"Value %ld exceeds maximum %d\n",
> +			"Value %u exceeds maximum %d\n",
>  			dev_nack_retry_cnt, DW_I3C_DEV_NACK_RETRY_CNT_MAX);
>  		return -ERANGE;
>  	}
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index f73cede87d36..6f0379bdbb6d 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -494,7 +494,7 @@ struct i3c_master_controller_ops {
>  	int (*disable_hotjoin)(struct i3c_master_controller *master);
>  	int (*set_speed)(struct i3c_master_controller *master, enum i3c_open_drain_speed speed);
>  	int (*set_dev_nack_retry)(struct i3c_master_controller *master,
> -				  unsigned long dev_nack_retry_cnt);
> +				  unsigned int dev_nack_retry_cnt);
>  };
>
>  /**
> --
> 2.51.0
>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 0/3] i3c: master: Small fixes for dev_nack_retry_count

[Alexandre Belloni]

On Tue, 16 Jun 2026 14:37:49 +0300, Adrian Hunter wrote:
> Here are some small fixes for dev_nack_retry_count.
> 
> 
> Adrian Hunter (3):
>       i3c: master: Update dev_nack_retry_count under maintenance lock
>       i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()
>       i3c: master: Use unsigned int for dev_nack_retry_count consistently
> 
> [...]

Applied, thanks!

[1/3] i3c: master: Update dev_nack_retry_count under maintenance lock
      https://git.kernel.org/i3c/c/ab5f9c5cb527
[2/3] i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()
      https://git.kernel.org/i3c/c/79ce29e100ab
[3/3] i3c: master: Use unsigned int for dev_nack_retry_count consistently
      https://git.kernel.org/i3c/c/225b76e2a711

Best regards,

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 0/3] i3c: master: Small fixes for dev_nack_retry_count

[Alexandre Belloni]

On Tue, 16 Jun 2026 14:37:49 +0300, Adrian Hunter wrote:
> Here are some small fixes for dev_nack_retry_count.
> 
> 
> Adrian Hunter (3):
>       i3c: master: Update dev_nack_retry_count under maintenance lock
>       i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()
>       i3c: master: Use unsigned int for dev_nack_retry_count consistently
> 
> [...]

Applied, thanks!

[1/3] i3c: master: Update dev_nack_retry_count under maintenance lock
      https://git.kernel.org/i3c/c/ab5f9c5cb527
[2/3] i3c: master: Add missing runtime PM get in dev_nack_retry_count_store()
      https://git.kernel.org/i3c/c/79ce29e100ab
[3/3] i3c: master: Use unsigned int for dev_nack_retry_count consistently
      https://git.kernel.org/i3c/c/225b76e2a711

Best regards,

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com