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

Hi,

Pawel Laszczak [off-list ref] writes:

quoted

quoted

I have such situation in which one interrupt line is shared with ehci and cdns3 driver.
In such case this function returns error code.

which function returns error code?

devm_request_threaded_irq, of course if I set IRQF_SHARED | IRQF_ONESHOT.
As I remember it was EBUSY error.

oh, right. That's probably because the handlers must agree on IRQ flags.

quoted

quoted

So probably I will need to mask only the reported interrupts.

you should mask all interrupts from your device, otherwise you top-halt
may still end up reentrant.

quoted

I can't mask all interrupt using controller register because I can miss some of them.

why would you miss them? They would be left in the register until you
unmask them and the line is raised again.

I consult this with author of controller. 
We have:
USB_IEN  and USB_ISTS for  generic interrupts
EP_IEN and EP_ISTS for endpoint interrupts 

Both these group works different.
For endpoint I can disable all interrupt and I don't miss any of them. 
So it's normal behavior.

But USB_ISTS work little different. If we mask all interrupt in USB_IEN
then when new event occurs the EP_ISTS will not be updated. 

wait a minute. When you mask USB_ISTS, then EP_ISTS isn't updated? Is
this a quirk on the controller or a design choice?

It's not standard and not expected behavior but it works in this way. 

Yeah, sounds rather odd.

quoted

quoted

quoted

quoted

quoted

quoted

+	/* check USB device interrupt */
+	reg = readl(&priv_dev->regs->usb_ists);
+
+	if (reg) {
+		writel(reg, &priv_dev->regs->usb_ists);
+		cdns3_check_usb_interrupt_proceed(priv_dev, reg);
+		ret = IRQ_HANDLED;

now, because you _don't_ mask this interrupt, you're gonna have
issues. Say we actually get both device and endpoint interrupts while
the thread is already running with previous endpoint interrupts. Now
we're gonna reenter the top half, because device interrupts are *not*
masked, which will read usb_ists and handle it here.

Endpoint interrupts are masked in cdns3_device_irq_handler and stay masked
until they are not handled in threaded handler.

Quick question, then: these ISTS registers, are they masked interrupt
status or raw interrupt status?

Yes it's masked, but after masking them the new interrupts will not be reported
In ISTS registers. Form this reason I can mask only reported interrupt.

and what happens when you unmask the registers? Do they get reported?

No they are not reported in case of USB_ISTS register.
They should be reported in case EP_ISTS, but I need to test it. 

okay, please _do_ test and verify the behavior. The description above
sounds really surprising to me. Does it really mean that if you mask all
USB_ISTS and then disconnect the cable while interrupt is masked, you
won't know cable was disconnected?

quoted

quoted

quoted

quoted

quoted

quoted

+		struct cdns3_aligned_buf *buf, *tmp;
+
+		list_for_each_entry_safe(buf, tmp, &priv_dev->aligned_buf_list,
+					 list) {
+			if (!buf->in_use) {
+				list_del(&buf->list);
+
+				spin_unlock_irqrestore(&priv_dev->lock, flags);

creates the possibility of a race condition

Why? In this place the buf can't be used.

but you're reenabling interrupts, right?

Yes, driver frees not used buffers here.
I think that it's the safest place for this purpose.

I guess you missed the point a little. Since you reenable interrupts
just to free the buffer, you end up creating the possibility for a race
condition. Specially since you don't mask all interrupt events. The
moment you reenable interrupts, one of your not-unmasked interrupt
sources could trigger, then top-half gets scheduled which tries to wake
up the IRQ thread again and things go boom.

Ok, I think I understand.  So I have 3 options:
1. Mask the USB_IEN and EP_IEN interrupts, but then I can lost some USB_ISTS
events. It's dangerous options. 

sure sounds dangerous, but also sounds quite "peculiar" :-)

2. Remove implementation of handling unaligned buffers and assume that 
    upper layer will worry about this. What with vendor specific drivers that 
    can be used by companies and not upstreamed  ? 
    It could be good to have such safety mechanism even if it is not currently used.

dunno. It may become dead code that's NEVER used :-)

3. Delegate this part of code for instance to separate thread that will be called 
   In free time. 

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?

-- 
balbi