Re: [PATCH 1/9] iosys-map: add new accessor interfaces and use them internally.

[Lucas De Marchi]

On Thu, May 22, 2025 at 01:58:54PM +0200, Thomas Zimmermann wrote:

Am 22.05.25 um 08:52 schrieb Dave Airlie:

From: Dave Airlie <airl...@redhat.com>

This adds accessors inlines to the iosys-map. The intent is to
roll the iomem flag into the lower bits of the vaddr eventually.

First just add accessors to move all current in-tree users over to.

Signed-off-by: Dave Airlie <airl...@redhat.com>
---
 include/linux/iosys-map.h | 53 +++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
index 4696abfd311c..5ce5df1db60a 100644
--- a/include/linux/iosys-map.h
+++ b/include/linux/iosys-map.h
@@ -114,6 +114,21 @@ struct iosys_map {
        bool is_iomem;
 };
+static inline bool iosys_map_is_iomem(const struct iosys_map *map)
+{
+       return map->is_iomem;
+}
+
+static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
+{
+       return map->vaddr_iomem;
+}
+
+static inline void *iosys_map_ptr(const struct iosys_map *map)
+{
+       return map->vaddr;
+}
+

These helpers need documentation.

agreed

We should encourage users to the other helpers for interacting with iosys-map structures instead of decoding them manually. OTOH there are cases where decoding them by hand is clearly better. I'd suggest to prefix the new helpers with __ so mark them an internal/special.

From the other patches there are quite a few cases that would be using
"internal"  API. From those there are just a few cases in which we'd
have a direct translation to existing API... so I wouldn't make this
internal when they are clearly needed externally.

Lucas De Marchi

Best regards
Thomas

 /**
  * IOSYS_MAP_INIT_VADDR - Initializes struct iosys_map to an address in system 
memory
  * @vaddr_:    A system-memory address
@@ -234,9 +249,9 @@ static inline bool iosys_map_is_equal(const struct 
iosys_map *lhs,
  */
 static inline bool iosys_map_is_null(const struct iosys_map *map)
 {
-       if (map->is_iomem)
-               return !map->vaddr_iomem;
-       return !map->vaddr;
+       if (iosys_map_is_iomem(map))
+               return !iosys_map_ioptr(map);
+       return !iosys_map_ptr(map);
 }
 /**
@@ -286,10 +301,10 @@ static inline void iosys_map_clear(struct iosys_map *map)
 static inline void iosys_map_memcpy_to(struct iosys_map *dst, size_t 
dst_offset,
                                       const void *src, size_t len)
 {
-       if (dst->is_iomem)
-               memcpy_toio(dst->vaddr_iomem + dst_offset, src, len);
+       if (iosys_map_is_iomem(dst))
+               memcpy_toio(iosys_map_ioptr(dst) + dst_offset, src, len);
        else
-               memcpy(dst->vaddr + dst_offset, src, len);
+               memcpy(iosys_map_ptr(dst) + dst_offset, src, len);
 }
 /**
@@ -306,10 +321,10 @@ static inline void iosys_map_memcpy_to(struct iosys_map 
*dst, size_t dst_offset,
 static inline void iosys_map_memcpy_from(void *dst, const struct iosys_map 
*src,
                                         size_t src_offset, size_t len)
 {
-       if (src->is_iomem)
-               memcpy_fromio(dst, src->vaddr_iomem + src_offset, len);
+       if (iosys_map_is_iomem(src))
+               memcpy_fromio(dst, iosys_map_ioptr(src) + src_offset, len);
        else
-               memcpy(dst, src->vaddr + src_offset, len);
+               memcpy(dst, iosys_map_ptr(src) + src_offset, len);
 }
 /**
@@ -322,7 +337,7 @@ static inline void iosys_map_memcpy_from(void *dst, const 
struct iosys_map *src,
  */
 static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
 {
-       if (map->is_iomem)
+       if (iosys_map_is_iomem(map))
                map->vaddr_iomem += incr;
        else
                map->vaddr += incr;
@@ -341,10 +356,10 @@ static inline void iosys_map_incr(struct iosys_map *map, 
size_t incr)
 static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
                                    int value, size_t len)
 {
-       if (dst->is_iomem)
-               memset_io(dst->vaddr_iomem + offset, value, len);
+       if (iosys_map_is_iomem(dst))
+               memset_io(iosys_map_ioptr(dst) + offset, value, len);
        else
-               memset(dst->vaddr + offset, value, len);
+               memset(iosys_map_ptr(dst) + offset, value, len);
 }
 #ifdef CONFIG_64BIT
@@ -393,10 +408,10 @@ static inline void iosys_map_memset(struct iosys_map 
*dst, size_t offset,
  */
 #define iosys_map_rd(map__, offset__, type__) ({                               
        \
        type__ val_;                                                            
        \
-       if ((map__)->is_iomem) {                                                
     \
-               __iosys_map_rd_io(val_, (map__)->vaddr_iomem + (offset__), 
type__);  \
+       if (iosys_map_is_iomem(map__)) {                                        
        \
+               __iosys_map_rd_io(val_, iosys_map_ioptr(map__) + (offset__), 
type__);   \
        } else {                                                                
        \
-               __iosys_map_rd_sys(val_, (map__)->vaddr + (offset__), type__);  
             \
+               __iosys_map_rd_sys(val_, iosys_map_ptr(map__) + (offset__), 
type__);    \
        }                                                                       
        \
        val_;                                                                   
        \
 })
@@ -415,10 +430,10 @@ static inline void iosys_map_memset(struct iosys_map 
*dst, size_t offset,
  */
 #define iosys_map_wr(map__, offset__, type__, val__) ({                        
                \
        type__ val_ = (val__);                                                  
        \
-       if ((map__)->is_iomem) {                                                
     \
-               __iosys_map_wr_io(val_, (map__)->vaddr_iomem + (offset__), 
type__);  \
+       if (iosys_map_is_iomem(map__)) {                                        
        \
+               __iosys_map_wr_io(val_, iosys_map_ioptr(map__) + (offset__), 
type__);   \
        } else {                                                                
        \
-               __iosys_map_wr_sys(val_, (map__)->vaddr + (offset__), type__);  
             \
+               __iosys_map_wr_sys(val_, iosys_map_ptr(map__) + (offset__), 
type__);    \
        }                                                                       
        \
 })

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)