Hi Fredrik,
Thanks very much for taking the time to look into this. Please see
comments inline.
On 15.05.2019 19:28, Fredrik Noring wrote:
> Hi Lauretniu,
>
>> I think that any recent kernel will do, so I'd say your current branch
>> should be fine.
>
> The kernel oopses with "unable to handle kernel paging request at virtual
> address 000aba0b" in hcd_alloc_coherent via usb_hcd_map_urb_for_dma.
By any chance, does this address looks like the dma_addr that the device
should target?
> This relates to patch 2/3 that I didn't quite understand, where
>
> - retval = dma_declare_coherent_memory(dev, mem->start,
> - mem->start - mem->parent->start,
> - resource_size(mem));
>
> becomes
>
> + retval = gen_pool_add_virt(hcd->localmem_pool,
> + (unsigned long)mem->start -
> + mem->parent->start,
> + mem->start, resource_size(mem),
>
> so that arguments two and three switch places. Given
>
> int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr,
> dma_addr_t device_addr, size_t size);
>
> and
>
> int gen_pool_add_virt(struct gen_pool *pool, unsigned long virt, phys_addr_t
> phys
> size_t size, int nid)
>
> it seems that the device address (dma_addr_t device_addr) now becomes a
> virtual address (unsigned long virt). Is that intended?
Actually, I think I'm misusing genalloc and also it appears that i need
to add a mapping on the phys address. So my plan is to change the
"unsigned long virt" to be the void * returned by the mapping operation
and the phys_addr_t be the dma_addr_t. I'll return with a patch.
Regarding the usage of unsigned long in genalloc, yeah seems a bit
strange but by looking at other users in the kernel it appears that
that's the design.
---
Best Regards, Laurentiu
> My corresponding patch is below for reference. I applied your 1/3 patch
> to test it.
>
> Fredrik
>
> diff --git a/drivers/usb/host/ohci-ps2.c b/drivers/usb/host/ohci-ps2.c
> --- a/drivers/usb/host/ohci-ps2.c
> +++ b/drivers/usb/host/ohci-ps2.c
> @@ -7,6 +7,7 @@
> */
>
> #include <linux/dma-mapping.h>
> +#include <linux/genalloc.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/usb.h>
> @@ -92,12 +93,12 @@ static irqreturn_t ohci_ps2_irq(struct usb_hcd *hcd)
> return ohci_irq(hcd); /* Call normal IRQ handler. */
> }
>
> -static int iopheap_alloc_coherent(struct platform_device *pdev,
> - size_t size, int flags)
> +static int iopheap_alloc_coherent(struct platform_device *pdev, size_t size)
> {
> struct device *dev = &pdev->dev;
> struct usb_hcd *hcd = platform_get_drvdata(pdev);
> struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
> + int err;
>
> if (ps2priv->iop_dma_addr != 0)
> return 0;
> @@ -112,28 +113,37 @@ static int iopheap_alloc_coherent(struct
> platform_device *pdev,
> return -ENOMEM;
> }
>
> - if (dma_declare_coherent_memory(dev,
> - iop_bus_to_phys(ps2priv->iop_dma_addr),
> - ps2priv->iop_dma_addr, size, flags)) {
> - dev_err(dev, "dma_declare_coherent_memory failed\n");
> - iop_free(ps2priv->iop_dma_addr);
> - ps2priv->iop_dma_addr = 0;
> - return -ENOMEM;
> + hcd->localmem_pool = devm_gen_pool_create(dev,
> + PAGE_SHIFT, dev_to_node(dev), DRV_NAME);
> + if (IS_ERR(hcd->localmem_pool)) {
> + err = PTR_ERR(hcd->localmem_pool);
> + goto out_err;
> + }
> +
> + err = gen_pool_add_virt(hcd->localmem_pool, ps2priv->iop_dma_addr,
> + iop_bus_to_phys(ps2priv->iop_dma_addr), size, dev_to_node(dev));
> + if (err < 0) {
> + dev_err(dev, "gen_pool_add_virt failed with %d\n", err);
> + goto out_err;
> }
>
> return 0;
> +
> +out_err:
> + iop_free(ps2priv->iop_dma_addr);
> + ps2priv->iop_dma_addr = 0;
> +
> + return err;
> }
>
> static void iopheap_free_coherent(struct platform_device *pdev)
> {
> - struct device *dev = &pdev->dev;
> struct usb_hcd *hcd = platform_get_drvdata(pdev);
> struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
>
> if (ps2priv->iop_dma_addr == 0)
> return;
>
> - dma_release_declared_memory(dev);
> iop_free(ps2priv->iop_dma_addr);
> ps2priv->iop_dma_addr = 0;
> }
> @@ -177,8 +187,7 @@ static int ohci_hcd_ps2_probe(struct platform_device
> *pdev)
> goto err;
> }
>
> - ret = iopheap_alloc_coherent(pdev,
> - DMA_BUFFER_SIZE, DMA_MEMORY_EXCLUSIVE);
> + ret = iopheap_alloc_coherent(pdev, DMA_BUFFER_SIZE);
> if (ret != 0)
> goto err_alloc_coherent;
>
>
