Re: [PATCH 2/2] tracing: add checks for printing pci devices in mmiotrace.

[Wenji Huang]

Steven Rostedt wrote:
> On Thu, 12 Feb 2009, Wenji Huang wrote:
>
> > This patch is to provide checking return value of trace_seq_printf
> > so as to keep completeness of mmio_print_pcidev.
> >
> > Signed-off-by: Wenji Huang <wenji.huang@xxxxxxxxxx>
> > ---
> >  kernel/trace/trace_mmiotrace.c |   26 ++++++++++++++++----------
> >  1 files changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
> > index c401b90..ebb0b61 100644
> > --- a/kernel/trace/trace_mmiotrace.c
> > +++ b/kernel/trace/trace_mmiotrace.c
> > @@ -56,17 +56,19 @@ static void mmio_trace_start(struct trace_array *tr)
> >  	mmio_reset_data(tr);
> >  }
> >  
> > -static int mmio_print_pcidev(struct trace_seq *s, const struct pci_dev *dev)
> > +static enum print_line_t mmio_print_pcidev(struct trace_seq *s,
> > +					   const struct pci_dev *dev)
> >  {
> >  	int ret = 0;
> >  	int i;
> >  	resource_size_t start, end;
> >  	const struct pci_driver *drv = pci_dev_driver(dev);
> >  
> > -	/* XXX: incomplete checks for trace_seq_printf() return value */
> > -	ret += trace_seq_printf(s, "PCIDEV %02x%02x %04x%04x %x",
> > +	ret = trace_seq_printf(s, "PCIDEV %02x%02x %04x%04x %x",
> >  				dev->bus->number, dev->devfn,
> >  				dev->vendor, dev->device, dev->irq);
> > +	if (!ret)
> > +		return TRACE_TYPE_PARTIAL_LINE;
> >  	/*
> >  	 * XXX: is pci_resource_to_user() appropriate, since we are
> >  	 * supposed to interpret the __ioremap() phys_addr argument based on
> > @@ -74,21 +76,25 @@ static int mmio_print_pcidev(struct trace_seq *s, const struct pci_dev *dev)
> >  	 */
> >  	for (i = 0; i < 7; i++) {
> >  		pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
> > -		ret += trace_seq_printf(s, " %llx",
> > +		ret = trace_seq_printf(s, " %llx",
> >  			(unsigned long long)(start |
> >  			(dev->resource[i].flags & PCI_REGION_FLAG_MASK)));
> > -	}
> > -	for (i = 0; i < 7; i++) {
> > +		if (!ret)
> > +			return TRACE_TYPE_PARTIAL_LINE;
>
> The above looks like a change of logic to me. I'm a bit tired so I can be 
> wrong. But it looks like the original was:
>
>      for (i = 0; i < 7; i++) {
>              pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
>              ret += trace_seq_printf(s, " %llx",
>                      (unsigned long long)(start |
>                      (dev->resource[i].flags & PCI_REGION_FLAG_MASK)));
>      }
>      for (i = 0; i < 7; i++) {
>              pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
>              ret += trace_seq_printf(s, " %llx",
>                      dev->resource[i].start < dev->resource[i].end ?
>                      (unsigned long long)(end - start) + 1 : 0);
>      }
>
> And the new is:
>
>      for (i = 0; i < 7; i++) {
>              pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
>              ret = trace_seq_printf(s, " %llx",   
>                      (unsigned long long)(start |
>                      (dev->resource[i].flags & PCI_REGION_FLAG_MASK)));
>              if (!ret)
>                      return TRACE_TYPE_PARTIAL_LINE;
>              pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
>              ret = trace_seq_printf(s, " %llx",
>                      dev->resource[i].start < dev->resource[i].end ?
>                      (unsigned long long)(end - start) + 1 : 0);
>              if (!ret)
>                      return TRACE_TYPE_PARTIAL_LINE;
>      }
>
> This changes the output format.
>
> Pekka,
>
> Is this OK?
>
> -- Steve

oops, output format is changed. Need to update it.


Regards,
wenji
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/