Re: [PATCH] tracing: Do not clean up hiter in mmiotrace read function

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Thu, 23 Jul 2026 09:49:58 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 23 Jul 2026 08:24:35 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > On Tue, 21 Jul 2026 21:20:10 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > From: Steven Rostedt <rostedt@goodmis.org>
> > > 
> > > 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/20260715143604.14481-1-gaikwad.dcg@gmail.com/
> > > Link: https://lore.kernel.org/all/20260721211143.36dbd559@gandalf.local.home/
> > > 
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > > ---
> > >  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) <mhiramat@kernel.org>

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) <mhiramat@kernel.org>