[PATCH] habanalabs: fix build warning

[PATCH] habanalabs: fix build warning

[Max Filippov]

allmodconfig build fails on ARCH=xtensa with the following message:

  drivers/misc/habanalabs/common/memory.c:153:49: error: cast from pointer
	to integer of different size [-Werror=pointer-to-int-cast]
	(u64) gen_pool_dma_alloc_align(vm->dram_pg_pool,

Fix it by adding intermediate conversion to uintptr_t as in other places
in that driver.

Fixes: e8458e20e0a3 ("habanalabs: make sure device mem alloc is page aligned")
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 drivers/misc/habanalabs/common/memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/habanalabs/common/memory.c b/drivers/misc/habanalabs/common/memory.c
index e008d82e4ba3..f0d373171d2a 100644
--- a/drivers/misc/habanalabs/common/memory.c
+++ b/drivers/misc/habanalabs/common/memory.c
@@ -150,9 +150,9 @@ static int alloc_device_memory(struct hl_ctx *ctx, struct hl_mem_in *args,
 		for (i = 0 ; i < num_pgs ; i++) {
 			if (is_power_of_2(page_size))
 				phys_pg_pack->pages[i] =
-						(u64) gen_pool_dma_alloc_align(vm->dram_pg_pool,
-										page_size, NULL,
-										page_size);
+					(u64)(uintptr_t)gen_pool_dma_alloc_align(vm->dram_pg_pool,
+										 page_size, NULL,
+										 page_size);
 			else
 				phys_pg_pack->pages[i] = (u64) gen_pool_alloc(vm->dram_pg_pool,
 										page_size);
-- 
2.30.2

Re: [PATCH] habanalabs: fix build warning

[Arnd Bergmann]

On Fri, Apr 1, 2022 at 6:16 AM Max Filippov <jcmvbkbc@gmail.com> wrote:
>
> allmodconfig build fails on ARCH=xtensa with the following message:
>
>   drivers/misc/habanalabs/common/memory.c:153:49: error: cast from pointer
>         to integer of different size [-Werror=pointer-to-int-cast]
>         (u64) gen_pool_dma_alloc_align(vm->dram_pg_pool,
>
> Fix it by adding intermediate conversion to uintptr_t as in other places
> in that driver.
>
> Fixes: e8458e20e0a3 ("habanalabs: make sure device mem alloc is page aligned")
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  drivers/misc/habanalabs/common/memory.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/misc/habanalabs/common/memory.c b/drivers/misc/habanalabs/common/memory.c
> index e008d82e4ba3..f0d373171d2a 100644
> --- a/drivers/misc/habanalabs/common/memory.c
> +++ b/drivers/misc/habanalabs/common/memory.c
> @@ -150,9 +150,9 @@ static int alloc_device_memory(struct hl_ctx *ctx, struct hl_mem_in *args,
>                 for (i = 0 ; i < num_pgs ; i++) {
>                         if (is_power_of_2(page_size))
>                                 phys_pg_pack->pages[i] =
> -                                               (u64) gen_pool_dma_alloc_align(vm->dram_pg_pool,
> -                                                                               page_size, NULL,
> -                                                                               page_size);
> +                                       (u64)(uintptr_t)gen_pool_dma_alloc_align(vm->dram_pg_pool,
> +                                                                                page_size, NULL,
> +                                                                                page_size);
>                         else
>                                 phys_pg_pack->pages[i] = (u64) gen_pool_alloc(vm->dram_pg_pool,
>                                                                                 page_size);

This addresses the warning, but I suspect there is still a problem in the code:
The description of that member lists it as '@pages: the physical page array',
but it is actually a kernel virtual address that gets passed to it. Since this
is a 'u64' member, it is hard to tell what type it actually is.

gen_pool_dma_alloc_align() returns both a virtual address and a dma (bus)
address. The dma address is ignored here, which makes me wonder why
this interface is used in the first place.

I can see four possible things that may be going on here:

- if the pages[] array is meant to be a kernel virtual address, it should be
  changed from a 'u64' to a normal pointer, with the cast removed.

- if the pages[] array is meant to be a physical address, as documented,
  it should be assigned using virt_to_phys() on the pointer, with a warning
  that this must not be used a as a dma address (which can easily get
  confused with a phys address as the binary representation is often the
  same in the absence of an iommu). In this case, it should also be
  changed to a phys_addr_t.

- if the pages[] array is meant to be a dma address, it should be changed
  to a dma_addr_t, and passed as the third argument to
  gen_pool_dma_alloc_align() in order to return the correct address.

- if there is a 'u64' member that is used for two (or all three) of the above
  depending on context, it should be replaced with either multiple
  struct members or a union.

Looking at other uses of the pages[] array, I see a dma_addr_t assigned
to it in init_phys_pg_pack_from_userptr(), but map_phys_pg_pack() and
alloc_sgt_from_device_pages appear to treat it as a cpu-physical phys_addr_t
rather than a device address again.

        Arnd

Re: [PATCH] habanalabs: fix build warning

[Oded Gabbay]

On Fri, Apr 1, 2022 at 9:40 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Apr 1, 2022 at 6:16 AM Max Filippov <jcmvbkbc@gmail.com> wrote:
> >
> > allmodconfig build fails on ARCH=xtensa with the following message:
> >
> >   drivers/misc/habanalabs/common/memory.c:153:49: error: cast from pointer
> >         to integer of different size [-Werror=pointer-to-int-cast]
> >         (u64) gen_pool_dma_alloc_align(vm->dram_pg_pool,
> >
> > Fix it by adding intermediate conversion to uintptr_t as in other places
> > in that driver.
> >
> > Fixes: e8458e20e0a3 ("habanalabs: make sure device mem alloc is page aligned")
> > Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> > ---
> >  drivers/misc/habanalabs/common/memory.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/misc/habanalabs/common/memory.c b/drivers/misc/habanalabs/common/memory.c
> > index e008d82e4ba3..f0d373171d2a 100644
> > --- a/drivers/misc/habanalabs/common/memory.c
> > +++ b/drivers/misc/habanalabs/common/memory.c
> > @@ -150,9 +150,9 @@ static int alloc_device_memory(struct hl_ctx *ctx, struct hl_mem_in *args,
> >                 for (i = 0 ; i < num_pgs ; i++) {
> >                         if (is_power_of_2(page_size))
> >                                 phys_pg_pack->pages[i] =
> > -                                               (u64) gen_pool_dma_alloc_align(vm->dram_pg_pool,
> > -                                                                               page_size, NULL,
> > -                                                                               page_size);
> > +                                       (u64)(uintptr_t)gen_pool_dma_alloc_align(vm->dram_pg_pool,
> > +                                                                                page_size, NULL,
> > +                                                                                page_size);
> >                         else
> >                                 phys_pg_pack->pages[i] = (u64) gen_pool_alloc(vm->dram_pg_pool,
> >                                                                                 page_size);
>
> This addresses the warning, but I suspect there is still a problem in the code:
> The description of that member lists it as '@pages: the physical page array',
> but it is actually a kernel virtual address that gets passed to it. Since this
> is a 'u64' member, it is hard to tell what type it actually is.
>
> gen_pool_dma_alloc_align() returns both a virtual address and a dma (bus)
> address. The dma address is ignored here, which makes me wonder why
> this interface is used in the first place.
>
> I can see four possible things that may be going on here:
>
> - if the pages[] array is meant to be a kernel virtual address, it should be
>   changed from a 'u64' to a normal pointer, with the cast removed.
>
> - if the pages[] array is meant to be a physical address, as documented,
>   it should be assigned using virt_to_phys() on the pointer, with a warning
>   that this must not be used a as a dma address (which can easily get
>   confused with a phys address as the binary representation is often the
>   same in the absence of an iommu). In this case, it should also be
>   changed to a phys_addr_t.
>
> - if the pages[] array is meant to be a dma address, it should be changed
>   to a dma_addr_t, and passed as the third argument to
>   gen_pool_dma_alloc_align() in order to return the correct address.
>
> - if there is a 'u64' member that is used for two (or all three) of the above
>   depending on context, it should be replaced with either multiple
>   struct members or a union.
>
> Looking at other uses of the pages[] array, I see a dma_addr_t assigned
> to it in init_phys_pg_pack_from_userptr(), but map_phys_pg_pack() and
> alloc_sgt_from_device_pages appear to treat it as a cpu-physical phys_addr_t
> rather than a device address again.
>
>         Arnd

Hi,
We use gen_pool in this function to manage our device memory
allocations (this is why it is called alloc_device_memory).

Basically, we initialize the genpool with the total size of the device memory,
and each bit represents a page according to a fixed page size, which
is dependent on asic type.
The addresses represent the physical address of the device memory, as
our device sees them.
As these addresses are not accessible from the host, it is appropriate
to hold them in u64, imo.

For future asics which will support multiple page sizes, we need to
use the gen_pool_dma_alloc_align() variant,
because then we need the allocation to be aligned to the page size as
requested by the user per allocation.

We ignore the DMA address because this is device memory, not host memory.
Therefore, our device's dma engine addresses the memory using the
virtual memory addresses we assign to it in our device's MMU.

Having said that, I'm wondering whether gen_pool_first_fit_align() can
also work here, which might be less confusing.

Oded

Re: [PATCH] habanalabs: fix build warning

[Arnd Bergmann]

On Fri, Apr 1, 2022 at 8:55 AM Oded Gabbay <ogabbay@kernel.org> wrote:
> On Fri, Apr 1, 2022 at 9:40 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Fri, Apr 1, 2022 at 6:16 AM Max Filippov <jcmvbkbc@gmail.com> wrote:
>
> We use gen_pool in this function to manage our device memory
> allocations (this is why it is called alloc_device_memory).

Ok, so it's none of the three I listed ;-)

> Basically, we initialize the genpool with the total size of the device memory,
> and each bit represents a page according to a fixed page size, which
> is dependent on asic type.
> The addresses represent the physical address of the device memory, as
> our device sees them.
> As these addresses are not accessible from the host, it is appropriate
> to hold them in u64, imo.
>
> For future asics which will support multiple page sizes, we need to
> use the gen_pool_dma_alloc_align() variant,
> because then we need the allocation to be aligned to the page size as
> requested by the user per allocation.
>
> We ignore the DMA address because this is device memory, not host memory.
> Therefore, our device's dma engine addresses the memory using the
> virtual memory addresses we assign to it in our device's MMU.
>
> Having said that, I'm wondering whether gen_pool_first_fit_align() can
> also work here, which might be less confusing.

Thank you for the explanation. I think the best way to make this less
confusing and to avoid the type casts would be to define your own
typedef for a device-internal address, and then wrap the allocator
functions such as gen_pool_dma_alloc_align() in helper functions that
do the type conversion safely.

       Arnd

Re: [PATCH] habanalabs: fix build warning

[Oded Gabbay]

On Fri, Apr 1, 2022 at 10:53 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Apr 1, 2022 at 8:55 AM Oded Gabbay <ogabbay@kernel.org> wrote:
> > On Fri, Apr 1, 2022 at 9:40 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Fri, Apr 1, 2022 at 6:16 AM Max Filippov <jcmvbkbc@gmail.com> wrote:
> >
> > We use gen_pool in this function to manage our device memory
> > allocations (this is why it is called alloc_device_memory).
>
> Ok, so it's none of the three I listed ;-)
>
> > Basically, we initialize the genpool with the total size of the device memory,
> > and each bit represents a page according to a fixed page size, which
> > is dependent on asic type.
> > The addresses represent the physical address of the device memory, as
> > our device sees them.
> > As these addresses are not accessible from the host, it is appropriate
> > to hold them in u64, imo.
> >
> > For future asics which will support multiple page sizes, we need to
> > use the gen_pool_dma_alloc_align() variant,
> > because then we need the allocation to be aligned to the page size as
> > requested by the user per allocation.
> >
> > We ignore the DMA address because this is device memory, not host memory.
> > Therefore, our device's dma engine addresses the memory using the
> > virtual memory addresses we assign to it in our device's MMU.
> >
> > Having said that, I'm wondering whether gen_pool_first_fit_align() can
> > also work here, which might be less confusing.
>
> Thank you for the explanation. I think the best way to make this less
> confusing and to avoid the type casts would be to define your own
> typedef for a device-internal address, and then wrap the allocator
> functions such as gen_pool_dma_alloc_align() in helper functions that
> do the type conversion safely.
>
>        Arnd

Thanks for the advice, we will prepare a patch accordingly.
Oded