On Thu, Jan 27, 2022 at 03:33:12PM +0100, Thomas Zimmermann wrote:


Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
When dma_buf_map struct is passed around, it's useful to be able to
initialize a second map that takes care of reading/writing to an offset
of the original map.

Add a helper that copies the struct and add the offset to the proper
address.

Cc: Sumit Semwal <sumit.sem...@linaro.org>
Cc: Christian König <christian.koe...@amd.com>
Cc: linux-me...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc: linaro-mm-...@lists.linaro.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Lucas De Marchi <lucas.demar...@intel.com>
---
 include/linux/dma-buf-map.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
index 65e927d9ce33..3514a859f628 100644
--- a/include/linux/dma-buf-map.h
+++ b/include/linux/dma-buf-map.h
@@ -131,6 +131,35 @@ struct dma_buf_map {
                .is_iomem = false, \
        }
+/**
+ * DMA_BUF_MAP_INIT_OFFSET - Initializes struct dma_buf_map from another 
dma_buf_map
+ * @map_:      The dma-buf mapping structure to copy from
+ * @offset:    Offset to add to the other mapping
+ *
+ * Initializes a new dma_buf_struct based on another. This is the equivalent 
of doing:
+ *
+ * .. code-block: c
+ *
+ *     dma_buf_map map = other_map;
+ *     dma_buf_map_incr(&map, &offset);
+ *
+ * Example usage:
+ *
+ * .. code-block: c
+ *
+ *     void foo(struct device *dev, struct dma_buf_map *base_map)
+ *     {
+ *             ...
+ *             struct dma_buf_map = DMA_BUF_MAP_INIT_OFFSET(base_map, 
FIELD_OFFSET);
+ *             ...
+ *     }
+ */
+#define DMA_BUF_MAP_INIT_OFFSET(map_, offset_) (struct dma_buf_map)    \
+       {                                                               \
+               .vaddr = (map_)->vaddr + (offset_),                  \
+               .is_iomem = (map_)->is_iomem,                                \
+       }
+

It's illegal to access .vaddr with raw pointer. Always use a dma_buf_memcpy_() interface. So why would you need this macro when you have dma_buf_memcpy_*() with an offset parameter?

I did a better job with an example in 
20220127093332.wnkd2qy4tvwg5i5l@ldmartin-desk2

While doing this series I had code like this when using the API in a function to
parse/update part of the struct mapped:

        int bla_parse_foo(struct dma_buf_map *bla_map)
        {
                struct dma_buf_map foo_map = *bla_map;
                ...

                dma_buf_map_incr(&foo_map, offsetof(struct bla, foo));

                ...
        }

Pasting the rest of the reply here:

I had exactly this code above, but after writting quite a few patches
using it, particularly with functions that have to write to 2 maps (see
patch 6 for example), it felt much better to have something to
initialize correctly from the start

        struct dma_buf_map other_map = *bla_map;
        /* poor Lucas forgetting dma_buf_map_incr(map, offsetof(...)); */

is error prone and hard to debug since you will be reading/writting
from/to another location rather than exploding

While with the construct below

        other_map;
        ...
        other_map = INITIALIZER()

I can rely on the compiler complaining about uninitialized var. And
in most of the cases I can just have this single line in the beggining of the
function when the offset is constant:

        struct dma_buf_map other_map = INITIALIZER(bla_map, offsetof(..));


This is useful when you have several small functions in charge of
updating/reading inner struct members.


I've also been very careful to distinguish between .vaddr and .vaddr_iomem, even in places where I wouldn't have to. This macro breaks the assumption.

That's one reason I think if we have this macro, it should be in the
dma_buf_map.h header (or whatever we rename these APIs to). It's the
only place where we can safely add code that relies on the implementation
of the "private" fields in struct dma_buf_map.

Lucas De Marchi


Best regards
Thomas

 /**
  * dma_buf_map_set_vaddr - Sets a dma-buf mapping structure to an address in 
system memory
  * @map:       The dma-buf mapping structure

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev



Reply via email to