linux-kernel - Re: [PATCH] i3c: master: Remove i3c_device_free_ibi from i3c_device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aRYLc/+KAD13g7T7@lizhi-Precision-Tower-5810>
Date: Thu, 13 Nov 2025 11:46:43 -0500
From: Frank Li <Frank.li@....com>
To: Jorge Marques <jorge.marques@...log.com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
	linux-i3c@...ts.infradead.org, linux-kernel@...r.kernel.org,
	gastmaier@...il.com
Subject: Re: [PATCH] i3c: master: Remove i3c_device_free_ibi from
 i3c_device_remove

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@...log.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@...log.com>
>