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 ;-)
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 ;-)
> >>+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 ?
> >>+ 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 ?
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().
> Thank you for the feedback!
no problem ;-)
--
balbi
Attachment:
signature.asc
Description: Digital signature
