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.
>>>> +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.
>>> 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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
