* [PATCH] i3c: master: Remove i3c_device_free_ibi from i3c_device_remove
@ 2025-11-12 21:30 Jorge Marques
2025-11-13 16:46 ` Frank Li
0 siblings, 1 reply; 2+ messages in thread
From: Jorge Marques @ 2025-11-12 21:30 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li
Cc: linux-i3c, linux-kernel, gastmaier, Jorge Marques
i3c_device_disable_ibi should be called before i3c_device_free_ibi,
however, a driver using devm actions cannot yield the call before the
bus_type.remove(), requiring to use a .remove method that is usually
discouraged for drivers that uses resources already manage. Since the
only consumer mctp-i3c.c of this method calls both
i3c_device_disable_ibi then i3c_device_free_ibi, remove the call from
the i3c_device_remove (bus_type.remove()).
Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
As is, if i3c_device_free_ibi is called before i3c_device_disable_ibi,
the following exception occurs:
------------[ cut here ]------------
WARNING: CPU: 0 PID: 12950 at drivers/i3c/master.c:3119 i3c_dev_free_ibi_locked+0x84/0xb0
Modules linked in: driver(-) at24 regmap_i2c regmap_i3c adi_i3c_master nvmem_axi_sysid
CPU: 0 UID: 0 PID: 12950 Comm: modprobe Not tainted 6.12.0+ #1
Hardware name: Xilinx Zynq Platform
Call trace:
unwind_backtrace from show_stack+0x10/0x14
show_stack from dump_stack_lvl+0x54/0x68
dump_stack_lvl from __warn+0x7c/0xe0
__warn from warn_slowpath_fmt+0x1b4/0x1bc
warn_slowpath_fmt from i3c_dev_free_ibi_locked+0x84/0xb0
i3c_dev_free_ibi_locked from i3c_device_free_ibi+0x2c/0x44
i3c_device_free_ibi from device_release_driver_internal+0x184/0x1f8
device_release_driver_internal from driver_detach+0x44/0x80
driver_detach from bus_remove_driver+0x58/0xa4
bus_remove_driver from sys_delete_module+0x188/0x274
sys_delete_module from ret_fast_syscall+0x0/0x54
Exception stack(0xe0d09fa8 to 0xe0d09ff0)
9fa0: 004a2438 004a2438 004a2474 00000800 00000000 00000000
9fc0: 004a2438 004a2438 00000000 00000081 00000000 004a2438 00000000 00000000
9fe0: 0049fe10 bed2b73c 0047f0a5 b6c26168
---[ end trace 0000000000000000 ]---
Forcing drivers to set a .remove method instead of the wrapper
devm_add_action_or_reset, due to the call order.
I believe the best ergonomics is: if the driver requested and enabled
the ibi, the driver is responsible to disable and free ibi.
The usage looks like this:
static void driver_remove_ibi(void *data)
{
struct i3c_device *i3cdev = data;
i3c_device_disable_ibi(i3cdev);
i3c_device_free_ibi(i3cdev);
}
static int driver_request_ibi(struct i3c_device *i3cdev)
{
const struct i3c_ibi_setup ibireq = {
.max_payload_len = 1,
.num_slots = 1,
.handler = driver_ibi_handler,
};
int ret;
ret = i3c_device_request_ibi(i3cdev, &ibireq);
if (ret)
return ret;
ret = i3c_device_enable_ibi(i3cdev);
if (ret)
goto err_enable_ibi;
return devm_add_action_or_reset(&i3cdev->dev, driver_remove_ibi, i3cdev);
err_enable_ibi:
i3c_device_free_ibi(i3cdev);
return ret;
}
Best regards,
---
drivers/i3c/master.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 66513a27e6e7..a0fe00e2487c 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -334,8 +334,6 @@ static void i3c_device_remove(struct device *dev)
if (driver->remove)
driver->remove(i3cdev);
-
- i3c_device_free_ibi(i3cdev);
}
const struct bus_type i3c_bus_type = {
---
base-commit: ddb37d5b130e173090c861b4d1c20a632fb49d7a
change-id: 20251112-ibi-unsafe-48f343e178b8
Best regards,
--
Jorge Marques <jorge.marques@analog.com>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH] i3c: master: Remove i3c_device_free_ibi from i3c_device_remove
2025-11-12 21:30 [PATCH] i3c: master: Remove i3c_device_free_ibi from i3c_device_remove Jorge Marques
@ 2025-11-13 16:46 ` Frank Li
0 siblings, 0 replies; 2+ messages in thread
From: Frank Li @ 2025-11-13 16:46 UTC (permalink / raw)
To: Jorge Marques; +Cc: Alexandre Belloni, linux-i3c, linux-kernel, gastmaier
On Wed, Nov 12, 2025 at 10:30:00PM +0100, Jorge Marques wrote:
> i3c_device_disable_ibi should be called before i3c_device_free_ibi,
> however, a driver using devm actions cannot yield the call before the
> bus_type.remove(), requiring to use a .remove method that is usually
> discouraged for drivers that uses resources already manage. Since the
> only consumer mctp-i3c.c of this method calls both
> i3c_device_disable_ibi then i3c_device_free_ibi, remove the call from
> the i3c_device_remove (bus_type.remove()).
i3c: master: Remove i3c_device_free_ibi() call from i3c_device_remove()
The IBI management functions "i3c_device_request_ibi()/i3c_device_free_ibi(),
i3c_device_enable_ibi()/i3c_device_disable_ibi()" should be handled by
individual I3C device drivers rather than the core framework.
Currently, i3c_device_free_ibi() is unconditionally called in the framework’s
i3c_device_remove() function, even if i3c_device_enable_ibi() was never called.
Although this has no functional impact (the function checks for NULL), these
calls should be symmetric and managed by the driver.
At present, only mctp-i3c.c uses IBI, and it already handles both
i3c_device_disable_ibi() and i3c_device_free_ibi() properly in its remove path.
Therefore, it is safe to remove the redundant i3c_device_free_ibi() call from
the framework.
>
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
> As is, if i3c_device_free_ibi is called before i3c_device_disable_ibi,
> the following exception occurs:
>
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 12950 at drivers/i3c/master.c:3119 i3c_dev_free_ibi_locked+0x84/0xb0
> Modules linked in: driver(-) at24 regmap_i2c regmap_i3c adi_i3c_master nvmem_axi_sysid
> CPU: 0 UID: 0 PID: 12950 Comm: modprobe Not tainted 6.12.0+ #1
> Hardware name: Xilinx Zynq Platform
> Call trace:
> unwind_backtrace from show_stack+0x10/0x14
> show_stack from dump_stack_lvl+0x54/0x68
> dump_stack_lvl from __warn+0x7c/0xe0
> __warn from warn_slowpath_fmt+0x1b4/0x1bc
> warn_slowpath_fmt from i3c_dev_free_ibi_locked+0x84/0xb0
> i3c_dev_free_ibi_locked from i3c_device_free_ibi+0x2c/0x44
> i3c_device_free_ibi from device_release_driver_internal+0x184/0x1f8
> device_release_driver_internal from driver_detach+0x44/0x80
> driver_detach from bus_remove_driver+0x58/0xa4
> bus_remove_driver from sys_delete_module+0x188/0x274
> sys_delete_module from ret_fast_syscall+0x0/0x54
> Exception stack(0xe0d09fa8 to 0xe0d09ff0)
> 9fa0: 004a2438 004a2438 004a2474 00000800 00000000 00000000
> 9fc0: 004a2438 004a2438 00000000 00000081 00000000 004a2438 00000000 00000000
> 9fe0: 0049fe10 bed2b73c 0047f0a5 b6c26168
> ---[ end trace 0000000000000000 ]---
>
> Forcing drivers to set a .remove method instead of the wrapper
> devm_add_action_or_reset, due to the call order.
>
> I believe the best ergonomics is: if the driver requested and enabled
> the ibi, the driver is responsible to disable and free ibi.
I agree. it'd better add devm_* help functions, simpify more error handle
patch.
Frank
>
> The usage looks like this:
>
> static void driver_remove_ibi(void *data)
> {
> struct i3c_device *i3cdev = data;
>
> i3c_device_disable_ibi(i3cdev);
> i3c_device_free_ibi(i3cdev);
> }
>
> static int driver_request_ibi(struct i3c_device *i3cdev)
> {
> const struct i3c_ibi_setup ibireq = {
> .max_payload_len = 1,
> .num_slots = 1,
> .handler = driver_ibi_handler,
> };
> int ret;
>
> ret = i3c_device_request_ibi(i3cdev, &ibireq);
> if (ret)
> return ret;
>
> ret = i3c_device_enable_ibi(i3cdev);
> if (ret)
> goto err_enable_ibi;
>
> return devm_add_action_or_reset(&i3cdev->dev, driver_remove_ibi, i3cdev);
>
> err_enable_ibi:
> i3c_device_free_ibi(i3cdev);
> return ret;
> }
>
> Best regards,
> ---
> drivers/i3c/master.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 66513a27e6e7..a0fe00e2487c 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -334,8 +334,6 @@ static void i3c_device_remove(struct device *dev)
>
> if (driver->remove)
> driver->remove(i3cdev);
> -
> - i3c_device_free_ibi(i3cdev);
> }
>
> const struct bus_type i3c_bus_type = {
>
> ---
> base-commit: ddb37d5b130e173090c861b4d1c20a632fb49d7a
> change-id: 20251112-ibi-unsafe-48f343e178b8
>
> Best regards,
> --
> Jorge Marques <jorge.marques@analog.com>
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-11-13 16:46 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-12 21:30 [PATCH] i3c: master: Remove i3c_device_free_ibi from i3c_device_remove Jorge Marques
2025-11-13 16:46 ` Frank Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).