Messages in this thread |  | | Date | Fri, 13 Feb 2009 20:19:32 +0200 | | From | Pekka Paalanen <> | | Subject | Re: [PATCH 2/2] tracing: add checks for printing pci devices in mmiotrace. |
| |
On Fri, 13 Feb 2009 14:09:52 +0800 Wenji Huang <wenji.huang@oracle.com> wrote:
> 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@oracle.com> > >> --- > >> 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.
Indeed, don't change the output format, please.
These checks are not enough to actually handle the case, mmio_read() needs to be updated, too, to do the right thing (I'm looking at Ingo's tip/master). I.e. it should skip to print_out to flush out old data. Shouldn't also *ppos be updated in mmio_read()?
Thanks for looking into this. -- Pekka Paalanen http://www.iki.fi/pq/
|  |