Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

[Fredrik Noring <noring-zgYzP9v7iJcdnm+yROfE0A@public.gmane.org>]

Hi Robin,

> Historically, that particular line of code appears to date back to commit
> aa24886e379d (and tracking it's ancestry was quite fun).
> 
> Now, I'm sure not all of the considerations of 11-and-a-half years ago still
> apply, but one certainly does: ARM* still uses non-cacheable mappings for
> coherent allocations on systems which aren't hardware-coherent (i.e. most of
> them), thus the alloc and free paths may respectively set up and tear down
> those mappings, and the latter involves this guy:
> 
> 	void vunmap(const void *addr)
> 	{
> 		BUG_ON(in_interrupt());
> 		might_sleep();
> 		if (addr)
> 			__vunmap(addr, 0);
> 	}
> 
> Even in the non-coherent ARM case it *would* technically be viable to free a
> coherent buffer from interrupt context iff it was allocated with GFP_ATOMIC,
> as those are satisfied from a statically-mapped pool rather than dynamically
> vmapped, but that doesn't really expand to the general case, and I certainly
> can't speak for all the other architectures.

Ah, thanks, good to know!

> As Alan implies, I guess the way forward is to figure out how similar
> drivers manage - your backtrace suggests that you might be using
> HCD_LOCAL_MEM, which probably isn't the common case but does seem to appear
> in at least a couple of other OHCI drivers.

There are two other OHCI drivers that do (as far as I understand)
essentially the same thing with dma_declare_coherent_memory and
HCD_LOCAL_MEM:

	drivers/usb/host/ohci-sm501.c
	drivers/usb/host/ohci-tmio.c

They are simple, but I cannot see how they could possibly avoid this issue
given the design of the USB core and the offending call trace which does not
pass through the device specific HCD:

	   usb_hcd_irq
	-> ohci_irq
	-> ohci_work
	-> process_done_list
	-> takeback_td
	-> finish_urb
	-> usb_hcd_giveback_urb
	-> __usb_hcd_giveback_urb
	-> unmap_urb_for_dma
	-> usb_hcd_unmap_urb_for_dma
	-> hcd_free_coherent
	-> hcd_buffer_free
	-> dma_free_coherent
	-> dma_free_attrs

The hc_driver struct is set to defaults, and they don't manage DMA
allocations except for probe and remove. How could they avoid the WARN_ON?

[ For reference, I attached the PS2 OHCI driver below. It has been tested
and works well provided the WARN_ON in dma_free_attrs is removed. ]

Fredrik

/*
 * PlayStation 2 USB OHCI HCD (Host Controller Driver)
 *
 * Copyright (C) 2017 Jürgen Urban
 * Copyright (C) 2017 Fredrik Noring
 *
 * SPDX-License-Identifier: GPL-2.0
 */

#include <linux/dma-mapping.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/usb.h>
#include <linux/usb/hcd.h>

#include <asm/mach-ps2/iop-memory.h>

#include "ohci.h"

/* Enable USB OHCI hardware. */
#define DPCR2_ENA_USB 0x08000000

/* Enable PS2DEV (required for PATA and USB). */
#define DPCR2_ENA_PS2DEV 0x00000080

#define DRIVER_DESC "OHCI PS2 driver"
#define DRV_NAME "ohci-ps2"

/* Size allocated from IOP heap (maximum size of DMA memory). */
#define DMA_BUFFER_SIZE (256 * 1024)

/* Get driver private data. */
#define hcd_to_priv(hcd) (struct ps2_hcd *)(hcd_to_ohci(hcd)->priv)

struct ps2_hcd {
	void __iomem *dpcr2;
	dma_addr_t iop_dma_addr;
	bool wakeup;			/* Saved wake-up state for resume. */
};

static struct hc_driver __read_mostly ohci_ps2_hc_driver;
static irqreturn_t (*ohci_irq)(struct usb_hcd *hcd);

static void ohci_ps2_enable(struct usb_hcd *hcd)
{
	struct ohci_hcd *ohci = hcd_to_ohci(hcd);

	BUG_ON(!ohci->regs);

	/* This is needed to get USB working on PS2. */
	ohci_writel(ohci, 1, &ohci->regs->roothub.portstatus[11]);
}

