Re: [PATCH v7 05/10] usb: dbc: add bulk out and bulk in interfaces

[Mathias Nyman]

On 26.01.2016 14:58, Lu Baolu wrote:
> This patch adds interfaces for bulk out and bulk in ops. These
> interfaces could be used to implement early printk bootconsole
> or hook to various system debuggers.
>
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> ---
>   drivers/usb/early/xhci-dbc.c | 373 +++++++++++++++++++++++++++++++++++++++++++
>   include/linux/usb/xhci-dbc.h |  30 ++++
>   2 files changed, 403 insertions(+)

...

> +
> +/*
> + * Check and dispatch events in event ring. It also checks status
> + * of hardware. This function will be called from multiple threads.
> + * An atomic lock is applied to protect the access of event ring.
> + */
> +static int xdbc_check_event(void)
> +{
> +	/* event ring is under checking by other thread? */
> +	if (!test_bit(XDBC_ATOMIC_EVENT, &xdbcp->atomic_flags) &&
> +			!test_and_set_bit(XDBC_ATOMIC_EVENT,
> +			&xdbcp->atomic_flags))
> +		return 0;

homemade trylock, can't the real ones be used?

> +
> +	xdbc_handle_events();
> +
> +	test_and_clear_bit(XDBC_ATOMIC_EVENT, &xdbcp->atomic_flags);
> +
> +	return 0;
>  +}
> +
> +#define	BULK_IN_COMPLETED(p)	((xdbcp->in_pending == (p)) && \
> +				 xdbcp->in_complete)
> +#define	BULK_OUT_COMPLETED(p)	((xdbcp->out_pending == (p)) && \
> +				 xdbcp->out_complete)
> +

...

> +}
> +
> +int xdbc_bulk_read(void *data, int size, int loops)
> +{
> +	int ret;
> +
> +	do {
> +		if (!test_bit(XDBC_ATOMIC_BULKIN, &xdbcp->atomic_flags) &&
> +				!test_and_set_bit(XDBC_ATOMIC_BULKIN,
> +				&xdbcp->atomic_flags))
> +			break;
> +	} while (1);

homemeade spin_lock, can't the real one be used?

If the xdbc_bulk_write() can be accessed from interrupt context (handler, soft, timer) it
may deadlock

> +
> +	ret = xdbc_bulk_transfer(data, size, loops, true);
> +
> +	test_and_clear_bit(XDBC_ATOMIC_BULKIN, &xdbcp->atomic_flags);
> +
> +	return ret;
> +}
> +
> +int xdbc_bulk_write(const char *bytes, int size)
> +{
> +	int ret;
> +
> +	do {
> +		if (!test_bit(XDBC_ATOMIC_BULKOUT, &xdbcp->atomic_flags) &&
> +				!test_and_set_bit(XDBC_ATOMIC_BULKOUT,
> +				&xdbcp->atomic_flags))
> +			break;
> +	} while (1);

Another homemeade spin_lock, can't the real one be used?

same issue here, deadlock if accessible from interrupt context.


Would it make sense to have only one spinlock, and start one separate thread for
reading the event ring. The thread would,  lock, handle pending events, unlock,
then call shedule, in a loop. ehci early debug code has some variant of this.

So the lock would be taken while events are being handled.

The same lock would be used for bulk_read and bulk_write. Yes this would prevent read and
write at the same time, and the read and writes need to be modified to not block until
the reansfer is finished, just to write the TRBs on the ring, update ring pointers,
and ring the doorbell.

Or is all this impossibe due to the earlyness of the code?

-Mathias