Re: [PATCH net-next 2/3] i2c: xiic: add KUnit coverage for register access modes

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new KUnit test xiic_register_access() in
  drivers/i2c/busses/tests/i2c_xiic_kunit.c takes a kunit_kmalloc() RAM…

--

i2c: xiic: add KUnit coverage for register access modes

This adds a KUnit suite that is included in the i2c-xiic.c translation
unit. It drives the private xiic_setreg*() and xiic_getreg*() accessors
against a kmalloc'd buffer that stands in for MMIO. It covers native and
forced 32-bit accesses in both byte orders across 22 register offsets. The
new I2C_XILINX_KUNIT_TEST symbol enables it and defaults to
KUNIT_ALL_TESTS.

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 275aaa7f35833..014281fa2be5c 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1322,6 +1322,20 @@ config I2C_XILINX
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called xilinx_i2c.
>  
> +config I2C_XILINX_KUNIT_TEST
> +	bool "KUnit tests for Xilinx I2C register accesses" if !KUNIT_ALL_TESTS
> +	depends on I2C_XILINX && KUNIT=y
> +	default KUNIT_ALL_TESTS
> +	help
> +	  Test the Xilinx I2C driver's register accessors with both byte orders
> +	  and with native-width or forced 32-bit accesses. KUnit is a kernel
> +	  unit testing framework. These tests use memory-backed registers and
> +	  do not require an I2C controller.

[ ... ]

> diff --git a/drivers/i2c/busses/tests/i2c_xiic_kunit.c b/drivers/i2c/busses/tests/i2c_xiic_kunit.c
> new file mode 100644
> index 0000000000000..f8df88ca65124
> --- /dev/null
> +++ b/drivers/i2c/busses/tests/i2c_xiic_kunit.c
> @@ -0,0 +1,88 @@

[ ... ]

> +	i2c = kunit_kzalloc(test, sizeof(*i2c), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, i2c);
> +	mem = kunit_kmalloc(test, 0x200, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, mem);
> +	/* Aligned RAM stands in for MMIO. No hardware is accessed. */
> +	i2c->base = (void __force __iomem *)(mem + sizeof(u32));

[Severity: Medium]
Is it safe to pass a kmalloc'd buffer to ioread*() and iowrite*() on every
architecture that can build this test?

This works where the MMIO accessors are plain loads and stores to the
virtual address, as on the x86-64 setup the changelog says was tested.

However, I2C_XILINX_KUNIT_TEST only has "depends on I2C_XILINX && KUNIT=y"
with "default KUNIT_ALL_TESTS". I2C_XILINX only needs HAS_IOMEM. That means
the suite is built and run automatically on architectures where those
accessors don't touch RAM.

On sparc64, arch/sparc/include/asm/io_64.h implements readb()/readl()/
writeb()/writel(), and __raw_readl()/__raw_writel() for the be variants,
with lduba/lduwa/stba/stwa and ASI_PHYS_BYPASS_EC_E_L:

    __asm__ __volatile__("lduwa\t[%1] %2, %0\t/* pci_readl */" ...

That uses the slab virtual address as a physical address. Could this trap?
Could it instead write 0xc0, 0x03ff, 0x12345678 and 0x89abcdef to an
unrelated physical location?

On s390 with CONFIG_PCI, __raw_readl() and friends map to zpci_read_u32()
and the other zpci helpers. On the non-MIO path, zpci_load_fh() and
zpci_store_fh() do:

    struct zpci_iomap_entry *entry = &zpci_iomap_start[ZPCI_IDX(addr)];

using bits from the fake address. They then issue a real PCI load or store
with whatever function handle and BAR they find there. On the MIO path,
PCI load/store instructions are issued against a RAM address.

On UML with INDIRECT_IOMEM, the chain is KUNIT_UML_PCI -> UML_PCI ->
UML_IOMEM_EMULATION -> INDIRECT_IOMEM. The default
tools/testing/kunit/configs/arch_uml.config sets CONFIG_KUNIT_UML_PCI=y.
For a slab address, get_area() in lib/logic_iomem.c hits:

    if (WARN_ON((a & IOREMAP_MASK) != IOREMAP_BIAS))
        return NULL;

Nothing selects INDIRECT_IOMEM_FALLBACK. So the fallback path WARNs again,
reads return ~0 and writes are dropped.

Wouldn't that give two WARN splats per access, across all 4 parameters and
22 offsets? It would also fail every expectation, and panic if
panic_on_warn is set.

These three statements don't hold on those architectures:

  - the Kconfig help: "These tests use memory-backed registers and do not
    require an I2C controller"
  - the comment above: "No hardware is accessed."
  - the changelog: "memory-backed registers instead of physical hardware"

Should I2C_XILINX_KUNIT_TEST get an architecture restriction, for example
excluding SPARC64, S390 and INDIRECT_IOMEM, or using an allow-list? Or
could the test go through an indirection it can mock, rather than passing
RAM to the MMIO accessors?

> +	i2c->endianness = param->endianness;
> +	i2c->force_32bit_access = param->force_32bit_access;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111041904.48916-1-ahmadexp%40gmail.com