Thread (44 messages) 44 messages, 6 authors, 2019-08-12

RE: [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver

From: Felipe Balbi <hidden>
Date: 2019-08-12 10:34:46
Also in: linux-usb, lkml

Hi,

Pawel Laszczak [off-list ref] writes:
quoted
quoted
quoted
Yet another thread? Can't you just run this right before giving back the
USB request? So, don't do it from IRQ handler, but from giveback path?
Do you mean in:
	if (request->complete) {
		spin_unlock(&priv_dev->lock);
		if (priv_dev->run_garbage_collector) {
			....
		}
		usb_gadget_giveback_request(&priv_ep->endpoint,
					    request);
		spin_lock(&priv_dev->lock);
	}
??
right, you can do it right before giving back the request. Or right
after.
quoted
I ask because this is finally also called from IRQ handler:

cdns3_device_thread_irq_handler
    -> cdns3_check_ep_interrupt_proceed
        -> cdns3_transfer_completed
            -> cdns3_gadget_giveback
                -> usb_gadget_giveback_request
Did you notice that it doesn't reenable interrupts, though?
I noticed that there is a lack of reenabling interrupts :)

The problem is that If I have disabled interrupt the kernel complains
for using dma_free_coherent function in such place. 

Here you have a fragment of complaints: 
[ 7420.502863] WARNING: CPU: 0 PID: 10260 at kernel/dma/mapping.c:281 dma_free_attrs+0xa0/0xd0
[ 7420.502866] Modules linked in: usb_f_mass_storage cdns3(OE) cdns3_pci_wrap(OE) libcomposite
		...
[ 7420.502965]  cdns3_gadget_giveback+0x159/0x2a0 [cdns3]
[ 7420.502975]  cdns3_transfer_completed+0xc5/0x3c0 [cdns3]
[ 7420.502986]  cdns3_device_thread_irq_handler+0x1b1/0xab0 [cdns3]
[ 7420.502991]  ? __schedule+0x333/0x7e0
[ 7420.503001]  irq_thread_fn+0x26/0x60
[ 7420.503006]  ? irq_thread+0xa8/0x1b0
[ 7420.503011]  irq_thread+0x10e/0x1b0
[ 7420.503015]  ? irq_forced_thread_fn+0x80/0x80
[ 7420.503021]  ? wake_threads_waitq+0x30/0x30
[ 7420.503029]  kthread+0x12c/0x150
[ 7420.503034]  ? irq_thread_check_affinity+0xe0/0xe0
[ 7420.503038]  ? kthread_park+0x90/0x90
[ 7420.503045]  ret_from_fork+0x3a/0x50
[ 7420.503061] irq event stamp: 2962
[ 7420.503065] hardirqs last  enabled at (2961): [<ffffffffb252672c>] _raw_spin_unlock_irq+0x2c/0x40
[ 7420.503070] hardirqs last disabled at (2962): [<ffffffffb25268f5>] _raw_spin_lock_irqsave+0x25/0x60
[ 7420.503074] softirqs last  enabled at (2918): [<ffffffffb2800340>] __do_softirq+0x340/0x451
[ 7420.503079] softirqs last disabled at (2657): [<ffffffffb1aa02b6>] irq_exit+0xc6/0xd0
[ 7420.503082] ---[ end trace d02652af11011c3b ]---

Maybe it's a bug in implementation of this function.  I allocate memory with flag GFP_ATOMIC with 
disabled interrupt, but I can't free such memory. 
I don't understand the intricacies of the coherent API to judge if it's
a bug in the API itself. In any case, here's where the splat comes from:

void dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
		dma_addr_t dma_handle, unsigned long attrs)
{
	const struct dma_map_ops *ops = get_dma_ops(dev);

	if (dma_release_from_dev_coherent(dev, get_order(size), cpu_addr))
		return;
	/*
	 * On non-coherent platforms which implement DMA-coherent buffers via
	 * non-cacheable remaps, ops->free() may call vunmap(). Thus getting
	 * this far in IRQ context is a) at risk of a BUG_ON() or trying to
	 * sleep on some machines, and b) an indication that the driver is
	 * probably misusing the coherent API anyway.
	 */
	WARN_ON(irqs_disabled());

	if (!cpu_addr)
		return;

	debug_dma_free_coherent(dev, size, cpu_addr, dma_handle);
	if (dma_is_direct(ops))
		dma_direct_free(dev, size, cpu_addr, dma_handle, attrs);
	else if (ops->free)
		ops->free(dev, size, cpu_addr, dma_handle, attrs);
}
EXPORT_SYMBOL(dma_free_attrs);

maybe you're gonna have to fire up a workqueue to free this memory for
you :-(

Unless someone else has better ideas. Alan, Greg, any ideas?

-- 
balbi

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help