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

[Andreas Larsson]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/