Re: [PATCH v3] usb: gadget: Add UDC driver for Aeroflex Gaisler GRUSBDC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 12/13/2013 08:52 PM, Felipe Balbi wrote:
Hi,

On Fri, Dec 13, 2013 at 08:48:30AM +0100, Andreas Larsson wrote:
On Wed, Dec 04, 2013 at 09:13:58AM +0100, Andreas Larsson wrote:
+static void gr_finish_request(struct gr_ep *ep, struct gr_request *req,
+			      int status)
+{
+	struct gr_udc *dev;
+
+	list_del_init(&req->queue);
+
+	if (likely(req->req.status == -EINPROGRESS))
+		req->req.status = status;
+	else
+		status = req->req.status;
+
+	dev = ep->dev;
+	usb_gadget_unmap_request(&dev->gadget, &req->req, ep->is_in);
+	gr_free_dma_desc_chain(dev, req);
+
+	if (ep->is_in) /* For OUT, actual gets updated bit by bit */
+		req->req.actual = req->req.length;
+
+	if (!status) {
+		if (ep->is_in)
+			gr_dbgprint_request("SENT", ep, req);
+		else
+			gr_dbgprint_request("RECV", ep, req);
+	}
+
+	/* Prevent changes to ep->queue during callback */
+	ep->callback = 1;
+	if (req == dev->ep0reqo && !status) {
+		if (req->setup)
+			gr_ep0_setup(dev, req);
+		else
+			dev_err(dev->dev,
+				"Unexpected non setup packet on ep0in\n");
+	} else if (req->req.complete) {
+		unsigned long flags;
+
+		/*
+		 * Complete should be called with interrupts disabled according
+		 * to the contract of struct usb_request
+		 */
+		local_irq_save(flags);

sorry but this driver isn't ready for inclusion. local_irq_save() is a
pretty good hint that there's something wrong in the driver. Consider
the fact that local_irq_save() will disable preemption even when
CONFIG_PREEMPT_FULL is enabled and you have a bit a problem.

This connection between local_irq_save and CONFIG_PREEMPT_RT_FULL was
unknown to me. Sure, I can disable interrupts right at spin lock
time.

that's better.

Alright, I'll put this in v4.


Also, the way you're using thread IRQs is quite wrong. I can't let that
pass and get merged upstream, sorry.

What is quite wrong? What is it that I need to fix?

Ideally the hardirq handler should be usually to actually check if
$this_device generated the IRQ, that should involve reading a IRQSTATUS
register of some sort.

Sure, check that IRQs are actually enabled, but you also need to read
STATUS register before waking the thread up.

I agree that that would be preferable. Unfortunately, the hardware lacks status register bits that indicates whether interrupts has been generated or not. That is why the only check is if interrupts are disabled, because that is the only sure way to know that the softirq handler does not need to go through everything.

Best regards,
Andreas Larsson

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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux