On Thu, 23 Jul 2026 08:24:35 +0900 Masami Hiramatsu (Google) <[email protected]> wrote:
> On Tue, 21 Jul 2026 21:20:10 -0400 > Steven Rostedt <[email protected]> wrote: > > > From: Steven Rostedt <[email protected]> > > > > When the mmiotrace trace was first created, it allocated a descriptor in > > its pipe_open() method. Since there was no pipe_close() method when it was > > created (in May of 2008, and pipe_close() was added in December of 2009), > > it cleaned up the allocated descriptors in the read. > > > > Now that the clean up is in the pipe_close() method that now exists, > > remove the clean up from the read as it is no longer needed. > > > > Also simplify the code by inverting the early exit conditional into a > > conditional to perform the logic and get rid of the goto. > > > > Link: > > https://lore.kernel.org/all/[email protected]/ > > Link: > > https://lore.kernel.org/all/[email protected]/ > > > > Signed-off-by: Steven Rostedt <[email protected]> > > --- > > kernel/trace/trace_mmiotrace.c | 14 +++----------- > > 1 file changed, 3 insertions(+), 11 deletions(-) > > > > diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c > > index b88b8d9923ad..ba604c22d2d2 100644 > > --- a/kernel/trace/trace_mmiotrace.c > > +++ b/kernel/trace/trace_mmiotrace.c > > @@ -142,21 +142,13 @@ static ssize_t mmio_read(struct trace_iterator *iter, > > struct file *filp, > > if (!overrun_detected) > > pr_warn("mmiotrace has lost events\n"); > > overrun_detected = true; > > - goto print_out; > > Is this intentional change? Removing this goto means we will change > the hiter->dev even if overrun happens. Previously we can resume > output in the next read for current hiter->dev, but this will skip > the current hiter->dev? But doesn't it still add to the buffer here? > > Thanks, > > > } > > > > - if (!hiter || !hiter->dev) > > - return 0; > > - > > - mmio_print_pcidev(s, hiter->dev); > > - hiter->dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, hiter->dev); > > - > > - if (!hiter->dev) { > > - destroy_header_iter(hiter); > > - iter->private = NULL; > > + if (hiter && hiter->dev) { > > + mmio_print_pcidev(s, hiter->dev); The current hiter->dev get's read here doesn't it? It's stored in the trace_seq and should still be printed. Or am I missing something? Either way, it looks like I should split this patch up into two: One that removes the unneeded clean up, and the other that removes the goto. The clean up removal is trivial with the new changes that cleans it up on close. But the removal of the goto has more subtle effects and we want to be able to detect it if it does cause issues. I wasn't planning on adding this patch to the next merge window anyway, so it can wait. -- Steve > > + hiter->dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, hiter->dev); > > } > > > > -print_out: > > ret = trace_seq_to_user(s, ubuf, cnt); > > return (ret == -EBUSY) ? 0 : ret; > > } > > -- > > 2.53.0 > > > >
