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

[Lu Baolu]

On 02/18/2016 09:32 PM, Mathias Nyman wrote:
> 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.

I will try to rework this spin_lock with the real one and keep avoiding deadlock in mind.

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

Let me try to find this part of code.

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

It's not only due to earlyness of the code. But also, these read/write ops were designed to
be used by a debugger (for example kgdb) as well. Using the kernel provided interface
might make things simple, but what should happen when the debugger is used
to debug the kernel subsystem itself?

So, it seems that I should implement read/write ops depends on the use case. For this
time being, let's focus on the boot console case.

Some transfers take place when the thread/lock subsystem is not initialized yet.
But after thread/lock subsystem is able to be used, we are able to use the real one.

Let me wrapper them in functions. For the transfers taken place before the subsystem
initialization (that's single thread context, no worry about deadlock), it will use the
current methods (it might be possible to drop lock due the single thread context),
and after the subsystem being initialized, it will use those provided by the kernel.

>
> -Mathias
>

Very appreciated for your time.

Regards,
-Baolu