On 21-03-02 09:56:05, Steven Rostedt wrote: > On Tue, 2 Mar 2021 16:23:55 +0800 > Peter Chen <peter.c...@kernel.org> wrote: > > s it looks like it uses %pa which IIUC from the printk code, it > > > >> dereferences the pointer to find it's virtual address. The event has > > > >> this as the field: > > > >> > > > >> __field(struct cdns3_trb *, start_trb_addr) > > > >> > > > >> Assigns it with: > > > >> > > > >> __entry->start_trb_addr = req->trb; > > > >> > > > >> And prints that with %pa, which will dereference pointer at the time of > > > >> reading, where the address in question may no longer be around. That > > > >> looks to me as a potential bug. > > > > Steven, thanks for reporting. Do you mind sending patch to fix it? > > If you have no time to do it, I will do it later. > > > > I would have already fixed it, but I wasn't exactly sure how this is used. > > In Documentation/core-api/printk-formats.rst we have: > > Physical address types phys_addr_t > ---------------------------------- > > :: > > %pa[p] 0x01234567 or 0x0123456789abcdef > > For printing a phys_addr_t type (and its derivatives, such as > resource_size_t) which can vary based on build options, regardless of the > width of the CPU data path. > > So it only looks like it is used to for the size of the pointer. > > I guess something like this might work: > > diff --git a/drivers/usb/cdns3/cdns3-trace.h b/drivers/usb/cdns3/cdns3-trace.h > index 8648c7a7a9dd..d3b8624fc427 100644 > --- a/drivers/usb/cdns3/cdns3-trace.h > +++ b/drivers/usb/cdns3/cdns3-trace.h > @@ -214,7 +214,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request, > __field(int, no_interrupt) > __field(int, start_trb) > __field(int, end_trb) > - __field(struct cdns3_trb *, start_trb_addr) > + __field(phys_addr_t, start_trb_addr) > __field(int, flags) > __field(unsigned int, stream_id) > ), > @@ -230,7 +230,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request, > __entry->no_interrupt = req->request.no_interrupt; > __entry->start_trb = req->start_trb; > __entry->end_trb = req->end_trb; > - __entry->start_trb_addr = req->trb; > + __entry->start_trb_addr = *(const phys_addr_t *)req->trb; > __entry->flags = req->flags; > __entry->stream_id = req->request.stream_id; > ), > @@ -244,7 +244,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request, > __entry->status, > __entry->start_trb, > __entry->end_trb, > - __entry->start_trb_addr, > + /* %pa dereferences */ &__entry->start_trb_addr, > __entry->flags, > __entry->stream_id > ) > > > Can you please test it? I don't have the hardware, but I also want to make > sure I don't break anything. >
Hi Steve, Regarding this issue, I have one question: - If the virtual address is got from dma_alloc_coherent, can't we print this address using %pa to get its physical address (the same with DMA address), or its DMA address using %pad? req->trb is the virtual address got from dma_alloc_coherent. And what's the logic for this "unsafe dereference" warning? Thanks. -- Thanks, Peter Chen