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@xxxxxxxx>
>>> ---
>>> 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
