Re: [RESEND PATCH v2 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Thu, Oct 20, 2016 at 04:08:17PM +0800, Lu Baolu wrote:
> Hi Peter,
> 
> Thanks for your comments.
> 
> On 10/19/2016 09:09 PM, Peter Zijlstra wrote:
> > On Wed, Oct 19, 2016 at 08:18:22AM +0800, Lu Baolu wrote:
> >> +++ b/drivers/usb/early/xhci-dbc.c
> >> +static int xdbc_bulk_write(const char *bytes, int size)
> >> +{
> >> +	unsigned long flags;
> >> +	int ret, timeout = 0;
> >> +
> >> +	spin_lock_irqsave(&xdbc.lock, flags);
> > Yikes!!
> >
> > So how is this supposed to work from NMI context and the like?
> >
> > (also, at the very least, that should be a raw_spinlock_t)
> 
> Totally agree. We should put it as a raw_spinlock_t().

Well, raw_spinlock_t still doesn't allow for NMI context operation. So
ideally you'd manage without any locks at all.

> > What do you need the spinlock for? Afaict this is a 'simple' polling
> > event handling loop on MMIO, right?
> 
> Not only for polling registers in MMIO, but also for handling the
> events in the event ring. The event ring is a memory block,
> which is allocated during hardware initialization and saved
> in a register in MMIO.
> 
> There is a single event ring for all events (read completion,
> write completion, port status change and transfer errors etc).
> The debugging hardware doesn't support interrupt, so software
> has to poll the event ring whenever it needs to.
> 
> Event ring polling happens at least in write interface (to make
> sure the previous transfer has been completed), and a worker
> (to check the read events and other things). That's the reason
> why I need a spin_lock here.

I'm not sure I understand. Sure you need someone polling, and you need
only a single CPU polling at the same time.

But the serialization I pointed to provides you that.

Sure, it get a tad tricky to allow a nested context to take over
processing in the middle of things, but that just means you should use
some cmpxchg and stay away from stack based variables.

> >> +static void xdbc_scrub_function(struct work_struct *work)
> >> +{
> >> +	unsigned long flags;
> >> +
> >> +	spin_lock_irqsave(&xdbc.lock, flags);
> >> +
> >> +	/*
> >> +	 * DbC is running, check the event ring and
> >> +	 * handle the events.
> >> +	 */
> >> +	if (readl(&xdbc.xdbc_reg->control) & CTRL_DRC)
> >> +		xdbc_handle_events();
> >> +
> >> +	/*
> >> +	 * External reset happened. Need to restart the
> >> +	 * debugging hardware.
> >> +	 */
> >> +	if (unlikely(!(readl(&xdbc.xdbc_reg->control) & CTRL_DCE)))
> >> +		xdbc_handle_external_reset();
> >> +
> >> +	spin_unlock_irqrestore(&xdbc.lock, flags);
> >> +
> >> +	queue_delayed_work(xdbc_wq, &xdbc.scrub, usecs_to_jiffies(100));
> >> +}
> > Excuse my total lack of USB knowledge, but WTH does this do and what do
> > we need it for?
> >
> 
> As I said above, I need a worker to check the read completion
> events and other hardware situations.
> 
> One hardware situation that needs to check regularly is that
> it might be aborted by the host controller itself. The xhci spec
> allows the debug hardware to share some logics with the host
> controller (to reduce cost?). As the result, when host controller
> driver resets the host (always happens in driver probe or
> resume) the debug hardware resets as well. Software needs
> to re-initialize and bring it back.
> 
> Early printk doesn't need to read anything from debug host.
> But if we use it for kernel debugging with kgdb (it's in my work
> queue),  we need a read interface. We need to check the event
> ring regularly for read completion events.

Urgh, but this is very non-robust. Who says the workqueue stuff still
works?

So now you're having your early_printk driver depend on the scheduler
still working and the workqueue stuff and..

As it stands, that renders the entire thing completely useless for
debugging the scheduler, workqueues and anything NMI. IOW, its
completely useless full stop.

I'm already only using early_printk() because regular printk() is an
unfixable piece of crap, and now you're making early_printk() useless
too.