Re: [PATCH 1/2] iosys-map: Add getter functions for virtual addresses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] iosys_map_get_vaddr_iomem() unconditionally triggers a kernel warning (WARN_ON) when called on an unset mapping, contradicting its documentation.
- [Low] The patch adds `iosys_map_get_vaddr` to `include/linux/iosys-map.h` but fails to remove the pre-existing static inline definition in `drivers/firmware/tegra/ivc.c`, causing a build failure.
- [Low] Missing <linux/bug.h> include for WARN_ON(), leading to potential build breaks.
--

--- Patch [1]: [PATCH 1/2] iosys-map: Add getter functions for virtual addresses ---
commit f14cef96efd32780d25ba0c941cea2c91611ddcd
Author: Shixiong Ou <oushixiong@kylinos.cn>

iosys-map: Add getter functions for virtual addresses

Add iosys_map_get_vaddr() and iosys_map_get_vaddr_iomem() as the
counterparts to iosys_map_set_vaddr() and iosys_map_set_vaddr_iomem().

This allows drivers to use the iosys_map abstraction layer properly
instead of directly accessing the structure members, improving code
maintainability and safety.

> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
> index 3e85afe794c0a..658d5038498a7 100644
> --- a/include/linux/iosys-map.h
> +++ b/include/linux/iosys-map.h
> @@ -200,6 +200,42 @@ static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
>  	map->is_iomem = true;
>  }
>  
> +/**
> + * iosys_map_get_vaddr - Gets the virtual address of a system-memory mapping
> + * @map:	The iosys_map structure
> + *
> + * Returns the system-memory address. The caller must ensure the mapping
> + * refers to system memory (not I/O memory).
> + *
> + * Returns:
> + * The system-memory address, or NULL if the mapping is not set or refers
> + * to I/O memory.
> + */
> +static inline void *iosys_map_get_vaddr(const struct iosys_map *map)
> +{
> +	if (WARN_ON(map->is_iomem))
> +		return NULL;

[Severity: Low]
Does this new usage of WARN_ON() require adding an include for <linux/bug.h>?

Files including <linux/iosys-map.h> that don't already include <linux/bug.h>
might fail to compile due to implicit declaration of the function.

[Severity: Low]
Could this conflict with the existing static inline definition of
iosys_map_get_vaddr() in drivers/firmware/tegra/ivc.c?

drivers/firmware/tegra/ivc.c already defines this function locally:

static inline void *iosys_map_get_vaddr(const struct iosys_map *map)
{
	if (WARN_ON(map->is_iomem))
		return NULL;
	return map->vaddr;
}

It appears drivers/firmware/tegra/ivc.c might encounter a build failure due
to function redefinition since it includes <linux/iosys-map.h> through
<soc/tegra/ivc.h>.

> +	return map->vaddr;
> +}
> +
> +/**
> + * iosys_map_get_vaddr_iomem - Gets the virtual address of an I/O-memory mapping
> + * @map:	The iosys_map structure
> + *
> + * Returns the I/O-memory address. The caller must ensure the mapping
> + * refers to I/O memory (not system memory).
> + *
> + * Returns:
> + * The I/O-memory address, or NULL if the mapping is not set or refers
> + * to system memory.
> + */
> +static inline void __iomem *iosys_map_get_vaddr_iomem(const struct iosys_map *map)
> +{
> +	if (WARN_ON(!map->is_iomem))

[Severity: Medium]
Will this unconditionally trigger a kernel warning for an unset mapping?

The kernel-doc for this function says it returns NULL if the mapping is not
set. However, if iosys_map_clear() is called, the structure is zeroed and
map->is_iomem becomes false.

Calling iosys_map_get_vaddr_iomem() on this cleared mapping would then evaluate
!map->is_iomem as true and trigger the WARN_ON.

> +		return NULL;
> +	return map->vaddr_iomem;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724093808.776138-1-oushixiong1025@163.com?part=1