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

[Peter Zijlstra <peterz@infradead.org>]

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)

What do you need the spinlock for? Afaict this is a 'simple' polling
event handling loop on MMIO, right?

All we really need to guarantee is that there's only a single CPU trying
to do that at any one time.

Wouldn't something like:

  https://marc.info/?l=linux-kernel&m=147681099108509&w=2

already take care of that? Then you can drop the lock and things will
work 'nested'.

> +
> +	xdbc_handle_events();
> +
> +	/* Check completion of the previous request. */
> +	while (xdbc.flags & XDBC_FLAGS_OUT_PROCESS) {
> +		if (timeout > 1000000)
> +			break;
> +
> +		spin_unlock_irqrestore(&xdbc.lock, flags);
> +		xdbc_delay(100);
> +		spin_lock_irqsave(&xdbc.lock, flags);
> +		timeout += 100;
> +
> +		xdbc_handle_events();
> +	}
> +
> +	if (xdbc.flags & XDBC_FLAGS_OUT_PROCESS) {
> +		spin_unlock_irqrestore(&xdbc.lock, flags);
> +
> +		/*
> +		 * Oops, hardware wasn't able to complete the
> +		 * previous transfer.
> +		 */
> +		xdbc_trace("oops: previous transfer not completed yet\n");
> +
> +		return -ETIMEDOUT;
> +	}
> +
> +	ret = xdbc_bulk_transfer((void *)bytes, size, false);
> +
> +	spin_unlock_irqrestore(&xdbc.lock, flags);
> +
> +	return ret;
> +}
> +
> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
> +{
> +	int chunk, ret;
> +	static char buf[XDBC_MAX_PACKET];
> +	int use_cr = 0;
> +
> +	if (!xdbc.xdbc_reg)
> +		return;
> +	memset(buf, 0, XDBC_MAX_PACKET);
> +	while (n > 0) {
> +		for (chunk = 0; chunk < XDBC_MAX_PACKET && n > 0;
> +				str++, chunk++, n--) {
> +			if (!use_cr && *str == '\n') {
> +				use_cr = 1;
> +				buf[chunk] = '\r';
> +				str--;
> +				n++;
> +				continue;
> +			}
> +			if (use_cr)
> +				use_cr = 0;
> +			buf[chunk] = *str;
> +		}
> +		if (chunk > 0) {
> +			ret = xdbc_bulk_write(buf, chunk);
> +			if (ret < 0)
> +				break;
> +		}
> +	}
> +}
> +
> +static struct console early_xdbc_console = {
> +	.name =		"earlyxdbc",
> +	.write =	early_xdbc_write,
> +	.flags =	CON_PRINTBUFFER,
> +	.index =	-1,
> +};
> +
> +void __init early_xdbc_register_console(void)
> +{
> +	if (early_console)
> +		return;
> +
> +	early_console = &early_xdbc_console;
> +	if (early_console_keep)
> +		early_console->flags &= ~CON_BOOT;
> +	else
> +		early_console->flags |= CON_BOOT;
> +	register_console(early_console);
> +}
> +
> +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?