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
