Re: [PATCH 30/37] usb: host: xhci: make a generic TRB tracer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 01/16/2017 04:55 PM, Felipe Balbi wrote:
> Hi,
>
> Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> writes:
>>>>> +		__entry->type = ring->type;
>>>>> +		__entry->field0 = le32_to_cpu(trb->field[0]);
>>>>> +		__entry->field1 = le32_to_cpu(trb->field[1]);
>>>>> +		__entry->field2 = le32_to_cpu(trb->field[2]);
>>>>> +		__entry->field3 = le32_to_cpu(trb->field[3]);
>>>>>  	),
>>>>> -	TP_printk("\ntrb_dma=@%llx, trb_va=@%p, status=%08x, flags=%08x",
>>>>> -			(unsigned long long) __entry->dma, __entry->va,
>>>>> -			__entry->status, __entry->flags
>>>>> +	TP_printk("%s: %s", xhci_ring_type_string(__entry->type),
>>>> How about moving the common fields of a TRB (like TRB type and
>>>> the cycle bit) to the place between ring type and trb decoding string?
>>>> And remove type and cycle decoding in xhci_decode_trb() as well.
>>>>
>>>> Something like:
>>>>
>>>> diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
>>>> index d01524b..f8ef7b8 100644
>>>> --- a/drivers/usb/host/xhci-trace.h
>>>> +++ b/drivers/usb/host/xhci-trace.h
>>>> @@ -132,9 +132,11 @@ DECLARE_EVENT_CLASS(xhci_log_trb,
>>>>                 __entry->field2 = le32_to_cpu(trb->field[2]);
>>>>                 __entry->field3 = le32_to_cpu(trb->field[3]);
>>>>         ),
>>>> -       TP_printk("%s: %s", xhci_ring_type_string(__entry->type),
>>>> -                       xhci_decode_trb(__entry->field0, __entry->field1,
>>>> -                                       __entry->field2, __entry->field3)
>>>> +       TP_printk("%s: %s: %c: %s", xhci_ring_type_string(__entry->type),
>>>> +                 xhci_trb_type_string(TRB_FIELD_TO_TYPE(__entry->field3)),
>>>> +                 __entry->field3 & TRB_CYCLE ? 'C' : 'c',
>>>> +                 xhci_decode_trb(__entry->field0, __entry->field1,
>>>> +                                 __entry->field2, __entry->field3)
>>> and what do I get with that? What's the actual benefit?
>> I thought that it could make xhci_decode_trb() a bit simpler.
> It'll have a very marginal simplification of a single function that's
> only called from tracepoints. I wonder if there's an actual benefit
> there.

Fair enough.

>
>>>>> +DEFINE_EVENT(xhci_log_trb, xhci_handle_event,
>>>>> +	TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
>>>>> +	TP_ARGS(ring, trb)
>>>>> +);
>>>>> +
>>>>> +DEFINE_EVENT(xhci_log_trb, xhci_handle_command,
>>>>> +	TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
>>>>> +	TP_ARGS(ring, trb)
>>>>> +);
>>>>> +
>>>>> +DEFINE_EVENT(xhci_log_trb, xhci_handle_transfer,
>>>>> +	TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
>>>>> +	TP_ARGS(ring, trb)
>>>>> +);
>>>>> +
>>>>> +DEFINE_EVENT(xhci_log_trb, xhci_queue_trb,
>>>>> +	TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
>>>>> +	TP_ARGS(ring, trb)
>>>>>  );
>>>> xhci_handle_command and xhci_handle_transfer are duplications
>>>> of xhci_handle_event. I suggest to remove them.
>>> no, they are not. They give us more granularity into which events we
>>> want to enable. 
>> Fair enough.
>>
>> But why not defining events for all possible event types as well
>> and dropping the all-in-one xhci_handle_event switch.
>>
>> A single event TRB will be traced twice in the same execution
>> path if xhci_handle_event and xhci_handle_command/transfer
>> are both enabled.
> no, it won't. Commands sit in the Command Ring. Events sit in the Event
> Ring. Transfers sit in the various Endpoint Rings.
>
> Compile the branch, enable the tracers and look at the output.
>
>>> They also make it clear where the even is coming
>>> from. Imagine how the trace would look like if I had a single event and
>>> just called trace_xhci_handle_event() from all locations. How would you
>>> differentiate from all possible call sites?
>> These events happen only in interrupt handler. There are no other
>> possible call sites.
> you misunderstand. Look at the output of the tracepoints and imagine how
> they would look if we had a single event for the TRB class of
> tracepoints.

Oh, yes, I totally misunderstood what events of xhci_handle_command
and xhci_handle_transfer do. I ever thought they are for command or
transfer completion event TRB. Sorry for that.

>>>> How about adding an event for the link TRBs. Something like,
>>>>
>>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>>>> index 4bad432..6dc8efb 100644
>>>> --- a/drivers/usb/host/xhci-ring.c
>>>> +++ b/drivers/usb/host/xhci-ring.c
>>>> @@ -173,6 +173,7 @@ static void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring)
>>>>                 ring->num_trbs_free++;
>>>>         }
>>>>         while (trb_is_link(ring->dequeue)) {
>>>> +               trace_xhci_link_trb(ring, ring->dequeue);
>>>>                 ring->deq_seg = ring->deq_seg->next;
>>>>                 ring->dequeue = ring->deq_seg->trbs;
>>>>         }
>>>> @@ -211,6 +212,7 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring,
>>>>         ring->enq_updates++;
>>>>         /* Update the dequeue pointer further if that was a link TRB */
>>>>         while (trb_is_link(next)) {
>>>> +               trace_xhci_link_trb(ring, next);
>>>>  
>>>>                 /*
>>>>                  * If the caller doesn't plan on enqueueing more TDs before
>>>> diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
>>>> index d01524b..f1a06b5 100644
>>>> --- a/drivers/usb/host/xhci-trace.h
>>>> +++ b/drivers/usb/host/xhci-trace.h
>>>> @@ -158,6 +158,10 @@ DEFINE_EVENT(xhci_log_trb, xhci_queue_trb,
>>>>         TP_ARGS(ring, trb)
>>>>  );
>>>>  
>>>> +DEFINE_EVENT(xhci_log_trb, xhci_link_trb,
>>>> +       TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
>>>> +       TP_ARGS(ring, trb)
>>>> +);
>>>>  #endif /* __XHCI_TRACE_H */
>>> what are you trying to achieve with this?
>> To trace and debug the ring itself, especially for ring auto expending and shrinking.
> every TRB is already traced. Every single one of them. Please compile
> the branch and look at the tracepoint output. Many of your questions
> will be answered by that simple exercise.
>

After reconsideration, I think there is no need to trace link trb. Link trb is a
static trb used to link multiple segments to form the space of a ring. There
is no need to trace a static trb. So please ignore this comment.

I have no further questions about this patch. I am very impressed by the
powerful trb decoder. And I am sorry for the misunderstanding. :-)

Best regards,
Lu Baolu

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux