Re: [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support()

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Mario, author of 440da737cf8d ("i2c: designware: Use PCI PSP
driver for communication")]

On Mon, Oct 13, 2025 at 06:05:03PM -0700, Nathan Chancellor wrote:
> When building certain configurations with CONFIG_FINEIBT=y after
> commit 894af4a1cde6 ("objtool: Validate kCFI calls"), there is a
> warning due to an indirect call in dw_i2c_plat_remove():
> 
>   $ cat allno.config
>   CONFIG_ACPI=y
>   CONFIG_CFI=y
>   CONFIG_COMMON_CLK=y
>   CONFIG_CPU_MITIGATIONS=y
>   CONFIG_I2C=y
>   CONFIG_I2C_DESIGNWARE_BAYTRAIL=y
>   CONFIG_I2C_DESIGNWARE_CORE=y
>   CONFIG_I2C_DESIGNWARE_PLATFORM=y
>   CONFIG_IOSF_MBI=y
>   CONFIG_MITIGATION_RETPOLINE=y
>   CONFIG_MODULES=y
>   CONFIG_PCI=y
>   CONFIG_X86_KERNEL_IBT=y
> 
>   $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 clean allnoconfig vmlinux
>   vmlinux.o: warning: objtool: dw_i2c_plat_remove+0x3c: no-cfi indirect call!
> 
> With this configuration, i2c_dw_semaphore_cb_table has the BAYTRAIL
> member and the sentinel (i.e., 2 members), both of which have an
> implicit
> 
>   .remove = NULL,
> 
> so Clang effectively turns i2c_dw_remove_lock_support(), which is later
> inlined into dw_i2c_plat_remove(), into:
> 
>   static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
>   {
>       if (dev->semaphore_idx > 2)
>           (*NULL)(dev):
>   }
> 
> which is not necessarily problematic from a logic perspective (as the
> code was not bounds checking semaphore_idx so an out of bounds index
> could already crash) but objtool's new __nocfi indirect call checking
> trips over Clang dropping the kCFI setup from a known NULL indirect
> call.
> 
> While it would be possible to fix this by transforming the initial check
> into
> 
>   if (dev->semaphore_idx < 0 || dev->semaphore_idx >= ARRAY_SIZE(i2c_dw_semaphore_cb_table))
> 
> the remove member is unused after commit 440da737cf8d ("i2c: designware:
> Use PCI PSP driver for communication"), so i2c_dw_remove_lock_support()
> can be removed altogether, as it will never actually do anything.
> 
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2133
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

I'm totally fine with the patch itself, but I think the commit log
could be trimmed to something like the following with no loss:

  Remove struct i2c_dw_semaphore_callbacks.remove() and
  i2c_dw_remove_lock_support().

  440da737cf8d ("i2c: designware: Use PCI PSP driver for
  communication") removed the last place that set
  i2c_dw_semaphore_callbacks.remove(), which made
  i2c_dw_remove_lock_support() a no-op.

  This has the side effect of avoiding this kCFI warning (see Link):

    dw_i2c_plat_remove+0x3c: no-cfi indirect call!

  Link: https://lore.kernel.org/r/20251013-dw_i2c_plat_remove-avoid-objtool-no-cfi-warning-v1-1-8cc4842967bf@kernel.org

FWIW,
Reviewed-by: Bjorn Helgaas <bhelgaas@google.com> 

> ---
>  drivers/i2c/busses/i2c-designware-core.h    |  1 -
>  drivers/i2c/busses/i2c-designware-platdrv.c | 11 -----------
>  2 files changed, 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 347843b4f5dd..d50664377c6b 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -330,7 +330,6 @@ struct dw_i2c_dev {
>  
>  struct i2c_dw_semaphore_callbacks {
>  	int	(*probe)(struct dw_i2c_dev *dev);
> -	void	(*remove)(struct dw_i2c_dev *dev);
>  };
>  
>  int i2c_dw_init_regmap(struct dw_i2c_dev *dev);
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 34d881572351..cff7e03dea7b 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -197,15 +197,6 @@ static int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
>  	return 0;
>  }
>  
> -static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
> -{
> -	if (dev->semaphore_idx < 0)
> -		return;
> -
> -	if (i2c_dw_semaphore_cb_table[dev->semaphore_idx].remove)
> -		i2c_dw_semaphore_cb_table[dev->semaphore_idx].remove(dev);
> -}
> -
>  static int dw_i2c_plat_probe(struct platform_device *pdev)
>  {
>  	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> @@ -339,8 +330,6 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
>  
>  	i2c_dw_prepare_clk(dev, false);
>  
> -	i2c_dw_remove_lock_support(dev);
> -
>  	reset_control_assert(dev->rst);
>  }
>  
> 
> ---
> base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
> change-id: 20251013-dw_i2c_plat_remove-avoid-objtool-no-cfi-warning-5f2040eaadc2
> 
> Best regards,
> --  
> Nathan Chancellor <nathan@kernel.org>
>