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

[Andreas Larsson]

On 2013-10-01 16:19, Felipe Balbi wrote:
> Hi,
>
> On Tue, Oct 01, 2013 at 10:34:47AM +0200, Andreas Larsson wrote:
> > > > +/* #define VERBOSE_DEBUG */
> > >
> > > we don't want this, we want verbose debug to be selectable on Kconfig,
> > > which already is ;-)
> >
> > I was only aware of CONFIG_USB_GADGET_DEBUG leading to DEBUG being
> > defined, not that any Kconfig turned on VERBOSE_DEBUG. Where is this
> > happening?
>
> you're right there :-) My bad. Do you mind adding a patch which sets
> VERBOSE_DEBUG when building drivers/usb/gadget/ directory ?
> drivers/usb/dwc3/ has an example, if you need ;-)

Sure, I'll do that.

> Or I can patch that myself, if you prefer. works both ways.
>
> > > > +#include "gr_udc.h"
> > > > +
> > > > +#define	DRIVER_NAME	"gr_udc"
> > > > +#define	DRIVER_DESC	"Aeroflex Gaisler GRUSBDC USB Peripheral Controller"
> > > > +
> > > > +static const char driver_name[] = DRIVER_NAME;
> > > > +static const char driver_desc[] = DRIVER_DESC;
> > > > +
> > > > +#define gr_read32(x) (ioread32be((x)))
> > > > +#define gr_write32(x, v) (iowrite32be((v), (x)))
> > > > +
> > > > +/* USB speed and corresponding string calculated from status register value */
> > > > +#define GR_SPEED(status) \
> > > > +	((status & GR_STATUS_SP) ? USB_SPEED_FULL : USB_SPEED_HIGH)
> > > > +#define GR_SPEED_STR(status) usb_speed_string(GR_SPEED(status))
> > > > +
> > > > +/* Size of hardware buffer calculated from epctrl register value */
> > > > +#define GR_BUFFER_SIZE(epctrl)					      \
> > > > +	((((epctrl) & GR_EPCTRL_BUFSZ_MASK) >> GR_EPCTRL_BUFSZ_POS) * \
> > > > +	 GR_EPCTRL_BUFSZ_SCALER)
> > > > +
> > > > +/* ---------------------------------------------------------------------- */
> > > > +/* Debug printout functionality */
> > > > +
> > > > +static const char * const gr_modestring[] = {"control", "iso", "bulk", "int"};
> > > > +
> > > > +static const char *gr_ep0state_string(enum gr_ep0state state)
> > > > +{
> > > > +	static const char *const names[] = {
> > > > +		[GR_EP0_DISCONNECT] = "disconnect",
> > > > +		[GR_EP0_SETUP] = "setup",
> > > > +		[GR_EP0_IDATA] = "idata",
> > > > +		[GR_EP0_ODATA] = "odata",
> > > > +		[GR_EP0_ISTATUS] = "istatus",
> > > > +		[GR_EP0_OSTATUS] = "ostatus",
> > > > +		[GR_EP0_STALL] = "stall",
> > > > +		[GR_EP0_SUSPEND] = "suspend",
> > > > +	};
> > > > +
> > > > +	if (state < 0 || state >= ARRAY_SIZE(names))
> > > > +		return "UNKNOWN";
> > > > +
> > > > +	return names[state];
> > > > +}
> > > > +
> > > > +#ifdef VERBOSE_DEBUG
> > > > +
> > > > +#define BPRINTF(buf, left, fmt, args...)			\
> > > > +	do {							\
> > > > +		int ret = snprintf(buf, left, fmt, ## args);	\
> > > > +		buf += ret;					\
> > > > +		left -= ret;					\
> > > > +	} while (0)
> > >
> > > nack, use dev_vdbg() instead.
> > >
> > > > +static void gr_dbgprint_request(const char *str, struct gr_ep *ep,
> > > > +				struct gr_request *req)
> > > > +{
> > > > +	char buffer[100];
> > >
> > > NAK^10000000
> > >
> > > use kernel facilities instead. printk() and all its friends already
> > > print to a ring buffer.
> >
> > Alright. The concern was that repeatedly calling printk for multiple
> > parts of the same message could lead to intermixing with other unrelated
> > printouts.
>
> hmm, there are two ways to look at this.
>
> a) we have KERN_CONT to continue printing messages
> b) you might prefer using debugfs and seq_puts() for dumping large(-ish)
> amounts of debugging data ;-)

I just found print_hex_dump_debug that takes care of everything 
including dynamic debug support, so I'll use that.

> > > > +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 by the work handler */
> > > > +		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 irqs disabled */
> > > > +		local_irq_save(flags);
> > >
> > > I guess it'd be better if you called this with spin_lock_irqsave()
> > > called before, then you can remove local_irq_save from here.
> >
> > That would increase the amount of time interrupts are disabled quite a
> > lot, so I would prefer not to.
>
> that's what every other UDC driver is doing. I don't think you need to
> worry about that. Can you run some benchmarks with both constructs just
> so I can have peace of mind ?

I'll look into this.

