| Message ID | 20211208163255.114660-6-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | libata: rework logging, take II | expand |
On 2021/12/09 1:31, Hannes Reinecke wrote: > Implement module parameter 'pci_dump' and move the DPRINTK calls > over to dev_printk(). > > Signed-off-by: Hannes Reinecke <hare@suse.de> > Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > --- > drivers/ata/sata_mv.c | 88 ++++++++++++++++++++++++------------------- > 1 file changed, 49 insertions(+), 39 deletions(-) > > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c > index cae4c1eab102..f0257685495f 100644 > --- a/drivers/ata/sata_mv.c > +++ b/drivers/ata/sata_mv.c > @@ -83,6 +83,10 @@ module_param(irq_coalescing_usecs, int, S_IRUGO); > MODULE_PARM_DESC(irq_coalescing_usecs, > "IRQ coalescing time threshold in usecs"); > > +static int pci_dump; > +module_param(pci_dump, int, S_IRUGO); > +MODULE_PARM_DESC(pci_dump, "Enable dumping of PCI registers on error"); > + > enum { > /* BAR's are enumerated in terms of pci_resource_start() terms */ > MV_PRIMARY_BAR = 0, /* offset 0x10: memory space */ > @@ -1248,42 +1252,43 @@ static int mv_stop_edma(struct ata_port *ap) > return err; > } > > -#ifdef ATA_DEBUG > -static void mv_dump_mem(void __iomem *start, unsigned bytes) > +static void mv_dump_mem(struct device *dev, void __iomem *start, unsigned bytes) > { > - int b, w; > + int b, w, o; > + unsigned char linebuf[38]; > + > for (b = 0; b < bytes; ) { > - DPRINTK("%p: ", start + b); > - for (w = 0; b < bytes && w < 4; w++) { > - printk("%08x ", readl(start + b)); > + for (w = 0, o = 0; b < bytes && w < 4; w++) { > + o += snprintf(linebuf + o, 38 - o, > + "%08x ", readl(start + b)); > b += sizeof(u32); > } > - printk("\n"); > + dev_printk(KERN_DEBUG, dev, "%s: %p: %s\n", > + __func__, start + b, linebuf); Why not dev_dbg() ? Same comment for all the prints below. > } > } > -#endif > -#if defined(ATA_DEBUG) || defined(CONFIG_PCI) > + > static void mv_dump_pci_cfg(struct pci_dev *pdev, unsigned bytes) > { > -#ifdef ATA_DEBUG > - int b, w; > + int b, w, o; > u32 dw; > + unsigned char linebuf[38]; > + > for (b = 0; b < bytes; ) { > - DPRINTK("%02x: ", b); > - for (w = 0; b < bytes && w < 4; w++) { > + for (w = 0, o = 0; b < bytes && w < 4; w++) { > (void) pci_read_config_dword(pdev, b, &dw); > - printk("%08x ", dw); > + o += snprintf(linebuf + o, 38 - o, > + "%08x ", dw); > b += sizeof(u32); > } > - printk("\n"); > + dev_printk(KERN_DEBUG, &pdev->dev, "%s: %02x: %s\n", > + __func__, b, linebuf); > } > -#endif > } > -#endif > + > static void mv_dump_all_regs(void __iomem *mmio_base, int port, > struct pci_dev *pdev) > { > -#ifdef ATA_DEBUG > void __iomem *hc_base = mv_hc_base(mmio_base, > port >> MV_PORT_HC_SHIFT); > void __iomem *port_base; > @@ -1298,31 +1303,34 @@ static void mv_dump_all_regs(void __iomem *mmio_base, int port, > start_port = port; > num_ports = num_hcs = 1; > } > - DPRINTK("All registers for port(s) %u-%u:\n", start_port, > - num_ports > 1 ? num_ports - 1 : start_port); > + dev_printk(KERN_DEBUG, &pdev->dev, > + "%s: All registers for port(s) %u-%u:\n", __func__, > + start_port, num_ports > 1 ? num_ports - 1 : start_port); > > - if (NULL != pdev) { > - DPRINTK("PCI config space regs:\n"); > - mv_dump_pci_cfg(pdev, 0x68); > - } > - DPRINTK("PCI regs:\n"); > - mv_dump_mem(mmio_base+0xc00, 0x3c); > - mv_dump_mem(mmio_base+0xd00, 0x34); > - mv_dump_mem(mmio_base+0xf00, 0x4); > - mv_dump_mem(mmio_base+0x1d00, 0x6c); > + dev_printk(KERN_DEBUG, &pdev->dev, > + "%s: PCI config space regs:\n", __func__); > + mv_dump_pci_cfg(pdev, 0x68); > + > + dev_printk(KERN_DEBUG, &pdev->dev, "%s: PCI regs:\n", __func__); > + mv_dump_mem(&pdev->dev, mmio_base+0xc00, 0x3c); > + mv_dump_mem(&pdev->dev, mmio_base+0xd00, 0x34); > + mv_dump_mem(&pdev->dev, mmio_base+0xf00, 0x4); > + mv_dump_mem(&pdev->dev, mmio_base+0x1d00, 0x6c); > for (hc = start_hc; hc < start_hc + num_hcs; hc++) { > hc_base = mv_hc_base(mmio_base, hc); > - DPRINTK("HC regs (HC %i):\n", hc); > - mv_dump_mem(hc_base, 0x1c); > + dev_printk(KERN_DEBUG, &pdev->dev, "%s: HC regs (HC %i):\n", > + __func__, hc); > + mv_dump_mem(&pdev->dev, hc_base, 0x1c); > } > for (p = start_port; p < start_port + num_ports; p++) { > port_base = mv_port_base(mmio_base, p); > - DPRINTK("EDMA regs (port %i):\n", p); > - mv_dump_mem(port_base, 0x54); > - DPRINTK("SATA regs (port %i):\n", p); > - mv_dump_mem(port_base+0x300, 0x60); > + dev_printk(KERN_DEBUG, &pdev->dev, "%s: EDMA regs (port %i):\n", > + __func__, p); > + mv_dump_mem(&pdev->dev, port_base, 0x54); > + dev_printk(KERN_DEBUG, &pdev->dev, "%s: SATA regs (port %i):\n", > + __func__, p); > + mv_dump_mem(&pdev->dev, port_base+0x300, 0x60); > } > -#endif > } > > static unsigned int mv_scr_offset(unsigned int sc_reg_in) > @@ -2962,9 +2970,11 @@ static int mv_pci_error(struct ata_host *host, void __iomem *mmio) > > dev_err(host->dev, "PCI ERROR; PCI IRQ cause=0x%08x\n", err_cause); > > - DPRINTK("All regs @ PCI error\n"); > - mv_dump_all_regs(mmio, -1, to_pci_dev(host->dev)); > - > + if (pci_dump) { > + dev_printk(KERN_DEBUG, host->dev, "%s: All regs @ PCI error\n", > + __func__); > + mv_dump_all_regs(mmio, -1, to_pci_dev(host->dev)); > + } > writelfl(0, mmio + hpriv->irq_cause_offset); > > for (i = 0; i < host->n_ports; i++) { >
On 12/9/21 1:38 AM, Damien Le Moal wrote: > On 2021/12/09 1:31, Hannes Reinecke wrote: >> Implement module parameter 'pci_dump' and move the DPRINTK calls >> over to dev_printk(). >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >> --- >> drivers/ata/sata_mv.c | 88 ++++++++++++++++++++++++------------------- >> 1 file changed, 49 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c >> index cae4c1eab102..f0257685495f 100644 >> --- a/drivers/ata/sata_mv.c >> +++ b/drivers/ata/sata_mv.c >> @@ -83,6 +83,10 @@ module_param(irq_coalescing_usecs, int, S_IRUGO); >> MODULE_PARM_DESC(irq_coalescing_usecs, >> "IRQ coalescing time threshold in usecs"); >> >> +static int pci_dump; >> +module_param(pci_dump, int, S_IRUGO); >> +MODULE_PARM_DESC(pci_dump, "Enable dumping of PCI registers on error"); >> + >> enum { >> /* BAR's are enumerated in terms of pci_resource_start() terms */ >> MV_PRIMARY_BAR = 0, /* offset 0x10: memory space */ >> @@ -1248,42 +1252,43 @@ static int mv_stop_edma(struct ata_port *ap) >> return err; >> } >> >> -#ifdef ATA_DEBUG >> -static void mv_dump_mem(void __iomem *start, unsigned bytes) >> +static void mv_dump_mem(struct device *dev, void __iomem *start, unsigned bytes) >> { >> - int b, w; >> + int b, w, o; >> + unsigned char linebuf[38]; >> + >> for (b = 0; b < bytes; ) { >> - DPRINTK("%p: ", start + b); >> - for (w = 0; b < bytes && w < 4; w++) { >> - printk("%08x ", readl(start + b)); >> + for (w = 0, o = 0; b < bytes && w < 4; w++) { >> + o += snprintf(linebuf + o, 38 - o, >> + "%08x ", readl(start + b)); >> b += sizeof(u32); >> } >> - printk("\n"); >> + dev_printk(KERN_DEBUG, dev, "%s: %p: %s\n", >> + __func__, start + b, linebuf); > > Why not dev_dbg() ? Same comment for all the prints below. > Because the entire thing is encapsulated by the 'pci_dump' parameter, ie it'll be only ever called when 'pci_dump' is set. So it feels redundant to require both, 'pci_dump' _and_ dynamic debugging. But I can move it over to dynamic debugging and kill the pci_dump parameter. Cheers, Hannes
On 2021/12/09 16:17, Hannes Reinecke wrote: > On 12/9/21 1:38 AM, Damien Le Moal wrote: >> On 2021/12/09 1:31, Hannes Reinecke wrote: >>> Implement module parameter 'pci_dump' and move the DPRINTK calls >>> over to dev_printk(). >>> >>> Signed-off-by: Hannes Reinecke <hare@suse.de> >>> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >>> --- >>> drivers/ata/sata_mv.c | 88 ++++++++++++++++++++++++------------------- >>> 1 file changed, 49 insertions(+), 39 deletions(-) >>> >>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c >>> index cae4c1eab102..f0257685495f 100644 >>> --- a/drivers/ata/sata_mv.c >>> +++ b/drivers/ata/sata_mv.c >>> @@ -83,6 +83,10 @@ module_param(irq_coalescing_usecs, int, S_IRUGO); >>> MODULE_PARM_DESC(irq_coalescing_usecs, >>> "IRQ coalescing time threshold in usecs"); >>> >>> +static int pci_dump; >>> +module_param(pci_dump, int, S_IRUGO); >>> +MODULE_PARM_DESC(pci_dump, "Enable dumping of PCI registers on error"); >>> + >>> enum { >>> /* BAR's are enumerated in terms of pci_resource_start() terms */ >>> MV_PRIMARY_BAR = 0, /* offset 0x10: memory space */ >>> @@ -1248,42 +1252,43 @@ static int mv_stop_edma(struct ata_port *ap) >>> return err; >>> } >>> >>> -#ifdef ATA_DEBUG >>> -static void mv_dump_mem(void __iomem *start, unsigned bytes) >>> +static void mv_dump_mem(struct device *dev, void __iomem *start, unsigned bytes) >>> { >>> - int b, w; >>> + int b, w, o; >>> + unsigned char linebuf[38]; >>> + >>> for (b = 0; b < bytes; ) { >>> - DPRINTK("%p: ", start + b); >>> - for (w = 0; b < bytes && w < 4; w++) { >>> - printk("%08x ", readl(start + b)); >>> + for (w = 0, o = 0; b < bytes && w < 4; w++) { >>> + o += snprintf(linebuf + o, 38 - o, >>> + "%08x ", readl(start + b)); >>> b += sizeof(u32); >>> } >>> - printk("\n"); >>> + dev_printk(KERN_DEBUG, dev, "%s: %p: %s\n", >>> + __func__, start + b, linebuf); >> >> Why not dev_dbg() ? Same comment for all the prints below. >> > Because the entire thing is encapsulated by the 'pci_dump' parameter, ie > it'll be only ever called when 'pci_dump' is set. > So it feels redundant to require both, 'pci_dump' _and_ dynamic debugging. > > But I can move it over to dynamic debugging and kill the pci_dump parameter. That sounds simpler to me. > > Cheers, > > Hannes
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c index cae4c1eab102..f0257685495f 100644 --- a/drivers/ata/sata_mv.c +++ b/drivers/ata/sata_mv.c @@ -83,6 +83,10 @@ module_param(irq_coalescing_usecs, int, S_IRUGO); MODULE_PARM_DESC(irq_coalescing_usecs, "IRQ coalescing time threshold in usecs"); +static int pci_dump; +module_param(pci_dump, int, S_IRUGO); +MODULE_PARM_DESC(pci_dump, "Enable dumping of PCI registers on error"); + enum { /* BAR's are enumerated in terms of pci_resource_start() terms */ MV_PRIMARY_BAR = 0, /* offset 0x10: memory space */ @@ -1248,42 +1252,43 @@ static int mv_stop_edma(struct ata_port *ap) return err; } -#ifdef ATA_DEBUG -static void mv_dump_mem(void __iomem *start, unsigned bytes) +static void mv_dump_mem(struct device *dev, void __iomem *start, unsigned bytes) { - int b, w; + int b, w, o; + unsigned char linebuf[38]; + for (b = 0; b < bytes; ) { - DPRINTK("%p: ", start + b); - for (w = 0; b < bytes && w < 4; w++) { - printk("%08x ", readl(start + b)); + for (w = 0, o = 0; b < bytes && w < 4; w++) { + o += snprintf(linebuf + o, 38 - o, + "%08x ", readl(start + b)); b += sizeof(u32); } - printk("\n"); + dev_printk(KERN_DEBUG, dev, "%s: %p: %s\n", + __func__, start + b, linebuf); } } -#endif -#if defined(ATA_DEBUG) || defined(CONFIG_PCI) + static void mv_dump_pci_cfg(struct pci_dev *pdev, unsigned bytes) { -#ifdef ATA_DEBUG - int b, w; + int b, w, o; u32 dw; + unsigned char linebuf[38]; + for (b = 0; b < bytes; ) { - DPRINTK("%02x: ", b); - for (w = 0; b < bytes && w < 4; w++) { + for (w = 0, o = 0; b < bytes && w < 4; w++) { (void) pci_read_config_dword(pdev, b, &dw); - printk("%08x ", dw); + o += snprintf(linebuf + o, 38 - o, + "%08x ", dw); b += sizeof(u32); } - printk("\n"); + dev_printk(KERN_DEBUG, &pdev->dev, "%s: %02x: %s\n", + __func__, b, linebuf); } -#endif } -#endif + static void mv_dump_all_regs(void __iomem *mmio_base, int port, struct pci_dev *pdev) { -#ifdef ATA_DEBUG void __iomem *hc_base = mv_hc_base(mmio_base, port >> MV_PORT_HC_SHIFT); void __iomem *port_base; @@ -1298,31 +1303,34 @@ static void mv_dump_all_regs(void __iomem *mmio_base, int port, start_port = port; num_ports = num_hcs = 1; } - DPRINTK("All registers for port(s) %u-%u:\n", start_port, - num_ports > 1 ? num_ports - 1 : start_port); + dev_printk(KERN_DEBUG, &pdev->dev, + "%s: All registers for port(s) %u-%u:\n", __func__, + start_port, num_ports > 1 ? num_ports - 1 : start_port); - if (NULL != pdev) { - DPRINTK("PCI config space regs:\n"); - mv_dump_pci_cfg(pdev, 0x68); - } - DPRINTK("PCI regs:\n"); - mv_dump_mem(mmio_base+0xc00, 0x3c); - mv_dump_mem(mmio_base+0xd00, 0x34); - mv_dump_mem(mmio_base+0xf00, 0x4); - mv_dump_mem(mmio_base+0x1d00, 0x6c); + dev_printk(KERN_DEBUG, &pdev->dev, + "%s: PCI config space regs:\n", __func__); + mv_dump_pci_cfg(pdev, 0x68); + + dev_printk(KERN_DEBUG, &pdev->dev, "%s: PCI regs:\n", __func__); + mv_dump_mem(&pdev->dev, mmio_base+0xc00, 0x3c); + mv_dump_mem(&pdev->dev, mmio_base+0xd00, 0x34); + mv_dump_mem(&pdev->dev, mmio_base+0xf00, 0x4); + mv_dump_mem(&pdev->dev, mmio_base+0x1d00, 0x6c); for (hc = start_hc; hc < start_hc + num_hcs; hc++) { hc_base = mv_hc_base(mmio_base, hc); - DPRINTK("HC regs (HC %i):\n", hc); - mv_dump_mem(hc_base, 0x1c); + dev_printk(KERN_DEBUG, &pdev->dev, "%s: HC regs (HC %i):\n", + __func__, hc); + mv_dump_mem(&pdev->dev, hc_base, 0x1c); } for (p = start_port; p < start_port + num_ports; p++) { port_base = mv_port_base(mmio_base, p); - DPRINTK("EDMA regs (port %i):\n", p); - mv_dump_mem(port_base, 0x54); - DPRINTK("SATA regs (port %i):\n", p); - mv_dump_mem(port_base+0x300, 0x60); + dev_printk(KERN_DEBUG, &pdev->dev, "%s: EDMA regs (port %i):\n", + __func__, p); + mv_dump_mem(&pdev->dev, port_base, 0x54); + dev_printk(KERN_DEBUG, &pdev->dev, "%s: SATA regs (port %i):\n", + __func__, p); + mv_dump_mem(&pdev->dev, port_base+0x300, 0x60); } -#endif } static unsigned int mv_scr_offset(unsigned int sc_reg_in) @@ -2962,9 +2970,11 @@ static int mv_pci_error(struct ata_host *host, void __iomem *mmio) dev_err(host->dev, "PCI ERROR; PCI IRQ cause=0x%08x\n", err_cause); - DPRINTK("All regs @ PCI error\n"); - mv_dump_all_regs(mmio, -1, to_pci_dev(host->dev)); - + if (pci_dump) { + dev_printk(KERN_DEBUG, host->dev, "%s: All regs @ PCI error\n", + __func__); + mv_dump_all_regs(mmio, -1, to_pci_dev(host->dev)); + } writelfl(0, mmio + hpriv->irq_cause_offset); for (i = 0; i < host->n_ports; i++) {