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

[Mario Limonciello (AMD) (kernel.org)]

On 10/14/2025 3:33 PM, Bjorn Helgaas wrote:
> [+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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx
>
> FWIW,
> Reviewed-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

I echo Bjorn's comments on the lengthy commit message.
Code change looks fine.

Reviewed-by: Mario Limonciello (AMD) <superm1@xxxxxxxxxx>

> > ---
> >   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@xxxxxxxxxx>