On Thu, 23 Jul 2026 09:49:58 -0400 Steven Rostedt <[email protected]> wrote:
> 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? Ah, I got it. Previously, it was separated into 2 reads, but this makes it 1 read. Got it. > > 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. OK, this looks good to me. Reviewed-by: Masami Hiramatsu (Google) <[email protected]> Thanks, > > -- 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 > > > > > > > > -- Masami Hiramatsu (Google) <[email protected]>