> > > > +		spin_unlock(&dev->lock);
> > > > +
> > > > +		req->req.complete(&ep->ep, &req->req);
> > > > +
> > > > +		spin_lock(&dev->lock);
> > > > +		local_irq_restore(flags);
> > > > +	}
> > > > +	ep->callback = 0;
> > > > +
> > > > +	/* Catch up possible prevented ep handling during completion callback */
> > > > +	if (!ep->stopped)
> > > > +		schedule_work(&dev->work);
> > >
> > > this workqueue is awkward, what's up with that ?
> >
> > The reason for the scheduling here is that during the completion call
> > the handling of endpoint events needs to be stopped. This is
> > accomplished by the ep->callback flag. When that is done we might have
> > ep events that needs to be taken care of.
> >
> > The same situation arises after unhalting an endpoint further down. All
> > potential handling of that endpoint was on pause during halt, and thus
> > the work handler needs to be scheduled to catch up.
>
> not so sure. Other UDC drivers also support EP halt and they don't need
> the workqueue at all.
>
> > > > +/* Call with non-NULL dev to do a devm-allocation */
> > > > +static struct usb_request *__gr_alloc_request(struct device *dev,
> > > > +					      struct usb_ep *_ep,
> > > > +					      gfp_t gfp_flags)
> > > > +{
> > > > +	struct gr_request *req;
> > > > +
> > > > +	if (dev)
> > > > +		req = devm_kzalloc(dev, sizeof(*req), gfp_flags);
> > > > +	else
> > > > +		req = kzalloc(sizeof(*req), gfp_flags);
> > >
> > > why would "dev" ever be NULL ?
> >
> > When the gadget allocates a request it will free it explicitely later
> > on. Thus there is no need for any devm allocation. Therefore, the calls
> > from the gadget to gr_alloc_request then calls this function with a NULL
> > argument so that non-devm allocation is done in that case.
>
> then couldn't you just stick with direct kzalloc() instead of trying to
> use devm_kzalloc() for allocating requests ?

Alright.

> That's the righ way to handle usb_request lifetime anyway; leave it to
> the gadget driver. If that gadget driver doesn't free the usb_requests
> it allocated, we want the memory leak as an indication of a buggy
> gadget driver.
>
> > > > +	epctrl = gr_read32(&ep->regs->epctrl);
> > > > +	if (halt) {
> > > > +		/* Set HALT */
> > > > +		gr_write32(&ep->regs->epctrl, epctrl | GR_EPCTRL_EH);
> > > > +		ep->stopped = 1;
> > > > +		if (wedge)
> > > > +			ep->wedged = 1;
> > > > +	} else {
> > > > +		gr_write32(&ep->regs->epctrl, epctrl & ~GR_EPCTRL_EH);
> > > > +		ep->stopped = 0;
> > > > +		ep->wedged = 0;
> > > > +
> > > > +		/* Things might have been queued up in the meantime */
> > > > +		if (!ep->dma_start)
> > > > +			gr_start_dma(ep);
> > > > +
> > > > +		/* Ep handling might have been hindered during halt */
> > > > +		schedule_work(&ep->dev->work);
> >
> > Here is the second place where we need to schedule work as mentioned
> > above.
>
> that's fine, but we still have other gadget drivers which don't take the
> route of a workqueue after unhalting the endpoint.
>
> If the endpoint is halted, why do you even have anything to process at
> all for this endpoint ? nothing should have been queued, right ?
>
> And if you did queue requests while EP was halted, you could just
> restart your EP queue right here, instead of scheduling a work_struct to
> do that for you.
>
> > > > +	}
> > > > +
> > > > +	return retval;
> > > > +}
> > > > +
> > > > +/* Must be called with dev->lock held */
> > > > +static inline void gr_set_ep0state(struct gr_udc *dev, enum gr_ep0state value)
> > > > +{
> > > > +	if (dev->ep0state != value)
> > > > +		VDBG("STATE:  ep0state=%s\n",
> > > > +		     gr_ep0state_string(value));
> > >
> > > dev_vdbg()
> > >
> > > > +	dev->ep0state = value;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Should only be called when endpoints can not generate interrupts.
> > > > + *
> > > > + * Must be called with dev->lock held.
> > > > + */
> > > > +static void gr_disable_interrupts_and_pullup(struct gr_udc *dev)
> > > > +{
> > > > +	gr_write32(&dev->regs->control, 0);
> > > > +	wmb(); /* Make sure that we do not deny one of our interrupts */
> > > > +	dev->irq_enabled = 0;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Stop all device activity and disable data line pullup.
> > > > + *
> > > > + * Must be called with dev->lock held.
> > > > + */
> > > > +static void gr_stop_activity(struct gr_udc *dev)
> > > > +{
> > > > +	struct gr_ep *ep;
> > > > +
> > > > +	list_for_each_entry(ep, &dev->ep_list, ep_list)
> > > > +		gr_ep_nuke(ep);
> > > > +
> > > > +	gr_disable_interrupts_and_pullup(dev);
> > > > +
> > > > +	gr_set_ep0state(dev, GR_EP0_DISCONNECT);
> > > > +	usb_gadget_set_state(&dev->gadget, USB_STATE_ATTACHED);
> > >
> > > ATTACHED ??
> >
> > Maybe NOTATTACHED is clearer, even if it is the same state in all
> > respects.
>
> for the sake of being clear, yes :-)
>
> > > > +static irqreturn_t gr_irq(int irq, void *_dev)
> > > > +{
> > > > +	struct gr_udc *dev = _dev;
> > > > +
> > > > +	if (!dev->irq_enabled)
> > > > +		return IRQ_NONE;
> > > > +
> > > > +	schedule_work(&dev->work);
> > >
> > > why do you need this ? We have threaded IRQ handlers. Why a workqueue ?
> >
> > As mentioned above, to to be able to schedule work after pausing
> > endpoint handling during a completion callback call or during an
> > endpoint halt.
>
> doesn't look like you need that work_struct at all. Handle your IRQ
> directly and for the pieces you need to do after ClearHalt, re-factor
> that to a separate function which you call conditionally on
> ->set_halt().

OK, I'll look into this for v2.

Cheers,
Andreas

--
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/