static void ohci_ps2_disable(struct usb_hcd *hcd)
{
	struct ohci_hcd *ohci = hcd_to_ohci(hcd);

	WARN_ON(!ohci->regs);

	if (ohci->regs)
		ohci_writel(ohci, 0, &ohci->regs->roothub.portstatus[11]);
}

static void ohci_ps2_start_hc(struct usb_hcd *hcd)
{
	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);

	/*
	 * Enable USB and PS2DEV.
	 *
	 * FIXME: What is the purpose of PS2DEV for USB?
	 * FIXME: As far as I remember the following call enables the clock,
	 * so that ohci->regs->fminterval can count.
	 */
	writel(readl(ps2priv->dpcr2) | DPCR2_ENA_USB | DPCR2_ENA_PS2DEV,
		ps2priv->dpcr2);
}

static void ohci_ps2_stop_hc(struct usb_hcd *hcd)
{
	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);

	/* Disable USB. Leave PS2DEV enabled (could be still needed for PATA). */
	writel(readl(ps2priv->dpcr2) & ~DPCR2_ENA_USB, ps2priv->dpcr2);
}

static int ohci_ps2_reset(struct usb_hcd *hcd)
{
	int ret;

	ohci_ps2_start_hc(hcd);

	ret = ohci_setup(hcd);

	if (ret < 0) {
		ohci_ps2_stop_hc(hcd);
		return ret;
	}

	ohci_ps2_enable(hcd);

	return ret;
}

static irqreturn_t ohci_ps2_irq(struct usb_hcd *hcd)
{
	struct ohci_hcd	*ohci = hcd_to_ohci(hcd);
	struct ohci_regs __iomem *regs = ohci->regs;

	/*
	 * FIXME: For some reason OHCI_INTR_MIE is required in the
	 * IRQ handler. Without it, reading a large amount of data
	 * (> 1 GB) from a mass storage device results in a freeze.
	 */
	ohci_writel(ohci, OHCI_INTR_MIE, &regs->intrdisable);

	return ohci_irq(hcd); /* Call normal IRQ handler. */
}

static int iopheap_alloc_coherent(struct platform_device *pdev, size_t size,
	int flags)
{
	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 0;

	ps2priv->iop_dma_addr = iop_alloc(size);
	if (ps2priv->iop_dma_addr == 0) {
		dev_err(dev, "iop_alloc failed\n");
		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;
	}

	return 0;
}

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;
}

static int ohci_hcd_ps2_probe(struct platform_device *pdev)
{
	struct device *dev = &pdev->dev;
	struct resource *regs;
	struct resource *dpcr2;
	struct usb_hcd *hcd;
	struct ps2_hcd *ps2priv;
	int irq;
	int ret;

	irq = platform_get_irq(pdev, 0);
	if (irq < 0) {
		dev_err(dev, "platform_get_irq failed\n");
		return irq;
	}

	/* FIXME: Is request_mem_region recommended here? */

	hcd = usb_create_hcd(&ohci_ps2_hc_driver, dev, dev_name(dev));
	if (hcd == NULL)
		return -ENOMEM;

	ps2priv = hcd_to_priv(hcd);
	memset(ps2priv, 0, sizeof(*ps2priv));

	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	if (regs == NULL) {
		dev_err(dev, "platform_get_resource 0 failed\n");
		ret = -ENOENT;
		goto err;
	}
	dpcr2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
	if (dpcr2 == NULL) {
		dev_err(dev, "platform_get_resource 1 failed\n");
		ret = -ENOENT;
		goto err;
	}

	hcd->rsrc_start = regs->start;
	hcd->rsrc_len = resource_size(regs);
	hcd->regs = ioremap(hcd->rsrc_start, hcd->rsrc_len);
	if (IS_ERR(hcd->regs)) {
		ret = PTR_ERR(hcd->regs);
		goto err;
	}

	ps2priv->dpcr2 = ioremap(dpcr2->start, resource_size(dpcr2));
	if (ps2priv->dpcr2 == NULL) {
		ret = -ENOMEM;
		goto err_ioremap_dpcr2;
	}

	ret = iopheap_alloc_coherent(pdev, DMA_BUFFER_SIZE, DMA_MEMORY_EXCLUSIVE);
	if (ret != 0)
		goto err_alloc_coherent;

	ret = usb_add_hcd(hcd, irq, 0);
	if (ret != 0)
		goto err_add_hcd;

	ret = device_wakeup_enable(hcd->self.controller);
	if (ret == 0)
		return ret;

	usb_remove_hcd(hcd);
err_add_hcd:
	iopheap_free_coherent(pdev);
err_alloc_coherent:
	iounmap(ps2priv->dpcr2);
err_ioremap_dpcr2:
	iounmap(hcd->regs);
err:
	usb_put_hcd(hcd);

	return ret;
}

