re: RDMA/cxgb4: Support on-chip SQs

re: RDMA/cxgb4: Support on-chip SQs

[Dan Carpenter]

Hello Steve Wise,

The patch c6d7b26791a2: "RDMA/cxgb4: Support on-chip SQs" from Sep
13, 2010, leads to the following warning:
"drivers/infiniband/hw/cxgb4/qp.c:98 alloc_host_sq()
	error: 'sq->queue' came from dma_alloc_coherent() so we can't
	do virt_to_phys()"

drivers/infiniband/hw/cxgb4/qp.c
    92  static int alloc_host_sq(struct c4iw_rdev *rdev, struct t4_sq *sq)
    93  {
    94          sq->queue = dma_alloc_coherent(&(rdev->lldi.pdev->dev), sq->memsize,
    95                                         &(sq->dma_addr), GFP_KERNEL);
    96          if (!sq->queue)
    97                  return -ENOMEM;
    98          sq->phys_addr = virt_to_phys(sq->queue);
    99          pci_unmap_addr_set(sq, mapping, sq->dma_addr);
   100          return 0;
   101  }

This is a new Smatch check.  I don't know the rules on virt_to_phys yet
but here is the relevant comment.

/**
 *      virt_to_phys    -       map virtual addresses to physical
 *      @address: address to remap
 *
 *      The returned physical address is the physical (CPU) mapping for
 *      the memory address given. It is only valid to use this function on
 *      addresses directly mapped or allocated via kmalloc.
 *
 *      This function does not give bus mappings for DMA transfers. In
 *      almost all conceivable cases a device driver should not be using
 *      this function
 */

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Steve Wise]

I wonder, then, what the correct service is to get the cpu physical 
address from a kernel virtual address returned from 
dma_alloc_coherent()?  I think this is correct as-is, since I think 
dma_alloc_coherent() falls under the "directly mapped" addresses in the 
virt_to_phys() prototype comment.

What do you think Roland?

On 1/30/2013 3:00 PM, Dan Carpenter wrote:
> Hello Steve Wise,
>
> The patch c6d7b26791a2: "RDMA/cxgb4: Support on-chip SQs" from Sep
> 13, 2010, leads to the following warning:
> "drivers/infiniband/hw/cxgb4/qp.c:98 alloc_host_sq()
> 	error: 'sq->queue' came from dma_alloc_coherent() so we can't
> 	do virt_to_phys()"
>
> drivers/infiniband/hw/cxgb4/qp.c
>      92  static int alloc_host_sq(struct c4iw_rdev *rdev, struct t4_sq *sq)
>      93  {
>      94          sq->queue = dma_alloc_coherent(&(rdev->lldi.pdev->dev), sq->memsize,
>      95                                         &(sq->dma_addr), GFP_KERNEL);
>      96          if (!sq->queue)
>      97                  return -ENOMEM;
>      98          sq->phys_addr = virt_to_phys(sq->queue);
>      99          pci_unmap_addr_set(sq, mapping, sq->dma_addr);
>     100          return 0;
>     101  }
>
> This is a new Smatch check.  I don't know the rules on virt_to_phys yet
> but here is the relevant comment.
>
> /**
>   *      virt_to_phys    -       map virtual addresses to physical
>   *      @address: address to remap
>   *
>   *      The returned physical address is the physical (CPU) mapping for
>   *      the memory address given. It is only valid to use this function on
>   *      addresses directly mapped or allocated via kmalloc.
>   *
>   *      This function does not give bus mappings for DMA transfers. In
>   *      almost all conceivable cases a device driver should not be using
>   *      this function
>   */
>
> regards,
> dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Jason Gunthorpe]

On Wed, Jan 30, 2013 at 03:36:48PM -0600, Steve Wise wrote:
> I wonder, then, what the correct service is to get the cpu physical
> address from a kernel virtual address returned from
> dma_alloc_coherent()?  I think this is correct as-is, since I think
> dma_alloc_coherent() falls under the "directly mapped" addresses in
> the virt_to_phys() prototype comment.

DMA-API.txt says:

 This routine allocates a region of <size> bytes of consistent memory.
 It also returns a <dma_handle> which may be cast to an unsigned
 integer the same width as the bus and used as the physical address
 base of the region.

So instead of virt_to_phys you should use dma_addr??

I think the note about 'directly mapped' refers to things the arch
setups during early boot, not dma_alloc stuff.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Dan Carpenter]

On Wed, Jan 30, 2013 at 03:36:48PM -0600, Steve Wise wrote:
> I wonder, then, what the correct service is to get the cpu physical
> address from a kernel virtual address returned from
> dma_alloc_coherent()?  I think this is correct as-is, since I think
> dma_alloc_coherent() falls under the "directly mapped" addresses in
> the virt_to_phys() prototype comment.
> 

Here is another relevant comment I should have included.

commit ae6a5d37725853325a2b3460165fbc5613ce2916
Author: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Date:   Tue Nov 20 12:17:51 2012 +0000

    ASoC: kirkwood-dma: fix use of virt_to_phys()

    This is part of a patch found in Rabeeh Khoury's git tree for the
    cubox.

    You can not use virt_to_phys() on the address returned from
    dma_alloc_coherent(); it may not be part of the kernel direct-mapped
    memory.  Fix this to use the DMA address instead.

    Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
    Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>

I don't know the details though, and I certainly don't know how
someone would fix this.  ;)

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Steve Wise]

On 1/30/2013 3:44 PM, Jason Gunthorpe wrote:
> On Wed, Jan 30, 2013 at 03:36:48PM -0600, Steve Wise wrote:
>> I wonder, then, what the correct service is to get the cpu physical
>> address from a kernel virtual address returned from
>> dma_alloc_coherent()?  I think this is correct as-is, since I think
>> dma_alloc_coherent() falls under the "directly mapped" addresses in
>> the virt_to_phys() prototype comment.
> DMA-API.txt says:
>
>   This routine allocates a region of <size> bytes of consistent memory.
>   It also returns a <dma_handle> which may be cast to an unsigned
>   integer the same width as the bus and used as the physical address
>   base of the region.
>
> So instead of virt_to_phys you should use dma_addr??

I don't think so.  The result of virt_to_phys() is used in 
remap_pfn_range() to map this memory into user space...  The dma_addr is 
give to HW for DMA operations.

> I think the note about 'directly mapped' refers to things the arch
> setups during early boot, not dma_alloc stuff.
>
> Jason

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Steve Wise]

On 1/30/2013 3:45 PM, Dan Carpenter wrote:
> On Wed, Jan 30, 2013 at 03:36:48PM -0600, Steve Wise wrote:
>> I wonder, then, what the correct service is to get the cpu physical
>> address from a kernel virtual address returned from
>> dma_alloc_coherent()?  I think this is correct as-is, since I think
>> dma_alloc_coherent() falls under the "directly mapped" addresses in
>> the virt_to_phys() prototype comment.
>>
> Here is another relevant comment I should have included.
>
> commit ae6a5d37725853325a2b3460165fbc5613ce2916
> Author: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Date:   Tue Nov 20 12:17:51 2012 +0000
>
>      ASoC: kirkwood-dma: fix use of virt_to_phys()
>
>      This is part of a patch found in Rabeeh Khoury's git tree for the
>      cubox.
>
>      You can not use virt_to_phys() on the address returned from
>      dma_alloc_coherent(); it may not be part of the kernel direct-mapped
>      memory.  Fix this to use the DMA address instead.
>
>      Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>      Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
>
> I don't know the details though, and I certainly don't know how
> someone would fix this.  ;)
>

How do I map the dma address into a physical address suitable for 
passing to remap_pfn_range()?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Steve Wise]

On 1/30/2013 3:52 PM, Steve Wise wrote:
>
> On 1/30/2013 3:45 PM, Dan Carpenter wrote:
>> On Wed, Jan 30, 2013 at 03:36:48PM -0600, Steve Wise wrote:
>>> I wonder, then, what the correct service is to get the cpu physical
>>> address from a kernel virtual address returned from
>>> dma_alloc_coherent()?  I think this is correct as-is, since I think
>>> dma_alloc_coherent() falls under the "directly mapped" addresses in
>>> the virt_to_phys() prototype comment.
>>>
>> Here is another relevant comment I should have included.
>>
>> commit ae6a5d37725853325a2b3460165fbc5613ce2916
>> Author: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>> Date:   Tue Nov 20 12:17:51 2012 +0000
>>
>>      ASoC: kirkwood-dma: fix use of virt_to_phys()
>>
>>      This is part of a patch found in Rabeeh Khoury's git tree for the
>>      cubox.
>>
>>      You can not use virt_to_phys() on the address returned from
>>      dma_alloc_coherent(); it may not be part of the kernel 
>> direct-mapped
>>      memory.  Fix this to use the DMA address instead.
>>
>>      Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>>      Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
>>
>> I don't know the details though, and I certainly don't know how
>> someone would fix this.  ;)
>>
>
> How do I map the dma address into a physical address suitable for 
> passing to remap_pfn_range()?
>

To clarify, on some systems the dma bus address != the host physical 
address.  So i'm not sure remap_pfn_range() really wants a dma bus address.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Jason Gunthorpe]

On Wed, Jan 30, 2013 at 03:51:24PM -0600, Steve Wise wrote:
> On 1/30/2013 3:44 PM, Jason Gunthorpe wrote:
> >On Wed, Jan 30, 2013 at 03:36:48PM -0600, Steve Wise wrote:
> >>I wonder, then, what the correct service is to get the cpu physical
> >>address from a kernel virtual address returned from
> >>dma_alloc_coherent()?  I think this is correct as-is, since I think
> >>dma_alloc_coherent() falls under the "directly mapped" addresses in
> >>the virt_to_phys() prototype comment.
> >DMA-API.txt says:
> >
> >  This routine allocates a region of <size> bytes of consistent memory.
> >  It also returns a <dma_handle> which may be cast to an unsigned
> >  integer the same width as the bus and used as the physical address
> >  base of the region.
> >
> >So instead of virt_to_phys you should use dma_addr??
> 
> I don't think so.  The result of virt_to_phys() is used in
> remap_pfn_range() to map this memory into user space...  The
> dma_addr is give to HW for DMA operations.

Oh now I see what this is for.. I've looked at this kind of question
before and I didn't find a satisfactory answer either :(

Just a quick perusal around, it looks like the DRM code (drm_vm) is
using virt_to_page/page_to_pfn/remap_pfn_range for its consistent
mappings.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Roland Dreier]

On Wed, Jan 30, 2013 at 1:44 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> So instead of virt_to_phys you should use dma_addr??

No, you clearly can't use the bus address. That has no relationship to
the CPU view, even in cases as common as VT-d on x86.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Steve Wise]

On 1/30/2013 4:34 PM, Roland Dreier wrote:
> On Wed, Jan 30, 2013 at 1:44 PM, Jason Gunthorpe
> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
>> So instead of virt_to_phys you should use dma_addr??
> No, you clearly can't use the bus address. That has no relationship to
> the CPU view, even in cases as common as VT-d on x86.
>

By the way, this usage of virt_to_phys() is not new to the commit that 
Dan referenced.  It has been there since day one of the iw_cxgb[34] 
drivers.   This doesn't mean its correct however. :)


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Dan Carpenter]

On Wed, Jan 30, 2013 at 03:36:48PM -0600, Steve Wise wrote:
> I wonder, then, what the correct service is to get the cpu physical
> address from a kernel virtual address returned from
> dma_alloc_coherent()?  I think this is correct as-is, since I think
> dma_alloc_coherent() falls under the "directly mapped" addresses in
> the virt_to_phys() prototype comment.
> 
> What do you think Roland?

In the end, we decided this was correct as-is?

I'm about to complain to the caif network people about the
dma_alloc_coherent() -> virt_to_phys() thing, but the truth is I
don't understand this stuff very well and I'm not sure when it's ok
or not.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Steve Wise]

On 3/23/2013 4:30 PM, Dan Carpenter wrote:
> On Wed, Jan 30, 2013 at 03:36:48PM -0600, Steve Wise wrote:
>> I wonder, then, what the correct service is to get the cpu physical
>> address from a kernel virtual address returned from
>> dma_alloc_coherent()?  I think this is correct as-is, since I think
>> dma_alloc_coherent() falls under the "directly mapped" addresses in
>> the virt_to_phys() prototype comment.
>>
>> What do you think Roland?
> In the end, we decided this was correct as-is?

I think the code is correct as-is.

> I'm about to complain to the caif network people about the
> dma_alloc_coherent() -> virt_to_phys() thing, but the truth is I
> don't understand this stuff very well and I'm not sure when it's ok
> or not.
>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RDMA/cxgb4: Support on-chip SQs

[Roland Dreier]

On Sat, Mar 23, 2013 at 2:30 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> In the end, we decided this was correct as-is?
>
> I'm about to complain to the caif network people about the
> dma_alloc_coherent() -> virt_to_phys() thing, but the truth is I
> don't understand this stuff very well and I'm not sure when it's ok
> or not.


I'm not sure it's strictly correct in all cases, but we don't really
have anything better to replace it with at the moment.

If there is a platform where it breaks, and they want to use one of
these drivers, then I guess we'll have to add more core infrastructure
to provide a way to map this type of memory into userspace.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html