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 <[email protected]> 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/[email protected]?part=1