static int ohci_hcd_ps2_remove(struct platform_device *pdev)
{
	struct usb_hcd *hcd = platform_get_drvdata(pdev);
	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);

	usb_remove_hcd(hcd);

	ohci_ps2_disable(hcd);
	ohci_ps2_stop_hc(hcd);

	iopheap_free_coherent(pdev);
	iounmap(ps2priv->dpcr2);
	iounmap(hcd->regs);

	usb_put_hcd(hcd);

	return 0;
}

#ifdef CONFIG_PM
static int ohci_hcd_ps2_suspend(struct platform_device *pdev,
				pm_message_t message)
{
	struct device *dev = &pdev->dev;
	struct usb_hcd *hcd = platform_get_drvdata(pdev);
	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
	int ret;

	ps2priv->wakeup = device_may_wakeup(dev);
	if (ps2priv->wakeup)
		enable_irq_wake(hcd->irq);

	ret = ohci_suspend(hcd, ps2priv->wakeup);
	if (ret)
		return ret;

	ohci_ps2_disable(hcd);
	ohci_ps2_stop_hc(hcd);

	return ret;
}

static int ohci_hcd_ps2_resume(struct platform_device *pdev)
{
	struct usb_hcd *hcd = platform_get_drvdata(pdev);
	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);

	if (ps2priv->wakeup)
		disable_irq_wake(hcd->irq);

	ohci_ps2_start_hc(hcd);
	ohci_ps2_enable(hcd);

	ohci_resume(hcd, ps2priv->wakeup);

	return 0;
}
#endif

static struct platform_driver ohci_hcd_ps2_driver = {
	.probe		= ohci_hcd_ps2_probe,
	.remove		= ohci_hcd_ps2_remove,
	.shutdown	= usb_hcd_platform_shutdown,
#ifdef	CONFIG_PM
	.suspend	= ohci_hcd_ps2_suspend,
	.resume		= ohci_hcd_ps2_resume,
#endif
	.driver		= {
		.name	= DRV_NAME,
	},
};

static const struct ohci_driver_overrides ps2_overrides __initconst = {
	.reset		= ohci_ps2_reset,
	.product_desc	= DRIVER_DESC,
	.extra_priv_size = sizeof(struct ps2_hcd),
};

static int __init ohci_ps2_init(void)
{
	if (usb_disabled())
		return -ENODEV;

	pr_info("%s: " DRIVER_DESC "\n", DRV_NAME);

	ohci_init_driver(&ohci_ps2_hc_driver, &ps2_overrides);
	ohci_ps2_hc_driver.flags |= HCD_LOCAL_MEM;

	/*
	 * FIXME: For some reason
	 *
	 *   ohci_writel(ohci, OHCI_INTR_MIE, &regs->intrdisable);
	 *
	 * is required in the IRQ handler. Without it, reading a large
	 * amount of data (> 1 GB) from a mass storage device results in
	 * a freeze.
	 */
	ohci_irq = ohci_ps2_hc_driver.irq; /* Save normal IRQ handler. */
	ohci_ps2_hc_driver.irq = ohci_ps2_irq; /* Install IRQ workaround. */

	return platform_driver_register(&ohci_hcd_ps2_driver);
}
module_init(ohci_ps2_init);

static void __exit ohci_ps2_cleanup(void)
{
	platform_driver_unregister(&ohci_hcd_ps2_driver);
}
module_exit(ohci_ps2_cleanup);

MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL");
MODULE_ALIAS("platform:" DRV_NAME);
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu