| Message ID | 20181213104716.31930-13-hare@suse.de |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | None | expand |
On 12/13/2018 01:46 PM, Hannes Reinecke wrote: > Replace all DPRINTK calls with the ata_XXX_dbg functions. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/ata/sata_nv.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c > index 72c9b922a77b..aa2611d638ea 100644 > --- a/drivers/ata/sata_nv.c > +++ b/drivers/ata/sata_nv.c > @@ -1451,7 +1451,7 @@ static unsigned int nv_adma_qc_issue(struct ata_queued_cmd *qc) > > writew(qc->hw_tag, mmio + NV_ADMA_APPEND); > > - DPRINTK("Issued tag %u\n", qc->hw_tag); > + ata_dev_dbg(qc->dev, "Issued tag %u\n", qc->hw_tag); Don't we lose printing out __func__ this way? > > return 0; > } > @@ -2029,8 +2029,6 @@ static unsigned int nv_swncq_issue_atacmd(struct ata_port *ap, > if (qc == NULL) > return 0; > > - DPRINTK("Enter\n"); > - You said "replace all", not "remove some". :-) Though w/o __func__ this is pretty useless indeed... [...] > @@ -2053,7 +2051,7 @@ static unsigned int nv_swncq_qc_issue(struct ata_queued_cmd *qc) > if (qc->tf.protocol != ATA_PROT_NCQ) > return ata_bmdma_qc_issue(qc); > > - DPRINTK("Enter\n"); > + ata_dev_dbg(qc->dev, "Enter\n"); Same here, do we print out __func__ now? Else this is quite pointless. > > if (!pp->qc_active) > nv_swncq_issue_atacmd(ap, qc); [...] > @@ -2136,10 +2134,10 @@ static int nv_swncq_sdbfis(struct ata_port *ap) > */ > lack_dhfis = 1; > > - DPRINTK("id 0x%x QC: qc_active 0x%x," > + ata_port_dbg(ap, "QC: qc_active 0x%llx," Why silently change "%x" to "%llx"? > "SWNCQ:qc_active 0x%X defer_bits %X " > "dhfis 0x%X dmafis 0x%X last_issue_tag %x\n", > - ap->print_id, ap->qc_active, pp->qc_active, > + ap->qc_active, pp->qc_active, > pp->defer_queue.defer_bits, pp->dhfis_bits, > pp->dmafis_bits, pp->last_issue_tag); > [...] MBR, Sergei
On 12/14/18 4:27 PM, Sergei Shtylyov wrote: > On 12/13/2018 01:46 PM, Hannes Reinecke wrote: > >> Replace all DPRINTK calls with the ata_XXX_dbg functions. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> drivers/ata/sata_nv.c | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c >> index 72c9b922a77b..aa2611d638ea 100644 >> --- a/drivers/ata/sata_nv.c >> +++ b/drivers/ata/sata_nv.c >> @@ -1451,7 +1451,7 @@ static unsigned int nv_adma_qc_issue(struct ata_queued_cmd *qc) >> >> writew(qc->hw_tag, mmio + NV_ADMA_APPEND); >> >> - DPRINTK("Issued tag %u\n", qc->hw_tag); >> + ata_dev_dbg(qc->dev, "Issued tag %u\n", qc->hw_tag); > > Don't we lose printing out __func__ this way? > Yes, but given that this message is pretty unique (for this driver) I thought the omission wasn't too bad. I can re-add it if you insist... >> >> return 0; >> } >> @@ -2029,8 +2029,6 @@ static unsigned int nv_swncq_issue_atacmd(struct ata_port *ap, >> if (qc == NULL) >> return 0; >> >> - DPRINTK("Enter\n"); >> - > > You said "replace all", not "remove some". :-) > Though w/o __func__ this is pretty useless indeed... > Which is why I removed it. I'll be updating the description. > [...] >> @@ -2053,7 +2051,7 @@ static unsigned int nv_swncq_qc_issue(struct ata_queued_cmd *qc) >> if (qc->tf.protocol != ATA_PROT_NCQ) >> return ata_bmdma_qc_issue(qc); >> >> - DPRINTK("Enter\n"); >> + ata_dev_dbg(qc->dev, "Enter\n"); > > > Same here, do we print out __func__ now? Else this is quite pointless. > From what I can see this is primarily so that you can trace the control flow, but I wonder if there are not better ways nowadays (one thinks of ftrace ...). I guess I'll just drop the pointless "ENTER" messages. >> >> if (!pp->qc_active) >> nv_swncq_issue_atacmd(ap, qc); > [...] >> @@ -2136,10 +2134,10 @@ static int nv_swncq_sdbfis(struct ata_port *ap) >> */ >> lack_dhfis = 1; >> >> - DPRINTK("id 0x%x QC: qc_active 0x%x," >> + ata_port_dbg(ap, "QC: qc_active 0x%llx," > > Why silently change "%x" to "%llx"? > Because the compiler complained? Do I need to update the description for this change, too? Cheers, Hannes
On 12/14/2018 09:13 PM, Hannes Reinecke wrote: >>> Replace all DPRINTK calls with the ata_XXX_dbg functions. >>> >>> Signed-off-by: Hannes Reinecke <hare@suse.com> >>> --- >>> drivers/ata/sata_nv.c | 22 ++++++++++------------ >>> 1 file changed, 10 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c >>> index 72c9b922a77b..aa2611d638ea 100644 >>> --- a/drivers/ata/sata_nv.c >>> +++ b/drivers/ata/sata_nv.c >>> @@ -1451,7 +1451,7 @@ static unsigned int nv_adma_qc_issue(struct ata_queued_cmd *qc) >>> writew(qc->hw_tag, mmio + NV_ADMA_APPEND); >>> - DPRINTK("Issued tag %u\n", qc->hw_tag); >>> + ata_dev_dbg(qc->dev, "Issued tag %u\n", qc->hw_tag); >> >> Don't we lose printing out __func__ this way? >> > Yes, but given that this message is pretty unique (for this driver) I thought the omission wasn't too bad. > I can re-add it if you insist... Well, may be it makes sense to just re-#define DPRINTK(), so that it doesn't require #define ATA_DEBUG and calls *_dbg() and leave the invocations themselves alone? [...] >>> @@ -2053,7 +2051,7 @@ static unsigned int nv_swncq_qc_issue(struct ata_queued_cmd *qc) >>> if (qc->tf.protocol != ATA_PROT_NCQ) >>> return ata_bmdma_qc_issue(qc); >>> - DPRINTK("Enter\n"); >>> + ata_dev_dbg(qc->dev, "Enter\n"); >> >> >> Same here, do we print out __func__ now? Else this is quite pointless. >> > From what I can see this is primarily so that you can trace the control flow, but I wonder if there are not better ways nowadays (one thinks of ftrace ...). Yeah, I've heard about ftrace but never used it... > I guess I'll just drop the pointless "ENTER" messages. That's an option too... >>> if (!pp->qc_active) >>> nv_swncq_issue_atacmd(ap, qc); >> [...] >>> @@ -2136,10 +2134,10 @@ static int nv_swncq_sdbfis(struct ata_port *ap) >>> */ >>> lack_dhfis = 1; >>> - DPRINTK("id 0x%x QC: qc_active 0x%x," >>> + ata_port_dbg(ap, "QC: qc_active 0x%llx," >> >> Why silently change "%x" to "%llx"? >> > Because the compiler complained? > > Do I need to update the description for this change, too? No, this is pre-existing bug -- you need to fix it first, before your clean-ups. > Cheers, > > Hannes MBR, Sergei
On 12/13/18 11:46 AM, Hannes Reinecke wrote: > Replace all DPRINTK calls with the ata_XXX_dbg functions. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/ata/sata_nv.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c > index 72c9b922a77b..aa2611d638ea 100644 > --- a/drivers/ata/sata_nv.c > +++ b/drivers/ata/sata_nv.c > @@ -1451,7 +1451,7 @@ static unsigned int nv_adma_qc_issue(struct ata_queued_cmd *qc) > > writew(qc->hw_tag, mmio + NV_ADMA_APPEND); > > - DPRINTK("Issued tag %u\n", qc->hw_tag); > + ata_dev_dbg(qc->dev, "Issued tag %u\n", qc->hw_tag); Please preserve __func__ printing in the conversion. > return 0; > } > @@ -2029,8 +2029,6 @@ static unsigned int nv_swncq_issue_atacmd(struct ata_port *ap, > if (qc == NULL) > return 0; > > - DPRINTK("Enter\n"); > - Please either keep it or document the removal in the patch description. > writel((1 << qc->hw_tag), pp->sactive_block); > pp->last_issue_tag = qc->hw_tag; > pp->dhfis_bits &= ~(1 << qc->hw_tag); > @@ -2040,7 +2038,7 @@ static unsigned int nv_swncq_issue_atacmd(struct ata_port *ap, > ap->ops->sff_tf_load(ap, &qc->tf); /* load tf registers */ > ap->ops->sff_exec_command(ap, &qc->tf); > > - DPRINTK("Issued tag %u\n", qc->hw_tag); > + ata_dev_dbg(qc->dev, "Issued tag %u\n", qc->hw_tag); Please preserve __func__ printing in the conversion. > return 0; > } > @@ -2053,7 +2051,7 @@ static unsigned int nv_swncq_qc_issue(struct ata_queued_cmd *qc) > if (qc->tf.protocol != ATA_PROT_NCQ) > return ata_bmdma_qc_issue(qc); > > - DPRINTK("Enter\n"); > + ata_dev_dbg(qc->dev, "Enter\n"); ditto > if (!pp->qc_active) > nv_swncq_issue_atacmd(ap, qc); > @@ -2121,7 +2119,7 @@ static int nv_swncq_sdbfis(struct ata_port *ap) > ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask); > > if (!ap->qc_active) { > - DPRINTK("over\n"); > + ata_port_dbg(ap, "over\n"); ditto > nv_swncq_pp_reinit(ap); > return 0; > } > @@ -2136,10 +2134,10 @@ static int nv_swncq_sdbfis(struct ata_port *ap) > */ > lack_dhfis = 1; > > - DPRINTK("id 0x%x QC: qc_active 0x%x," > + ata_port_dbg(ap, "QC: qc_active 0x%llx," ditto > "SWNCQ:qc_active 0x%X defer_bits %X " > "dhfis 0x%X dmafis 0x%X last_issue_tag %x\n", > - ap->print_id, ap->qc_active, pp->qc_active, > + ap->qc_active, pp->qc_active, > pp->defer_queue.defer_bits, pp->dhfis_bits, > pp->dmafis_bits, pp->last_issue_tag); > > @@ -2181,7 +2179,7 @@ static void nv_swncq_dmafis(struct ata_port *ap) > __ata_bmdma_stop(ap); > tag = nv_swncq_tag(ap); > > - DPRINTK("dma setup tag 0x%x\n", tag); > + ata_port_dbg(ap, "dma setup tag 0x%x\n", tag); ditto > qc = ata_qc_from_tag(ap, tag); > > if (unlikely(!qc)) > @@ -2249,9 +2247,9 @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis) > > if (fis & NV_SWNCQ_IRQ_SDBFIS) { > pp->ncq_flags |= ncq_saw_sdb; > - DPRINTK("id 0x%x SWNCQ: qc_active 0x%X " > + ata_port_dbg(ap, "SWNCQ: qc_active 0x%X " ditto > "dhfis 0x%X dmafis 0x%X sactive 0x%X\n", > - ap->print_id, pp->qc_active, pp->dhfis_bits, > + pp->qc_active, pp->dhfis_bits, > pp->dmafis_bits, readl(pp->sactive_block)); > if (nv_swncq_sdbfis(ap) < 0) > goto irq_error; > @@ -2277,7 +2275,7 @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis) > goto irq_exit; > > if (pp->defer_queue.defer_bits) { > - DPRINTK("send next command\n"); > + ata_port_dbg(ap, "send next command\n"); ditto > qc = nv_swncq_qc_from_dq(ap); > nv_swncq_issue_atacmd(ap, qc); > } > Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On 1/30/20 11:46 AM, Bartlomiej Zolnierkiewicz wrote: > > On 12/13/18 11:46 AM, Hannes Reinecke wrote: >> Replace all DPRINTK calls with the ata_XXX_dbg functions. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> drivers/ata/sata_nv.c | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c >> index 72c9b922a77b..aa2611d638ea 100644 >> --- a/drivers/ata/sata_nv.c >> +++ b/drivers/ata/sata_nv.c >> @@ -1451,7 +1451,7 @@ static unsigned int nv_adma_qc_issue(struct ata_queued_cmd *qc) >> >> writew(qc->hw_tag, mmio + NV_ADMA_APPEND); >> >> - DPRINTK("Issued tag %u\n", qc->hw_tag); >> + ata_dev_dbg(qc->dev, "Issued tag %u\n", qc->hw_tag); > > Please preserve __func__ printing in the conversion. > >> return 0; >> } >> @@ -2029,8 +2029,6 @@ static unsigned int nv_swncq_issue_atacmd(struct ata_port *ap, >> if (qc == NULL) >> return 0; >> >> - DPRINTK("Enter\n"); >> - > > Please either keep it or document the removal in the patch description. > >> writel((1 << qc->hw_tag), pp->sactive_block); >> pp->last_issue_tag = qc->hw_tag; >> pp->dhfis_bits &= ~(1 << qc->hw_tag); >> @@ -2040,7 +2038,7 @@ static unsigned int nv_swncq_issue_atacmd(struct ata_port *ap, >> ap->ops->sff_tf_load(ap, &qc->tf); /* load tf registers */ >> ap->ops->sff_exec_command(ap, &qc->tf); >> >> - DPRINTK("Issued tag %u\n", qc->hw_tag); >> + ata_dev_dbg(qc->dev, "Issued tag %u\n", qc->hw_tag); > > Please preserve __func__ printing in the conversion. > >> return 0; >> } >> @@ -2053,7 +2051,7 @@ static unsigned int nv_swncq_qc_issue(struct ata_queued_cmd *qc) >> if (qc->tf.protocol != ATA_PROT_NCQ) >> return ata_bmdma_qc_issue(qc); >> >> - DPRINTK("Enter\n"); >> + ata_dev_dbg(qc->dev, "Enter\n"); > > ditto > >> if (!pp->qc_active) >> nv_swncq_issue_atacmd(ap, qc); >> @@ -2121,7 +2119,7 @@ static int nv_swncq_sdbfis(struct ata_port *ap) >> ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask); >> >> if (!ap->qc_active) { >> - DPRINTK("over\n"); >> + ata_port_dbg(ap, "over\n"); > > ditto > >> nv_swncq_pp_reinit(ap); >> return 0; >> } >> @@ -2136,10 +2134,10 @@ static int nv_swncq_sdbfis(struct ata_port *ap) >> */ >> lack_dhfis = 1; >> >> - DPRINTK("id 0x%x QC: qc_active 0x%x," >> + ata_port_dbg(ap, "QC: qc_active 0x%llx," > > ditto > >> "SWNCQ:qc_active 0x%X defer_bits %X " >> "dhfis 0x%X dmafis 0x%X last_issue_tag %x\n", >> - ap->print_id, ap->qc_active, pp->qc_active, >> + ap->qc_active, pp->qc_active, >> pp->defer_queue.defer_bits, pp->dhfis_bits, >> pp->dmafis_bits, pp->last_issue_tag); >> >> @@ -2181,7 +2179,7 @@ static void nv_swncq_dmafis(struct ata_port *ap) >> __ata_bmdma_stop(ap); >> tag = nv_swncq_tag(ap); >> >> - DPRINTK("dma setup tag 0x%x\n", tag); >> + ata_port_dbg(ap, "dma setup tag 0x%x\n", tag); > > ditto > >> qc = ata_qc_from_tag(ap, tag); >> >> if (unlikely(!qc)) >> @@ -2249,9 +2247,9 @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis) >> >> if (fis & NV_SWNCQ_IRQ_SDBFIS) { >> pp->ncq_flags |= ncq_saw_sdb; >> - DPRINTK("id 0x%x SWNCQ: qc_active 0x%X " >> + ata_port_dbg(ap, "SWNCQ: qc_active 0x%X " > > ditto > >> "dhfis 0x%X dmafis 0x%X sactive 0x%X\n", >> - ap->print_id, pp->qc_active, pp->dhfis_bits, >> + pp->qc_active, pp->dhfis_bits, >> pp->dmafis_bits, readl(pp->sactive_block)); >> if (nv_swncq_sdbfis(ap) < 0) >> goto irq_error; >> @@ -2277,7 +2275,7 @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis) >> goto irq_exit; >> >> if (pp->defer_queue.defer_bits) { >> - DPRINTK("send next command\n"); >> + ata_port_dbg(ap, "send next command\n"); > > ditto > >> qc = nv_swncq_qc_from_dq(ap); >> nv_swncq_issue_atacmd(ap, qc); >> } >> I've moved the __func__ argument into the ata_XXX_dbg() macros; this should take care of it. Cheers, Hannes
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c index 72c9b922a77b..aa2611d638ea 100644 --- a/drivers/ata/sata_nv.c +++ b/drivers/ata/sata_nv.c @@ -1451,7 +1451,7 @@ static unsigned int nv_adma_qc_issue(struct ata_queued_cmd *qc) writew(qc->hw_tag, mmio + NV_ADMA_APPEND); - DPRINTK("Issued tag %u\n", qc->hw_tag); + ata_dev_dbg(qc->dev, "Issued tag %u\n", qc->hw_tag); return 0; } @@ -2029,8 +2029,6 @@ static unsigned int nv_swncq_issue_atacmd(struct ata_port *ap, if (qc == NULL) return 0; - DPRINTK("Enter\n"); - writel((1 << qc->hw_tag), pp->sactive_block); pp->last_issue_tag = qc->hw_tag; pp->dhfis_bits &= ~(1 << qc->hw_tag); @@ -2040,7 +2038,7 @@ static unsigned int nv_swncq_issue_atacmd(struct ata_port *ap, ap->ops->sff_tf_load(ap, &qc->tf); /* load tf registers */ ap->ops->sff_exec_command(ap, &qc->tf); - DPRINTK("Issued tag %u\n", qc->hw_tag); + ata_dev_dbg(qc->dev, "Issued tag %u\n", qc->hw_tag); return 0; } @@ -2053,7 +2051,7 @@ static unsigned int nv_swncq_qc_issue(struct ata_queued_cmd *qc) if (qc->tf.protocol != ATA_PROT_NCQ) return ata_bmdma_qc_issue(qc); - DPRINTK("Enter\n"); + ata_dev_dbg(qc->dev, "Enter\n"); if (!pp->qc_active) nv_swncq_issue_atacmd(ap, qc); @@ -2121,7 +2119,7 @@ static int nv_swncq_sdbfis(struct ata_port *ap) ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask); if (!ap->qc_active) { - DPRINTK("over\n"); + ata_port_dbg(ap, "over\n"); nv_swncq_pp_reinit(ap); return 0; } @@ -2136,10 +2134,10 @@ static int nv_swncq_sdbfis(struct ata_port *ap) */ lack_dhfis = 1; - DPRINTK("id 0x%x QC: qc_active 0x%x," + ata_port_dbg(ap, "QC: qc_active 0x%llx," "SWNCQ:qc_active 0x%X defer_bits %X " "dhfis 0x%X dmafis 0x%X last_issue_tag %x\n", - ap->print_id, ap->qc_active, pp->qc_active, + ap->qc_active, pp->qc_active, pp->defer_queue.defer_bits, pp->dhfis_bits, pp->dmafis_bits, pp->last_issue_tag); @@ -2181,7 +2179,7 @@ static void nv_swncq_dmafis(struct ata_port *ap) __ata_bmdma_stop(ap); tag = nv_swncq_tag(ap); - DPRINTK("dma setup tag 0x%x\n", tag); + ata_port_dbg(ap, "dma setup tag 0x%x\n", tag); qc = ata_qc_from_tag(ap, tag); if (unlikely(!qc)) @@ -2249,9 +2247,9 @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis) if (fis & NV_SWNCQ_IRQ_SDBFIS) { pp->ncq_flags |= ncq_saw_sdb; - DPRINTK("id 0x%x SWNCQ: qc_active 0x%X " + ata_port_dbg(ap, "SWNCQ: qc_active 0x%X " "dhfis 0x%X dmafis 0x%X sactive 0x%X\n", - ap->print_id, pp->qc_active, pp->dhfis_bits, + pp->qc_active, pp->dhfis_bits, pp->dmafis_bits, readl(pp->sactive_block)); if (nv_swncq_sdbfis(ap) < 0) goto irq_error; @@ -2277,7 +2275,7 @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis) goto irq_exit; if (pp->defer_queue.defer_bits) { - DPRINTK("send next command\n"); + ata_port_dbg(ap, "send next command\n"); qc = nv_swncq_qc_from_dq(ap); nv_swncq_issue_atacmd(ap, qc); }
Replace all DPRINTK calls with the ata_XXX_dbg functions. Signed-off-by: Hannes Reinecke <hare@suse.com> --- drivers/ata/sata_nv.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)