* [PATCH v11 1/9] i3c: master: rename i3c_master_reattach_i3c_dev() to *_locked
2026-06-12 11:18 [PATCH v11 0/9] Add support for NXP P3H2x4x I3C hub driver Lakshay Piplani
@ 2026-06-12 11:18 ` Lakshay Piplani
2026-06-12 11:41 ` sashiko-bot
2026-06-12 11:18 ` [PATCH v11 2/9] i3c: master: Expose the APIs to support I3C hub Lakshay Piplani
` (7 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Lakshay Piplani @ 2026-06-12 11:18 UTC (permalink / raw)
To: linux-kernel, linux-i3c, alexandre.belloni, krzk+dt, robh,
conor+dt, devicetree, broonie, lee, Frank.Li, lgirdwood
Cc: vikash.bansal, priyanka.jain, aman.kumarpandey, Lakshay Piplani
Rename i3c_master_reattach_i3c_dev() to *_locked() to make the locking
requirement explicit and consistent with other I3C core helpers that
require the bus lock to be held by the caller.
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v11:
- No change, added Reviewed-By tag
Changes in v10:
- Rename i3c_master_reattach_i3c_dev() to *_locked to reflect required
bus locking
---
---
drivers/i3c/master.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5cd4e5da2233..e89d73508b9a 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1652,8 +1652,8 @@ static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
return 0;
}
-static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
- u8 old_dyn_addr)
+static int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
+ u8 old_dyn_addr)
{
struct i3c_master_controller *master = i3c_dev_get_master(dev);
int ret;
@@ -1742,7 +1742,7 @@ static int i3c_master_early_i3c_dev_add(struct i3c_master_controller *master,
goto err_detach_dev;
i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
- ret = i3c_master_reattach_i3c_dev(i3cdev, 0);
+ ret = i3c_master_reattach_i3c_dev_locked(i3cdev, 0);
if (ret)
goto err_rstdaa;
@@ -2358,7 +2358,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
if (!ret) {
old_dyn_addr = newdev->info.dyn_addr;
newdev->info.dyn_addr = expected_dyn_addr;
- i3c_master_reattach_i3c_dev(newdev, old_dyn_addr);
+ i3c_master_reattach_i3c_dev_locked(newdev, old_dyn_addr);
} else {
dev_err(&master->dev,
"Failed to assign reserved/old address to device %d%llx",
--
2.25.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v11 1/9] i3c: master: rename i3c_master_reattach_i3c_dev() to *_locked
2026-06-12 11:18 ` [PATCH v11 1/9] i3c: master: rename i3c_master_reattach_i3c_dev() to *_locked Lakshay Piplani
@ 2026-06-12 11:41 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-12 11:41 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: Frank.Li, conor+dt, devicetree, linux-i3c, robh
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] The return value of `i3c_master_reattach_i3c_dev_locked()` is ignored in `i3c_master_add_i3c_dev_locked()`, leading to bus state corruption if reattachment fails.
- [High] Aggressive cleanup in `i3c_master_reattach_i3c_dev_locked()` sabotages the caller's hardware rollback path (`RSTDAA`) in `i3c_master_early_i3c_dev_add()`.
- [High] `i3c_device_uevent()` accesses `i3cdev->desc` without acquiring `bus->lock`, leading to a Use-After-Free if a hot-join device replacement occurs concurrently.
--
commit 8a7af37c292c3791d232cdb1c2f37ae5aba2ca10
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
i3c: master: rename i3c_master_reattach_i3c_dev() to *_locked
Rename i3c_master_reattach_i3c_dev() to *_locked() to make the locking
requirement explicit and consistent with other I3C core helpers that
require the bus lock to be held by the caller.
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 6b8df8089a357..c45b450af319d 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
[ ... ]
> @@ -1742,7 +1742,7 @@ static int i3c_master_early_i3c_dev_add(struct i3c_master_controller *master,
> goto err_detach_dev;
>
> i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
> - ret = i3c_master_reattach_i3c_dev(i3cdev, 0);
> + ret = i3c_master_reattach_i3c_dev_locked(i3cdev, 0);
[Severity: High]
This isn't a bug introduced by this patch, but doesn't the error path
here break if the reattach fails?
If master->ops->reattach_i3c_dev() fails,
i3c_master_reattach_i3c_dev_locked() immediately marks the
dev->info.dyn_addr slot as I3C_ADDR_SLOT_FREE via i3c_master_put_i3c_addrs().
This seems to sabotage the fallback path at err_rstdaa:
drivers/i3c/master.c:i3c_master_early_i3c_dev_add() {
...
err_rstdaa:
i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);
...
}
Since the address slot is now marked free, i3c_master_rstdaa_locked()
will instantly abort with -EINVAL because the slot status is no longer
I3C_ADDR_SLOT_I3C_DEV. Will this leave the physical device retaining
the dynamic address while the master controller incorrectly considers it free?
> if (ret)
> goto err_rstdaa;
>
[ ... ]
> @@ -2358,7 +2358,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> if (!ret) {
> old_dyn_addr = newdev->info.dyn_addr;
> newdev->info.dyn_addr = expected_dyn_addr;
> - i3c_master_reattach_i3c_dev(newdev, old_dyn_addr);
> + i3c_master_reattach_i3c_dev_locked(newdev, old_dyn_addr);
[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return
value of i3c_master_reattach_i3c_dev_locked() here?
If the underlying master->ops->reattach_i3c_dev() fails, the reattach
function calls i3c_master_put_i3c_addrs(), marking the device's assigned
dynamic address slot as I3C_ADDR_SLOT_FREE in the bus bitmask.
Since this failure is completely ignored and the caller returns success,
the hardware remains configured to listen on the new dynamic address, but
the software allocator considers the slot free. Could a subsequent
hot-join or device addition be assigned this exact same dynamic address,
leading to unrecoverable bus collisions?
[Severity: High]
Additionally, this isn't a bug introduced by this patch, but does
i3c_master_add_i3c_dev_locked() race with userspace uevents during a
hot-join device replacement?
Later in this function, it identifies duplicate devices, migrates
the generic struct device from the old descriptor to the new one, and
eventually frees the old descriptor via i3c_master_free_i3c_dev(olddev).
This process is protected by the bus maintenance lock.
However, i3c_device_uevent() is triggered by sysfs and directly
dereferences i3cdev->desc without acquiring the bus lock or employing RCU:
drivers/i3c/master.c:i3c_device_uevent() {
...
if (i3cdev->desc)
devinfo = i3cdev->desc->info;
manuf = I3C_PID_MANUF_ID(devinfo.pid);
...
}
If a thread executing i3c_device_uevent() evaluates i3cdev->desc, gets
preempted while i3c_master_add_i3c_dev_locked() frees olddev, and then
resumes, will this trigger a use-after-free during the structure copy?
> } else {
> dev_err(&master->dev,
> "Failed to assign reserved/old address to device %d%llx",
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v11 2/9] i3c: master: Expose the APIs to support I3C hub
2026-06-12 11:18 [PATCH v11 0/9] Add support for NXP P3H2x4x I3C hub driver Lakshay Piplani
2026-06-12 11:18 ` [PATCH v11 1/9] i3c: master: rename i3c_master_reattach_i3c_dev() to *_locked Lakshay Piplani
@ 2026-06-12 11:18 ` Lakshay Piplani
2026-06-12 11:44 ` sashiko-bot
2026-06-12 11:18 ` [PATCH v11 3/9] i3c: master: Add APIs for I3C hub support Lakshay Piplani
` (6 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Lakshay Piplani @ 2026-06-12 11:18 UTC (permalink / raw)
To: linux-kernel, linux-i3c, alexandre.belloni, krzk+dt, robh,
conor+dt, devicetree, broonie, lee, Frank.Li, lgirdwood
Cc: vikash.bansal, priyanka.jain, aman.kumarpandey, Lakshay Piplani
From: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Change the below internal static functions to APIs to allow new I3C hub
driver to use them
1) i3c_dev_enable_ibi_locked()
2) i3c_dev_disable_ibi_locked()
3) i3c_dev_request_ibi_locked()
4) i3c_dev_free_ibi_locked()
5) i3c_master_reattach_i3c_dev_locked()
Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v11:
- No change, added Reviewed-By tag
Changes in v10:
- Expose the renamed reattach API.
Changes in v9:
- No change
Changes in v8:
- No change
Changes in v7:
- Fix kernel-doc warnings for *_locked() APIs
- Clarify API exposure in commit message
Changes in v6:
- Split the patch into two parts:
1) expose the existing API
2) add new APIs.
Changes in v5:
- No change
Changes in v4:
- Updated I3C master to handle hub support
---
---
drivers/i3c/master.c | 70 ++++++++++++++++++++++++++++++++++++--
include/linux/i3c/master.h | 2 ++
2 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index e89d73508b9a..0636e3e21758 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1652,8 +1652,23 @@ static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
return 0;
}
-static int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
- u8 old_dyn_addr)
+/**
+ * i3c_master_reattach_i3c_dev_locked() - reattach an I3C device with a new address
+ * @dev: I3C device descriptor to reattach
+ * @old_dyn_addr: previous dynamic address of the device
+ *
+ * This function reattaches an existing I3C device to the bus when its dynamic
+ * address has changed. It updates the bus address slot status accordingly:
+ * - Marks the new dynamic address as occupied by an I3C device.
+ * - Frees the old dynamic address slot if applicable.
+ *
+ * This function must be called with the bus lock held in write mode.
+ *
+ * Return: 0 on success, or a negative error code if reattachment fails
+ * (e.g. -EBUSY if the new address slot is not free).
+ */
+int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
+ u8 old_dyn_addr)
{
struct i3c_master_controller *master = i3c_dev_get_master(dev);
int ret;
@@ -1677,6 +1692,7 @@ static int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
return 0;
}
+EXPORT_SYMBOL_GPL(i3c_master_reattach_i3c_dev_locked);
static void i3c_master_detach_i3c_dev(struct i3c_dev_desc *dev)
{
@@ -3195,6 +3211,16 @@ int i3c_dev_do_xfers_locked(struct i3c_dev_desc *dev, struct i3c_xfer *xfers,
return master->ops->i3c_xfers(dev, xfers, nxfers, mode);
}
+/**
+ * i3c_dev_disable_ibi_locked() - Disable IBIs coming from a specific device
+ * @dev: device on which IBIs should be disabled
+ *
+ * This function disable IBIs coming from a specific device and wait for
+ * all pending IBIs to be processed.
+ *
+ * Context: Must be called with mutex_lock(&dev->desc->ibi_lock) held.
+ * Return: 0 in case of success, a negative error core otherwise.
+ */
int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev)
{
struct i3c_master_controller *master;
@@ -3216,7 +3242,22 @@ int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev)
return 0;
}
+EXPORT_SYMBOL_GPL(i3c_dev_disable_ibi_locked);
+/**
+ * i3c_dev_enable_ibi_locked() - Enable IBIs from a specific device (lock held)
+ * @dev: device on which IBIs should be enabled
+ *
+ * This function enable IBIs coming from a specific device and wait for
+ * all pending IBIs to be processed. This should be called on a device
+ * where i3c_device_request_ibi() has succeeded.
+ *
+ * Note that IBIs from this device might be received before this function
+ * returns to its caller.
+ *
+ * Context: Must be called with mutex_lock(&dev->desc->ibi_lock) held.
+ * Return: 0 on success, or a negative error code on failure.
+ */
int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev)
{
struct i3c_master_controller *master = i3c_dev_get_master(dev);
@@ -3231,7 +3272,20 @@ int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev)
return ret;
}
+EXPORT_SYMBOL_GPL(i3c_dev_enable_ibi_locked);
+/**
+ * i3c_dev_request_ibi_locked() - Request an IBI
+ * @dev: device for which we should enable IBIs
+ * @req: setup requested for this IBI
+ *
+ * This function is responsible for pre-allocating all resources needed to
+ * process IBIs coming from @dev. When this function returns, the IBI is not
+ * enabled until i3c_device_enable_ibi() is called.
+ *
+ * Context: Must be called with mutex_lock(&dev->desc->ibi_lock) held.
+ * Return: 0 in case of success, a negative error core otherwise.
+ */
int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
const struct i3c_ibi_setup *req)
{
@@ -3270,7 +3324,18 @@ int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
return ret;
}
+EXPORT_SYMBOL_GPL(i3c_dev_request_ibi_locked);
+/**
+ * i3c_dev_free_ibi_locked() - Free all resources needed for IBI handling
+ * @dev: device on which you want to release IBI resources
+ *
+ * This function is responsible for de-allocating resources previously
+ * allocated by i3c_device_request_ibi(). It should be called after disabling
+ * IBIs with i3c_device_disable_ibi().
+ *
+ * Context: Must be called with mutex_lock(&dev->desc->ibi_lock) held.
+ */
void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
{
struct i3c_master_controller *master = i3c_dev_get_master(dev);
@@ -3301,6 +3366,7 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
kfree(dev->ibi);
dev->ibi = NULL;
}
+EXPORT_SYMBOL_GPL(i3c_dev_free_ibi_locked);
static int __init i3c_init(void)
{
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 592b646f6134..355e9b3d9ae3 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -613,6 +613,8 @@ void i3c_master_dma_unmap_single(struct i3c_dma *dma_xfer);
DEFINE_FREE(i3c_master_dma_unmap_single, void *,
if (_T) i3c_master_dma_unmap_single(_T))
+int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
+ u8 old_dyn_addr);
int i3c_master_set_info(struct i3c_master_controller *master,
const struct i3c_device_info *info);
--
2.25.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v11 2/9] i3c: master: Expose the APIs to support I3C hub
2026-06-12 11:18 ` [PATCH v11 2/9] i3c: master: Expose the APIs to support I3C hub Lakshay Piplani
@ 2026-06-12 11:44 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-12 11:44 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: devicetree, Frank.Li, linux-i3c, conor+dt, robh
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [Medium] API documentation mismatch: i3c_master_reattach_i3c_dev_locked() claims to return -EBUSY on collision, but unconditionally overwrites slot status.
- [Low] Invalid lock context expressions and typos in newly added IBI kernel-doc.
Pre-existing issues:
- [Critical] Multiple Use-After-Free vulnerabilities in i3c_dev_free_ibi_locked() due to improper synchronization and teardown order.
- [High] Unbalanced release of address slots in i3c_master_reattach_i3c_dev_locked() failure path leads to bus corruption.
--
commit 13a1ff5c64b48ceff03545ae3bebd5e60a136c1e
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
i3c: master: Expose the APIs to support I3C hub
This patch exposes several internal I3C master functions to allow new I3C
hub drivers to use them. It adds kernel-doc comments and exports symbols
like i3c_master_reattach_i3c_dev_locked and various IBI handling functions.
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index c45b450af319d..ec3547f75469e 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1652,8 +1652,23 @@ static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
> return 0;
> }
>
> -static int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
> - u8 old_dyn_addr)
> +/**
> + * i3c_master_reattach_i3c_dev_locked() - reattach an I3C device with a new address
> + * @dev: I3C device descriptor to reattach
> + * @old_dyn_addr: previous dynamic address of the device
> + *
> + * This function reattaches an existing I3C device to the bus when its dynamic
> + * address has changed. It updates the bus address slot status accordingly:
> + * - Marks the new dynamic address as occupied by an I3C device.
> + * - Frees the old dynamic address slot if applicable.
> + *
> + * This function must be called with the bus lock held in write mode.
> + *
> + * Return: 0 on success, or a negative error code if reattachment fails
> + * (e.g. -EBUSY if the new address slot is not free).
> + */
> +int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
> + u8 old_dyn_addr)
> {
[Severity: Medium]
The kernel-doc states this returns -EBUSY if the new address slot is not free,
but does the implementation actually check this?
Looking at the rest of i3c_master_reattach_i3c_dev_locked(), it appears
i3c_bus_set_addr_slot_status() is called unconditionally to set
I3C_ADDR_SLOT_I3C_DEV without first checking if the slot is available.
Could this silently overwrite an existing occupied slot if an external driver
relies on the documented failure behavior?
[Severity: High]
This is a pre-existing issue, but does the error path safely handle a failure
from master->ops->reattach_i3c_dev()?
When the reattach operation fails, i3c_master_put_i3c_addrs(dev) is called,
which marks the address slots as free. However, it doesn't appear to revert
the hardware device's assigned address.
Since callers like i3c_master_add_i3c_dev_locked() completely ignore the
return value of this function, could this leave the bus manager out of sync
with the hardware state and cause severe address collisions if the slot is
subsequently reassigned?
[ ... ]
> @@ -3195,6 +3211,16 @@ int i3c_dev_do_xfers_locked(struct i3c_dev_desc *dev, struct i3c_xfer *xfers,
> return master->ops->i3c_xfers(dev, xfers, nxfers, mode);
> }
>
> +/**
> + * i3c_dev_disable_ibi_locked() - Disable IBIs coming from a specific device
> + * @dev: device on which IBIs should be disabled
> + *
> + * This function disable IBIs coming from a specific device and wait for
> + * all pending IBIs to be processed.
> + *
> + * Context: Must be called with mutex_lock(&dev->desc->ibi_lock) held.
> + * Return: 0 in case of success, a negative error core otherwise.
> + */
[Severity: Low]
Is the lock context expression correct here?
The parameter is of type struct i3c_dev_desc, which does not have a desc
member, so &dev->desc->ibi_lock appears to be structurally invalid.
Should this be &dev->ibi_lock instead?
Also, there is a minor typo: "error core" should likely be "error code".
[ ... ]
> +/**
> + * i3c_dev_free_ibi_locked() - Free all resources needed for IBI handling
> + * @dev: device on which you want to release IBI resources
> + *
> + * This function is responsible for de-allocating resources previously
> + * allocated by i3c_device_request_ibi(). It should be called after disabling
> + * IBIs with i3c_device_disable_ibi().
> + *
> + * Context: Must be called with mutex_lock(&dev->desc->ibi_lock) held.
> + */
> void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
[Severity: Critical]
This is a pre-existing issue, but does this teardown order introduce a
use-after-free regression when exposed to external drivers?
Looking at the body of i3c_dev_free_ibi_locked(), the function calls
master->ops->free_ibi(dev) before destroy_workqueue(dev->ibi->wq).
If free_ibi() frees the underlying IBI pool memory, and destroy_workqueue()
subsequently flushes pending async work items, could those work items
execute and dereference the already freed pool?
Additionally, if i3c_dev_disable_ibi_locked() had failed to disable the
hardware interrupt, could a firing interrupt access dev->ibi after it is
unconditionally freed by kfree(dev->ibi) at the end of this function?
> {
> struct i3c_master_controller *master = i3c_dev_get_master(dev);
> @@ -3301,6 +3366,7 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
> kfree(dev->ibi);
> dev->ibi = NULL;
> }
> +EXPORT_SYMBOL_GPL(i3c_dev_free_ibi_locked);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=2
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v11 3/9] i3c: master: Add APIs for I3C hub support
2026-06-12 11:18 [PATCH v11 0/9] Add support for NXP P3H2x4x I3C hub driver Lakshay Piplani
2026-06-12 11:18 ` [PATCH v11 1/9] i3c: master: rename i3c_master_reattach_i3c_dev() to *_locked Lakshay Piplani
2026-06-12 11:18 ` [PATCH v11 2/9] i3c: master: Expose the APIs to support I3C hub Lakshay Piplani
@ 2026-06-12 11:18 ` Lakshay Piplani
2026-06-12 11:48 ` sashiko-bot
2026-06-12 11:18 ` [PATCH v11 4/9] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support Lakshay Piplani
` (5 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Lakshay Piplani @ 2026-06-12 11:18 UTC (permalink / raw)
To: linux-kernel, linux-i3c, alexandre.belloni, krzk+dt, robh,
conor+dt, devicetree, broonie, lee, Frank.Li, lgirdwood
Cc: vikash.bansal, priyanka.jain, aman.kumarpandey, Lakshay Piplani
From: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Add helpers for attaching and detaching I3C devices and CCC helpers
to check CCC support and send CCC commands. These additions prepare
for I3C hub support.
The attach and detach helpers must be called with the bus lock held in
write mode.
1) i3c_master_direct_attach_i3c_dev_locked()
2) i3c_master_direct_detach_i3c_dev_locked()
3) i3c_master_send_ccc_cmd()
4) i3c_master_supports_ccc_cmd()
Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
Changes in v11:
- Convert i3c_master_supports_ccc_cmd() to return bool and align
semantics with CCC support checks used by the I3C core
Changes in v10:
- Rename i3c_master_direct_attach_i3c_dev and i3c_master_direct_detach_i3c_dev
APIs to *_locked, as these APIs must be called with the bus lock held in
write mode
Changes in v9:
- No change
Changes in v8:
- No change
Changes in v7:
- Update commit message to clarify purpose (prepare for I3C hub support)
Changes in v6:
- Split the patch into two parts:
1) expose the existing API
2) add new APIs.
---
---
drivers/i3c/master.c | 101 +++++++++++++++++++++++++++++++++++++
include/linux/i3c/master.h | 7 +++
2 files changed, 108 insertions(+)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 0636e3e21758..9cd2e9e3bd67 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1652,6 +1652,63 @@ static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
return 0;
}
+/**
+ * i3c_master_direct_attach_i3c_dev_locked() - attach an I3C device to a master
+ * @master: I3C master controller to attach the device to
+ * @dev: I3C device descriptor representing the device
+ *
+ * This function attaches an I3C device to its master controller once the
+ * device has a valid address on the bus. Devices without an assigned address
+ * are ignored. The master device itself is never attached through this bus.
+ *
+ * Context: Caller must hold master->bus.lock in write mode.
+ *
+ * Return: 0 on success, or a negative error code if the attach operation
+ * fails in the master controller driver.
+ */
+int i3c_master_direct_attach_i3c_dev_locked(struct i3c_master_controller *master,
+ struct i3c_dev_desc *dev)
+{
+ int ret = 0;
+
+ /*
+ * We don't attach devices to the controller until they are
+ * addressable on the bus.
+ */
+
+ if (!dev->info.static_addr && !dev->info.dyn_addr)
+ return 0;
+
+ /* Do not attach the master device itself. */
+ if (master->this != dev && master->ops->attach_i3c_dev)
+ ret = master->ops->attach_i3c_dev(dev);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(i3c_master_direct_attach_i3c_dev_locked);
+
+/**
+ * i3c_master_direct_detach_i3c_dev_locked() - Detach an I3C device from its
+ * master controller.
+ * @dev: I3C device descriptor to be detached
+ *
+ * This function detaches an I3C device from its master controller.
+ * It ensures that the master itself is not detached. If the device is not
+ * the master and the master controller provides a detach operation,
+ * the detach callback is invoked to perform the actual removal.
+ *
+ * Context: Caller must hold master->bus.lock in write mode.
+ */
+void i3c_master_direct_detach_i3c_dev_locked(struct i3c_dev_desc *dev)
+{
+ struct i3c_master_controller *master = i3c_dev_get_master(dev);
+
+ /* Do not detach the master device itself. */
+ if (master->this != dev && master->ops->detach_i3c_dev)
+ master->ops->detach_i3c_dev(dev);
+}
+EXPORT_SYMBOL_GPL(i3c_master_direct_detach_i3c_dev_locked);
+
/**
* i3c_master_reattach_i3c_dev_locked() - reattach an I3C device with a new address
* @dev: I3C device descriptor to reattach
@@ -1816,6 +1873,50 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
}
}
+/**
+ * i3c_master_supports_ccc_cmd() - check CCC command support
+ * @master: I3C master controller
+ * @cmd: CCC command to verify
+ *
+ * Return: true if @cmd is supported, false otherwise.
+ */
+bool i3c_master_supports_ccc_cmd(struct i3c_master_controller *master,
+ const struct i3c_ccc_cmd *cmd)
+{
+ if (!master || !cmd)
+ return false;
+
+ if (!master->ops->supports_ccc_cmd)
+ return true;
+
+ return master->ops->supports_ccc_cmd(master, cmd);
+}
+EXPORT_SYMBOL_GPL(i3c_master_supports_ccc_cmd);
+
+/**
+ * i3c_master_send_ccc_cmd() - send a CCC command
+ * @master: I3C master controller issuing the command
+ * @cmd: CCC command to be sent
+ *
+ * This function sends a Common Command Code (CCC) command to devices on the
+ * I3C bus. It acquires the bus maintenance lock, executes the command, and
+ * then releases the lock to ensure safe access to the bus.
+ *
+ * Return: 0 on success, or a negative error code on failure.
+ */
+int i3c_master_send_ccc_cmd(struct i3c_master_controller *master,
+ struct i3c_ccc_cmd *cmd)
+{
+ int ret;
+
+ i3c_bus_maintenance_lock(&master->bus);
+ ret = i3c_master_send_ccc_cmd_locked(master, cmd);
+ i3c_bus_maintenance_unlock(&master->bus);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(i3c_master_send_ccc_cmd);
+
/**
* i3c_master_do_daa_ext() - Dynamic Address Assignment (extended version)
* @master: controller
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 355e9b3d9ae3..7e358b6774c5 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -615,6 +615,13 @@ DEFINE_FREE(i3c_master_dma_unmap_single, void *,
int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
u8 old_dyn_addr);
+int i3c_master_direct_attach_i3c_dev_locked(struct i3c_master_controller *master,
+ struct i3c_dev_desc *dev);
+void i3c_master_direct_detach_i3c_dev_locked(struct i3c_dev_desc *dev);
+int i3c_master_send_ccc_cmd(struct i3c_master_controller *master,
+ struct i3c_ccc_cmd *cmd);
+bool i3c_master_supports_ccc_cmd(struct i3c_master_controller *master,
+ const struct i3c_ccc_cmd *cmd);
int i3c_master_set_info(struct i3c_master_controller *master,
const struct i3c_device_info *info);
--
2.25.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v11 3/9] i3c: master: Add APIs for I3C hub support
2026-06-12 11:18 ` [PATCH v11 3/9] i3c: master: Add APIs for I3C hub support Lakshay Piplani
@ 2026-06-12 11:48 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-12 11:48 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: linux-i3c, devicetree, conor+dt, Frank.Li, robh
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] i3c_master_direct_detach_i3c_dev_locked lacks the unaddressable device check present in its attach counterpart, leading to a NULL pointer dereference in controller drivers.
--
commit a7f74140d15672b8f2824666071ce06e3051e0d8
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
i3c: master: Add APIs for I3C hub support
Add helpers for attaching and detaching I3C devices and CCC helpers
to check CCC support and send CCC commands to prepare for I3C hub support.
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index ec3547f75469e..59e0f82de6ebd 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
[ ... ]
> +int i3c_master_direct_attach_i3c_dev_locked(struct i3c_master_controller *master,
> + struct i3c_dev_desc *dev)
> +{
> + int ret = 0;
> +
> + /*
> + * We don't attach devices to the controller until they are
> + * addressable on the bus.
> + */
> +
> + if (!dev->info.static_addr && !dev->info.dyn_addr)
> + return 0;
> +
> + /* Do not attach the master device itself. */
> + if (master->this != dev && master->ops->attach_i3c_dev)
> + ret = master->ops->attach_i3c_dev(dev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_direct_attach_i3c_dev_locked);
[ ... ]
> +void i3c_master_direct_detach_i3c_dev_locked(struct i3c_dev_desc *dev)
> +{
> + struct i3c_master_controller *master = i3c_dev_get_master(dev);
> +
> + /* Do not detach the master device itself. */
> + if (master->this != dev && master->ops->detach_i3c_dev)
> + master->ops->detach_i3c_dev(dev);
[Severity: High]
Does this missing address check in the detach helper lead to a NULL pointer
dereference in controller drivers?
The attach helper i3c_master_direct_attach_i3c_dev_locked() gracefully skips
attaching devices without a static or dynamic address and returns 0 (success).
Because it returns success, callers might naturally invoke this companion
detach API during teardown for these same devices.
However, since this helper lacks the corresponding address check, it
unconditionally invokes the controller's detach callback. Controller drivers
often retrieve the device's private data and immediately dereference it. Since
the attach phase was skipped, this private data would be NULL.
Should this function also check if the device has an address before calling
the detach callback?
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_direct_detach_i3c_dev_locked);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v11 4/9] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support
2026-06-12 11:18 [PATCH v11 0/9] Add support for NXP P3H2x4x I3C hub driver Lakshay Piplani
` (2 preceding siblings ...)
2026-06-12 11:18 ` [PATCH v11 3/9] i3c: master: Add APIs for I3C hub support Lakshay Piplani
@ 2026-06-12 11:18 ` Lakshay Piplani
2026-06-12 11:33 ` sashiko-bot
2026-06-12 11:18 ` [PATCH v11 5/9] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator Lakshay Piplani
` (4 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Lakshay Piplani @ 2026-06-12 11:18 UTC (permalink / raw)
To: linux-kernel, linux-i3c, alexandre.belloni, krzk+dt, robh,
conor+dt, devicetree, broonie, lee, Frank.Li, lgirdwood
Cc: vikash.bansal, priyanka.jain, aman.kumarpandey, Lakshay Piplani
From: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Add bindings for the NXP P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841)
multiport I3C hub family. These devices connect to a host via
I3C/I2C/SMBus and allow communication with multiple downstream
peripherals.
Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Signed-off-by: Vikash Bansal <vikash.bansal@nxp.com>
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
Changes in v11:
- No change
Changes in v10:
- No change, added Reviewed-By tag
Changes in v9:
- Referenced i3c.yaml and i2c-controller.yaml for child nodes
- Dropped unnecessary #address-cells and #size-cells from child nodes
Changes in v8:
- Add compatible in i3c example
Changes in v7:
- Fix schema validation issues
- Adjust required properties
- Add I2C example
Changes in v6:
- Use a vendor prefix for the attributes
Changes in v5:
- Removed SW properties: cp0-ldo-microvolt,cp1-ldo-microvolt,
tp0145-ldo-microvolt, tp2367-ldo-microvolt
- Changed supply entries and its descriptions
Changes in v4:
- Fixed DT binding check warning
- Removed SW properties: ibi-enable, local-dev, and always-enable
Changes in v3:
- Added MFD (Multi-Function Device) support for I3C hub and on-die regulator
- Added Regulator supply node
Changes in v2:
- Fixed DT binding check warning
- Revised logic for parsing DTS nodes
---
---
.../devicetree/bindings/i3c/nxp,p3h2840.yaml | 291 ++++++++++++++++++
MAINTAINERS | 9 +
2 files changed, 300 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
diff --git a/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
new file mode 100644
index 000000000000..c080eeb0eeaa
--- /dev/null
+++ b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
@@ -0,0 +1,291 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2025 NXP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i3c/nxp,p3h2840.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP P3H2X4X I3C HUB
+
+maintainers:
+ - Aman Kumar Pandey <aman.kumarpandey@nxp.com>
+ - Vikash Bansal <vikash.bansal@nxp.com>
+ - Lakshay Piplani <lakshay.piplani@nxp.com>
+
+description: |
+ P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841) is a family of multiport I3C
+ hub devices that connect to:-
+ 1. A host CPU via I3C/I2C/SMBus bus on upstream side and connect to multiple
+ peripheral devices on the downstream side.
+ 2. Have two Controller Ports which can support either
+ I2C/SMBus or I3C buses and connect to a CPU, BMC or SOC.
+ 3. P3H2840/ P3H2841 are 8 port I3C hub with eight I3C/I2C Target Port.
+ 4. P3H2440/ P3H2441 are 4 port I3C hub with four I3C/I2C Target Port.
+ Target ports can be configured as I2C/SMBus, I3C or GPIO and connect to
+ peripherals.
+
+properties:
+ compatible:
+ const: nxp,p3h2840
+
+ reg:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ assigned-address:
+ maximum: 0x7f
+
+ nxp,tp0145-pullup-ohms:
+ description:
+ Selects the pull up resistance for target Port 0/1/4/5, in ohms.
+ enum: [250, 500, 1000, 2000]
+ default: 500
+
+ nxp,tp2367-pullup-ohms:
+ description:
+ Selects the pull up resistance for target Port 2/3/6/7, in ohms.
+ enum: [250, 500, 1000, 2000]
+ default: 500
+
+ nxp,cp0-io-strength-ohms:
+ description:
+ Selects the IO drive strength for controller Port 0, in ohms.
+ enum: [20, 30, 40, 50]
+ default: 20
+
+ nxp,cp1-io-strength-ohms:
+ description:
+ Selects the IO drive strength for controller Port 1, in ohms.
+ enum: [20, 30, 40, 50]
+ default: 20
+
+ nxp,tp0145-io-strength-ohms:
+ description:
+ Selects the IO drive strength for target port 0/1/4/5, in ohms.
+ enum: [20, 30, 40, 50]
+ default: 20
+
+ nxp,tp2367-io-strength-ohms:
+ description:
+ Selects the IO drive strength for target port 2/3/6/7, in ohms.
+ enum: [20, 30, 40, 50]
+ default: 20
+
+ vcc1-supply:
+ description: Controller port 0 power supply.
+
+ vcc2-supply:
+ description: Controller port 1 power supply.
+
+ vcc3-supply:
+ description: Target port 0/1/4/5 power supply.
+
+ vcc4-supply:
+ description: Target port 2/3/6/7 power supply.
+
+ regulators:
+ type: object
+ additionalProperties: false
+
+ properties:
+ ldo-cp0:
+ type: object
+ $ref: /schemas/regulator/regulator.yaml#
+ unevaluatedProperties: false
+
+ ldo-cp1:
+ type: object
+ $ref: /schemas/regulator/regulator.yaml#
+ unevaluatedProperties: false
+
+ ldo-tpg0:
+ type: object
+ $ref: /schemas/regulator/regulator.yaml#
+ unevaluatedProperties: false
+
+ ldo-tpg1:
+ type: object
+ $ref: /schemas/regulator/regulator.yaml#
+ unevaluatedProperties: false
+
+required:
+ - reg
+
+patternProperties:
+ "^i3c@[0-7]$":
+ type: object
+ $ref: /schemas/i3c/i3c.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ description:
+ The I3C HUB Target Port number.
+ maximum: 7
+
+ nxp,pullup-enable:
+ type: boolean
+ description:
+ Enables the on-die pull-up for Target Port.
+
+ required:
+ - reg
+
+ "^(i2c|smbus)@[0-7]$":
+ type: object
+ $ref: /schemas/i2c/i2c-controller.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ description:
+ The I3C HUB Target Port number.
+ maximum: 7
+
+ nxp,pullup-enable:
+ type: boolean
+ description:
+ Enables the on-die pull-up for Target Port.
+
+ required:
+ - reg
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ i3c {
+ #address-cells = <3>;
+ #size-cells = <0>;
+
+ hub@70,236153000c2 {
+ reg = <0x70 0x236 0x3000c2>;
+ compatible = "nxp,p3h2840";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ assigned-address = <0x50>;
+
+ nxp,tp0145-pullup-ohms = <1000>;
+ nxp,tp2367-pullup-ohms = <1000>;
+ nxp,cp0-io-strength-ohms = <50>;
+ nxp,cp1-io-strength-ohms = <50>;
+ nxp,tp0145-io-strength-ohms = <50>;
+ nxp,tp2367-io-strength-ohms = <50>;
+ vcc3-supply = <®_tpg0>;
+ vcc4-supply = <®_tpg1>;
+
+ regulators {
+ reg_cp0: ldo-cp0 {
+ regulator-name = "ldo-cp0";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+
+ reg_cp1: ldo-cp1 {
+ regulator-name = "ldo-cp1";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+
+ reg_tpg0: ldo-tpg0 {
+ regulator-name = "ldo-tpg0";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+
+ reg_tpg1: ldo-tpg1 {
+ regulator-name = "ldo-tpg1";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ };
+
+ smbus@0 {
+ reg = <0x0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ nxp,pullup-enable;
+ };
+
+ i2c@1 {
+ reg = <0x1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ nxp,pullup-enable;
+ };
+
+ i3c@2 {
+ reg = <0x2>;
+ #address-cells = <3>;
+ #size-cells = <0>;
+ nxp,pullup-enable;
+ };
+ };
+ };
+
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ hub@70 {
+ reg = <0x70>;
+ compatible = "nxp,p3h2840";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ nxp,tp0145-pullup-ohms = <1000>;
+ nxp,tp2367-pullup-ohms = <1000>;
+ nxp,cp0-io-strength-ohms = <50>;
+ nxp,cp1-io-strength-ohms = <50>;
+ nxp,tp0145-io-strength-ohms = <50>;
+ nxp,tp2367-io-strength-ohms = <50>;
+ vcc3-supply = <®_tpg0_i2c>;
+ vcc4-supply = <®_tpg1_i2c>;
+
+ regulators {
+ reg_cp0_i2c: ldo-cp0 {
+ regulator-name = "ldo-cp0";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+
+ reg_cp1_i2c: ldo-cp1 {
+ regulator-name = "ldo-cp1";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+
+ reg_tpg0_i2c: ldo-tpg0 {
+ regulator-name = "ldo-tpg0";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+
+ reg_tpg1_i2c: ldo-tpg1 {
+ regulator-name = "ldo-tpg1";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ };
+
+ smbus@0 {
+ reg = <0x0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ nxp,pullup-enable;
+ };
+
+ i2c@1 {
+ reg = <0x1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ nxp,pullup-enable;
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index c2c6d79275c6..ba65ae5a008b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19303,6 +19303,15 @@ S: Maintained
F: Documentation/devicetree/bindings/ptp/nxp,ptp-netc.yaml
F: drivers/ptp/ptp_netc.c
+NXP P3H2X4X I3C-HUB DRIVER
+M: Vikash Bansal <vikash.bansal@nxp.com>
+M: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
+M: Lakshay Piplani <lakshay.piplani@nxp.com>
+L: linux-kernel@vger.kernel.org
+L: linux-i3c@lists.infradead.org
+S: Maintained
+F: Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
+
NXP PF5300/PF5301/PF5302 PMIC REGULATOR DEVICE DRIVER
M: Woodrow Douglass <wdouglass@carnegierobotics.com>
S: Maintained
--
2.25.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v11 4/9] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support
2026-06-12 11:18 ` [PATCH v11 4/9] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support Lakshay Piplani
@ 2026-06-12 11:33 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-12 11:33 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: Frank.Li, conor+dt, devicetree, robh, linux-i3c
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing GPIO provider properties and constraints in the DT binding schema, violating subsystem guidelines.
- [Medium] Missing compatible strings and port constraints for the documented 4-port hardware variants (P3H2440/P3H2441).
--
commit cfd8cd31027f9ddcdd30fe595d5d947dc8bae49b
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support
Adds bindings for the NXP P3H2x4x multiport I3C hub family. These devices
connect to a host via I3C/I2C/SMBus and allow communication with multiple
downstream peripherals.
> diff --git a/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> new file mode 100644
> index 0000000000000..c080eeb0eeaa8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> @@ -0,0 +1,291 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2025 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i3c/nxp,p3h2840.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP P3H2X4X I3C HUB
[ ... ]
> +description: |
> + P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841) is a family of multiport I3C
> + hub devices that connect to:-
> + 1. A host CPU via I3C/I2C/SMBus bus on upstream side and connect to multiple
> + peripheral devices on the downstream side.
> + 2. Have two Controller Ports which can support either
> + I2C/SMBus or I3C buses and connect to a CPU, BMC or SOC.
> + 3. P3H2840/ P3H2841 are 8 port I3C hub with eight I3C/I2C Target Port.
> + 4. P3H2440/ P3H2441 are 4 port I3C hub with four I3C/I2C Target Port.
> + Target ports can be configured as I2C/SMBus, I3C or GPIO and connect to
> + peripherals.
[Severity: Medium]
If the target ports can be configured as GPIO, should the schema include the
corresponding provider properties (such as gpio-controller and #gpio-cells)?
Without these properties defined, device trees attempting to use the
documented GPIO capabilities will be strictly rejected due to
unevaluatedProperties: false at the top level.
> +
> +properties:
> + compatible:
> + const: nxp,p3h2840
[Severity: Medium]
The description mentions support for the P3H2440 and P3H2441 4-port hardware
variants, but the compatible property is restricted strictly to nxp,p3h2840.
Should the compatible strings for the 4-port variants also be included here?
[ ... ]
> +patternProperties:
> + "^i3c@[0-7]$":
> + type: object
> + $ref: /schemas/i3c/i3c.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + reg:
> + description:
> + The I3C HUB Target Port number.
> + maximum: 7
[Severity: Medium]
The maximum port number is currently hardcoded to 7 for all variants. If the
4-port variants (P3H2440/P3H2441) are supported, wouldn't we need conditional
if-then constraints to restrict the reg maximum to 3 for the 4-port hubs?
Otherwise, invalid device trees defining non-existent ports on 4-port hubs
might pass validation.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=4
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v11 5/9] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator
2026-06-12 11:18 [PATCH v11 0/9] Add support for NXP P3H2x4x I3C hub driver Lakshay Piplani
` (3 preceding siblings ...)
2026-06-12 11:18 ` [PATCH v11 4/9] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support Lakshay Piplani
@ 2026-06-12 11:18 ` Lakshay Piplani
2026-06-12 11:41 ` sashiko-bot
2026-06-12 11:18 ` [PATCH v11 6/9] regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub Lakshay Piplani
` (3 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Lakshay Piplani @ 2026-06-12 11:18 UTC (permalink / raw)
To: linux-kernel, linux-i3c, alexandre.belloni, krzk+dt, robh,
conor+dt, devicetree, broonie, lee, Frank.Li, lgirdwood
Cc: vikash.bansal, priyanka.jain, aman.kumarpandey, Lakshay Piplani
From: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Add core MFD support for the NXP P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841)
family of multiport I3C hub devices. These devices connect to a host via
I3C/I2C/SMBus and expose multiple downstream target ports.
Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Signed-off-by: Vikash Bansal <vikash.bansal@nxp.com>
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
Changes in v11:
- Use MFD_CELL_NAME() for child device registration
- Rename local variables for consistency
- Rename driver names to follow subsystem conventions:
- Use '-' instead of '_' in driver names
- Drop the "_drv" suffix from driver names
Changes in v10:
- Drop redundant is_p3h2x4x_in_i3c flag
Changes in v9:
- Renamed macros to follow consistent uppercase naming conventions
- Made REGMAP selects in the P3H2X4X MFD Kconfig conditional,
to avoid I3C/I2C dependency issues
Changes in v8:
- No change
Changes in v7:
- Use new config I3C_OR_I2C
Changes in v6:
- No change
Changes in v5:
- Corrected the ordering in the Makefile and Kconfig for MFD_P3H2X4X
- Updated dev_err_probe() for regmap_init failure.
- Updated module description
Changes in v4:
- Split the driver into three separate patches(mfd, regulator and I3C hub)
- Added support for NXP P3H2x4x MFD functionality
---
---
MAINTAINERS | 2 +
drivers/mfd/Kconfig | 13 ++++
drivers/mfd/Makefile | 1 +
drivers/mfd/p3h2840.c | 119 ++++++++++++++++++++++++++++++++++++
include/linux/i3c/device.h | 1 +
include/linux/mfd/p3h2840.h | 26 ++++++++
6 files changed, 162 insertions(+)
create mode 100644 drivers/mfd/p3h2840.c
create mode 100644 include/linux/mfd/p3h2840.h
diff --git a/MAINTAINERS b/MAINTAINERS
index ba65ae5a008b..3420701a75c5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19311,6 +19311,8 @@ L: linux-kernel@vger.kernel.org
L: linux-i3c@lists.infradead.org
S: Maintained
F: Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
+F: drivers/mfd/p3h2840.c
+F: include/linux/mfd/p3h2840.h
NXP PF5300/PF5301/PF5302 PMIC REGULATOR DEVICE DRIVER
M: Woodrow Douglass <wdouglass@carnegierobotics.com>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 7192c9d1d268..405b50c3c77b 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -617,6 +617,19 @@ config MFD_MX25_TSADC
i.MX25 processors. They consist of a conversion queue for general
purpose ADC and a queue for Touchscreens.
+config MFD_P3H2X4X
+ tristate "NXP P3H2X4X I3C Hub Device"
+ depends on I3C_OR_I2C
+ select MFD_CORE
+ select REGMAP_I3C if I3C
+ select REGMAP_I2C if I2C
+ help
+ Enable Support for NXP P3H244x/P3H284x I3C HUB device using I3C/I2C
+ communication interface.
+
+ This driver provides support for I3C hub and regulator, each subdriver
+ can be enabled independently depending on the required functionality.
+
config MFD_PF1550
tristate "NXP PF1550 PMIC Support"
depends on I2C=y && OF
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index e75e8045c28a..a284b22c7b13 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -122,6 +122,7 @@ obj-$(CONFIG_MFD_MC13XXX) += mc13xxx-core.o
obj-$(CONFIG_MFD_MC13XXX_SPI) += mc13xxx-spi.o
obj-$(CONFIG_MFD_MC13XXX_I2C) += mc13xxx-i2c.o
+obj-$(CONFIG_MFD_P3H2X4X) += p3h2840.o
obj-$(CONFIG_MFD_PF1550) += pf1550.o
obj-$(CONFIG_MFD_NCT6694) += nct6694.o
diff --git a/drivers/mfd/p3h2840.c b/drivers/mfd/p3h2840.c
new file mode 100644
index 000000000000..0a80992d6250
--- /dev/null
+++ b/drivers/mfd/p3h2840.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025-2026 NXP
+ * P3H2X4X i3c hub and regulator device.
+ */
+
+#include <linux/i2c.h>
+#include <linux/i3c/master.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/p3h2840.h>
+#include <linux/regmap.h>
+
+static const struct mfd_cell p3h2x4x_devs[] = {
+ MFD_CELL_NAME("p3h2x4x-regulator"),
+ MFD_CELL_NAME("p3h2x4x-i3c-hub"),
+};
+
+static const struct regmap_config p3h2x4x_regmap_config = {
+ .reg_bits = P3H2X4X_REG_BITS,
+ .val_bits = P3H2X4X_VAL_BITS,
+ .max_register = 0xFF,
+};
+
+static int p3h2x4x_device_probe_i3c(struct i3c_device *i3cdev)
+{
+ struct p3h2x4x_dev *ddata;
+ int ret;
+
+ ddata = devm_kzalloc(&i3cdev->dev, sizeof(*ddata), GFP_KERNEL);
+ if (!ddata)
+ return -ENOMEM;
+
+ i3cdev_set_drvdata(i3cdev, ddata);
+
+ ddata->regmap = devm_regmap_init_i3c(i3cdev, &p3h2x4x_regmap_config);
+ if (IS_ERR(ddata->regmap))
+ return dev_err_probe(&i3cdev->dev, PTR_ERR(ddata->regmap),
+ "Failed to register HUB regmap\n");
+
+ // The hub child driver retrieves information from i3cdev
+ ddata->i3cdev = i3cdev;
+
+ ret = devm_mfd_add_devices(&i3cdev->dev, PLATFORM_DEVID_AUTO,
+ p3h2x4x_devs, ARRAY_SIZE(p3h2x4x_devs),
+ NULL, 0, NULL);
+ if (ret)
+ return dev_err_probe(&i3cdev->dev, ret, "Failed to add sub devices\n");
+
+ return 0;
+}
+
+static int p3h2x4x_device_probe_i2c(struct i2c_client *client)
+{
+ struct p3h2x4x_dev *ddata;
+ int ret;
+
+ ddata = devm_kzalloc(&client->dev, sizeof(*ddata), GFP_KERNEL);
+ if (!ddata)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, ddata);
+
+ ddata->regmap = devm_regmap_init_i2c(client, &p3h2x4x_regmap_config);
+ if (IS_ERR(ddata->regmap))
+ return dev_err_probe(&client->dev, PTR_ERR(ddata->regmap),
+ "Failed to register HUB regmap\n");
+
+ ddata->i3cdev = NULL;
+
+ ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO,
+ p3h2x4x_devs, ARRAY_SIZE(p3h2x4x_devs),
+ NULL, 0, NULL);
+ if (ret)
+ return dev_err_probe(&client->dev, ret, "Failed to add sub devices\n");
+
+ return 0;
+}
+
+static const struct i3c_device_id p3h2x4x_i3c_ids[] = {
+ I3C_CLASS(I3C_DCR_HUB, NULL),
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, p3h2x4x_i3c_ids);
+
+static const struct i2c_device_id p3h2x4x_i2c_id_table[] = {
+ { "nxp-i3c-hub" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, p3h2x4x_i2c_id_table);
+
+static const struct of_device_id p3h2x4x_i2c_of_match[] = {
+ { .compatible = "nxp,p3h2840", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, p3h2x4x_i2c_of_match);
+
+static struct i3c_driver p3h2x4x_i3c = {
+ .driver = {
+ .name = "p3h2x4x-i3c",
+ },
+ .probe = p3h2x4x_device_probe_i3c,
+ .id_table = p3h2x4x_i3c_ids,
+};
+
+static struct i2c_driver p3h2x4x_i2c = {
+ .driver = {
+ .name = "p3h2x4x-i2c",
+ .of_match_table = p3h2x4x_i2c_of_match,
+ },
+ .probe = p3h2x4x_device_probe_i2c,
+ .id_table = p3h2x4x_i2c_id_table,
+};
+module_i3c_i2c_driver(p3h2x4x_i3c, &p3h2x4x_i2c);
+
+MODULE_AUTHOR("Aman Kumar Pandey <aman.kumarpandey@nxp.com>");
+MODULE_AUTHOR("Vikash Bansal <vikash.bansal@nxp.com>");
+MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
+MODULE_DESCRIPTION("NXP P3H2X4X I3C HUB multi function driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
index 971d53349b6f..6188082599dd 100644
--- a/include/linux/i3c/device.h
+++ b/include/linux/i3c/device.h
@@ -85,6 +85,7 @@ struct i3c_xfer {
*/
enum i3c_dcr {
I3C_DCR_GENERIC_DEVICE = 0,
+ I3C_DCR_HUB = 194,
};
#define I3C_PID_MANUF_ID(pid) (((pid) & GENMASK_ULL(47, 33)) >> 33)
diff --git a/include/linux/mfd/p3h2840.h b/include/linux/mfd/p3h2840.h
new file mode 100644
index 000000000000..2d4fae2176ae
--- /dev/null
+++ b/include/linux/mfd/p3h2840.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2025-2026 NXP
+ * This header file contains register definitions and bit masks for the P3H2X4X.
+ */
+
+#ifndef _LINUX_MFD_P3H2840_H
+#define _LINUX_MFD_P3H2840_H
+
+#include <linux/types.h>
+
+/* Device Configuration Registers */
+#define P3H2X4X_DEV_REG_PROTECTION_CODE 0x10
+#define P3H2X4X_REGISTERS_LOCK_CODE 0x00
+#define P3H2X4X_REGISTERS_UNLOCK_CODE 0x69
+#define P3H2X4X_CP1_REGISTERS_UNLOCK_CODE 0x6a
+
+/* Reg config for Regmap */
+#define P3H2X4X_REG_BITS 8
+#define P3H2X4X_VAL_BITS 8
+
+struct p3h2x4x_dev {
+ struct i3c_device *i3cdev;
+ struct regmap *regmap;
+};
+#endif /* _LINUX_MFD_P3H2840_H */
--
2.25.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v11 5/9] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator
2026-06-12 11:18 ` [PATCH v11 5/9] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator Lakshay Piplani
@ 2026-06-12 11:41 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-12 11:41 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: robh, devicetree, linux-i3c, Frank.Li, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver incorrectly binds to any generic I3C hub device on the bus.
- [Low] The driver bypasses the I3C subsystem abstraction by including <linux/i3c/master.h> instead of <linux/i3c/device.h>.
--
commit 3682ca3c66e35b06e88823e8081c57b3ea1d4084
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator
Add core MFD support for the NXP P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841)
family of multiport I3C hub devices. These devices connect to a host via
I3C/I2C/SMBus and expose multiple downstream target ports.
> diff --git a/drivers/mfd/p3h2840.c b/drivers/mfd/p3h2840.c
> new file mode 100644
> index 0000000000000..0a80992d6250a
> --- /dev/null
> +++ b/drivers/mfd/p3h2840.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025-2026 NXP
> + * P3H2X4X i3c hub and regulator device.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/i3c/master.h>
[Severity: Low]
Does this include bypass the I3C subsystem abstraction? By including the
master header, the driver has access to the internal definition of
struct i3c_device. Should this include <linux/i3c/device.h> instead?
[ ... ]
> +static int p3h2x4x_device_probe_i3c(struct i3c_device *i3cdev)
> +{
> + struct p3h2x4x_dev *ddata;
> + int ret;
> +
> + ddata = devm_kzalloc(&i3cdev->dev, sizeof(*ddata), GFP_KERNEL);
[Severity: Low]
This directly accesses the internal dev field of struct i3c_device. Should
this use i3cdev_to_dev(i3cdev) to obtain the underlying struct device pointer
to maintain API boundaries?
[ ... ]
> +static const struct i3c_device_id p3h2x4x_i3c_ids[] = {
> + I3C_CLASS(I3C_DCR_HUB, NULL),
> + { /* sentinel */ },
> +};
[Severity: High]
Will this driver inappropriately bind to any generic I3C hub device on the
bus?
Using I3C_CLASS(I3C_DCR_HUB, NULL) instructs the subsystem to match any
device that advertises the generic Hub DCR (194), without filtering by
vendor or part ID.
Because the probe function doesn't verify the Manufacturer ID, it seems this
could eagerly bind to third-party hubs and attempt NXP-specific register
operations on them.
Should this use I3C_DEVICE(I3C_VENDOR_NXP, ...) to exclusively match the
supported NXP devices instead of the generic hub class?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=5
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v11 6/9] regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub
2026-06-12 11:18 [PATCH v11 0/9] Add support for NXP P3H2x4x I3C hub driver Lakshay Piplani
` (4 preceding siblings ...)
2026-06-12 11:18 ` [PATCH v11 5/9] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator Lakshay Piplani
@ 2026-06-12 11:18 ` Lakshay Piplani
2026-06-12 11:37 ` sashiko-bot
2026-06-12 11:18 ` [PATCH v11 7/9] i3c: hub: Add support for the I3C interface in the I3C hub Lakshay Piplani
` (2 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Lakshay Piplani @ 2026-06-12 11:18 UTC (permalink / raw)
To: linux-kernel, linux-i3c, alexandre.belloni, krzk+dt, robh,
conor+dt, devicetree, broonie, lee, Frank.Li, lgirdwood
Cc: vikash.bansal, priyanka.jain, aman.kumarpandey, Lakshay Piplani,
Frank Li
From: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
The NXP P3H2x4x family integrates on-die regulators alongside I3C hub
functionality. This driver registers the regulators using the MFD
framework and exposes them via the regulator subsystem.
Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Signed-off-by: Vikash Bansal <vikash.bansal@nxp.com>
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Reviewed-by: Frank Li <frank.li@nxp.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
Changes in v11:
- No change, added Reviewed-By tag
Changes in v10:
- No change
Changes in v9:
- No change
Changes in v8:
- No change
Changes in v7:
- No change, added Reviewed-By tag
Changes in v6:
- Use DEFINE_LOCK_GUARD_1 for reg lock/unlock
Changes in v5:
- Updated dev_err_probe() for regmap_init failure.
- Updated module description
Changes in v4:
- Split the driver into three separate patches (mfd, regulator and I3C hub)
- Introduced driver for on-die regulators in NXP P3H2x4x I3C hub
---
---
MAINTAINERS | 1 +
drivers/regulator/Kconfig | 10 +
drivers/regulator/Makefile | 1 +
drivers/regulator/p3h2840_i3c_hub_regulator.c | 218 ++++++++++++++++++
4 files changed, 230 insertions(+)
create mode 100644 drivers/regulator/p3h2840_i3c_hub_regulator.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 3420701a75c5..fcb3917aa096 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19312,6 +19312,7 @@ L: linux-i3c@lists.infradead.org
S: Maintained
F: Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
F: drivers/mfd/p3h2840.c
+F: drivers/regulator/p3h2840_i3c_hub_regulator.c
F: include/linux/mfd/p3h2840.h
NXP PF5300/PF5301/PF5302 PMIC REGULATOR DEVICE DRIVER
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index d71dac9436e3..88809f493fd4 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1019,6 +1019,16 @@ config REGULATOR_MTK_DVFSRC
of Mediatek. It allows for voting on regulator state
between multiple users.
+config REGULATOR_P3H2X4X
+ tristate "NXP P3H2X4X regulator support"
+ depends on MFD_P3H2X4X
+ help
+ This driver provides support for the voltage regulators of the
+ P3H244x/P3H284x multi-function I3C Hub device.
+
+ Say M here if you want to include support for this regulator as
+ a module. The module will be named "p3h2840_i3c_hub_regulator".
+
config REGULATOR_PALMAS
tristate "TI Palmas PMIC Regulators"
depends on MFD_PALMAS
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 35639f3115fd..46f586ccde63 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -128,6 +128,7 @@ obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_SPMI) += qcom_spmi-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_USB_VBUS) += qcom_usb_vbus-regulator.o
+obj-$(CONFIG_REGULATOR_P3H2X4X) += p3h2840_i3c_hub_regulator.o
obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
obj-$(CONFIG_REGULATOR_PCA9450) += pca9450-regulator.o
obj-$(CONFIG_REGULATOR_PF0900) += pf0900-regulator.o
diff --git a/drivers/regulator/p3h2840_i3c_hub_regulator.c b/drivers/regulator/p3h2840_i3c_hub_regulator.c
new file mode 100644
index 000000000000..4f2514d4d928
--- /dev/null
+++ b/drivers/regulator/p3h2840_i3c_hub_regulator.c
@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025-2026 NXP
+ * This P3H2X4X driver file contain functions for enable/disable regulator and voltage set/get.
+ */
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/mfd/p3h2840.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+
+#define P3H2X4X_LDO_AND_PULLUP_CONF 0x19
+#define P3H2X4X_LDO_ENABLE_DISABLE_MASK GENMASK(3, 0)
+#define P3H2X4X_CP0_EN_LDO BIT(0)
+#define P3H2X4X_CP1_EN_LDO BIT(1)
+#define P3H2X4X_TP0145_EN_LDO BIT(2)
+#define P3H2X4X_TP2367_EN_LDO BIT(3)
+
+#define P3H2X4X_NET_OPER_MODE_CONF 0x15
+#define P3H2X4X_VCCIO_LDO_CONF 0x16
+#define P3H2X4X_CP0_VCCIO_LDO_VOLTAGE_MASK GENMASK(1, 0)
+#define P3H2X4X_CP0_VCCIO_LDO_VOLTAGE(x) \
+ FIELD_PREP(P3H2X4X_CP0_VCCIO_LDO_VOLTAGE_MASK, x)
+#define P3H2X4X_CP1_VCCIO_LDO_VOLTAGE_MASK GENMASK(3, 2)
+#define P3H2X4X_CP1_VCCIO_LDO_VOLTAGE(x) \
+ FIELD_PREP(P3H2X4X_CP1_VCCIO_LDO_VOLTAGE_MASK, x)
+#define P3H2X4X_TP0145_VCCIO_LDO_VOLTAGE_MASK GENMASK(5, 4)
+#define P3H2X4X_TP0145_VCCIO_LDO_VOLTAGE(x) \
+ FIELD_PREP(P3H2X4X_TP0145_VCCIO_LDO_VOLTAGE_MASK, x)
+#define P3H2X4X_TP2367_VCCIO_LDO_VOLTAGE_MASK GENMASK(7, 6)
+#define P3H2X4X_TP2367_VCCIO_LDO_VOLTAGE(x) \
+ FIELD_PREP(P3H2X4X_TP2367_VCCIO_LDO_VOLTAGE_MASK, x)
+#define P3H2X4X_LDO_COUNT 4
+
+struct p3h2x4x_regulator_dev {
+ struct regulator_dev *rp3h2x4x_dev[P3H2X4X_LDO_COUNT];
+ struct regmap *regmap;
+};
+
+struct p3h2x4x_reg_state {
+ unsigned int orig;
+ bool restore;
+};
+
+static void p3h2x4x_reg_guard_enter(struct regulator_dev *rdev,
+ struct p3h2x4x_reg_state *state)
+{
+ state->restore = false;
+
+ if (regmap_read(rdev->regmap,
+ P3H2X4X_DEV_REG_PROTECTION_CODE,
+ &state->orig))
+ return;
+
+ if (state->orig != P3H2X4X_REGISTERS_UNLOCK_CODE) {
+ regmap_write(rdev->regmap,
+ P3H2X4X_DEV_REG_PROTECTION_CODE,
+ P3H2X4X_REGISTERS_UNLOCK_CODE);
+ state->restore = true;
+ }
+}
+
+static void p3h2x4x_reg_guard_exit(struct regulator_dev *rdev,
+ struct p3h2x4x_reg_state *state)
+{
+ if (state->restore)
+ regmap_write(rdev->regmap,
+ P3H2X4X_DEV_REG_PROTECTION_CODE,
+ state->orig);
+}
+
+DEFINE_LOCK_GUARD_1(p3h2x4x_reg, struct regulator_dev,
+ p3h2x4x_reg_guard_enter(_T->lock, &_T->state),
+ p3h2x4x_reg_guard_exit(_T->lock, &_T->state),
+ struct p3h2x4x_reg_state state);
+
+static int p3h2x4x_regulator_enable(struct regulator_dev *rdev)
+{
+ guard(p3h2x4x_reg)(rdev);
+ return regulator_enable_regmap(rdev);
+}
+
+static int p3h2x4x_regulator_disable(struct regulator_dev *rdev)
+{
+ guard(p3h2x4x_reg)(rdev);
+ return regulator_disable_regmap(rdev);
+}
+
+static int p3h2x4x_regulator_set_voltage_sel(struct regulator_dev *rdev,
+ unsigned int sel)
+{
+ guard(p3h2x4x_reg)(rdev);
+ return regulator_set_voltage_sel_regmap(rdev, sel);
+}
+
+static const struct regulator_ops p3h2x4x_ldo_ops = {
+ .list_voltage = regulator_list_voltage_table,
+ .map_voltage = regulator_map_voltage_iterate,
+ .set_voltage_sel = p3h2x4x_regulator_set_voltage_sel,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .enable = p3h2x4x_regulator_enable,
+ .disable = p3h2x4x_regulator_disable,
+ .is_enabled = regulator_is_enabled_regmap,
+};
+
+static const unsigned int p3h2x4x_voltage_table[] = {
+ 1000000,
+ 1100000,
+ 1200000,
+ 1800000,
+};
+
+static struct regulator_desc p3h2x4x_regulators[] = {
+ {
+ .name = "ldo-cp0",
+ .of_match = of_match_ptr("ldo-cp0"),
+ .regulators_node = of_match_ptr("regulators"),
+ .volt_table = p3h2x4x_voltage_table,
+ .n_voltages = ARRAY_SIZE(p3h2x4x_voltage_table),
+ .ops = &p3h2x4x_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .enable_reg = P3H2X4X_LDO_AND_PULLUP_CONF,
+ .enable_mask = P3H2X4X_CP0_EN_LDO,
+ .vsel_reg = P3H2X4X_VCCIO_LDO_CONF,
+ .vsel_mask = P3H2X4X_CP0_VCCIO_LDO_VOLTAGE_MASK,
+ },
+ {
+ .name = "ldo-cp1",
+ .of_match = of_match_ptr("ldo-cp1"),
+ .regulators_node = of_match_ptr("regulators"),
+ .volt_table = p3h2x4x_voltage_table,
+ .n_voltages = ARRAY_SIZE(p3h2x4x_voltage_table),
+ .ops = &p3h2x4x_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .enable_reg = P3H2X4X_LDO_AND_PULLUP_CONF,
+ .enable_mask = P3H2X4X_CP1_EN_LDO,
+ .vsel_reg = P3H2X4X_VCCIO_LDO_CONF,
+ .vsel_mask = P3H2X4X_CP1_VCCIO_LDO_VOLTAGE_MASK,
+ },
+ {
+ .name = "ldo-tpg0",
+ .of_match = of_match_ptr("ldo-tpg0"),
+ .regulators_node = of_match_ptr("regulators"),
+ .volt_table = p3h2x4x_voltage_table,
+ .n_voltages = ARRAY_SIZE(p3h2x4x_voltage_table),
+ .ops = &p3h2x4x_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .enable_reg = P3H2X4X_LDO_AND_PULLUP_CONF,
+ .enable_mask = P3H2X4X_TP0145_EN_LDO,
+ .vsel_reg = P3H2X4X_VCCIO_LDO_CONF,
+ .vsel_mask = P3H2X4X_TP0145_VCCIO_LDO_VOLTAGE_MASK,
+ },
+ {
+ .name = "ldo-tpg1",
+ .of_match = of_match_ptr("ldo-tpg1"),
+ .regulators_node = of_match_ptr("regulators"),
+ .volt_table = p3h2x4x_voltage_table,
+ .n_voltages = ARRAY_SIZE(p3h2x4x_voltage_table),
+ .ops = &p3h2x4x_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .enable_reg = P3H2X4X_LDO_AND_PULLUP_CONF,
+ .enable_mask = P3H2X4X_TP2367_EN_LDO,
+ .vsel_reg = P3H2X4X_VCCIO_LDO_CONF,
+ .vsel_mask = P3H2X4X_TP2367_VCCIO_LDO_VOLTAGE_MASK,
+ },
+};
+
+static int p3h2x4x_regulator_probe(struct platform_device *pdev)
+{
+ struct p3h2x4x_dev *p3h2x4x = dev_get_drvdata(pdev->dev.parent);
+ struct p3h2x4x_regulator_dev *p3h2x4x_regulator;
+ struct regulator_config rcfg = { };
+ struct device *dev = &pdev->dev;
+ struct regulator_dev *rdev;
+ int i;
+
+ p3h2x4x_regulator = devm_kzalloc(dev, sizeof(*p3h2x4x_regulator), GFP_KERNEL);
+ if (!p3h2x4x_regulator)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, p3h2x4x_regulator);
+
+ p3h2x4x_regulator->regmap = p3h2x4x->regmap;
+
+ rcfg.dev = dev->parent;
+ rcfg.regmap = p3h2x4x_regulator->regmap;
+ rcfg.driver_data = p3h2x4x_regulator;
+
+ for (i = 0; i < ARRAY_SIZE(p3h2x4x_regulators); i++) {
+ rdev = devm_regulator_register(&pdev->dev, &p3h2x4x_regulators[i], &rcfg);
+ if (IS_ERR(rdev))
+ return dev_err_probe(dev, PTR_ERR(rdev), "Failed to register %s\n",
+ p3h2x4x_regulators[i].name);
+ p3h2x4x_regulator->rp3h2x4x_dev[i] = rdev;
+ }
+ return 0;
+}
+
+static struct platform_driver p3h2x4x_regulator_driver = {
+ .driver = {
+ .name = "p3h2x4x-regulator",
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+ .probe = p3h2x4x_regulator_probe,
+};
+module_platform_driver(p3h2x4x_regulator_driver);
+
+MODULE_AUTHOR("Aman Kumar Pandey <aman.kumarpandey@nxp.com>");
+MODULE_AUTHOR("Vikash Bansal <vikash.bansal@nxp.com>");
+MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
+MODULE_DESCRIPTION("NXP P3H2X4X I3C HUB Regulator driver");
+MODULE_LICENSE("GPL");
--
2.25.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v11 6/9] regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub
2026-06-12 11:18 ` [PATCH v11 6/9] regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub Lakshay Piplani
@ 2026-06-12 11:37 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-12 11:37 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: conor+dt, linux-i3c, Frank.Li, robh, devicetree
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent hardware operations on different regulators share the same protection lock register without a cross-device shared lock, leading to a race condition where regulator writes fail silently.
- [High] I/O errors during hardware unlock are silently ignored, leading to false success returns for regulator operations.
--
commit 8fb846835511c487dd04df576ca53fdbc089839a
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Subject: regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub
The NXP P3H2x4x family integrates on-die regulators alongside I3C hub
functionality. This driver registers the regulators using the MFD
framework and exposes them via the regulator subsystem.
> diff --git a/drivers/regulator/p3h2840_i3c_hub_regulator.c b/drivers/regulator/p3h2840_i3c_hub_regulator.c
> new file mode 100644
> index 0000000000000..4f2514d4d928e
> --- /dev/null
> +++ b/drivers/regulator/p3h2840_i3c_hub_regulator.c
[ ... ]
> +static void p3h2x4x_reg_guard_enter(struct regulator_dev *rdev,
> + struct p3h2x4x_reg_state *state)
> +{
> + state->restore = false;
> +
> + if (regmap_read(rdev->regmap,
> + P3H2X4X_DEV_REG_PROTECTION_CODE,
> + &state->orig))
> + return;
[Severity: High]
If this regmap_read() fails, the function returns silently since it
has a void return type. Will this cause subsequent regulator operations
like regulator_enable_regmap() to execute blindly on locked hardware,
ignoring the configuration write but returning a false success to the caller?
> +
> + if (state->orig != P3H2X4X_REGISTERS_UNLOCK_CODE) {
> + regmap_write(rdev->regmap,
> + P3H2X4X_DEV_REG_PROTECTION_CODE,
> + P3H2X4X_REGISTERS_UNLOCK_CODE);
> + state->restore = true;
> + }
> +}
> +
> +static void p3h2x4x_reg_guard_exit(struct regulator_dev *rdev,
> + struct p3h2x4x_reg_state *state)
> +{
> + if (state->restore)
> + regmap_write(rdev->regmap,
> + P3H2X4X_DEV_REG_PROTECTION_CODE,
> + state->orig);
> +}
> +
> +DEFINE_LOCK_GUARD_1(p3h2x4x_reg, struct regulator_dev,
> + p3h2x4x_reg_guard_enter(_T->lock, &_T->state),
> + p3h2x4x_reg_guard_exit(_T->lock, &_T->state),
> + struct p3h2x4x_reg_state state);
> +
> +static int p3h2x4x_regulator_enable(struct regulator_dev *rdev)
> +{
> + guard(p3h2x4x_reg)(rdev);
[Severity: High]
Could there be a race condition here between different regulator instances?
The P3H2X4X_DEV_REG_PROTECTION_CODE register appears to be a shared
device-wide resource, but guard(p3h2x4x_reg) relies on rdev->mutex, which
only serializes operations for a single regulator instance.
If two threads access different regulators (e.g., ldo-cp0 and ldo-cp1)
concurrently, could they interleave like this:
Thread A reads state->orig, sees locked, writes unlock, sets restore=true
Thread B reads state->orig, sees unlocked, sets restore=false
Thread A completes its update, runs guard_exit, and relocks the device
Thread B attempts its regulator update, but the hardware is now locked
Would it be safer to use a shared device-level lock in the parent MFD
device to protect the hardware protection register?
> + return regulator_enable_regmap(rdev);
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=6
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v11 7/9] i3c: hub: Add support for the I3C interface in the I3C hub
2026-06-12 11:18 [PATCH v11 0/9] Add support for NXP P3H2x4x I3C hub driver Lakshay Piplani
` (5 preceding siblings ...)
2026-06-12 11:18 ` [PATCH v11 6/9] regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub Lakshay Piplani
@ 2026-06-12 11:18 ` Lakshay Piplani
2026-06-12 11:41 ` sashiko-bot
2026-06-12 12:09 ` Julian Braha
2026-06-12 11:18 ` [PATCH v11 8/9] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality Lakshay Piplani
2026-06-12 11:18 ` [PATCH v11 9/9] i3c: hub: p3h2x4x: Add SMBus slave mode support Lakshay Piplani
8 siblings, 2 replies; 20+ messages in thread
From: Lakshay Piplani @ 2026-06-12 11:18 UTC (permalink / raw)
To: linux-kernel, linux-i3c, alexandre.belloni, krzk+dt, robh,
conor+dt, devicetree, broonie, lee, Frank.Li, lgirdwood
Cc: vikash.bansal, priyanka.jain, aman.kumarpandey, Lakshay Piplani
Add virtual I3C bus support for the hub and provide interface to enable
or disable downstream ports.
Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Signed-off-by: Vikash Bansal <vikash.bansal@nxp.com>
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
Changes in v11:
- Fix virtual hub address reattach handling
- Fix parent bus locking during address reattach operations
- Improve transfer and IBI request error handling
Changes in v10:
- Remove unnecessary ibi_lock handling in request/enable/disable/free
IBI APIs
- Remove redundant parent pointer from struct i3c_hub and derive upstream
master from hub_dev
Changes in v9:
- No change
Changes in v8:
- No change
Changes in v7:
- Convert Kconfig option to tristate
- Fix signedness issue in return value
- Fix kernel-doc warnings
Changes in v6:
- Add support for the generic I3C interface in the I3C Hub
---
---
MAINTAINERS | 2 +
drivers/i3c/Kconfig | 15 ++
drivers/i3c/Makefile | 1 +
drivers/i3c/hub.c | 484 ++++++++++++++++++++++++++++++++++++++++
include/linux/i3c/hub.h | 99 ++++++++
5 files changed, 601 insertions(+)
create mode 100644 drivers/i3c/hub.c
create mode 100644 include/linux/i3c/hub.h
diff --git a/MAINTAINERS b/MAINTAINERS
index fcb3917aa096..a0b7fd85f8e3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19311,8 +19311,10 @@ L: linux-kernel@vger.kernel.org
L: linux-i3c@lists.infradead.org
S: Maintained
F: Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
+F: drivers/i3c/hub.c
F: drivers/mfd/p3h2840.c
F: drivers/regulator/p3h2840_i3c_hub_regulator.c
+F: include/linux/i3c/hub.h
F: include/linux/mfd/p3h2840.h
NXP PF5300/PF5301/PF5302 PMIC REGULATOR DEVICE DRIVER
diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig
index 626c54b386d5..65304b416bb4 100644
--- a/drivers/i3c/Kconfig
+++ b/drivers/i3c/Kconfig
@@ -21,6 +21,21 @@ menuconfig I3C
if I3C
source "drivers/i3c/master/Kconfig"
+
+config I3C_HUB
+ tristate "I3C Hub Support"
+ depends on I3C
+ help
+ Enable support for the I3C interface in hub devices.
+
+ This option adds virtual I3C bus support for hubs by creating
+ virtual master controllers for downstream ports and forwarding
+ bus operations through the hub device. It also provides an
+ interface used by hub drivers to enable or disable downstream
+ ports during bus transactions.
+
+ Say Y here if your platform includes an I3C hub device
+
endif # I3C
config I3C_OR_I2C
diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile
index 11982efbc6d9..9ddee56a6338 100644
--- a/drivers/i3c/Makefile
+++ b/drivers/i3c/Makefile
@@ -2,3 +2,4 @@
i3c-y := device.o master.o
obj-$(CONFIG_I3C) += i3c.o
obj-$(CONFIG_I3C) += master/
+obj-$(CONFIG_I3C_HUB) += hub.o
diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c
new file mode 100644
index 000000000000..c37b01fddcb4
--- /dev/null
+++ b/drivers/i3c/hub.c
@@ -0,0 +1,484 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2026 NXP
+ * Generic I3C Hub core implementing virtual controller operations.
+ */
+#include <linux/i3c/device.h>
+#include <linux/i3c/hub.h>
+
+#include "internals.h"
+
+/**
+ * i3c_hub_master_bus_init() - Bind controller to hub device
+ * @controller: Virtual controller for a hub port
+ *
+ * Associates the virtual controller with the hub device descriptor so that
+ * transfers are executed through the hub on the parent bus.
+ */
+static int i3c_hub_master_bus_init(struct i3c_master_controller *controller)
+{
+ struct i3c_hub_controller *hub_controller;
+ struct i3c_hub *hub;
+
+ hub_controller = dev_get_drvdata(&controller->dev);
+ if (!hub_controller || !hub_controller->hub)
+ return -ENODEV;
+
+ hub = hub_controller->hub;
+
+ if (!hub->hub_dev)
+ return -ENODEV;
+
+ controller->this = hub->hub_dev->desc;
+ return 0;
+}
+
+static void i3c_hub_master_bus_cleanup(struct i3c_master_controller *controller)
+{
+ controller->this = NULL;
+}
+
+static int i3c_hub_attach_i3c_dev(struct i3c_dev_desc *dev)
+{
+ return 0;
+}
+
+static int i3c_hub_reattach_i3c_dev(struct i3c_dev_desc *dev, u8 old_dyn_addr)
+{
+ return 0;
+}
+
+static void i3c_hub_detach_i3c_dev(struct i3c_dev_desc *dev)
+{
+}
+
+/**
+ * i3c_hub_do_daa() - Perform DAA via hub port
+ * @hub: Hub instance
+ * @controller: Virtual controller for a hub port
+ *
+ * Enables the port connection, performs DAA on the parent controller,
+ * then disables the connection.
+ */
+static int i3c_hub_do_daa(struct i3c_hub *hub,
+ struct i3c_master_controller *controller)
+{
+ struct i3c_master_controller *parent;
+ int ret;
+
+ if (!hub || !hub->hub_dev)
+ return -ENODEV;
+
+ parent = i3c_dev_get_master(hub->hub_dev->desc);
+ if (!parent)
+ return -ENODEV;
+
+ i3c_hub_enable_port(controller);
+ ret = i3c_master_do_daa(parent);
+ i3c_hub_disable_port(controller);
+
+ return ret;
+}
+
+static bool i3c_hub_supports_ccc_cmd(struct i3c_hub *hub,
+ const struct i3c_ccc_cmd *cmd)
+{
+ struct i3c_master_controller *parent;
+
+ if (!hub || !hub->hub_dev)
+ return false;
+
+ parent = i3c_dev_get_master(hub->hub_dev->desc);
+ if (!parent)
+ return false;
+
+ return i3c_master_supports_ccc_cmd(parent, cmd);
+}
+
+/**
+ * i3c_hub_send_ccc_cmd() - Send CCC through hub port
+ * @hub: Hub instance
+ * @controller: Virtual controller
+ * @cmd: CCC command
+ *
+ * Enables the port connection while issuing CCC on the parent controller.
+ */
+static int i3c_hub_send_ccc_cmd(struct i3c_hub *hub,
+ struct i3c_master_controller *controller,
+ struct i3c_ccc_cmd *cmd)
+{
+ struct i3c_master_controller *parent;
+ int ret;
+
+ if (!hub || !hub->hub_dev)
+ return -ENODEV;
+
+ parent = i3c_dev_get_master(hub->hub_dev->desc);
+ if (!parent)
+ return -ENODEV;
+
+ i3c_hub_enable_port(controller);
+ ret = i3c_master_send_ccc_cmd(parent, cmd);
+ i3c_hub_disable_port(controller);
+
+ return ret;
+}
+
+/**
+ * i3c_hub_master_priv_xfers() - Execute private transfers via hub
+ * @dev: Target device descriptor
+ * @xfers: Transfer array
+ * @nxfers: Number of transfers
+ * @mode: transfer mode (SDR, HDR, etc.)
+ *
+ * Handles address adjustment and forwards private transfers through the hub
+ * device.
+ */
+static int i3c_hub_master_priv_xfers(struct i3c_dev_desc *dev,
+ struct i3c_xfer *xfers,
+ int nxfers,
+ enum i3c_xfer_mode mode)
+{
+ struct i3c_master_controller *controller = i3c_dev_get_master(dev);
+ struct i3c_hub_controller *hub_controller;
+ struct i3c_dev_desc *hub_dev;
+ struct i3c_master_controller *parent;
+ u8 hub_addr, target_addr;
+ struct i3c_hub *hub;
+ int ret, xfer_ret;
+
+ hub_controller = dev_get_drvdata(&controller->dev);
+ if (!hub_controller || !hub_controller->hub)
+ return -ENODEV;
+
+ hub = hub_controller->hub;
+
+ if (!hub->hub_dev)
+ return -ENODEV;
+
+ hub_dev = hub->hub_dev->desc;
+
+ parent = i3c_dev_get_master(hub_dev);
+ if (!parent)
+ return -ENODEV;
+
+ i3c_hub_enable_port(controller);
+
+ hub_addr = hub_dev->info.dyn_addr ?
+ hub_dev->info.dyn_addr : hub_dev->info.static_addr;
+
+ target_addr = dev->info.dyn_addr ?
+ dev->info.dyn_addr : dev->info.static_addr;
+
+ if (hub_addr != target_addr) {
+ down_write(&parent->bus.lock);
+ hub_dev->info.dyn_addr = target_addr;
+
+ ret = i3c_master_reattach_i3c_dev_locked(hub_dev, hub_addr);
+ up_write(&parent->bus.lock);
+ if (ret) {
+ hub_dev->info.dyn_addr = hub_addr;
+ goto disable;
+ }
+ }
+
+ ret = i3c_device_do_xfers(hub->hub_dev, xfers, nxfers, mode);
+
+ if (hub_addr != target_addr) {
+ down_write(&parent->bus.lock);
+ hub_dev->info.dyn_addr = hub_addr;
+
+ xfer_ret = i3c_master_reattach_i3c_dev_locked(hub_dev,
+ target_addr);
+
+ up_write(&parent->bus.lock);
+ if (!ret)
+ ret = xfer_ret;
+ }
+disable:
+ i3c_hub_disable_port(controller);
+ return ret;
+}
+
+static int i3c_hub_attach_i2c_dev(struct i2c_dev_desc *dev)
+{
+ return 0;
+}
+
+static void i3c_hub_detach_i2c_dev(struct i2c_dev_desc *dev)
+{
+}
+
+static int i3c_hub_i2c_xfers(struct i2c_dev_desc *dev,
+ struct i2c_msg *xfers, int nxfers)
+{
+ return 0;
+}
+
+static int i3c_hub_master_do_daa(struct i3c_master_controller *controller)
+{
+ struct i3c_hub_controller *hub_controller;
+ struct i3c_hub *hub;
+
+ hub_controller = dev_get_drvdata(&controller->dev);
+ if (!hub_controller || !hub_controller->hub)
+ return -ENODEV;
+
+ hub = hub_controller->hub;
+
+ return i3c_hub_do_daa(hub, controller);
+}
+
+static int i3c_hub_master_send_ccc_cmd(struct i3c_master_controller *controller,
+ struct i3c_ccc_cmd *cmd)
+{
+ struct i3c_hub_controller *hub_controller;
+ struct i3c_hub *hub;
+
+ hub_controller = dev_get_drvdata(&controller->dev);
+ if (!hub_controller || !hub_controller->hub)
+ return -ENODEV;
+
+ hub = hub_controller->hub;
+
+ if (!hub->hub_dev)
+ return -ENODEV;
+
+ if (cmd->id == I3C_CCC_RSTDAA(true))
+ return 0;
+
+ return i3c_hub_send_ccc_cmd(hub, controller, cmd);
+}
+
+static bool i3c_hub_master_supports_ccc_cmd(struct i3c_master_controller *controller,
+ const struct i3c_ccc_cmd *cmd)
+{
+ struct i3c_hub_controller *hub_controller;
+ struct i3c_hub *hub;
+
+ hub_controller = dev_get_drvdata(&controller->dev);
+ if (!hub_controller || !hub_controller->hub)
+ return false;
+
+ hub = hub_controller->hub;
+
+ return i3c_hub_supports_ccc_cmd(hub, cmd);
+}
+
+/**
+ * i3c_hub_request_ibi() - Request IBI through parent controller
+ * @desc: Target device descriptor
+ * @req: IBI setup
+ *
+ * Temporarily updates parent controller context to request IBI for a device
+ * connected through the hub.
+ */
+static int i3c_hub_request_ibi(struct i3c_dev_desc *desc,
+ const struct i3c_ibi_setup *req)
+{
+ struct i3c_master_controller *controller = i3c_dev_get_master(desc);
+ struct i3c_hub_controller *hub_controller;
+ struct i3c_master_controller *orig_parent;
+ struct i3c_master_controller *parent;
+ struct i3c_hub *hub;
+ int ret;
+
+ hub_controller = dev_get_drvdata(&controller->dev);
+ if (!hub_controller || !hub_controller->hub)
+ return -ENODEV;
+
+ hub = hub_controller->hub;
+
+ if (!hub->hub_dev)
+ return -ENODEV;
+
+ parent = i3c_dev_get_master(hub->hub_dev->desc);
+
+ down_write(&parent->bus.lock);
+
+ orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
+
+ ret = i3c_master_direct_attach_i3c_dev_locked(parent, desc);
+ if (ret) {
+ i3c_hub_update_desc_parent(&desc->common, orig_parent);
+ up_write(&parent->bus.lock);
+ return ret;
+ }
+
+ kfree(desc->ibi);
+ desc->ibi = NULL;
+ ret = i3c_dev_request_ibi_locked(desc, req);
+ if (ret)
+ i3c_master_direct_detach_i3c_dev_locked(desc);
+
+ i3c_hub_update_desc_parent(&desc->common, orig_parent);
+
+ up_write(&parent->bus.lock);
+
+ return ret;
+}
+
+static void i3c_hub_free_ibi(struct i3c_dev_desc *desc)
+{
+ struct i3c_master_controller *controller = i3c_dev_get_master(desc);
+ struct i3c_hub_controller *hub_controller;
+ struct i3c_master_controller *orig_parent;
+ struct i3c_master_controller *parent;
+ struct i3c_hub *hub;
+
+ hub_controller = dev_get_drvdata(&controller->dev);
+ if (!hub_controller || !hub_controller->hub)
+ return;
+
+ hub = hub_controller->hub;
+
+ parent = i3c_dev_get_master(hub->hub_dev->desc);
+
+ i3c_hub_enable_port(controller);
+
+ down_write(&parent->bus.lock);
+ orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
+ i3c_master_direct_detach_i3c_dev_locked(desc);
+ i3c_dev_free_ibi_locked(desc);
+ i3c_hub_update_desc_parent(&desc->common, orig_parent);
+ up_write(&parent->bus.lock);
+
+ i3c_hub_disable_port(controller);
+}
+
+/**
+ * i3c_hub_enable_ibi() - Enable IBI via hub port
+ * @desc: Target device descriptor
+ *
+ * Enables port connection and forwards the IBI enable request to the parent
+ * controller.
+ */
+static int i3c_hub_enable_ibi(struct i3c_dev_desc *desc)
+{
+ struct i3c_master_controller *controller = i3c_dev_get_master(desc);
+ struct i3c_hub_controller *hub_controller;
+ struct i3c_master_controller *orig_parent;
+ struct i3c_master_controller *parent;
+ struct i3c_hub *hub;
+ int ret;
+
+ hub_controller = dev_get_drvdata(&controller->dev);
+ if (!hub_controller || !hub_controller->hub)
+ return -ENODEV;
+
+ hub = hub_controller->hub;
+
+ if (!hub->hub_dev)
+ return -ENODEV;
+
+ parent = i3c_dev_get_master(hub->hub_dev->desc);
+
+ i3c_hub_enable_port(controller);
+
+ orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
+
+ down_write(&parent->bus.lock);
+ ret = i3c_dev_enable_ibi_locked(desc);
+ up_write(&parent->bus.lock);
+
+ i3c_hub_update_desc_parent(&desc->common, orig_parent);
+
+ i3c_hub_disable_port(controller);
+
+ return ret;
+}
+
+/**
+ * i3c_hub_disable_ibi() - Disable IBI via hub port
+ * @desc: Target device descriptor
+ *
+ * Enables port connection and forwards the IBI disable request to the parent
+ * controller.
+ */
+static int i3c_hub_disable_ibi(struct i3c_dev_desc *desc)
+{
+ struct i3c_master_controller *controller = i3c_dev_get_master(desc);
+ struct i3c_hub_controller *hub_controller;
+ struct i3c_master_controller *orig_parent;
+ struct i3c_master_controller *parent;
+ struct i3c_hub *hub;
+ int ret;
+
+ hub_controller = dev_get_drvdata(&controller->dev);
+ if (!hub_controller || !hub_controller->hub)
+ return -ENODEV;
+
+ hub = hub_controller->hub;
+
+ if (!hub->hub_dev)
+ return -ENODEV;
+
+ parent = i3c_dev_get_master(hub->hub_dev->desc);
+
+ i3c_hub_enable_port(controller);
+
+ orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
+
+ down_write(&parent->bus.lock);
+ ret = i3c_dev_disable_ibi_locked(desc);
+ up_write(&parent->bus.lock);
+
+ i3c_hub_update_desc_parent(&desc->common, orig_parent);
+
+ i3c_hub_disable_port(controller);
+
+ return ret;
+}
+
+static void i3c_hub_recycle_ibi_slot(struct i3c_dev_desc *desc,
+ struct i3c_ibi_slot *slot)
+{
+}
+
+static const struct i3c_master_controller_ops i3c_hub_master_ops_data = {
+ .bus_init = i3c_hub_master_bus_init,
+ .bus_cleanup = i3c_hub_master_bus_cleanup,
+ .attach_i3c_dev = i3c_hub_attach_i3c_dev,
+ .reattach_i3c_dev = i3c_hub_reattach_i3c_dev,
+ .detach_i3c_dev = i3c_hub_detach_i3c_dev,
+ .do_daa = i3c_hub_master_do_daa,
+ .supports_ccc_cmd = i3c_hub_master_supports_ccc_cmd,
+ .send_ccc_cmd = i3c_hub_master_send_ccc_cmd,
+ .i3c_xfers = i3c_hub_master_priv_xfers,
+ .attach_i2c_dev = i3c_hub_attach_i2c_dev,
+ .detach_i2c_dev = i3c_hub_detach_i2c_dev,
+ .i2c_xfers = i3c_hub_i2c_xfers,
+ .request_ibi = i3c_hub_request_ibi,
+ .free_ibi = i3c_hub_free_ibi,
+ .enable_ibi = i3c_hub_enable_ibi,
+ .disable_ibi = i3c_hub_disable_ibi,
+ .recycle_ibi_slot = i3c_hub_recycle_ibi_slot,
+};
+
+/**
+ * i3c_hub_init() - Initialize hub context
+ * @hub: Hub instance
+ * @parent: Parent I3C master controller
+ * @ops: Vendor callbacks
+ * @hub_dev: I3C hub device
+ */
+void i3c_hub_init(struct i3c_hub *hub,
+ const struct i3c_hub_ops *ops,
+ struct i3c_device *hub_dev)
+{
+ hub->ops = ops;
+ hub->hub_dev = hub_dev;
+}
+EXPORT_SYMBOL_GPL(i3c_hub_init);
+
+const struct i3c_master_controller_ops *i3c_hub_master_ops(void)
+{
+ return &i3c_hub_master_ops_data;
+}
+EXPORT_SYMBOL_GPL(i3c_hub_master_ops);
+
+MODULE_AUTHOR("Aman Kumar Pandey <aman.kumarpandey@nxp.com>");
+MODULE_AUTHOR("Vikash Bansal <vikash.bansal@nxp.com>");
+MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
+MODULE_DESCRIPTION("Generic I3C hub support");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/i3c/hub.h b/include/linux/i3c/hub.h
new file mode 100644
index 000000000000..8b5c162067c3
--- /dev/null
+++ b/include/linux/i3c/hub.h
@@ -0,0 +1,99 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2026 NXP
+ * Generic hub definitions and helper interfaces.
+ */
+#ifndef _LINUX_I3C_HUB_H
+#define _LINUX_I3C_HUB_H
+
+#include <linux/i3c/master.h>
+
+static inline struct i3c_master_controller *
+i3c_hub_update_desc_parent(struct i3c_i2c_dev_desc *desc,
+ struct i3c_master_controller *parent)
+{
+ struct i3c_master_controller *orig_parent = desc->master;
+
+ desc->master = parent;
+ return orig_parent;
+}
+
+/**
+ * struct i3c_hub - Generic I3C hub context
+ * @parent: Parent I3C master controller
+ * @ops: Vendor callbacks for port connection control
+ * @hub_dev: I3C device representing the hub on the parent bus
+ */
+struct i3c_hub {
+ const struct i3c_hub_ops *ops;
+ struct i3c_device *hub_dev;
+};
+
+struct i3c_hub_controller {
+ struct i3c_master_controller *parent;
+ struct i3c_master_controller controller;
+ struct i3c_hub *hub;
+};
+
+struct i3c_hub_ops {
+ void (*enable_port)(struct i3c_master_controller *controller);
+ void (*disable_port)(struct i3c_master_controller *controller);
+};
+
+/**
+ * i3c_hub_enable_port() - Enable hub connection for a controller
+ * @controller: Virtual controller representing a hub port
+ *
+ * Retrieves hub context from controller drvdata and invokes the vendor
+ * callback to enable the associated port connection.
+ */
+static inline void i3c_hub_enable_port(struct i3c_master_controller *controller)
+{
+ struct i3c_hub_controller *hub_controller;
+ struct i3c_hub *hub;
+
+ hub_controller = dev_get_drvdata(&controller->dev);
+ if (!hub_controller || !hub_controller->hub)
+ return;
+
+ hub = hub_controller->hub;
+
+ if (hub && hub->ops && hub->ops->enable_port)
+ hub->ops->enable_port(controller);
+}
+
+/**
+ * i3c_hub_disable_port() - Disable hub connection for a controller
+ * @controller: Virtual controller representing a hub port
+ *
+ * Retrieves hub context from controller drvdata and invokes the vendor
+ * callback to disable the associated port connection.
+ */
+static inline void i3c_hub_disable_port(struct i3c_master_controller *controller)
+{
+ struct i3c_hub_controller *hub_controller;
+ struct i3c_hub *hub;
+
+ hub_controller = dev_get_drvdata(&controller->dev);
+ if (!hub_controller || !hub_controller->hub)
+ return;
+
+ hub = hub_controller->hub;
+
+ if (hub && hub->ops && hub->ops->disable_port)
+ hub->ops->disable_port(controller);
+}
+
+/**
+ * i3c_hub_master_ops() - Return virtual controller ops for hub ports
+ *
+ * Provides i3c_master_controller_ops used by controllers created for hub
+ * ports.
+ */
+const struct i3c_master_controller_ops *i3c_hub_master_ops(void);
+
+void i3c_hub_init(struct i3c_hub *hub,
+ const struct i3c_hub_ops *ops,
+ struct i3c_device *hub_dev);
+
+#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v11 7/9] i3c: hub: Add support for the I3C interface in the I3C hub
2026-06-12 11:18 ` [PATCH v11 7/9] i3c: hub: Add support for the I3C interface in the I3C hub Lakshay Piplani
@ 2026-06-12 11:41 ` sashiko-bot
2026-06-12 12:09 ` Julian Braha
1 sibling, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-12 11:41 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: devicetree, robh, conor+dt, linux-i3c, Frank.Li
Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] Concurrent private transfers through the hub race on `hub_dev->info.dyn_addr`, causing data corruption.
- [Critical] Delegating DAA to the parent controller incorrectly attaches downstream devices to the parent bus.
- [Critical] Unsynchronized hub port enablement breaks virtual bus isolation, causing bus collisions.
- [Critical] `i3c_hub_master_priv_xfers` corrupts the parent bus address slots by freeing slots that belong to unrelated physical devices.
- [High] Double free and memory leak of the IBI workqueue in `i3c_hub_request_ibi` error path.
- [High] Deterministic NULL pointer dereference in `i3c_dev_free_ibi_locked` after hub recursive call.
- [High] Unlocked write to `hub_dev->info.dyn_addr` introduces a data race on the error path.
- [High] Failure in `priv_xfers` address reattachment permanently orphans the hub device.
- [High] IBI support for downstream hub devices is broken because devices are never added to the physical parent's bus list.
- [High] Lockdep splat due to nested acquisition of `bus.lock` without subclass annotations.
- [Medium] I2C transfers silently succeed without performing any communication, masking failures.
--
commit f3f90aaf2c0040d96007fed285ccd49e4c6e8e7a
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
i3c: hub: Add support for the I3C interface in the I3C hub
Add virtual I3C bus support for the hub and provide interface to enable
or disable downstream ports.
> diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c
> new file mode 100644
> index 0000000000000..c37b01fddcb4c
> --- /dev/null
> +++ b/drivers/i3c/hub.c
[ ... ]
> +static int i3c_hub_do_daa(struct i3c_hub *hub,
> + struct i3c_master_controller *controller)
> +{
> + struct i3c_master_controller *parent;
> + int ret;
> +
> + if (!hub || !hub->hub_dev)
> + return -ENODEV;
> +
> + parent = i3c_dev_get_master(hub->hub_dev->desc);
> + if (!parent)
> + return -ENODEV;
> +
> + i3c_hub_enable_port(controller);
[Severity: Critical]
Does enabling the port outside of the parent controller's bus lock break
virtual bus isolation? If multiple virtual ports execute global broadcasts
concurrently, they might both enable their respective hardware ports before
blocking on parent->bus.lock. This could cause the parent to execute the
broadcast across all enabled ports simultaneously, potentially resulting in
severe bus collisions.
> + ret = i3c_master_do_daa(parent);
[Severity: Critical]
Could delegating the Dynamic Address Assignment procedure to the physical
parent controller incorrectly attach downstream devices to the parent bus?
Since i3c_master_do_daa() natively creates and attaches devices to the
physical parent bus, any transfers targeting these devices would bypass the
hub's virtual controller callbacks. As a result, i3c_hub_enable_port()
would never be invoked prior to communication, potentially causing I3C
transfers to the downstream devices to fail.
> + i3c_hub_disable_port(controller);
> +
> + return ret;
> +}
[ ... ]
> +static int i3c_hub_send_ccc_cmd(struct i3c_hub *hub,
> + struct i3c_master_controller *controller,
> + struct i3c_ccc_cmd *cmd)
> +{
> + struct i3c_master_controller *parent;
> + int ret;
> +
> + if (!hub || !hub->hub_dev)
> + return -ENODEV;
> +
> + parent = i3c_dev_get_master(hub->hub_dev->desc);
> + if (!parent)
> + return -ENODEV;
> +
> + i3c_hub_enable_port(controller);
[Severity: Critical]
Is it possible for concurrent CCC broadcasts to cause bus collisions here?
Similar to i3c_hub_do_daa(), enabling the port outside of parent->bus.lock
could allow multiple ports to be enabled simultaneously if they execute
broadcasts concurrently.
> + ret = i3c_master_send_ccc_cmd(parent, cmd);
> + i3c_hub_disable_port(controller);
> +
> + return ret;
> +}
[ ... ]
> +static int i3c_hub_master_priv_xfers(struct i3c_dev_desc *dev,
> + struct i3c_xfer *xfers,
> + int nxfers,
> + enum i3c_xfer_mode mode)
> +{
> + struct i3c_master_controller *controller = i3c_dev_get_master(dev);
> + struct i3c_hub_controller *hub_controller;
> + struct i3c_dev_desc *hub_dev;
> + struct i3c_master_controller *parent;
> + u8 hub_addr, target_addr;
> + struct i3c_hub *hub;
> + int ret, xfer_ret;
[ ... ]
> + if (hub_addr != target_addr) {
> + down_write(&parent->bus.lock);
[Severity: High]
Does acquiring this lock cause a lockdep splat?
This function is invoked from contexts where the core framework has already
locked the virtual downstream controller's bus.lock (e.g., via
i3c_bus_normaluse_lock() which takes a down_read). Acquiring a second lock
of the same class without using down_write_nested() might violate lock
nesting rules.
> + hub_dev->info.dyn_addr = target_addr;
> +
> + ret = i3c_master_reattach_i3c_dev_locked(hub_dev, hub_addr);
> + up_write(&parent->bus.lock);
> + if (ret) {
> + hub_dev->info.dyn_addr = hub_addr;
[Severity: High]
Is it safe to modify hub_dev->info.dyn_addr outside of the bus write lock?
Modifying this shared bus state variable after up_write(&parent->bus.lock)
appears to introduce a data race with concurrent bus operations.
[Severity: High]
If i3c_master_reattach_i3c_dev_locked() fails, are we permanently orphaning
the hub device?
When it fails, the core code automatically marks both the new and old address
slots as I3C_ADDR_SLOT_FREE via i3c_master_put_i3c_addrs(). Because this
error path restores dyn_addr but fails to reinstate the address allocation
on the parent bus, the parent controller might re-assign the hub's legitimate
address to new hot-joined devices.
> + goto disable;
> + }
> + }
> +
> + ret = i3c_device_do_xfers(hub->hub_dev, xfers, nxfers, mode);
[Severity: Critical]
Could concurrent private transfers through the hub race on
hub_dev->info.dyn_addr?
This function drops the write lock before calling i3c_device_do_xfers(),
which only acquires the lock in read mode. If multiple threads concurrently
initiate transfers to different downstream ports, they might take the read
lock simultaneously and race to overwrite hub_dev->info.dyn_addr, potentially
causing transfers to be misrouted.
> +
> + if (hub_addr != target_addr) {
> + down_write(&parent->bus.lock);
> + hub_dev->info.dyn_addr = hub_addr;
> +
> + xfer_ret = i3c_master_reattach_i3c_dev_locked(hub_dev,
> + target_addr);
[Severity: Critical]
Does passing target_addr as the old dynamic address corrupt the parent bus
address slots?
When the core function interprets target_addr as the old dynamic address of
the device on the parent bus:
drivers/i3c/master.c:i3c_master_reattach_i3c_dev_locked() {
...
if (old_dyn_addr)
i3c_bus_set_addr_slot_status(&master->bus, old_dyn_addr,
I3C_ADDR_SLOT_FREE);
...
}
Because target_addr is a downstream address independently assigned by the
virtual controller, this might blindly free an in-use address slot on the
parent bus that belongs to an unrelated physical device.
> +
> + up_write(&parent->bus.lock);
> + if (!ret)
> + ret = xfer_ret;
> + }
> +disable:
> + i3c_hub_disable_port(controller);
> + return ret;
> +}
[ ... ]
> +static int i3c_hub_i2c_xfers(struct i2c_dev_desc *dev,
> + struct i2c_msg *xfers, int nxfers)
> +{
> + return 0;
> +}
[Severity: Medium]
Should this callback attempt to forward the I2C transfers through the hub to
the parent bus, or return an error like -EOPNOTSUPP? Unconditionally
returning 0 might lead the kernel and userspace applications to falsely
assume that I2C transactions to downstream devices were successful when no
actual hardware communication occurred.
[ ... ]
> +static int i3c_hub_request_ibi(struct i3c_dev_desc *desc,
> + const struct i3c_ibi_setup *req)
> +{
> + struct i3c_master_controller *controller = i3c_dev_get_master(desc);
> + struct i3c_hub_controller *hub_controller;
> + struct i3c_master_controller *orig_parent;
> + struct i3c_master_controller *parent;
> + struct i3c_hub *hub;
> + int ret;
[ ... ]
> + parent = i3c_dev_get_master(hub->hub_dev->desc);
> +
> + down_write(&parent->bus.lock);
[Severity: High]
Does acquiring parent->bus.lock here cause a lockdep splat? As with
i3c_hub_master_priv_xfers(), this might acquire a second lock of the
same class without using down_write_nested().
> +
> + orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> +
> + ret = i3c_master_direct_attach_i3c_dev_locked(parent, desc);
[Severity: High]
Are downstream devices missing from the physical parent's bus list?
i3c_master_direct_attach_i3c_dev_locked() explicitly skips adding the device
to the parent's bus.devs.i3c list. When the hardware subsequently receives an
IBI, the core framework (i3c_master_handle_ibi) might look up the source
device by matching the IBI address against its bus.devs.i3c list. Since the
downstream device is missing from the list, the lookup would predictably
fail, causing IBIs to be ignored.
> + if (ret) {
> + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> + up_write(&parent->bus.lock);
> + return ret;
> + }
> +
> + kfree(desc->ibi);
> + desc->ibi = NULL;
[Severity: High]
Are we leaking the IBI workqueue here?
When i3c_dev_request_ibi_locked() handles an IBI request, it allocates a
new ibi structure and a workqueue (ibi->wq). Freeing desc->ibi without
destroying ibi->wq seems to leak the workqueue.
Furthermore, if the nested i3c_dev_request_ibi_locked() call fails, this
hub callback will return an error to the outer caller. The outer caller's
error handling path might then unconditionally execute kfree(ibi) using its
original local pointer:
drivers/i3c/master.c:i3c_dev_request_ibi_locked() {
...
ret = master->ops->request_ibi(dev, req);
if (ret) {
kfree(ibi);
dev->ibi = NULL;
}
...
}
Since the hub callback already freed it, wouldn't this result in a
double-free?
> + ret = i3c_dev_request_ibi_locked(desc, req);
> + if (ret)
> + i3c_master_direct_detach_i3c_dev_locked(desc);
> +
> + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> +
> + up_write(&parent->bus.lock);
> +
> + return ret;
> +}
> +
> +static void i3c_hub_free_ibi(struct i3c_dev_desc *desc)
> +{
> + struct i3c_master_controller *controller = i3c_dev_get_master(desc);
> + struct i3c_hub_controller *hub_controller;
> + struct i3c_master_controller *orig_parent;
> + struct i3c_master_controller *parent;
> + struct i3c_hub *hub;
[ ... ]
> + down_write(&parent->bus.lock);
> + orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> + i3c_master_direct_detach_i3c_dev_locked(desc);
> + i3c_dev_free_ibi_locked(desc);
[Severity: High]
Does this nested call to i3c_dev_free_ibi_locked() cause a NULL pointer
dereference upon returning to the outer caller?
The inner call tears down the IBI state and sets desc->ibi = NULL.
When control returns to the outer i3c_dev_free_ibi_locked():
drivers/i3c/master.c:i3c_dev_free_ibi_locked() {
...
master->ops->free_ibi(dev);
if (dev->ibi->wq) {
...
}
It seems to unconditionally evaluate dev->ibi->wq immediately after the
callback returns, which would blindly dereference the newly nullified
pointer.
> + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> + up_write(&parent->bus.lock);
> +
> + i3c_hub_disable_port(controller);
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=7
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v11 7/9] i3c: hub: Add support for the I3C interface in the I3C hub
2026-06-12 11:18 ` [PATCH v11 7/9] i3c: hub: Add support for the I3C interface in the I3C hub Lakshay Piplani
2026-06-12 11:41 ` sashiko-bot
@ 2026-06-12 12:09 ` Julian Braha
1 sibling, 0 replies; 20+ messages in thread
From: Julian Braha @ 2026-06-12 12:09 UTC (permalink / raw)
To: Lakshay Piplani, linux-kernel, linux-i3c, alexandre.belloni,
krzk+dt, robh, conor+dt, devicetree, broonie, lee, Frank.Li,
lgirdwood
Cc: vikash.bansal, priyanka.jain, aman.kumarpandey
On 6/12/26 12:18, Lakshay Piplani wrote:
> if I3C
> source "drivers/i3c/master/Kconfig"
> +
> +config I3C_HUB
> + tristate "I3C Hub Support"
> + depends on I3C
> + help
> + Enable support for the I3C interface in hub devices.
> +
> + This option adds virtual I3C bus support for hubs by creating
> + virtual master controllers for downstream ports and forwarding
> + bus operations through the hub device. It also provides an
> + interface used by hub drivers to enable or disable downstream
> + ports during bus transactions.
> +
> + Say Y here if your platform includes an I3C hub device
> +
> endif # I3C
Hi Lakshay,
The 'depends on I3C' here is unnecessary because that's what the
'if I3C..endif' is already doing.
- Julian Braha
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v11 8/9] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality
2026-06-12 11:18 [PATCH v11 0/9] Add support for NXP P3H2x4x I3C hub driver Lakshay Piplani
` (6 preceding siblings ...)
2026-06-12 11:18 ` [PATCH v11 7/9] i3c: hub: Add support for the I3C interface in the I3C hub Lakshay Piplani
@ 2026-06-12 11:18 ` Lakshay Piplani
2026-06-12 11:39 ` sashiko-bot
2026-06-12 11:18 ` [PATCH v11 9/9] i3c: hub: p3h2x4x: Add SMBus slave mode support Lakshay Piplani
8 siblings, 1 reply; 20+ messages in thread
From: Lakshay Piplani @ 2026-06-12 11:18 UTC (permalink / raw)
To: linux-kernel, linux-i3c, alexandre.belloni, krzk+dt, robh,
conor+dt, devicetree, broonie, lee, Frank.Li, lgirdwood
Cc: vikash.bansal, priyanka.jain, aman.kumarpandey, Lakshay Piplani
From: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Add I3C hub functionality for the NXP P3H2x4x family of multiport hubs.
These devices support downstream target ports that can be configured
as I3C, I2C, or SMBus.
This driver enables:
- I3C/I2C communication between host and hub
- Transparent communication with downstream devices
- Target port configuration (I3C/I2C/SMBus)
P3H2440/P3H2441 support 4 target ports.
P3H2840/P3H2841 support 8 target ports.
Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Signed-off-by: Vikash Bansal <vikash.bansal@nxp.com>
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
Changes in v11:
- Fix IBI resource cleanup on error paths
- Fix adapter unregister cleanup handling
Changes in v10:
- Split SMBus target/slave mode support, including IBI and MCTP receive
handling, into a separate patch
Changes in v9:
- Added CONFIG_I2C_SLAVE guards where necessary to avoid build issues
when I2C slave support is disabled.
Changes in v8:
- No change
Changes in v7:
- Remove CONFIG_I2C_SLAVE guards
- Use Kernel API find_closest instead of custom helper
- Use devm_regulator_get_enable_optional()
- Fix kernel-doc warnings
Changes in v6:
- Remove generic I3C code and keep reg dependent code only.
Changes in v5:
- Updated supply names.
Changes in v4:
- Split the driver into three separate patches (mfd, regulator and I3C hub)
- Added support for NXP P3H2x4x I3C hub functionality
- Integrated hub driver with its on-die regulator
Changes in v3:
- Added MFD (Multi-Function Device) support for I3C hub and on-die regulator
Changes in v2:
- Refined coding style and incorporated review feedback
- Updated directory structure
- Revised logic for parsing DTS nodes
---
---
MAINTAINERS | 1 +
drivers/i3c/Kconfig | 1 +
drivers/i3c/Makefile | 1 +
drivers/i3c/hub/Kconfig | 11 +
drivers/i3c/hub/Makefile | 4 +
drivers/i3c/hub/p3h2840_i3c_hub.h | 327 +++++++++++++++++++++
drivers/i3c/hub/p3h2840_i3c_hub_common.c | 352 +++++++++++++++++++++++
drivers/i3c/hub/p3h2840_i3c_hub_i3c.c | 124 ++++++++
drivers/i3c/hub/p3h2840_i3c_hub_smbus.c | 289 +++++++++++++++++++
9 files changed, 1110 insertions(+)
create mode 100644 drivers/i3c/hub/Kconfig
create mode 100644 drivers/i3c/hub/Makefile
create mode 100644 drivers/i3c/hub/p3h2840_i3c_hub.h
create mode 100644 drivers/i3c/hub/p3h2840_i3c_hub_common.c
create mode 100644 drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
create mode 100644 drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
diff --git a/MAINTAINERS b/MAINTAINERS
index a0b7fd85f8e3..718c5fca4746 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19312,6 +19312,7 @@ L: linux-i3c@lists.infradead.org
S: Maintained
F: Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
F: drivers/i3c/hub.c
+F: drivers/i3c/hub/*
F: drivers/mfd/p3h2840.c
F: drivers/regulator/p3h2840_i3c_hub_regulator.c
F: include/linux/i3c/hub.h
diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig
index 65304b416bb4..74727d614492 100644
--- a/drivers/i3c/Kconfig
+++ b/drivers/i3c/Kconfig
@@ -36,6 +36,7 @@ config I3C_HUB
Say Y here if your platform includes an I3C hub device
+source "drivers/i3c/hub/Kconfig"
endif # I3C
config I3C_OR_I2C
diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile
index 9ddee56a6338..2950820db9ea 100644
--- a/drivers/i3c/Makefile
+++ b/drivers/i3c/Makefile
@@ -3,3 +3,4 @@ i3c-y := device.o master.o
obj-$(CONFIG_I3C) += i3c.o
obj-$(CONFIG_I3C) += master/
obj-$(CONFIG_I3C_HUB) += hub.o
+obj-$(CONFIG_I3C_HUB) += hub/
diff --git a/drivers/i3c/hub/Kconfig b/drivers/i3c/hub/Kconfig
new file mode 100644
index 000000000000..f725f3e2bfbe
--- /dev/null
+++ b/drivers/i3c/hub/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2025 NXP
+config P3H2X4X_I3C_HUB
+ tristate "NXP P3H2X4X I3C HUB support"
+ depends on MFD_P3H2X4X
+ select I3C_HUB
+ help
+ This enables support for NXP P3H244x/P3H284x I3C HUB. These hubs
+ connect to a host via I3C/I2C/SMBus and allow communication with
+ multiple downstream peripherals. The Say Y or M here to use I3C
+ HUB driver to configure I3C HUB device.
diff --git a/drivers/i3c/hub/Makefile b/drivers/i3c/hub/Makefile
new file mode 100644
index 000000000000..9dbd8a7b4184
--- /dev/null
+++ b/drivers/i3c/hub/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2025 NXP
+p3h2840_i3c_hub-y := p3h2840_i3c_hub_common.o p3h2840_i3c_hub_i3c.o p3h2840_i3c_hub_smbus.o
+obj-$(CONFIG_P3H2X4X_I3C_HUB) += p3h2840_i3c_hub.o
diff --git a/drivers/i3c/hub/p3h2840_i3c_hub.h b/drivers/i3c/hub/p3h2840_i3c_hub.h
new file mode 100644
index 000000000000..d69fafbac584
--- /dev/null
+++ b/drivers/i3c/hub/p3h2840_i3c_hub.h
@@ -0,0 +1,327 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2025-2026 NXP
+ * This header file contain private device structure definition.
+ */
+
+#ifndef P3H2840_I3C_HUB_H
+#define P3H2840_I3C_HUB_H
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/i3c/device.h>
+#include <linux/i3c/hub.h>
+#include <linux/i3c/master.h>
+#include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
+
+/* I3C HUB REGISTERS */
+
+/* Device Information Registers */
+#define P3H2X4X_DEV_INFO_0 0x00
+#define P3H2X4X_DEV_INFO_1 0x01
+#define P3H2X4X_PID_5 0x02
+#define P3H2X4X_PID_4 0x03
+#define P3H2X4X_PID_3 0x04
+#define P3H2X4X_PID_2 0x05
+#define P3H2X4X_PID_1 0x06
+#define P3H2X4X_PID_0 0x07
+#define P3H2X4X_BCR 0x08
+#define P3H2X4X_DCR 0x09
+#define P3H2X4X_DEV_CAPAB 0x0a
+#define P3H2X4X_DEV_REV 0x0b
+
+/* Device Configuration Registers */
+#define P3H2X4X_CP_CONF 0x11
+#define P3H2X4X_TP_ENABLE 0x12
+
+#define P3H2X4X_DEV_CONF 0x13
+#define P3H2X4X_IO_STRENGTH 0x14
+#define P3H2X4X_TP0145_IO_STRENGTH_MASK GENMASK(1, 0)
+#define P3H2X4X_TP0145_IO_STRENGTH(x) \
+ FIELD_PREP(P3H2X4X_TP0145_IO_STRENGTH_MASK, x)
+#define P3H2X4X_TP2367_IO_STRENGTH_MASK GENMASK(3, 2)
+#define P3H2X4X_TP2367_IO_STRENGTH(x) \
+ FIELD_PREP(P3H2X4X_TP2367_IO_STRENGTH_MASK, x)
+#define P3H2X4X_CP0_IO_STRENGTH_MASK GENMASK(5, 4)
+#define P3H2X4X_CP0_IO_STRENGTH(x) \
+ FIELD_PREP(P3H2X4X_CP0_IO_STRENGTH_MASK, x)
+#define P3H2X4X_CP1_IO_STRENGTH_MASK GENMASK(7, 6)
+#define P3H2X4X_CP1_IO_STRENGTH(x) \
+ FIELD_PREP(P3H2X4X_CP1_IO_STRENGTH_MASK, x)
+#define P3H2X4X_IO_STRENGTH_MASK GENMASK(7, 0)
+
+#define P3H2X4X_TP_IO_MODE_CONF 0x17
+#define P3H2X4X_TP_SMBUS_AGNT_EN 0x18
+
+#define P3H2X4X_LDO_AND_PULLUP_CONF 0x19
+
+#define P3H2X4X_TP0145_PULLUP_CONF_MASK GENMASK(7, 6)
+#define P3H2X4X_TP0145_PULLUP_CONF(x) \
+ FIELD_PREP(P3H2X4X_TP0145_PULLUP_CONF_MASK, x)
+#define P3H2X4X_TP2367_PULLUP_CONF_MASK GENMASK(5, 4)
+#define P3H2X4X_TP2367_PULLUP_CONF(x) \
+ FIELD_PREP(P3H2X4X_TP2367_PULLUP_CONF_MASK, x)
+#define P3H2X4X_PULLUP_CONF_MASK GENMASK(7, 4)
+
+#define P3H2X4X_CP_IBI_CONF 0x1a
+
+#define P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG 0x1b
+
+#define P3H2X4X_IBI_MDB_CUSTOM 0x1c
+#define P3H2X4X_JEDEC_CONTEXT_ID 0x1d
+#define P3H2X4X_TP_GPIO_MODE_EN 0x1e
+
+/* Device Status and IBI Registers */
+#define P3H2X4X_DEV_AND_IBI_STS 0x20
+#define P3H2X4X_TP_SMBUS_AGNT_IBI_STS 0x21
+#define P3H2X4X_SMBUS_AGENT_EVENT_FLAG_STATUS BIT(4)
+
+/* Controller Port Control/Status Registers */
+#define P3H2X4X_CP_MUX_SET 0x38
+#define P3H2X4X_CONTROLLER_PORT_MUX_REQ BIT(0)
+#define P3H2X4X_CP_MUX_STS 0x39
+#define P3H2X4X_CONTROLLER_PORT_MUX_CONNECTION_STATUS BIT(0)
+
+/* Target Ports Control Registers */
+#define P3H2X4X_TP_SMBUS_AGNT_TRANS_START 0x50
+#define P3H2X4X_TP_NET_CON_CONF 0x51
+
+#define P3H2X4X_TP_PULLUP_EN 0x53
+
+#define P3H2X4X_TP_SCL_OUT_EN 0x54
+#define P3H2X4X_TP_SDA_OUT_EN 0x55
+#define P3H2X4X_TP_SCL_OUT_LEVEL 0x56
+#define P3H2X4X_TP_SDA_OUT_LEVEL 0x57
+#define P3H2X4X_TP_IN_DETECT_MODE_CONF 0x58
+#define P3H2X4X_TP_SCL_IN_DETECT_IBI_EN 0x59
+#define P3H2X4X_TP_SDA_IN_DETECT_IBI_EN 0x5a
+
+/* Target Ports Status Registers */
+#define P3H2X4X_TP_SCL_IN_LEVEL_STS 0x60
+#define P3H2X4X_TP_SDA_IN_LEVEL_STS 0x61
+#define P3H2X4X_TP_SCL_IN_DETECT_FLG 0x62
+#define P3H2X4X_TP_SDA_IN_DETECT_FLG 0x63
+
+/* SMBus Agent Configuration and Status Registers */
+#define P3H2X4X_TP0_SMBUS_AGNT_STS 0x64
+#define P3H2X4X_TP1_SMBUS_AGNT_STS 0x65
+#define P3H2X4X_TP2_SMBUS_AGNT_STS 0x66
+#define P3H2X4X_TP3_SMBUS_AGNT_STS 0x67
+#define P3H2X4X_TP4_SMBUS_AGNT_STS 0x68
+#define P3H2X4X_TP5_SMBUS_AGNT_STS 0x69
+#define P3H2X4X_TP6_SMBUS_AGNT_STS 0x6a
+#define P3H2X4X_TP7_SMBUS_AGNT_STS 0x6b
+#define P3H2X4X_ONCHIP_TD_AND_SMBUS_AGNT_CONF 0x6c
+
+/* buf receive flag set */
+#define P3H2X4X_TARGET_BUF_CA_TF BIT(0)
+#define P3H2X4X_TARGET_BUF_0_RECEIVE BIT(1)
+#define P3H2X4X_TARGET_BUF_1_RECEIVE BIT(2)
+#define P3H2X4X_TARGET_BUF_0_1_RECEIVE GENMASK(2, 1)
+#define P3H2X4X_TARGET_BUF_OVRFL GENMASK(3, 1)
+#define BUF_RECEIVED_FLAG_MASK GENMASK(3, 1)
+#define BUF_RECEIVED_FLAG_TF_MASK GENMASK(3, 0)
+
+#define P3H2X4X_TARGET_AGENT_LOCAL_DEV 0x11
+#define P3H2X4X_TARGET_BUFF_0_PAGE 0x12
+#define P3H2X4X_TARGET_BUFF_1_PAGE 0x13
+
+/* Special Function Registers */
+#define P3H2X4X_LDO_AND_CPSEL_STS 0x79
+#define P3H2X4X_CP_SDA1_LEVEL BIT(7)
+#define P3H2X4X_CP_SCL1_LEVEL BIT(6)
+
+#define P3H2X4X_CP_SEL_PIN_INPUT_CODE_MASK GENMASK(5, 4)
+#define P3H2X4X_CP_SEL_PIN_INPUT_CODE_GET(x) \
+ (((x) & P3H2X4X_CP_SEL_PIN_INPUT_CODE_MASK) >> 4)
+#define P3H2X4X_CP_SDA1_SCL1_PINS_CODE_MASK GENMASK(7, 6)
+#define P3H2X4X_CP_SDA1_SCL1_PINS_CODE_GET(x) \
+ (((x) & P3H2X4X_CP_SDA1_SCL1_PINS_CODE_MASK) >> 6)
+#define P3H2X4X_VCCIO1_PWR_GOOD BIT(3)
+#define P3H2X4X_VCCIO0_PWR_GOOD BIT(2)
+#define P3H2X4X_CP1_VCCIO_PWR_GOOD BIT(1)
+#define P3H2X4X_CP0_VCCIO_PWR_GOOD BIT(0)
+
+#define P3H2X4X_BUS_RESET_SCL_TIMEOUT 0x7a
+#define P3H2X4X_ONCHIP_TD_PROTO_ERR_FLG 0x7b
+#define P3H2X4X_DEV_CMD 0x7c
+#define P3H2X4X_ONCHIP_TD_STS 0x7d
+#define P3H2X4X_ONCHIP_TD_ADDR_CONF 0x7e
+#define P3H2X4X_PAGE_PTR 0x7f
+
+/* Paged Transaction Registers */
+#define P3H2X4X_CONTROLLER_BUFFER_PAGE 0x10
+#define P3H2X4X_CONTROLLER_AGENT_BUFF 0x80
+#define P3H2X4X_CONTROLLER_AGENT_BUFF_DATA 0x84
+
+#define P3H2X4X_TARGET_BUFF_LENGTH 0x80
+#define P3H2X4X_TARGET_BUFF_ADDRESS 0x81
+#define P3H2X4X_TARGET_BUFF_DATA 0x82
+
+#define P3H2X4X_TP_MAX_COUNT 0x08
+#define P3H2X4X_CP_MAX_COUNT 0x02
+#define P3H2X4X_TP_LOCAL_DEV 0x08
+
+/* LDO Disable/Enable DT settings */
+#define P3H2X4X_LDO_VOLT_1_0V 0x00
+#define P3H2X4X_LDO_VOLT_1_1V 0x01
+#define P3H2X4X_LDO_VOLT_1_2V 0x02
+#define P3H2X4X_LDO_VOLT_1_8V 0x03
+
+#define P3H2X4X_LDO_DISABLED 0x00
+#define P3H2X4X_LDO_ENABLED 0x01
+
+#define P3H2X4X_IBI_DISABLED 0x00
+#define P3H2X4X_IBI_ENABLED 0x01
+
+/* Pullup selection DT settings */
+#define P3H2X4X_TP_PULLUP_250R 0x00
+#define P3H2X4X_TP_PULLUP_500R 0x01
+#define P3H2X4X_TP_PULLUP_1000R 0x02
+#define P3H2X4X_TP_PULLUP_2000R 0x03
+
+#define P3H2X4X_TP_PULLUP_DISABLED 0x00
+#define P3H2X4X_TP_PULLUP_ENABLED 0x01
+
+#define P3H2X4X_IO_STRENGTH_20_OHM 0x00
+#define P3H2X4X_IO_STRENGTH_30_OHM 0x01
+#define P3H2X4X_IO_STRENGTH_40_OHM 0x02
+#define P3H2X4X_IO_STRENGTH_50_OHM 0x03
+
+#define P3H2X4X_TP_MODE_I3C 0x00
+#define P3H2X4X_TP_MODE_SMBUS 0x01
+#define P3H2X4X_TP_MODE_GPIO 0x02
+#define P3H2X4X_TP_MODE_I2C 0x03
+
+#define ONE_BYTE_SIZE 0x01
+
+/* holding SDA low when both SMBus Target Agent received data buffers are full.
+ * This feature can be used as a flow-control mechanism for MCTP applications to
+ * avoid MCTP transmitters on Target Ports time out when the SMBus agent buffers
+ * are not serviced in time by upstream controller and only receives write message
+ * from its downstream ports.
+ * SMBUS_AGENT_TX_RX_LOOPBACK_EN/TARGET_AGENT_BUF_FULL_SDA_LOW_EN
+ */
+
+#define P3H2X4X_TARGET_AGENT_DFT_IBI_CONF 0x20
+#define P3H2X4X_TARGET_AGENT_DFT_IBI_CONF_MASK 0x21
+
+/* Transaction status checking mask */
+
+#define P3H2X4X_SMBUS_TRANSACTION_FINISH_FLAG 1
+#define P3H2X4X_SMBUS_CNTRL_STATUS_TXN_SHIFT 4
+
+#define P3H2X4X_SMBUS_CNTRL_STATUS_TXN_OK 0
+#define P3H2X4X_SMBUS_CNTRL_STATUS_TXN_ADDR_NAK 1
+#define P3H2X4X_SMBUS_CNTRL_STATUS_TXN_DATA_NAK 2
+#define P3H2X4X_SMBUS_CNTRL_STATUS_TXN_WTR_NAK 3
+#define P3H2X4X_SMBUS_CNTRL_STATUS_TXN_SYNC_RCV 4
+#define P3H2X4X_SMBUS_CNTRL_STATUS_TXN_SYNC_RCVCLR 5
+#define P3H2X4X_SMBUS_CNTRL_STATUS_TXN_FAULT 6
+#define P3H2X4X_SMBUS_CNTRL_STATUS_TXN_ARB_LOSS 7
+#define P3H2X4X_SMBUS_CNTRL_STATUS_TXN_SCL_TO 8
+
+#define P3H2X4X_TP_BUFFER_STATUS_MASK 0x0f
+#define P3H2X4X_TP_TRANSACTION_CODE_MASK 0xf0
+
+/* SMBus transaction types fields */
+#define P3H2X4X_SMBUS_400kHz BIT(2)
+
+/* SMBus polling */
+#define P3H2X4X_POLLING_ROLL_PERIOD_MS 10
+
+/* Hub buffer size */
+#define P3H2X4X_CONTROLLER_BUFFER_SIZE 88
+#define P3H2X4X_TARGET_BUFFER_SIZE 80
+#define P3H2X4X_SMBUS_DESCRIPTOR_SIZE 4
+#define P3H2X4X_SMBUS_PAYLOAD_SIZE \
+ (P3H2X4X_CONTROLLER_BUFFER_SIZE - P3H2X4X_SMBUS_DESCRIPTOR_SIZE)
+#define P3H2X4X_SMBUS_TARGET_PAYLOAD_SIZE (P3H2X4X_TARGET_BUFFER_SIZE - 2)
+
+/* Hub SMBus transaction time */
+#define P3H2X4X_SMBUS_400kHz_TRANSFER_TIMEOUT(x) ((20 * (x)) + 80)
+
+#define P3H2X4X_NO_PAGE_PER_TP 4
+
+#define P3H2X4X_MAX_PAYLOAD_LEN 2
+#define P3H2X4X_NUM_SLOTS 6
+
+#define P3H2X4X_HUB_ID 0
+
+#define P3H2X4X_SET_BIT(n) BIT(n)
+
+enum p3h2x4x_tp {
+ TP_0,
+ TP_1,
+ TP_2,
+ TP_3,
+ TP_4,
+ TP_5,
+ TP_6,
+ TP_7,
+};
+
+enum p3h2x4x_rcv_buf {
+ RCV_BUF_0,
+ RCV_BUF_1,
+ RCV_BUF_OF,
+};
+
+struct tp_configuration {
+ bool pullup_en;
+ bool ibi_en;
+ bool always_enable;
+ int mode;
+};
+
+struct hub_configuration {
+ int tp0145_pullup;
+ int tp2367_pullup;
+ int cp0_io_strength;
+ int cp1_io_strength;
+ int tp0145_io_strength;
+ int tp2367_io_strength;
+ struct tp_configuration tp_config[P3H2X4X_TP_MAX_COUNT];
+};
+
+struct tp_bus {
+ bool is_registered; /* bus was registered in the framework. */
+ u8 tp_mask;
+ u8 tp_port;
+ struct mutex port_mutex; /* per port mutex */
+ struct device_node *of_node;
+ struct i2c_client *tp_smbus_client;
+ struct i2c_adapter *tp_smbus_adapter;
+ struct i3c_hub_controller hub_controller;
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub;
+};
+
+struct p3h2x4x_i3c_hub_dev {
+ struct device *dev;
+ struct regmap *regmap;
+ struct mutex etx_mutex; /* all port mutex */
+ struct i3c_device *i3cdev;
+ struct i2c_client *i2c_client;
+ struct hub_configuration hub_config;
+ struct tp_bus tp_bus[P3H2X4X_TP_MAX_COUNT];
+ struct i3c_hub *hub;
+};
+
+/**
+ * p3h2x4x_tp_smbus_algo - add i2c adapter for target port configured as SMBus.
+ * @priv: p3h2x4x device structure.
+ * @tp: target port.
+ * Return: 0 in case of success, a negative EINVAL code if the error.
+ */
+int p3h2x4x_tp_smbus_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub);
+
+/**
+ * p3h2x4x_tp_i3c_algo - register i3c controller for target port configured as I3C.
+ * @priv: p3h2x4x device structure.
+ * @tp: target port.
+ * Return: 0 in case of success, a negative EINVAL code if the error.
+ */
+int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub);
+
+#endif /* P3H2840_I3C_HUB_H */
diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_common.c b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
new file mode 100644
index 000000000000..fedcb978f07e
--- /dev/null
+++ b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
@@ -0,0 +1,352 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025-2026 NXP
+ * This P3H2X4X driver file implements functions for Hub probe and DT parsing.
+ */
+
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/p3h2840.h>
+#include <linux/util_macros.h>
+
+#include "p3h2840_i3c_hub.h"
+
+/* LDO voltage DT settings */
+#define P3H2X4X_DT_LDO_VOLT_1_0V 1000000
+#define P3H2X4X_DT_LDO_VOLT_1_1V 1100000
+#define P3H2X4X_DT_LDO_VOLT_1_2V 1200000
+#define P3H2X4X_DT_LDO_VOLT_1_8V 1800000
+
+static const int p3h2x4x_pullup_tbl[] = {
+ 250, 500, 1000, 2000
+};
+
+static const int p3h2x4x_io_strength_tbl[] = {
+ 20, 30, 40, 50
+};
+
+static u8 p3h2x4x_pullup_dt_to_reg(int dt_value)
+{
+ return find_closest(dt_value, p3h2x4x_pullup_tbl,
+ ARRAY_SIZE(p3h2x4x_pullup_tbl));
+}
+
+static u8 p3h2x4x_io_strength_dt_to_reg(int dt_value)
+{
+ return find_closest(dt_value, p3h2x4x_io_strength_tbl,
+ ARRAY_SIZE(p3h2x4x_io_strength_tbl));
+}
+
+static int p3h2x4x_configure_pullup(struct device *dev)
+{
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
+ u8 pullup;
+
+ pullup = P3H2X4X_TP0145_PULLUP_CONF(p3h2x4x_pullup_dt_to_reg
+ (p3h2x4x_i3c_hub->hub_config.tp0145_pullup));
+
+ pullup |= P3H2X4X_TP2367_PULLUP_CONF(p3h2x4x_pullup_dt_to_reg
+ (p3h2x4x_i3c_hub->hub_config.tp2367_pullup));
+
+ return regmap_update_bits(p3h2x4x_i3c_hub->regmap, P3H2X4X_LDO_AND_PULLUP_CONF,
+ P3H2X4X_PULLUP_CONF_MASK, pullup);
+}
+
+static int p3h2x4x_configure_io_strength(struct device *dev)
+{
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
+ u8 io_strength;
+
+ io_strength = P3H2X4X_CP0_IO_STRENGTH(p3h2x4x_io_strength_dt_to_reg
+ (p3h2x4x_i3c_hub->hub_config.cp0_io_strength));
+
+ io_strength |= P3H2X4X_CP1_IO_STRENGTH(p3h2x4x_io_strength_dt_to_reg
+ (p3h2x4x_i3c_hub->hub_config.cp1_io_strength));
+
+ io_strength |= P3H2X4X_TP0145_IO_STRENGTH(p3h2x4x_io_strength_dt_to_reg
+ (p3h2x4x_i3c_hub->hub_config.tp0145_io_strength));
+
+ io_strength |= P3H2X4X_TP2367_IO_STRENGTH(p3h2x4x_io_strength_dt_to_reg
+ (p3h2x4x_i3c_hub->hub_config.tp2367_io_strength));
+
+ return regmap_update_bits(p3h2x4x_i3c_hub->regmap, P3H2X4X_IO_STRENGTH,
+ P3H2X4X_IO_STRENGTH_MASK, io_strength);
+}
+
+static int p3h2x4x_configure_ldo(struct device *dev)
+{
+ static const char * const supplies[] = {
+ "vcc1",
+ "vcc2",
+ "vcc3",
+ "vcc4"
+ };
+ int ret, i;
+
+ for (i = 0; i < ARRAY_SIZE(supplies); i++) {
+ ret = devm_regulator_get_enable_optional(dev->parent, supplies[i]);
+ if (ret == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ if (ret && ret != -ENODEV)
+ dev_warn(dev, "Failed to enable %s (%d)\n",
+ supplies[i], ret);
+ }
+
+ /* This delay is required for the regulator to stabilize its output voltage */
+ fsleep(5000);
+
+ return 0;
+}
+
+static int p3h2x4x_configure_tp(struct device *dev)
+{
+ struct p3h2x4x_i3c_hub_dev *hub = dev_get_drvdata(dev);
+ u8 mode = 0, smbus = 0, pullup = 0, target_port = 0;
+ int tp, ret;
+
+ for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
+ pullup |= hub->hub_config.tp_config[tp].pullup_en ? P3H2X4X_SET_BIT(tp) : 0;
+ mode |= (hub->hub_config.tp_config[tp].mode != P3H2X4X_TP_MODE_I3C) ?
+ P3H2X4X_SET_BIT(tp) : 0;
+ smbus |= (hub->hub_config.tp_config[tp].mode == P3H2X4X_TP_MODE_SMBUS) ?
+ P3H2X4X_SET_BIT(tp) : 0;
+ target_port |= (hub->tp_bus[tp].tp_mask == P3H2X4X_SET_BIT(tp)) ?
+ hub->tp_bus[tp].tp_mask : 0;
+ }
+
+ ret = regmap_update_bits(hub->regmap, P3H2X4X_TP_PULLUP_EN, pullup, pullup);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(hub->regmap, P3H2X4X_TP_IO_MODE_CONF, mode, mode);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(hub->regmap, P3H2X4X_TP_SMBUS_AGNT_EN, smbus, smbus);
+ if (ret)
+ return ret;
+
+ if (target_port & ~smbus) {
+ ret = regmap_write(hub->regmap, P3H2X4X_CP_MUX_SET,
+ P3H2X4X_CONTROLLER_PORT_MUX_REQ);
+ if (ret)
+ return ret;
+ }
+
+ return regmap_update_bits(hub->regmap, P3H2X4X_TP_ENABLE, target_port, target_port);
+}
+
+static int p3h2x4x_configure_hw(struct device *dev)
+{
+ int ret;
+
+ ret = p3h2x4x_configure_ldo(dev);
+ if (ret)
+ return ret;
+
+ ret = p3h2x4x_configure_pullup(dev);
+ if (ret)
+ return ret;
+
+ ret = p3h2x4x_configure_io_strength(dev);
+ if (ret)
+ return ret;
+
+ return p3h2x4x_configure_tp(dev);
+}
+
+static void p3h2x4x_get_target_port_dt_conf(struct device *dev,
+ const struct device_node *node)
+{
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
+ u64 tp_port;
+
+ for_each_available_child_of_node_scoped(node, dev_node) {
+ if (of_property_read_reg(dev_node, 0, &tp_port, NULL))
+ continue;
+
+ if (tp_port < P3H2X4X_TP_MAX_COUNT) {
+ p3h2x4x_i3c_hub->tp_bus[tp_port].of_node = of_node_get(dev_node);
+ p3h2x4x_i3c_hub->tp_bus[tp_port].tp_mask = P3H2X4X_SET_BIT(tp_port);
+ p3h2x4x_i3c_hub->tp_bus[tp_port].p3h2x4x_i3c_hub = p3h2x4x_i3c_hub;
+ p3h2x4x_i3c_hub->tp_bus[tp_port].tp_port = tp_port;
+ }
+ }
+}
+
+static void p3h2x4x_parse_tp_dt_settings(struct device *dev,
+ const struct device_node *node,
+ struct tp_configuration tp_config[])
+{
+ u64 id;
+
+ for_each_available_child_of_node_scoped(node, tp_node) {
+ if (of_property_read_reg(tp_node, 0, &id, NULL))
+ continue;
+
+ if (id >= P3H2X4X_TP_MAX_COUNT) {
+ dev_warn(dev, "Invalid target port index found in DT: %lli\n", id);
+ continue;
+ }
+
+ if (strcmp(tp_node->name, "i3c") == 0)
+ tp_config[id].mode = P3H2X4X_TP_MODE_I3C;
+
+ if (strcmp(tp_node->name, "i2c") == 0)
+ tp_config[id].mode = P3H2X4X_TP_MODE_I2C;
+
+ if (strcmp(tp_node->name, "smbus") == 0)
+ tp_config[id].mode = P3H2X4X_TP_MODE_SMBUS;
+
+ tp_config[id].pullup_en =
+ of_property_read_bool(tp_node, "nxp,pullup-enable");
+ }
+}
+
+static void p3h2x4x_get_hub_dt_conf(struct device *dev,
+ const struct device_node *node)
+{
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
+
+ of_property_read_u32(node, "nxp,tp0145-pullup-ohms",
+ &p3h2x4x_i3c_hub->hub_config.tp0145_pullup);
+ of_property_read_u32(node, "nxp,tp2367-pullup-ohms",
+ &p3h2x4x_i3c_hub->hub_config.tp2367_pullup);
+ of_property_read_u32(node, "nxp,cp0-io-strength-ohms",
+ &p3h2x4x_i3c_hub->hub_config.cp0_io_strength);
+ of_property_read_u32(node, "nxp,cp1-io-strength-ohms",
+ &p3h2x4x_i3c_hub->hub_config.cp1_io_strength);
+ of_property_read_u32(node, "nxp,tp0145-io-strength-ohms",
+ &p3h2x4x_i3c_hub->hub_config.tp0145_io_strength);
+ of_property_read_u32(node, "nxp,tp2367-io-strength-ohms",
+ &p3h2x4x_i3c_hub->hub_config.tp2367_io_strength);
+
+ p3h2x4x_parse_tp_dt_settings(dev, node, p3h2x4x_i3c_hub->hub_config.tp_config);
+}
+
+static void p3h2x4x_default_configuration(struct device *dev)
+{
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
+ int tp_count;
+
+ p3h2x4x_i3c_hub->hub_config.tp0145_pullup = P3H2X4X_TP_PULLUP_500R;
+ p3h2x4x_i3c_hub->hub_config.tp2367_pullup = P3H2X4X_TP_PULLUP_500R;
+ p3h2x4x_i3c_hub->hub_config.cp0_io_strength = P3H2X4X_IO_STRENGTH_20_OHM;
+ p3h2x4x_i3c_hub->hub_config.cp1_io_strength = P3H2X4X_IO_STRENGTH_20_OHM;
+ p3h2x4x_i3c_hub->hub_config.tp0145_io_strength = P3H2X4X_IO_STRENGTH_20_OHM;
+ p3h2x4x_i3c_hub->hub_config.tp2367_io_strength = P3H2X4X_IO_STRENGTH_20_OHM;
+
+ for (tp_count = 0; tp_count < P3H2X4X_TP_MAX_COUNT; ++tp_count)
+ p3h2x4x_i3c_hub->hub_config.tp_config[tp_count].mode = P3H2X4X_TP_MODE_I3C;
+}
+
+static int p3h2x4x_i3c_hub_probe(struct platform_device *pdev)
+{
+ struct p3h2x4x_dev *p3h2x4x = dev_get_drvdata(pdev->dev.parent);
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub;
+ struct device *dev = &pdev->dev;
+ struct device_node *node;
+ int ret, i;
+
+ p3h2x4x_i3c_hub = devm_kzalloc(dev, sizeof(*p3h2x4x_i3c_hub), GFP_KERNEL);
+ if (!p3h2x4x_i3c_hub)
+ return -ENOMEM;
+
+ p3h2x4x_i3c_hub->regmap = p3h2x4x->regmap;
+ p3h2x4x_i3c_hub->dev = dev;
+
+ platform_set_drvdata(pdev, p3h2x4x_i3c_hub);
+
+ p3h2x4x_default_configuration(dev);
+
+ ret = devm_mutex_init(dev, &p3h2x4x_i3c_hub->etx_mutex);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < P3H2X4X_TP_MAX_COUNT; i++) {
+ ret = devm_mutex_init(dev, &p3h2x4x_i3c_hub->tp_bus[i].port_mutex);
+ if (ret)
+ return ret;
+ }
+
+ /* get hub node from DT */
+ node = dev->parent->of_node;
+ if (!node)
+ return dev_err_probe(dev, -ENODEV, "No Device Tree entry found\n");
+
+ p3h2x4x_get_hub_dt_conf(dev, node);
+ p3h2x4x_get_target_port_dt_conf(dev, node);
+
+ /* Unlock access to protected registers */
+ ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
+ P3H2X4X_REGISTERS_UNLOCK_CODE);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to unlock HUB's protected registers\n");
+
+ ret = p3h2x4x_configure_hw(dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to configure the HUB\n");
+
+ /* Register virtual I3C master controllers for I3C target ports */
+ if (p3h2x4x->i3cdev) {
+ p3h2x4x_i3c_hub->i3cdev = p3h2x4x->i3cdev;
+ i3cdev_set_drvdata(p3h2x4x->i3cdev, p3h2x4x_i3c_hub);
+ ret = p3h2x4x_tp_i3c_algo(p3h2x4x_i3c_hub);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to register i3c bus\n");
+ }
+
+ /* Register virtual I2C adapters for SMBus target ports */
+ ret = p3h2x4x_tp_smbus_algo(p3h2x4x_i3c_hub);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to add i2c adapter\n");
+
+ /* Lock access to protected registers */
+ ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
+ P3H2X4X_REGISTERS_LOCK_CODE);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to lock HUB's protected registers\n");
+
+ return 0;
+}
+
+static void p3h2x4x_i3c_hub_remove(struct platform_device *pdev)
+{
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = platform_get_drvdata(pdev);
+ struct p3h2x4x_dev *p3h2x4x = dev_get_drvdata(pdev->dev.parent);
+ u8 i;
+
+ for (i = 0; i < P3H2X4X_TP_MAX_COUNT; i++) {
+ if (p3h2x4x_i3c_hub->tp_bus[i].is_registered) {
+ if (p3h2x4x_i3c_hub->hub_config.tp_config[i].mode ==
+ P3H2X4X_TP_MODE_SMBUS) {
+ i2c_del_adapter(p3h2x4x_i3c_hub->tp_bus[i].tp_smbus_adapter);
+ p3h2x4x_i3c_hub->tp_bus[i].tp_smbus_adapter = NULL;
+ p3h2x4x_i3c_hub->tp_bus[i].is_registered = false;
+ }
+ }
+
+ if (p3h2x4x_i3c_hub->tp_bus[i].of_node) {
+ of_node_put(p3h2x4x_i3c_hub->tp_bus[i].of_node);
+ p3h2x4x_i3c_hub->tp_bus[i].of_node = NULL;
+ }
+ }
+}
+
+static struct platform_driver p3h2x4x_i3c_hub_driver = {
+ .driver = {
+ .name = "p3h2x4x-i3c-hub",
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+ .probe = p3h2x4x_i3c_hub_probe,
+ .remove = p3h2x4x_i3c_hub_remove,
+};
+module_platform_driver(p3h2x4x_i3c_hub_driver);
+
+MODULE_AUTHOR("Aman Kumar Pandey <aman.kumarpandey@nxp.com>");
+MODULE_AUTHOR("Vikash Bansal <vikash.bansal@nxp.com>");
+MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
+MODULE_DESCRIPTION("P3H2X4X I3C HUB driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
new file mode 100644
index 000000000000..38505dda0e81
--- /dev/null
+++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025-2026 NXP
+ * This P3H2X4X driver file contain functions for I3C virtual Bus creation, connect/disconnect
+ * hub network and read/write.
+ */
+#include <linux/i3c/hub.h>
+#include <linux/mfd/p3h2840.h>
+#include <linux/regmap.h>
+
+#include "p3h2840_i3c_hub.h"
+
+static inline struct tp_bus *
+p3h2x4x_bus_from_controller(struct i3c_master_controller *controller)
+{
+ struct i3c_hub_controller *hub_controller;
+
+ hub_controller = container_of(controller, struct i3c_hub_controller, controller);
+
+ return container_of(hub_controller, struct tp_bus, hub_controller);
+}
+
+static void p3h2x4x_hub_enable_port(struct i3c_master_controller *controller)
+{
+ struct tp_bus *bus = p3h2x4x_bus_from_controller(controller);
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = bus->p3h2x4x_i3c_hub;
+
+ if (p3h2x4x_i3c_hub->hub_config.tp_config[bus->tp_port].always_enable)
+ return;
+
+ regmap_set_bits(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP_NET_CON_CONF, bus->tp_mask);
+}
+
+static void p3h2x4x_hub_disable_port(struct i3c_master_controller *controller)
+{
+ struct tp_bus *bus = p3h2x4x_bus_from_controller(controller);
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = bus->p3h2x4x_i3c_hub;
+
+ if (p3h2x4x_i3c_hub->hub_config.tp_config[bus->tp_port].always_enable)
+ return;
+
+ regmap_clear_bits(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP_NET_CON_CONF, bus->tp_mask);
+}
+
+static const struct i3c_hub_ops p3h2x4x_hub_ops = {
+ .enable_port = p3h2x4x_hub_enable_port,
+ .disable_port = p3h2x4x_hub_disable_port,
+};
+
+static void p3h2x4x_unregister_i3c_master(void *data)
+{
+ struct i3c_master_controller *controller = data;
+
+ i3c_master_unregister(controller);
+}
+
+/**
+ * p3h2x4x_tp_i3c_algo - register i3c master for target port who
+ * configured as i3c.
+ * @p3h2x4x_hub: p3h2x4x device structure.
+ * Return: 0 in case of success, negative error code on failur.
+ */
+int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
+{
+ struct i3c_master_controller *parent = i3c_dev_get_master(p3h2x4x_hub->i3cdev->desc);
+ u8 tp, ntwk_mask = 0;
+ int ret;
+
+ p3h2x4x_hub->hub = devm_kzalloc(p3h2x4x_hub->dev,
+ sizeof(*p3h2x4x_hub->hub),
+ GFP_KERNEL);
+
+ if (!p3h2x4x_hub->hub)
+ return -ENOMEM;
+
+ i3c_hub_init(p3h2x4x_hub->hub,
+ &p3h2x4x_hub_ops,
+ p3h2x4x_hub->i3cdev);
+
+ if (IS_ERR(p3h2x4x_hub->hub))
+ return PTR_ERR(p3h2x4x_hub->hub);
+
+ for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
+ if (!p3h2x4x_hub->tp_bus[tp].of_node ||
+ p3h2x4x_hub->hub_config.tp_config[tp].mode != P3H2X4X_TP_MODE_I3C)
+ continue;
+
+ /* Assign DT node for this TP */
+ p3h2x4x_hub->dev->of_node = p3h2x4x_hub->tp_bus[tp].of_node;
+
+ struct i3c_hub_controller *hub_controller =
+ &p3h2x4x_hub->tp_bus[tp].hub_controller;
+ struct i3c_master_controller *controller = &hub_controller->controller;
+
+ hub_controller->parent = parent;
+ hub_controller->hub = p3h2x4x_hub->hub;
+
+ dev_set_drvdata(&controller->dev, hub_controller);
+
+ ret = i3c_master_register(controller,
+ p3h2x4x_hub->dev,
+ i3c_hub_master_ops(),
+ false);
+
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(p3h2x4x_hub->dev,
+ p3h2x4x_unregister_i3c_master,
+ controller);
+ if (ret)
+ return ret;
+
+ /* Perform DAA */
+ ret = i3c_master_do_daa(parent);
+ if (ret)
+ return ret;
+
+ ntwk_mask |= p3h2x4x_hub->tp_bus[tp].tp_mask;
+ p3h2x4x_hub->tp_bus[tp].is_registered = true;
+ p3h2x4x_hub->hub_config.tp_config[tp].always_enable = true;
+ }
+ return regmap_write(p3h2x4x_hub->regmap, P3H2X4X_TP_NET_CON_CONF, ntwk_mask);
+}
diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
new file mode 100644
index 000000000000..edb75f790e92
--- /dev/null
+++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025-2026 NXP
+ * This P3H2X4X driver file contain functions for SMBus/I2C virtual Bus creation and read/write.
+ */
+#include <linux/mfd/p3h2840.h>
+#include <linux/regmap.h>
+
+#include "p3h2840_i3c_hub.h"
+
+enum p3h2x4x_smbus_desc_idx {
+ P3H2X4X_DESC_ADDR,
+ P3H2X4X_DESC_TYPE,
+ P3H2X4X_DESC_WRITE_LEN,
+ P3H2X4X_DESC_READ_LEN,
+};
+
+static int p3h2x4x_read_smbus_transaction_status(struct p3h2x4x_i3c_hub_dev *hub,
+ u8 target_port_status,
+ u8 data_length)
+{
+ u32 status_read;
+ u8 status;
+ int ret;
+
+ fsleep(P3H2X4X_SMBUS_400kHz_TRANSFER_TIMEOUT(data_length));
+
+ ret = regmap_read(hub->regmap, target_port_status, &status_read);
+ if (ret)
+ return ret;
+
+ status = (u8)status_read;
+
+ status = (status & P3H2X4X_TP_TRANSACTION_CODE_MASK)
+ >> P3H2X4X_SMBUS_CNTRL_STATUS_TXN_SHIFT;
+
+ switch (status) {
+ case P3H2X4X_SMBUS_CNTRL_STATUS_TXN_OK:
+ return 0;
+ case P3H2X4X_SMBUS_CNTRL_STATUS_TXN_ADDR_NAK:
+ return -ENXIO;
+ case P3H2X4X_SMBUS_CNTRL_STATUS_TXN_DATA_NAK:
+ return -EIO;
+ case P3H2X4X_SMBUS_CNTRL_STATUS_TXN_SCL_TO:
+ return -ETIMEDOUT;
+ case P3H2X4X_SMBUS_CNTRL_STATUS_TXN_ARB_LOSS:
+ return -EAGAIN;
+ default:
+ return -EIO;
+ }
+}
+
+/*
+ * p3h2x4x_tp_i2c_xfer_msg() - This starts a SMBus write transaction by writing a descriptor
+ * and a message to the p3h2x4x registers. Controller buffer page is determined by multiplying the
+ * target port index by four and adding the base page number to it.
+ */
+static int p3h2x4x_tp_i2c_xfer_msg(struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub,
+ struct i2c_msg *xfers,
+ u8 target_port,
+ u8 nxfers_i, u8 rw)
+{
+ u8 controller_buffer_page = P3H2X4X_CONTROLLER_BUFFER_PAGE + 4 * target_port;
+ u8 target_port_status = P3H2X4X_TP0_SMBUS_AGNT_STS + target_port;
+ u8 desc[P3H2X4X_SMBUS_DESCRIPTOR_SIZE] = { 0 };
+ u8 transaction_type = P3H2X4X_SMBUS_400kHz;
+ int write_length, read_length;
+ u8 addr = xfers[nxfers_i].addr;
+ u8 rw_address = 2 * addr;
+ int ret, ret2;
+
+ if (rw == 2) { /* write and read */
+ write_length = xfers[nxfers_i].len;
+ read_length = xfers[nxfers_i + 1].len;
+ } else if (rw == 1) {
+ rw_address |= P3H2X4X_SET_BIT(0);
+ write_length = 0;
+ read_length = xfers[nxfers_i].len;
+ } else {
+ write_length = xfers[nxfers_i].len;
+ read_length = 0;
+ }
+
+ desc[P3H2X4X_DESC_ADDR] = rw_address;
+ if (rw == 2)
+ desc[P3H2X4X_DESC_TYPE] = transaction_type | P3H2X4X_SET_BIT(0);
+ else
+ desc[P3H2X4X_DESC_TYPE] = transaction_type;
+ desc[P3H2X4X_DESC_WRITE_LEN] = write_length;
+ desc[P3H2X4X_DESC_READ_LEN] = read_length;
+
+ ret = regmap_write(p3h2x4x_i3c_hub->regmap, target_port_status,
+ P3H2X4X_TP_BUFFER_STATUS_MASK);
+ if (ret)
+ goto out;
+
+ ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_PAGE_PTR, controller_buffer_page);
+
+ if (ret)
+ goto out;
+
+ ret = regmap_bulk_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_CONTROLLER_AGENT_BUFF,
+ desc, P3H2X4X_SMBUS_DESCRIPTOR_SIZE);
+
+ if (ret)
+ goto out;
+
+ if (!(rw % 2) && xfers[nxfers_i].len) {
+ ret = regmap_bulk_write(p3h2x4x_i3c_hub->regmap,
+ P3H2X4X_CONTROLLER_AGENT_BUFF_DATA,
+ xfers[nxfers_i].buf, xfers[nxfers_i].len);
+ if (ret)
+ goto out;
+ }
+
+ ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP_SMBUS_AGNT_TRANS_START,
+ p3h2x4x_i3c_hub->tp_bus[target_port].tp_mask);
+
+ if (ret)
+ goto out;
+
+ ret = p3h2x4x_read_smbus_transaction_status(p3h2x4x_i3c_hub,
+ target_port_status,
+ (write_length + read_length));
+ if (ret)
+ goto out;
+
+ if (rw) {
+ if (rw == 2)
+ nxfers_i += 1;
+
+ if (xfers[nxfers_i].len) {
+ ret = regmap_bulk_read(p3h2x4x_i3c_hub->regmap,
+ P3H2X4X_CONTROLLER_AGENT_BUFF_DATA + write_length,
+ xfers[nxfers_i].buf, xfers[nxfers_i].len);
+ if (ret)
+ goto out;
+ }
+ }
+out:
+ ret2 = regmap_write(p3h2x4x_i3c_hub->regmap,
+ P3H2X4X_PAGE_PTR, 0x00);
+ if (!ret && ret2)
+ ret = ret2;
+
+ return ret;
+}
+
+/*
+ * This function will be called whenever you call I2C read, write APIs like
+ * i2c_master_send(), i2c_master_recv() etc.
+ */
+static s32 p3h2x4x_tp_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+ int ret_sum = 0, ret;
+ u8 msg_count, rw;
+
+ struct tp_bus *bus = i2c_get_adapdata(adap);
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = bus->p3h2x4x_i3c_hub;
+
+ guard(mutex)(&p3h2x4x_i3c_hub->etx_mutex);
+ guard(mutex)(&bus->port_mutex);
+
+ for (msg_count = 0; msg_count < num; msg_count++) {
+ if (msgs[msg_count].len > P3H2X4X_SMBUS_PAYLOAD_SIZE) {
+ dev_err(p3h2x4x_i3c_hub->dev,
+ "Message nr. %d not sent - length over %d bytes.\n",
+ msg_count, P3H2X4X_SMBUS_PAYLOAD_SIZE);
+ return -EINVAL;
+ }
+
+ rw = (msgs[msg_count].flags & I2C_M_RD) ? 1 : 0;
+ if (!rw) {
+ /* If a read message is immediately followed by a write message to
+ * the same address, consider combining them into a single transaction.
+ */
+ if (msg_count + 1 < num &&
+ msgs[msg_count].addr == msgs[msg_count + 1].addr &&
+ (msgs[msg_count + 1].flags & I2C_M_RD)) {
+ if (msgs[msg_count].len + msgs[msg_count + 1].len >
+ P3H2X4X_SMBUS_PAYLOAD_SIZE)
+ return -EINVAL;
+
+ rw = 2;
+ msg_count += 1;
+ ret_sum += 1;
+ }
+ }
+
+ ret = p3h2x4x_tp_i2c_xfer_msg(p3h2x4x_i3c_hub,
+ msgs,
+ bus->tp_port,
+ (rw == 2) ? (msg_count - 1) : msg_count,
+ rw);
+ if (ret)
+ return ret;
+
+ ret_sum++;
+ }
+ return ret_sum;
+}
+
+static u32 p3h2x4x_tp_smbus_funcs(struct i2c_adapter *adapter)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+/*
+ * I2C algorithm Structure
+ */
+static struct i2c_algorithm p3h2x4x_tp_i2c_algorithm = {
+ .master_xfer = p3h2x4x_tp_i2c_xfer,
+ .functionality = p3h2x4x_tp_smbus_funcs,
+};
+
+static void p3h2x4x_unregister_smbus_adapters(struct p3h2x4x_i3c_hub_dev *hub)
+{
+ u8 tp;
+
+ for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
+ if (!hub->tp_bus[tp].tp_smbus_adapter)
+ continue;
+
+ i2c_del_adapter(hub->tp_bus[tp].tp_smbus_adapter);
+ hub->tp_bus[tp].tp_smbus_adapter = NULL;
+ hub->tp_bus[tp].is_registered = false;
+ }
+}
+
+/**
+ * p3h2x4x_tp_smbus_algo - add i2c adapter for target port who
+ * configured as SMBus.
+ * @hub: p3h2x4x device structure.
+ * Return: 0 in case of success, negative error code on failur.
+ */
+int p3h2x4x_tp_smbus_algo(struct p3h2x4x_i3c_hub_dev *hub)
+{
+ int ret;
+ u8 tp;
+
+ for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
+ if (!hub->tp_bus[tp].of_node ||
+ hub->hub_config.tp_config[tp].mode != P3H2X4X_TP_MODE_SMBUS)
+ continue;
+
+ /* Allocate adapter */
+ struct i2c_adapter *smbus_adapter =
+ devm_kzalloc(hub->dev, sizeof(*smbus_adapter), GFP_KERNEL);
+ if (!smbus_adapter)
+ return -ENOMEM;
+
+ /* Initialize adapter */
+ smbus_adapter->owner = THIS_MODULE;
+ smbus_adapter->class = I2C_CLASS_HWMON;
+ smbus_adapter->algo = &p3h2x4x_tp_i2c_algorithm;
+ smbus_adapter->dev.parent = hub->dev;
+ smbus_adapter->dev.of_node = hub->tp_bus[tp].of_node;
+ snprintf(smbus_adapter->name, sizeof(smbus_adapter->name),
+ "p3h2x4x-i3c-hub.tp-port-%d", tp);
+
+ i2c_set_adapdata(smbus_adapter, &hub->tp_bus[tp]);
+
+ /* Register adapter */
+ ret = i2c_add_adapter(smbus_adapter);
+ if (ret) {
+ p3h2x4x_unregister_smbus_adapters(hub);
+ return ret;
+ }
+
+ hub->tp_bus[tp].is_registered = true;
+ hub->hub_config.tp_config[tp].ibi_en = false;
+ hub->tp_bus[tp].tp_smbus_adapter = smbus_adapter;
+ }
+
+ /*
+ * holding SDA low when both SMBus Target Agent received data buffers are full.
+ * This feature can be used as a flow-control mechanism for MCTP applications to
+ * avoid MCTP transmitters on Target Ports time out when the SMBus agent buffers
+ * are not serviced in time by upstream controller and only receives write message
+ * from its downstream ports.
+ */
+ ret = regmap_update_bits(hub->regmap, P3H2X4X_ONCHIP_TD_AND_SMBUS_AGNT_CONF,
+ P3H2X4X_TARGET_AGENT_DFT_IBI_CONF_MASK,
+ P3H2X4X_TARGET_AGENT_DFT_IBI_CONF);
+ if (ret)
+ return ret;
+
+ return regmap_write(hub->regmap, P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG, P3H2X4X_IBI_DISABLED);
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v11 8/9] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality
2026-06-12 11:18 ` [PATCH v11 8/9] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality Lakshay Piplani
@ 2026-06-12 11:39 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-12 11:39 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: Frank.Li, linux-i3c, devicetree, conor+dt, robh
Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Regulator devres cleanup actions are erroneously attached to the parent device instead of the child device being probed.
- [High] SMBus I2C adapters are not unregistered on probe error paths, causing a Use-After-Free.
- [High] In-Band Interrupt (IBI) remains requested and enabled on probe failure, causing a Use-After-Free.
- [High] Unsafe reliance on non-deterministic delays without polling the SMBus finish flag causes premature data reads.
- [High] The driver dynamically overwrites the shared platform device's `of_node` pointer, corrupting the core device tree state.
- [High] `regmap_update_bits` uses the configuration value as the bitmask, failing to clear stale bits.
- [High] DAA is performed on the upstream bus before the downstream target ports are physically enabled.
- [Medium] Target port device tree node references acquired during probe are leaked on error paths.
- [Medium] Default configuration structures are populated with enum values instead of Ohms, causing misconfiguration.
- [Low] Dead code due to incorrect `IS_ERR` check on `devm_kzalloc` return value.
--
commit 8d18a29a52d0297c45635edaf397ce828e709ef9
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality
Add I3C hub functionality for the NXP P3H2x4x family of multiport hubs.
These devices support downstream target ports that can be configured
as I3C, I2C, or SMBus.
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_common.c b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
> new file mode 100644
> index 0000000000000..fedcb978f07ee
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
[ ... ]
> +static int p3h2x4x_configure_ldo(struct device *dev)
> +{
> + static const char * const supplies[] = {
> + "vcc1",
> + "vcc2",
> + "vcc3",
> + "vcc4"
> + };
> + int ret, i;
> +
> + for (i = 0; i < ARRAY_SIZE(supplies); i++) {
> + ret = devm_regulator_get_enable_optional(dev->parent, supplies[i]);
[Severity: High]
Could this leak devres memory on the parent device? By passing dev->parent
(the MFD parent device) instead of dev (the child hub device), the regulator
cleanup actions are attached to the parent. If the hub driver fails probe
or is repeatedly bound/unbound, it looks like these regulators won't be
disabled and the references will be leaked.
[ ... ]
> +static int p3h2x4x_configure_tp(struct device *dev)
> +{
> + struct p3h2x4x_i3c_hub_dev *hub = dev_get_drvdata(dev);
> + u8 mode = 0, smbus = 0, pullup = 0, target_port = 0;
> + int tp, ret;
> +
> + for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
> + pullup |= hub->hub_config.tp_config[tp].pullup_en ? P3H2X4X_SET_BIT(tp) : 0;
[ ... ]
> + ret = regmap_update_bits(hub->regmap, P3H2X4X_TP_PULLUP_EN, pullup, pullup);
[Severity: High]
Does this fail to clear stale bits? The third argument to regmap_update_bits()
is the mask. By using 'pullup' as the mask, it seems this only applies
operations to bits that are 1. Any bit that is 0 in 'pullup' is ignored,
which might leave stale hardware state (like pullups enabled by the
bootloader) unmodified. Should this use a proper mask covering all relevant
bits?
[ ... ]
> +static void p3h2x4x_get_target_port_dt_conf(struct device *dev,
> + const struct device_node *node)
> +{
> + struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
> + u64 tp_port;
> +
> + for_each_available_child_of_node_scoped(node, dev_node) {
> + if (of_property_read_reg(dev_node, 0, &tp_port, NULL))
> + continue;
> +
> + if (tp_port < P3H2X4X_TP_MAX_COUNT) {
> + p3h2x4x_i3c_hub->tp_bus[tp_port].of_node = of_node_get(dev_node);
[Severity: Medium]
Is there a resource leak here on probe error paths? This manually increments
the reference count of the child nodes using of_node_get(). While
p3h2x4x_i3c_hub_remove() correctly drops these references, if
p3h2x4x_i3c_hub_probe() fails and returns an error, the remove callback
isn't executed. Without a devm cleanup action, could these OF nodes leak
memory?
[ ... ]
> +static void p3h2x4x_default_configuration(struct device *dev)
> +{
> + struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
> + int tp_count;
> +
> + p3h2x4x_i3c_hub->hub_config.tp0145_pullup = P3H2X4X_TP_PULLUP_500R;
[Severity: Medium]
Are these default configuration values being set correctly? These fields
appear to be designed to store physical Ohm measurements, but they are
initialized to macros like P3H2X4X_TP_PULLUP_500R which evaluate to enum
indices (e.g., 1). When the default value 1 is later processed by
p3h2x4x_pullup_dt_to_reg() using find_closest(), it would resolve to the
closest table entry 250 instead of the intended 500. Should these initialize
with real-world units?
[ ... ]
> +static int p3h2x4x_i3c_hub_probe(struct platform_device *pdev)
> +{
[ ... ]
> + /* Register virtual I2C adapters for SMBus target ports */
> + ret = p3h2x4x_tp_smbus_algo(p3h2x4x_i3c_hub);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to add i2c adapter\n");
> +
> + /* Lock access to protected registers */
> + ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
> + P3H2X4X_REGISTERS_LOCK_CODE);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to lock HUB's protected registers\n");
[Severity: High]
Are the SMBus I2C adapters cleaned up if this final regmap_write() fails?
p3h2x4x_tp_smbus_algo() registers adapters using i2c_add_adapter(), which
requires an explicit i2c_del_adapter(). If the probe fails here, the adapters
are never unregistered, which leaves the I2C core with active references to
devm-freed memory. Could this cause a use-after-free?
> +
> + return 0;
> +}
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> new file mode 100644
> index 0000000000000..38505dda0e81b
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
[ ... ]
> +int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> +{
[ ... ]
> + p3h2x4x_hub->hub = devm_kzalloc(p3h2x4x_hub->dev,
> + sizeof(*p3h2x4x_hub->hub),
> + GFP_KERNEL);
> +
> + if (!p3h2x4x_hub->hub)
> + return -ENOMEM;
> +
> + i3c_hub_init(p3h2x4x_hub->hub,
> + &p3h2x4x_hub_ops,
> + p3h2x4x_hub->i3cdev);
> +
> + if (IS_ERR(p3h2x4x_hub->hub))
> + return PTR_ERR(p3h2x4x_hub->hub);
[Severity: Low]
Is this IS_ERR() check necessary? devm_kzalloc() returns either a valid
pointer or NULL on failure, never an error pointer. It seems this block of
code might be dead.
[ ... ]
> + for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
> + if (!p3h2x4x_hub->tp_bus[tp].of_node ||
> + p3h2x4x_hub->hub_config.tp_config[tp].mode != P3H2X4X_TP_MODE_I3C)
> + continue;
> +
> + /* Assign DT node for this TP */
> + p3h2x4x_hub->dev->of_node = p3h2x4x_hub->tp_bus[tp].of_node;
[Severity: High]
Can dynamically overwriting the platform device's of_node pointer corrupt the
core device tree state? By changing p3h2x4x_hub->dev->of_node in a loop
without saving and restoring the original, it looks like this permanently
changes the node the device points to. Could this break device tree links,
sysfs topologies, or devres lookups that rely on the original parent hub
of_node?
[ ... ]
> + /* Perform DAA */
> + ret = i3c_master_do_daa(parent);
> + if (ret)
> + return ret;
> +
> + ntwk_mask |= p3h2x4x_hub->tp_bus[tp].tp_mask;
> + p3h2x4x_hub->tp_bus[tp].is_registered = true;
> + p3h2x4x_hub->hub_config.tp_config[tp].always_enable = true;
> + }
[ ... ]
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + ret = i3c_device_request_ibi(p3h2x4x_hub->i3cdev, &p3h2x4x_ibireq);
> + if (ret)
> + return ret;
> +
> + ret = i3c_device_enable_ibi(p3h2x4x_hub->i3cdev);
[Severity: High]
If a subsequent step in p3h2x4x_i3c_hub_probe() fails, are the IBIs safely
cleaned up? The devm framework would free the p3h2x4x_i3c_hub structure,
but the IBI might remain active on the master. If an interrupt fires later
and accesses the freed structure via i3cdev_get_drvdata(i3cdev), could this
trigger a use-after-free?
> + if (ret) {
> + i3c_device_free_ibi(p3h2x4x_hub->i3cdev);
> + return ret;
> + }
> +#endif
> + ret = regmap_write(p3h2x4x_hub->regmap, P3H2X4X_TP_NET_CON_CONF, ntwk_mask);
[Severity: High]
Does performing DAA here actually reach the downstream targets? The DAA is
executed earlier on the upstream bus inside the target port loop, but the
downstream target ports don't appear to be logically connected to the upstream
bus until ntwk_mask is written to P3H2X4X_TP_NET_CON_CONF at the end of the
function. Will downstream targets receive the ENTDAA command if DAA is done
before the physical ports are enabled?
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> new file mode 100644
> index 0000000000000..edb75f790e92e
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
[ ... ]
> +static int p3h2x4x_read_smbus_transaction_status(struct p3h2x4x_i3c_hub_dev *hub,
> + u8 target_port_status,
> + u8 data_length)
> +{
> + u32 status_read;
> + u8 status;
> + int ret;
> +
> + fsleep(P3H2X4X_SMBUS_400kHz_TRANSFER_TIMEOUT(data_length));
> +
> + ret = regmap_read(hub->regmap, target_port_status, &status_read);
[Severity: High]
Could relying on a fixed timeout instead of polling the hardware finish flag
cause the driver to read stale data? Because the top 4 bits of the status
register are initialized to 0 before the transaction begins, they remain 0
while the hardware is busy. If the transaction exceeds the timeout (e.g., due
to clock stretching), status evaluates to 0
(P3H2X4X_SMBUS_CNTRL_STATUS_TXN_OK). Should this code poll the
P3H2X4X_SMBUS_TRANSACTION_FINISH_FLAG instead to ensure completion?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=8
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v11 9/9] i3c: hub: p3h2x4x: Add SMBus slave mode support
2026-06-12 11:18 [PATCH v11 0/9] Add support for NXP P3H2x4x I3C hub driver Lakshay Piplani
` (7 preceding siblings ...)
2026-06-12 11:18 ` [PATCH v11 8/9] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality Lakshay Piplani
@ 2026-06-12 11:18 ` Lakshay Piplani
2026-06-12 11:41 ` sashiko-bot
8 siblings, 1 reply; 20+ messages in thread
From: Lakshay Piplani @ 2026-06-12 11:18 UTC (permalink / raw)
To: linux-kernel, linux-i3c, alexandre.belloni, krzk+dt, robh,
conor+dt, devicetree, broonie, lee, Frank.Li, lgirdwood
Cc: vikash.bansal, priyanka.jain, aman.kumarpandey, Lakshay Piplani
Add SMBus slave mode support for the P3H2x4x hub SMBus target ports.
The hub SMBus slave agent can receive downstream payloads into target
buffers and report receive events through IBI. Add CONFIG_I2C_SLAVE
to support the receive path and forward the received payloads to the
registered I2C slave client through i2c_slave_event().
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
Signed-off-by: Vikash Bansal <vikash.bansal@nxp.com>
---
Changes in v11:
- Improve SMBus slave mode payload validation and parsing
Changes in v10:
- Split SMBus slave mode support into a separate patch
---
---
drivers/i3c/hub/p3h2840_i3c_hub.h | 10 ++
drivers/i3c/hub/p3h2840_i3c_hub_common.c | 7 +
drivers/i3c/hub/p3h2840_i3c_hub_i3c.c | 29 +++-
drivers/i3c/hub/p3h2840_i3c_hub_smbus.c | 192 +++++++++++++++++++++++
4 files changed, 237 insertions(+), 1 deletion(-)
diff --git a/drivers/i3c/hub/p3h2840_i3c_hub.h b/drivers/i3c/hub/p3h2840_i3c_hub.h
index d69fafbac584..84d9c66547c6 100644
--- a/drivers/i3c/hub/p3h2840_i3c_hub.h
+++ b/drivers/i3c/hub/p3h2840_i3c_hub.h
@@ -324,4 +324,14 @@ int p3h2x4x_tp_smbus_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub);
*/
int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub);
+/**
+ * p3h2x4x_ibi_handler - IBI handler.
+ * @i3cdev: i3c device.
+ * @payload: two byte IBI payload data.
+ */
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+void p3h2x4x_ibi_handler(struct i3c_device *i3cdev,
+ const struct i3c_ibi_payload *payload);
+#endif
+
#endif /* P3H2840_I3C_HUB_H */
diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_common.c b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
index fedcb978f07e..ca173afe1fa9 100644
--- a/drivers/i3c/hub/p3h2840_i3c_hub_common.c
+++ b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
@@ -318,6 +318,13 @@ static void p3h2x4x_i3c_hub_remove(struct platform_device *pdev)
struct p3h2x4x_dev *p3h2x4x = dev_get_drvdata(pdev->dev.parent);
u8 i;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+ if (p3h2x4x->i3cdev) {
+ i3c_device_disable_ibi(p3h2x4x->i3cdev);
+ i3c_device_free_ibi(p3h2x4x->i3cdev);
+ }
+#endif
+
for (i = 0; i < P3H2X4X_TP_MAX_COUNT; i++) {
if (p3h2x4x_i3c_hub->tp_bus[i].is_registered) {
if (p3h2x4x_i3c_hub->hub_config.tp_config[i].mode ==
diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
index 38505dda0e81..9bef8b8e557f 100644
--- a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
+++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
@@ -10,6 +10,14 @@
#include "p3h2840_i3c_hub.h"
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static const struct i3c_ibi_setup p3h2x4x_ibireq = {
+ .handler = p3h2x4x_ibi_handler,
+ .max_payload_len = P3H2X4X_MAX_PAYLOAD_LEN,
+ .num_slots = P3H2X4X_NUM_SLOTS,
+};
+#endif
+
static inline struct tp_bus *
p3h2x4x_bus_from_controller(struct i3c_master_controller *controller)
{
@@ -120,5 +128,24 @@ int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
p3h2x4x_hub->tp_bus[tp].is_registered = true;
p3h2x4x_hub->hub_config.tp_config[tp].always_enable = true;
}
- return regmap_write(p3h2x4x_hub->regmap, P3H2X4X_TP_NET_CON_CONF, ntwk_mask);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+ ret = i3c_device_request_ibi(p3h2x4x_hub->i3cdev, &p3h2x4x_ibireq);
+ if (ret)
+ return ret;
+
+ ret = i3c_device_enable_ibi(p3h2x4x_hub->i3cdev);
+ if (ret) {
+ i3c_device_free_ibi(p3h2x4x_hub->i3cdev);
+ return ret;
+ }
+#endif
+ ret = regmap_write(p3h2x4x_hub->regmap, P3H2X4X_TP_NET_CON_CONF, ntwk_mask);
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+ if (ret) {
+ i3c_device_disable_ibi(p3h2x4x_hub->i3cdev);
+ i3c_device_free_ibi(p3h2x4x_hub->i3cdev);
+ }
+#endif
+ return ret;
}
diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
index edb75f790e92..fba4345f86a7 100644
--- a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
+++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
@@ -15,6 +15,146 @@ enum p3h2x4x_smbus_desc_idx {
P3H2X4X_DESC_READ_LEN,
};
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static void p3h2x4x_read_smbus_agent_rx_buf(struct i3c_device *i3cdev, enum p3h2x4x_rcv_buf rfbuf,
+ enum p3h2x4x_tp tp, bool is_of)
+{
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = i3cdev_get_drvdata(i3cdev);
+ u8 slave_rx_buffer[P3H2X4X_SMBUS_TARGET_PAYLOAD_SIZE] = { 0 };
+ u8 target_buffer_page, flag_clear = 0x0f, temp = 0, i, addr;
+ u32 packet_len, slave_address, ret;
+ struct i2c_client *client;
+
+ target_buffer_page = (((rfbuf) ? P3H2X4X_TARGET_BUFF_1_PAGE : P3H2X4X_TARGET_BUFF_0_PAGE)
+ + (P3H2X4X_NO_PAGE_PER_TP * tp));
+ ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_PAGE_PTR, target_buffer_page);
+ if (ret)
+ goto ibi_err;
+
+ /* read buffer length */
+ ret = regmap_read(p3h2x4x_i3c_hub->regmap, P3H2X4X_TARGET_BUFF_LENGTH, &packet_len);
+ if (ret)
+ goto ibi_err;
+
+ if (packet_len)
+ packet_len = packet_len - 1;
+
+ if (packet_len > P3H2X4X_SMBUS_TARGET_PAYLOAD_SIZE) {
+ dev_err(&i3cdev->dev, "Received message too big for p3h2x4x buffer\n");
+ goto ibi_err;
+ }
+
+ /* read slave address */
+ ret = regmap_read(p3h2x4x_i3c_hub->regmap, P3H2X4X_TARGET_BUFF_ADDRESS, &slave_address);
+ if (ret)
+ goto ibi_err;
+
+ /* read data */
+ if (packet_len) {
+ ret = regmap_bulk_read(p3h2x4x_i3c_hub->regmap, P3H2X4X_TARGET_BUFF_DATA,
+ slave_rx_buffer, packet_len);
+ if (ret)
+ goto ibi_err;
+ }
+
+ if (is_of)
+ flag_clear = BUF_RECEIVED_FLAG_TF_MASK;
+ else
+ flag_clear = (((rfbuf == RCV_BUF_0) ? P3H2X4X_TARGET_BUF_0_RECEIVE :
+ P3H2X4X_TARGET_BUF_1_RECEIVE));
+
+ /* notify slave driver about received data */
+ if ((p3h2x4x_i3c_hub->tp_bus[tp].tp_smbus_client->addr & 0x7f) == (slave_address >> 1)) {
+ client = p3h2x4x_i3c_hub->tp_bus[tp].tp_smbus_client;
+ if (!client)
+ goto ibi_err;
+
+ addr = slave_address >> 1;
+ i2c_slave_event(client,
+ I2C_SLAVE_WRITE_REQUESTED, &addr);
+ for (i = 0; i < packet_len; i++) {
+ temp = slave_rx_buffer[i];
+ i2c_slave_event(client,
+ I2C_SLAVE_WRITE_RECEIVED, &temp);
+ }
+ i2c_slave_event(client, I2C_SLAVE_STOP, &temp);
+ }
+
+ibi_err:
+ regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_PAGE_PTR, 0x00);
+ regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP0_SMBUS_AGNT_STS + tp, flag_clear);
+}
+
+/**
+ * p3h2x4x_ibi_handler - IBI handler.
+ * @i3cdev: i3c device.
+ * @payload: two byte IBI payload data.
+ *
+ */
+void p3h2x4x_ibi_handler(struct i3c_device *i3cdev,
+ const struct i3c_ibi_payload *payload)
+{
+ struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub;
+ u8 payload_byte_one, payload_byte_two;
+ u32 target_port_status, ret, i;
+ const u8 *data;
+
+ if (!payload || payload->len < P3H2X4X_MAX_PAYLOAD_LEN)
+ return;
+
+ data = payload->data;
+ payload_byte_one = data[0];
+
+ if (!(payload_byte_one & P3H2X4X_SMBUS_AGENT_EVENT_FLAG_STATUS))
+ return;
+
+ p3h2x4x_i3c_hub = i3cdev_get_drvdata(i3cdev);
+
+ if (!p3h2x4x_i3c_hub || !p3h2x4x_i3c_hub->regmap)
+ return;
+
+ payload_byte_two = data[1];
+ guard(mutex)(&p3h2x4x_i3c_hub->etx_mutex);
+
+ for (i = 0; i < P3H2X4X_TP_MAX_COUNT; ++i) {
+ if (p3h2x4x_i3c_hub->tp_bus[i].is_registered && (payload_byte_two >> i) & 0x01) {
+ ret = regmap_read(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP0_SMBUS_AGNT_STS + i,
+ &target_port_status);
+ if (ret) {
+ dev_err(&i3cdev->dev, "target port read status failed %d\n", ret);
+ return;
+ }
+
+ /* process data receive buffer */
+ switch (target_port_status & BUF_RECEIVED_FLAG_MASK) {
+ case P3H2X4X_TARGET_BUF_CA_TF:
+ break;
+ case P3H2X4X_TARGET_BUF_0_RECEIVE:
+ p3h2x4x_read_smbus_agent_rx_buf(i3cdev, RCV_BUF_0, i, false);
+ break;
+ case P3H2X4X_TARGET_BUF_1_RECEIVE:
+ p3h2x4x_read_smbus_agent_rx_buf(i3cdev, RCV_BUF_1, i, false);
+ break;
+ case P3H2X4X_TARGET_BUF_0_1_RECEIVE:
+ p3h2x4x_read_smbus_agent_rx_buf(i3cdev, RCV_BUF_0, i, false);
+ p3h2x4x_read_smbus_agent_rx_buf(i3cdev, RCV_BUF_1, i, false);
+ break;
+ case P3H2X4X_TARGET_BUF_OVRFL:
+ p3h2x4x_read_smbus_agent_rx_buf(i3cdev, RCV_BUF_0, i, false);
+ p3h2x4x_read_smbus_agent_rx_buf(i3cdev, RCV_BUF_1, i, true);
+ dev_err(&i3cdev->dev, "Overflow, reading buffer zero and one\n");
+ break;
+ default:
+ regmap_write(p3h2x4x_i3c_hub->regmap,
+ P3H2X4X_TP0_SMBUS_AGNT_STS + i,
+ BUF_RECEIVED_FLAG_TF_MASK);
+ break;
+ }
+ }
+ }
+}
+#endif
+
static int p3h2x4x_read_smbus_transaction_status(struct p3h2x4x_i3c_hub_dev *hub,
u8 target_port_status,
u8 data_length)
@@ -205,11 +345,63 @@ static u32 p3h2x4x_tp_smbus_funcs(struct i2c_adapter *adapter)
return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BLOCK_DATA;
}
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static int p3h2x4x_tp_i2c_reg_slave(struct i2c_client *slave)
+{
+ struct tp_bus *bus = i2c_get_adapdata(slave->adapter);
+ struct p3h2x4x_i3c_hub_dev *hub = bus->p3h2x4x_i3c_hub;
+ int ret;
+
+ guard(mutex)(&hub->etx_mutex);
+
+ if (bus->tp_smbus_client)
+ return -EBUSY;
+
+ ret = regmap_set_bits(hub->regmap,
+ P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG,
+ bus->tp_mask);
+ if (ret)
+ return ret;
+
+ bus->tp_smbus_client = slave;
+ hub->hub_config.tp_config[bus->tp_port].ibi_en = true;
+
+ return 0;
+}
+
+static int p3h2x4x_tp_i2c_unreg_slave(struct i2c_client *slave)
+{
+ struct tp_bus *bus = i2c_get_adapdata(slave->adapter);
+ struct p3h2x4x_i3c_hub_dev *hub = bus->p3h2x4x_i3c_hub;
+ int ret;
+
+ guard(mutex)(&hub->etx_mutex);
+
+ if (bus->tp_smbus_client != slave)
+ return -EINVAL;
+
+ ret = regmap_clear_bits(hub->regmap,
+ P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG,
+ bus->tp_mask);
+ if (ret)
+ return ret;
+
+ bus->tp_smbus_client = NULL;
+ hub->hub_config.tp_config[bus->tp_port].ibi_en = false;
+
+ return 0;
+}
+#endif
+
/*
* I2C algorithm Structure
*/
static struct i2c_algorithm p3h2x4x_tp_i2c_algorithm = {
.master_xfer = p3h2x4x_tp_i2c_xfer,
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+ .reg_slave = p3h2x4x_tp_i2c_reg_slave,
+ .unreg_slave = p3h2x4x_tp_i2c_unreg_slave,
+#endif
.functionality = p3h2x4x_tp_smbus_funcs,
};
--
2.25.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v11 9/9] i3c: hub: p3h2x4x: Add SMBus slave mode support
2026-06-12 11:18 ` [PATCH v11 9/9] i3c: hub: p3h2x4x: Add SMBus slave mode support Lakshay Piplani
@ 2026-06-12 11:41 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-12 11:41 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: robh, devicetree, linux-i3c, conor+dt, Frank.Li
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] Unconditional dereference of `tp_smbus_client` prior to its NULL check in `p3h2x4x_read_smbus_agent_rx_buf`.
- [High] Failure to clear `tp_smbus_client` on hardware error in `p3h2x4x_tp_i2c_unreg_slave` leads to Use-After-Free.
- [High] IBI requests are not cleaned up on the `probe` error path, leaving active interrupts pointing to freed device memory.
- [High] Early return on `regmap_read` failure in `p3h2x4x_ibi_handler` aborts processing for other ports, risking an interrupt storm.
Pre-existing issues:
- [High] I2C adapters registered in `p3h2x4x_tp_smbus_algo` are leaked on subsequent probe errors, leading to Use-After-Free.
--
commit c3438427374cdabfa4f0a09443f69b5b076d009a
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
i3c: hub: p3h2x4x: Add SMBus slave mode support
Add SMBus slave mode support for the P3H2x4x hub SMBus target ports,
allowing the hub to receive downstream payloads into target buffers and
report receive events through IBI to registered I2C slave clients.
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
[ ... ]
> @@ -120,5 +128,24 @@ int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> p3h2x4x_hub->tp_bus[tp].is_registered = true;
> p3h2x4x_hub->hub_config.tp_config[tp].always_enable = true;
> }
> - return regmap_write(p3h2x4x_hub->regmap, P3H2X4X_TP_NET_CON_CONF, ntwk_mask);
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + ret = i3c_device_request_ibi(p3h2x4x_hub->i3cdev, &p3h2x4x_ibireq);
> + if (ret)
> + return ret;
> +
> + ret = i3c_device_enable_ibi(p3h2x4x_hub->i3cdev);
[Severity: High]
Because these IBI functions are not devres-managed, do we need to manually
clean them up on a probe failure?
If a subsequent step in p3h2x4x_i3c_hub_probe() fails after this completes,
probe will abort and devres will free the p3h2x4x_i3c_hub structure. Since
the I3C core isn't informed to free the IBI, the hardware remains
configured. If an IBI fires later, could p3h2x4x_ibi_handler() try to
dereference the freed driver data?
[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
[ ... ]
> @@ -15,6 +15,146 @@ enum p3h2x4x_smbus_desc_idx {
> P3H2X4X_DESC_READ_LEN,
> };
>
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static void p3h2x4x_read_smbus_agent_rx_buf(struct i3c_device *i3cdev, enum p3h2x4x_rcv_buf rfbuf,
> + enum p3h2x4x_tp tp, bool is_of)
> +{
[ ... ]
> + /* notify slave driver about received data */
> + if ((p3h2x4x_i3c_hub->tp_bus[tp].tp_smbus_client->addr & 0x7f) == (slave_address >> 1)) {
> + client = p3h2x4x_i3c_hub->tp_bus[tp].tp_smbus_client;
> + if (!client)
> + goto ibi_err;
[Severity: High]
Can this code cause a panic if an In-Band Interrupt occurs for a port
without a bound SMBus slave client?
We dereference tp_smbus_client->addr unconditionally here, right before
the NULL check happens.
[ ... ]
> +void p3h2x4x_ibi_handler(struct i3c_device *i3cdev,
> + const struct i3c_ibi_payload *payload)
> +{
[ ... ]
> + for (i = 0; i < P3H2X4X_TP_MAX_COUNT; ++i) {
> + if (p3h2x4x_i3c_hub->tp_bus[i].is_registered && (payload_byte_two >> i) & 0x01) {
> + ret = regmap_read(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP0_SMBUS_AGNT_STS + i,
> + &target_port_status);
> + if (ret) {
> + dev_err(&i3cdev->dev, "target port read status failed %d\n", ret);
> + return;
> + }
[Severity: High]
If regmap_read() fails for one port, this return statement prematurely
exits the handler.
Does this skip clearing the hardware status of the current port and prevent
servicing other ports with pending events? If the underlying hardware
interrupt conditions are not cleared, could the device immediately re-assert
the IBI line and cause an interrupt storm?
[ ... ]
> +static int p3h2x4x_tp_i2c_unreg_slave(struct i2c_client *slave)
> +{
[ ... ]
> + ret = regmap_clear_bits(hub->regmap,
> + P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG,
> + bus->tp_mask);
> + if (ret)
> + return ret;
> +
> + bus->tp_smbus_client = NULL;
[Severity: High]
If regmap_clear_bits() fails, we return the error without clearing
bus->tp_smbus_client.
Since the client driver might still free its i2c_client structure during
removal despite the unregistration failure, could this leave a dangling
pointer that gets dereferenced if an IBI fires later?
[ ... ]
> @@ -205,11 +345,63 @@ static u32 p3h2x4x_tp_smbus_funcs(struct i2c_adapter *adapter)
> return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BLOCK_DATA;
> }
>
[ ... ]
> /*
> * I2C algorithm Structure
> */
> static struct i2c_algorithm p3h2x4x_tp_i2c_algorithm = {
> .master_xfer = p3h2x4x_tp_i2c_xfer,
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + .reg_slave = p3h2x4x_tp_i2c_reg_slave,
> + .unreg_slave = p3h2x4x_tp_i2c_unreg_slave,
> +#endif
> .functionality = p3h2x4x_tp_smbus_funcs,
> };
[Severity: High]
This is a pre-existing issue, but I2C adapters registered in
p3h2x4x_tp_smbus_algo() lack a devres action for automatic cleanup.
If p3h2x4x_i3c_hub_probe() encounters an error after adapters are added
with i2c_add_adapter(), devres will free the memory backing the adapters,
but they remain registered with the I2C subsystem.
Could this lead to a use-after-free if the I2C core accesses these ghost
adapters?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=9
^ permalink raw reply [flat|nested] 20+ messages in thread