Re: [PATCH 00/24] ata: move DPRINTK to dynamic debugging

Re: [PATCH 00/24] ata: move DPRINTK to dynamic debugging

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:46 AM, Hannes Reinecke wrote:
> Hi all,

Hi,

> I got tired of always having to recompile the ATA drivers with ATA_DEBUG
> set so here's now a patchset of move every user of DPRINTK over to
> dynamic debugging, and drop ATA_DEBUG completely.
> 
> As usual, comments and reviews are welcome.

This patchset somehow slipped through the cracks which is
a shame because it seems to be a valuable contribution.

I agree with the general approach but have some comments to
the individual patches (please see my other replies).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Hannes Reinecke (24):
>   libata: move ata_{port,link,dev}_dbg to dynamic debugging
>   sata_nv: move DPRINTK to ata debugging
>   sata_sx4: move DPRINTK to ata debugging
>   sata_sil24: move DPRINTK to ata debugging
>   sata_rcar: move DPRINTK to ata debugging
>   sata_qstor: move DPRINTK to ata debugging
>   pdc_adma: move DPRINTK to ata debugging
>   ahci: move DPRINTK to ata debugging
>   ahci_qorig: move DPRINTK to ata debugging
>   pata_octeon_cf: move DPRINTK to ata debugging
>   pata_sil680: move DPRINTK to ata debugging
>   ata_piix: Remove DPRINTK usage
>   libahci: move DPRINTK to ata debugging
>   libata: move DPRINTK to ata debugging
>   pata_pdc2027x: Replace PDPRINTK() with dynamic debugging
>   sata_fsl: move DPRINTK to ata debugging
>   sata_mv: replace DPRINTK with 'pci_dump' module parameter
>   libata: add 'link' argument to ata_dev_classify()
>   libata: Use ata_port_printk() in ata_dump_id()
>   pata_pdc202xx_old: move DPRINTK to ata debugging
>   libata: remove DPRINTK() macro
>   ata: Remove remaining references to ATA_DEBUG
>   libata: drop BPRINTK()
>   libata.h: Whitespace cleanup
> 
>  drivers/ata/ahci.c              | 10 ++---
>  drivers/ata/ahci.h              |  2 +-
>  drivers/ata/ahci_qoriq.c        |  4 +-
>  drivers/ata/ata_piix.c          |  5 +--
>  drivers/ata/libahci.c           | 25 ++++++-----
>  drivers/ata/libata-core.c       | 76 ++++++++++++++++++---------------
>  drivers/ata/libata-eh.c         | 26 ++++++------
>  drivers/ata/libata-pmp.c        | 12 +++---
>  drivers/ata/libata-scsi.c       | 26 ++++++------
>  drivers/ata/libata-sff.c        | 44 +++++++++----------
>  drivers/ata/pata_octeon_cf.c    | 13 +++---
>  drivers/ata/pata_pdc2027x.c     | 72 +++++++++++++------------------
>  drivers/ata/pata_pdc202xx_old.c |  2 +-
>  drivers/ata/pata_sil680.c       |  2 +-
>  drivers/ata/pdc_adma.c          |  4 +-
>  drivers/ata/sata_fsl.c          | 94 ++++++++++++++++++++++-------------------
>  drivers/ata/sata_inic162x.c     |  2 +-
>  drivers/ata/sata_mv.c           | 71 ++++++++++++++++---------------
>  drivers/ata/sata_nv.c           | 22 +++++-----
>  drivers/ata/sata_qstor.c        |  5 ++-
>  drivers/ata/sata_rcar.c         | 10 ++---
>  drivers/ata/sata_sil24.c        |  6 +--
>  drivers/ata/sata_sx4.c          |  2 +-
>  drivers/scsi/libsas/sas_ata.c   |  3 +-
>  include/linux/libata.h          | 34 ++++++---------
>  25 files changed, 282 insertions(+), 290 deletions(-)

Re: [PATCH 01/24] libata: move ata_{port,link,dev}_dbg to dynamic debugging

[Bartlomiej Zolnierkiewicz]

[ added Tejun to Cc: ]

On 12/13/18 11:46 AM, Hannes Reinecke wrote:
> Use dev_dbg() for ata_{port,link,dev}_dbg to allow for selective
> debugging during runtime.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  include/linux/libata.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 38c95d66ab12..7b2f039d3d21 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1436,7 +1436,7 @@ void ata_dev_printk(const struct ata_device *dev, const char *level,
>  #define ata_port_info(ap, fmt, ...)				\
>  	ata_port_printk(ap, KERN_INFO, fmt, ##__VA_ARGS__)
>  #define ata_port_dbg(ap, fmt, ...)				\
> -	ata_port_printk(ap, KERN_DEBUG, fmt, ##__VA_ARGS__)
> +	dev_dbg(&ap->tdev, fmt, ##__VA_ARGS__)
>  
>  #define ata_link_err(link, fmt, ...)				\
>  	ata_link_printk(link, KERN_ERR, fmt, ##__VA_ARGS__)
> @@ -1447,7 +1447,7 @@ void ata_dev_printk(const struct ata_device *dev, const char *level,
>  #define ata_link_info(link, fmt, ...)				\
>  	ata_link_printk(link, KERN_INFO, fmt, ##__VA_ARGS__)
>  #define ata_link_dbg(link, fmt, ...)				\
> -	ata_link_printk(link, KERN_DEBUG, fmt, ##__VA_ARGS__)
> +	dev_dbg(&link->tdev, fmt, ##__VA_ARGS__)
>  
>  #define ata_dev_err(dev, fmt, ...)				\
>  	ata_dev_printk(dev, KERN_ERR, fmt, ##__VA_ARGS__)
> @@ -1458,7 +1458,7 @@ void ata_dev_printk(const struct ata_device *dev, const char *level,
>  #define ata_dev_info(dev, fmt, ...)				\
>  	ata_dev_printk(dev, KERN_INFO, fmt, ##__VA_ARGS__)
>  #define ata_dev_dbg(dev, fmt, ...)				\
> -	ata_dev_printk(dev, KERN_DEBUG, fmt, ##__VA_ARGS__)
> +	dev_dbg(&dev->tdev, fmt, ##__VA_ARGS__)

While you are at it please remove ata_{port,link,dev}_printk()
altogether.

[ Since code in libata-transport.c sets valid device names using
  dev_set_name() we can simply use generic dev_*() helpers. ]

Please also note that ata_{link,dev}_printk() differs slightly in PMP
handling for links and devices names from code in libata-transport.c:

void ata_link_printk(const struct ata_link *link, const char *level,
		     const char *fmt, ...)
...
	if (sata_pmp_attached(link->ap) || link->ap->slave_link)
		printk("%sata%u.%02u: %pV",
		       level, link->ap->print_id, link->pmp, &vaf);
	else
		printk("%sata%u: %pV",
		       level, link->ap->print_id, &vaf);
...

int ata_tlink_add(struct ata_link *link)
...
	if (ata_is_host_link(link))
		dev_set_name(dev, "link%d", ap->print_id);
        else
		dev_set_name(dev, "link%d.%d", ap->print_id, link->pmp);
...


void ata_dev_printk(const struct ata_device *dev, const char *level,
		    const char *fmt, ...)
...
	printk("%sata%u.%02u: %pV",
	       level, dev->link->ap->print_id, dev->link->pmp + dev->devno,
	       &vaf);
...

static int ata_tdev_add(struct ata_device *ata_dev)
...
	if (ata_is_host_link(link))
		dev_set_name(dev, "dev%d.%d", ap->print_id,ata_dev->devno);
        else
		dev_set_name(dev, "dev%d.%d.0", ap->print_id, link->pmp);
...

I assume that the code in libata-transport.c is the preferred one but
I would like Jens or Tejun to confirm this.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 02/24] sata_nv: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

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

Re: [PATCH 03/24] sata_sx4: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

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_sx4.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
> index 405e606a234d..88a0058ba7a5 100644
> --- a/drivers/ata/sata_sx4.c
> +++ b/drivers/ata/sata_sx4.c
> @@ -769,7 +769,7 @@ static inline unsigned int pdc20621_host_intr(struct ata_port *ap,
>  	} else if (qc->tf.protocol == ATA_PROT_NODATA) {
>  
>  		status = ata_sff_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
> -		DPRINTK("BUS_NODATA (drv_stat 0x%X)\n", status);
> +		ata_port_dbg(ap, "BUS_NODATA (drv_stat 0x%X)\n", status);

Please preserve __func__ printing in the conversion.

>  		qc->err_mask |= ac_err_mask(status);
>  		ata_qc_complete(qc);
>  		handled = 1;

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 04/24] sata_sil24: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

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_sil24.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index 319f517137cd..4b00dce151eb 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -663,7 +663,7 @@ static int sil24_softreset(struct ata_link *link, unsigned int *class,
>  	const char *reason;
>  	int rc;
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "%s: ENTER\n", __func__);
>  
>  	/* put the port into known state */
>  	if (sil24_init_port(ap)) {
> @@ -689,7 +689,7 @@ static int sil24_softreset(struct ata_link *link, unsigned int *class,
>  	sil24_read_tf(ap, 0, &tf);
>  	*class = ata_dev_classify(&tf);
>  
> -	DPRINTK("EXIT, class=%u\n", *class);
> +	ata_link_dbg(link, "%s: EXIT, class=%u\n", __func__, *class);

Please preserve __func__ printing in the conversion.

>  	return 0;
>  
>   err:

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 05/24] sata_rcar: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

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_rcar.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
> index 4b1ff5bc256a..0c60d46eb7ae 100644
> --- a/drivers/ata/sata_rcar.c
> +++ b/drivers/ata/sata_rcar.c
> @@ -323,7 +323,7 @@ static int sata_rcar_bus_softreset(struct ata_port *ap, unsigned long deadline)
>  {
>  	struct ata_ioports *ioaddr = &ap->ioaddr;
>  
> -	DPRINTK("ata%u: bus reset via SRST\n", ap->print_id);
> +	ata_port_dbg(ap, "bus reset via SRST\n");

Please preserve __func__ printing in the conversion.

>  	/* software reset.  causes dev0 to be selected */
>  	iowrite32(ap->ctl, ioaddr->ctl_addr);
> @@ -350,7 +350,7 @@ static int sata_rcar_softreset(struct ata_link *link, unsigned int *classes,
>  		devmask |= 1 << 0;
>  
>  	/* issue bus reset */
> -	DPRINTK("about to softreset, devmask=%x\n", devmask);
> +	ata_link_dbg(link, "about to softreset, devmask=%x\n", devmask);

ditto

>  	rc = sata_rcar_bus_softreset(ap, deadline);
>  	/* if link is occupied, -ENODEV too is an error */
>  	if (rc && (rc != -ENODEV || sata_scr_valid(link))) {
> @@ -361,7 +361,7 @@ static int sata_rcar_softreset(struct ata_link *link, unsigned int *classes,
>  	/* determine by signature whether we have ATA or ATAPI devices */
>  	classes[0] = ata_sff_dev_classify(&link->device[0], devmask, &err);
>  
> -	DPRINTK("classes[0]=%u\n", classes[0]);
> +	ata_link_dbg(link, "classes[0]=%u\n", classes[0]);

ditto

>  	return 0;
>  }
>  
> @@ -440,7 +440,7 @@ static void sata_rcar_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
>  static void sata_rcar_exec_command(struct ata_port *ap,
>  				   const struct ata_taskfile *tf)
>  {
> -	DPRINTK("ata%u: cmd 0x%X\n", ap->print_id, tf->command);
> +	ata_port_dbg(ap, "cmd 0x%X\n", tf->command);

ditto

>  	iowrite32(tf->command, ap->ioaddr.command_addr);
>  	ata_sff_pause(ap);
> @@ -683,7 +683,7 @@ static void sata_rcar_serr_interrupt(struct ata_port *ap)
>  	if (!serror)
>  		return;
>  
> -	DPRINTK("SError @host_intr: 0x%x\n", serror);
> +	ata_port_dbg(ap, "SError @host_intr: 0x%x\n", serror);

ditto

>  	/* first, analyze and record host port events */
>  	ata_ehi_clear_desc(ehi);

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 06/24] sata_qstor: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

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_qstor.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
> index 1fe941688e95..f38f15fe1f79 100644
> --- a/drivers/ata/sata_qstor.c
> +++ b/drivers/ata/sata_qstor.c
> @@ -388,8 +388,9 @@ static inline unsigned int qs_intr_pkt(struct ata_host *host)
>  			struct qs_port_priv *pp = ap->private_data;
>  			struct ata_queued_cmd *qc;
>  
> -			DPRINTK("SFF=%08x%08x: sCHAN=%u sHST=%d sDST=%02x\n",
> -					sff1, sff0, port_no, sHST, sDST);
> +			ata_port_dbg(ap,
> +				"SFF=%08x%08x: sCHAN=%u sHST=%d sDST=%02x\n",
> +				sff1, sff0, port_no, sHST, sDST);

Please preserve __func__ printing in the conversion.

>  			handled = 1;
>  			if (!pp || pp->state != qs_state_pkt)
>  				continue;
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 07/24] pdc_adma: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

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/pdc_adma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/pdc_adma.c b/drivers/ata/pdc_adma.c
> index f1e873a37465..ee9c26ef78c0 100644
> --- a/drivers/ata/pdc_adma.c
> +++ b/drivers/ata/pdc_adma.c
> @@ -490,8 +490,8 @@ static inline unsigned int adma_intr_mmio(struct ata_host *host)
>  			u8 status = ata_sff_check_status(ap);
>  			if ((status & ATA_BUSY))
>  				continue;
> -			DPRINTK("ata%u: protocol %d (dev_stat 0x%X)\n",
> -				ap->print_id, qc->tf.protocol, status);
> +			ata_port_dbg(ap, "protocol %d (dev_stat 0x%X)\n",
> +				qc->tf.protocol, status);

Please preserve __func__ printing in the conversion.

>  			/* complete taskfile transaction */
>  			pp->state = adma_state_idle;

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 08/24] ahci: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Replace all DPRINTK calls with the ata_XXX_dbg functions.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/ahci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 021ce46e2e57..89d9980f3de4 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -698,7 +698,7 @@ static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
>  	bool online;
>  	int rc;
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "ENTER\n");

Please preserve __func__ printing in the conversion.

>  	hpriv->stop_engine(ap);
>  
> @@ -707,7 +707,7 @@ static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
>  
>  	hpriv->start_engine(ap);
>  
> -	DPRINTK("EXIT, rc=%d, class=%u\n", rc, *class);
> +	ata_link_dbg(link, "EXIT, rc=%d, class=%u\n", rc, *class);

ditto

>  	/* vt8251 doesn't clear BSY on signature FIS reception,
>  	 * request follow-up softreset.
> @@ -788,7 +788,7 @@ static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
>  	bool online;
>  	int rc, i;
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "ENTER\n");

ditto

>  	hpriv->stop_engine(ap);
>  
> @@ -828,7 +828,7 @@ static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
>  	if (online)
>  		*class = ahci_dev_classify(ap);
>  
> -	DPRINTK("EXIT, rc=%d, class=%u\n", rc, *class);
> +	ata_link_dbg(link, "EXIT, rc=%d, class=%u\n", rc, *class);

ditto

>  	return rc;
>  }

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 09/24] ahci_qorig: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Replace all DPRINTK calls with the ata_XXX_dbg functions.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/ahci_qoriq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/ahci_qoriq.c b/drivers/ata/ahci_qoriq.c
> index ce59253ec158..b5136c28aa91 100644
> --- a/drivers/ata/ahci_qoriq.c
> +++ b/drivers/ata/ahci_qoriq.c
> @@ -94,7 +94,7 @@ static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
>  	int rc;
>  	bool ls1021a_workaround = (qoriq_priv->type == AHCI_LS1021A);
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "ENTER\n");

Please preserve __func__ printing in the conversion.

>  	hpriv->stop_engine(ap);
>  
> @@ -138,7 +138,7 @@ static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
>  	if (online)
>  		*class = ahci_dev_classify(ap);
>  
> -	DPRINTK("EXIT, rc=%d, class=%u\n", rc, *class);
> +	ata_link_dbg(link, "EXIT, rc=%d, class=%u\n", rc, *class);

ditto

>  	return rc;
>  }

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 10/24] pata_octeon_cf: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Replace all DPRINTK calls with the ata_XXX_dbg functions.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/pata_octeon_cf.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/pata_octeon_cf.c b/drivers/ata/pata_octeon_cf.c
> index d3d851b014a3..2e99a11020b0 100644
> --- a/drivers/ata/pata_octeon_cf.c
> +++ b/drivers/ata/pata_octeon_cf.c
> @@ -440,7 +440,7 @@ static int octeon_cf_softreset16(struct ata_link *link, unsigned int *classes,
>  	int rc;
>  	u8 err;
>  
> -	DPRINTK("about to softreset\n");
> +	ata_link_dbg(link, "about to softreset\n");

Please preserve __func__ printing in the conversion.

>  	__raw_writew(ap->ctl, base + 0xe);
>  	udelay(20);
>  	__raw_writew(ap->ctl | ATA_SRST, base + 0xe);
> @@ -455,7 +455,8 @@ static int octeon_cf_softreset16(struct ata_link *link, unsigned int *classes,
>  
>  	/* determine by signature whether we have ATA or ATAPI devices */
>  	classes[0] = ata_sff_dev_classify(&link->device[0], 1, &err);
> -	DPRINTK("EXIT, classes[0]=%u [1]=%u\n", classes[0], classes[1]);
> +	ata_link_dbg(link, "EXIT, classes[0]=%u [1]=%u\n",
> +		     classes[0], classes[1]);

ditto

>  	return 0;
>  }
>  
> @@ -525,7 +526,7 @@ static void octeon_cf_exec_command16(struct ata_port *ap,
>  		blob = 0;
>  	}
>  
> -	DPRINTK("ata%u: cmd 0x%X\n", ap->print_id, tf->command);
> +	ata_port_dbg(ap, "cmd 0x%X\n", tf->command);

ditto

>  	blob |= (tf->command << 8);
>  	__raw_writew(blob, base + 6);
>  
> @@ -543,12 +544,12 @@ static void octeon_cf_dma_setup(struct ata_queued_cmd *qc)
>  	struct octeon_cf_port *cf_port;
>  
>  	cf_port = ap->private_data;
> -	DPRINTK("ENTER\n");
> +	ata_port_dbg(ap, "ENTER\n");

ditto

>  	/* issue r/w command */
>  	qc->cursg = qc->sg;
>  	cf_port->dma_finished = 0;
>  	ap->ops->sff_exec_command(ap, &qc->tf);
> -	DPRINTK("EXIT\n");
> +	ata_port_dbg(ap, "EXIT\n");

ditto

>  }
>  
>  /**
> @@ -678,7 +679,6 @@ static irqreturn_t octeon_cf_interrupt(int irq, void *dev_instance)
>  
>  	spin_lock_irqsave(&host->lock, flags);
>  
> -	DPRINTK("ENTER\n");

Please either keep it or document the removal in the patch description.

>  	for (i = 0; i < host->n_ports; i++) {
>  		u8 status;
>  		struct ata_port *ap;
> @@ -732,7 +732,6 @@ static irqreturn_t octeon_cf_interrupt(int irq, void *dev_instance)
>  		}
>  	}
>  	spin_unlock_irqrestore(&host->lock, flags);
> -	DPRINTK("EXIT\n");

ditto

>  	return IRQ_RETVAL(handled);
>  }

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 11/24] pata_sil680: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Replace all DPRINTK calls with the ata_XXX_dbg functions.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/pata_sil680.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/pata_sil680.c b/drivers/ata/pata_sil680.c
> index c14071be4f55..5830808c932d 100644
> --- a/drivers/ata/pata_sil680.c
> +++ b/drivers/ata/pata_sil680.c
> @@ -211,7 +211,7 @@ static void sil680_set_dmamode(struct ata_port *ap, struct ata_device *adev)
>  static void sil680_sff_exec_command(struct ata_port *ap,
>  				    const struct ata_taskfile *tf)
>  {
> -	DPRINTK("ata%u: cmd 0x%X\n", ap->print_id, tf->command);
> +	ata_port_dbg(ap, "cmd 0x%X\n", tf->command);

Please preserve __func__ printing in the conversion.

>  	iowrite8(tf->command, ap->ioaddr.command_addr);
>  	ioread8(ap->ioaddr.bmdma_addr + ATA_DMA_CMD);
>  }

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 12/24] ata_piix: Remove DPRINTK usage

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Drop pointless DPRINTK calls around pci_register_driver() and move the
> remaining call over to dev_dbg().
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/ata_piix.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index 7ecb1322a514..f757ff8d0248 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -1362,7 +1362,8 @@ static void piix_init_pcs(struct ata_host *host,
>  	new_pcs = pcs | map_db->port_enable;
>  
>  	if (new_pcs != pcs) {
> -		DPRINTK("updating PCS from 0x%x to 0x%x\n", pcs, new_pcs);
> +		dev_dbg(&pdev->dev,
> +			"updating PCS from 0x%x to 0x%x\n", pcs, new_pcs);

Please preserve __func__ printing in the conversion.

>  		pci_write_config_word(pdev, ICH5_PCS, new_pcs);
>  		msleep(150);
>  	}
> @@ -1786,14 +1787,12 @@ static int __init piix_init(void)
>  {
>  	int rc;
>  
> -	DPRINTK("pci_register_driver\n");
>  	rc = pci_register_driver(&piix_pci_driver);
>  	if (rc)
>  		return rc;
>  
>  	in_module_init = 0;
>  
> -	DPRINTK("done\n");
>  	return 0;
>  }

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 00/24] ata: move DPRINTK to dynamic debugging

[Hannes Reinecke]

On 1/30/20 11:24 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> On 12/13/18 11:46 AM, Hannes Reinecke wrote:
>> Hi all,
> 
> Hi,
> 
>> I got tired of always having to recompile the ATA drivers with ATA_DEBUG
>> set so here's now a patchset of move every user of DPRINTK over to
>> dynamic debugging, and drop ATA_DEBUG completely.
>>
>> As usual, comments and reviews are welcome.
> 
> This patchset somehow slipped through the cracks which is
> a shame because it seems to be a valuable contribution.
> 
> I agree with the general approach but have some comments to
> the individual patches (please see my other replies).
> 
Whee!

The work's not been lost!

I've found myself to redo the patches occasionally, but then never
resend them due to lack of reviews (and, by implication, lack of
interest ...)

Thanks for the review!

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH 13/24] libahci: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Replace all DPRINTK calls with the ata_XXX_dbg functions.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/libahci.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index b5f57c69c487..d6c93ce83504 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1408,7 +1408,8 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
>  	bool fbs_disabled = false;
>  	int rc;
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "%s: ENTER, class %u pmp %d\n",
> +		     __func__, *class, pmp);

Please document enhancements in the patch description.

>  	/* prepare for SRST (AHCI-1.1 10.4.1) */
>  	rc = ahci_kick_engine(ap);
> @@ -1469,7 +1470,7 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
>  	if (fbs_disabled)
>  		ahci_enable_fbs(ap);
>  
> -	DPRINTK("EXIT, class=%u\n", *class);
> +	ata_link_dbg(link, "%s: EXIT, class=%u\n", __func__, *class);
>  	return 0;
>  
>   fail:
> @@ -1491,7 +1492,7 @@ static int ahci_softreset(struct ata_link *link, unsigned int *class,
>  {
>  	int pmp = sata_srst_pmp(link);
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "%s: ENTER, pmp %d\n", __func__, pmp);

ditto

>  	return ahci_do_softreset(link, class, pmp, deadline, ahci_check_ready);
>  }
> @@ -1522,7 +1523,7 @@ static int ahci_pmp_retry_softreset(struct ata_link *link, unsigned int *class,
>  	int rc;
>  	u32 irq_sts;
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "%s: ENTER, class %u\n", __func__, *class);

ditto

>  	rc = ahci_do_softreset(link, class, pmp, deadline,
>  			       ahci_bad_pmp_check_ready);
> @@ -1557,7 +1558,8 @@ int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
>  	struct ata_taskfile tf;
>  	int rc;
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "%s: ENTER, class %u %sline\n",
> +		     __func__, *class, *online ? "on" : "off");

ditto

>  	hpriv->stop_engine(ap);
>  
> @@ -1574,7 +1576,7 @@ int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
>  	if (*online)
>  		*class = ahci_dev_classify(ap);
>  
> -	DPRINTK("EXIT, rc=%d, class=%u\n", rc, *class);
> +	ata_link_dbg(link, "%s: EXIT, rc=%d, class=%u\n", __func__, rc, *class);
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(ahci_do_hardreset);
> @@ -1686,7 +1688,7 @@ static void ahci_fbs_dec_intr(struct ata_port *ap)
>  	u32 fbs = readl(port_mmio + PORT_FBS);
>  	int retries = 3;
>  
> -	DPRINTK("ENTER\n");
> +	ata_port_dbg(ap, "%s: ENTER\n", __func__);
>  	BUG_ON(!pp->fbs_enabled);
>  
>  	/* time to wait for DEC is not specified by AHCI spec,

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 14/24] libata: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Replace all DPRINTK calls with the ata_XXX_dbg functions.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/libata-core.c | 39 ++++++++++++++++++++-------------------
>  drivers/ata/libata-eh.c   | 26 +++++++++++++-------------
>  drivers/ata/libata-pmp.c  | 12 ++++++------
>  drivers/ata/libata-scsi.c | 26 ++++++++++++--------------
>  drivers/ata/libata-sff.c  | 39 ++++++++++++++++++++-------------------
>  5 files changed, 71 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index b8c3f9e6af89..f4b5a61b1c63 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -841,7 +841,7 @@ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
>  		head  = track % dev->heads;
>  		sect  = (u32)block % dev->sectors + 1;
>  
> -		DPRINTK("block %u track %u cyl %u head %u sect %u\n",
> +		ata_dev_dbg(dev, "block %u track %u cyl %u head %u sect %u\n",

Please preserve __func__ printing in the conversion.

>  			(u32)block, track, cyl, head, sect);
>  
>  		/* Check whether the converted CHS can fit.
> @@ -2063,7 +2063,7 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>  	unsigned int err_mask;
>  	bool dma = false;
>  
> -	DPRINTK("read log page - log 0x%x, page 0x%x\n", log, page);
> +	ata_dev_dbg(dev, "read log page - log 0x%x, page 0x%x\n", log, page);

ditto (+ please also remove redundant "read log page" while at it).

>  	/*
>  	 * Return error without actually issuing the command on controllers
> @@ -2099,7 +2099,7 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>  		goto retry;
>  	}
>  
> -	DPRINTK("EXIT, err_mask=%x\n", err_mask);
> +	ata_dev_dbg(dev, "read_log_page - err_mask=%x\n", err_mask);

ditto (+ please also remove redundant "read log page" while at it).

>  	return err_mask;
>  }
>  
> @@ -3564,7 +3564,7 @@ static int ata_dev_set_mode(struct ata_device *dev)
>  			dev_err_whine = " (device error ignored)";
>  	}
>  
> -	DPRINTK("xfer_shift=%u, xfer_mode=0x%x\n",
> +	ata_dev_dbg(dev, "xfer_shift=%u, xfer_mode=0x%x\n",

Please preserve __func__ printing in the conversion.

>  		dev->xfer_shift, (int)dev->xfer_mode);
>  
>  	if (!(ehc->i.flags & ATA_EHI_QUIET) ||
> @@ -4074,7 +4074,8 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
>  	u32 scontrol;
>  	int rc;
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "%s: ENTER, %s\n", __func__,
> +		     online ? (*online ? "online" : "offline") : "unknown");

Please document enhancements in the patch description.
 
>  	if (online)
>  		*online = false;
> @@ -4151,7 +4152,7 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
>  			*online = false;
>  		ata_link_err(link, "COMRESET failed (errno=%d)\n", rc);
>  	}
> -	DPRINTK("EXIT, rc=%d\n", rc);
> +	ata_link_dbg(link, "%s: EXIT, rc=%d\n", __func__, rc);
>  	return rc;
>  }
>  
> @@ -4197,7 +4198,7 @@ void ata_std_postreset(struct ata_link *link, unsigned int *classes)
>  {
>  	u32 serror;
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "%s: ENTER\n", __func__);
>  
>  	/* reset complete, clear SError */
>  	if (!sata_scr_read(link, SCR_ERROR, &serror))
> @@ -4206,7 +4207,7 @@ void ata_std_postreset(struct ata_link *link, unsigned int *classes)
>  	/* print link status */
>  	sata_print_link_status(link);
>  
> -	DPRINTK("EXIT\n");
> +	ata_link_dbg(link, "%s: EXIT\n", __func__);
>  }
>  
>  /**
> @@ -4823,7 +4824,7 @@ static unsigned int ata_dev_set_xfermode(struct ata_device *dev)
>  	unsigned int err_mask;
>  
>  	/* set up set-features taskfile */
> -	DPRINTK("set features - xfer mode\n");
> +	ata_dev_dbg(dev, "set features - xfer mode\n");

Please preserve __func__ printing in the conversion
(+ remove redundant "set features" while at it).

>  	/* Some controllers and ATAPI devices show flaky interrupt
>  	 * behavior after setting xfer mode.  Use polling instead.
> @@ -4845,7 +4846,7 @@ static unsigned int ata_dev_set_xfermode(struct ata_device *dev)
>  	/* On some disks, this command causes spin-up, so we need longer timeout */
>  	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 15000);
>  
> -	DPRINTK("EXIT, err_mask=%x\n", err_mask);
> +	ata_dev_dbg(dev, "%s: EXIT, err_mask=%x\n", __func__, err_mask);
>  	return err_mask;
>  }
>  
> @@ -4871,7 +4872,8 @@ unsigned int ata_dev_set_feature(struct ata_device *dev, u8 enable, u8 feature)
>  	unsigned long timeout = 0;
>  
>  	/* set up set-features taskfile */
> -	DPRINTK("set features - SATA features\n");
> +	ata_dev_dbg(dev, "set features - SATA features %x enable %x\n",
> +		    feature, enable);

Please preserve __func__ printing in the conversion
(+ remove redundant "set features" while at it).
 
>  	ata_tf_init(dev, &tf);
>  	tf.command = ATA_CMD_SET_FEATURES;
> @@ -4885,7 +4887,7 @@ unsigned int ata_dev_set_feature(struct ata_device *dev, u8 enable, u8 feature)
>  			  ata_probe_timeout * 1000 : SETFEATURES_SPINUP_TIMEOUT;
>  	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, timeout);
>  
> -	DPRINTK("EXIT, err_mask=%x\n", err_mask);
> +	ata_dev_dbg(dev, "%s: EXIT, err_mask=%x\n", __func__, err_mask);
>  	return err_mask;
>  }
>  EXPORT_SYMBOL_GPL(ata_dev_set_feature);
> @@ -4913,7 +4915,8 @@ static unsigned int ata_dev_init_params(struct ata_device *dev,
>  		return AC_ERR_INVALID;
>  
>  	/* set up init dev params taskfile */
> -	DPRINTK("init dev params \n");
> +	ata_dev_dbg(dev, "init dev params, heads %u sectors %u\n",
> +		    heads, sectors);

Please preserve __func__ printing in the conversion
(+ remove redundant "init dev params" while at it).

Please document enhancements in the patch description.
 
>  	ata_tf_init(dev, &tf);
>  	tf.command = ATA_CMD_INIT_DEV_PARAMS;
> @@ -4929,7 +4932,7 @@ static unsigned int ata_dev_init_params(struct ata_device *dev,
>  	if (err_mask == AC_ERR_DEV && (tf.feature & ATA_ABORTED))
>  		err_mask = 0;
>  
> -	DPRINTK("EXIT, err_mask=%x\n", err_mask);
> +	ata_dev_dbg(dev, "%s: EXIT, err_mask=%x\n", __func__, err_mask);
>  	return err_mask;
>  }
>  
> @@ -6001,8 +6004,6 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
>  {
>  	struct ata_port *ap;
>  
> -	DPRINTK("ENTER\n");

Please either keep it or document the removal in the patch description.

> -
>  	ap = kzalloc(sizeof(*ap), GFP_KERNEL);
>  	if (!ap)
>  		return NULL;
> @@ -6118,7 +6119,7 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
>  	int i;
>  	void *dr;
>  
> -	DPRINTK("ENTER\n");
> +	dev_dbg(dev, "%s: ENTER\n", __func__);
>  
>  	/* alloc a container for our list of ATA ports (buses) */
>  	sz = sizeof(struct ata_host) + (max_ports + 1) * sizeof(void *);
> @@ -6467,9 +6468,9 @@ int ata_port_probe(struct ata_port *ap)
>  		__ata_port_probe(ap);
>  		ata_port_wait_eh(ap);
>  	} else {
> -		DPRINTK("ata%u: bus probe begin\n", ap->print_id);
> +		ata_port_dbg(ap, "bus probe begin\n");

Please preserve __func__ printing in the conversion.

>  		rc = ata_bus_probe(ap);
> -		DPRINTK("ata%u: bus probe end\n", ap->print_id);
> +		ata_port_dbg(ap, "bus probe end\n");

ditto

>  	}
>  	return rc;
>  }
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 01306c018398..4c5e93499562 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -543,7 +543,7 @@ void ata_scsi_error(struct Scsi_Host *host)
>  	unsigned long flags;
>  	LIST_HEAD(eh_work_q);
>  
> -	DPRINTK("ENTER\n");
> +	ata_port_dbg(ap, "%s: ENTER\n", __func__);
>  
>  	spin_lock_irqsave(host->host_lock, flags);
>  	list_splice_init(&host->eh_cmd_q, &eh_work_q);
> @@ -558,7 +558,7 @@ void ata_scsi_error(struct Scsi_Host *host)
>  	/* finish or retry handled scmd's and clean up */
>  	WARN_ON(!list_empty(&eh_work_q));
>  
> -	DPRINTK("EXIT\n");
> +	ata_port_dbg(ap, "%s: EXIT\n", __func__);
>  }
>  
>  /**
> @@ -954,7 +954,7 @@ void ata_std_sched_eh(struct ata_port *ap)
>  	ata_eh_set_pending(ap, 1);
>  	scsi_schedule_eh(ap->scsi_host);
>  
> -	DPRINTK("port EH scheduled\n");
> +	ata_port_dbg(ap, "port EH scheduled\n");

Please preserve __func__ printing in the conversion.

>  }
>  EXPORT_SYMBOL_GPL(ata_std_sched_eh);
>  
> @@ -1081,7 +1081,7 @@ static void __ata_port_freeze(struct ata_port *ap)
>  
>  	ap->pflags |= ATA_PFLAG_FROZEN;
>  
> -	DPRINTK("ata%u port frozen\n", ap->print_id);
> +	ata_port_dbg(ap, "port frozen\n");

ditto

>  }
>  
>  /**
> @@ -1229,7 +1229,7 @@ void ata_eh_thaw_port(struct ata_port *ap)
>  
>  	spin_unlock_irqrestore(ap->lock, flags);
>  
> -	DPRINTK("ata%u port thawed\n", ap->print_id);
> +	ata_port_dbg(ap, "port thawed\n");

ditto

>  }
>  
>  static void ata_eh_scsidone(struct scsi_cmnd *scmd)
> @@ -1558,7 +1558,7 @@ static void ata_eh_request_sense(struct ata_queued_cmd *qc,
>  		return;
>  	}
>  
> -	DPRINTK("ATA request sense\n");
> +	ata_dev_dbg(dev, "ATA request sense\n");

ditto

>  	ata_tf_init(dev, &tf);
>  	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> @@ -1600,7 +1600,7 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev,
>  	struct ata_port *ap = dev->link->ap;
>  	struct ata_taskfile tf;
>  
> -	DPRINTK("ATAPI request sense\n");
> +	ata_dev_dbg(dev, "ATAPI request sense\n");

ditto
 
>  	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
>  
> @@ -2138,7 +2138,7 @@ static void ata_eh_link_autopsy(struct ata_link *link)
>  	u32 serror;
>  	int rc;
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "%s: ENTER\n", __func__);
>  
>  	if (ehc->i.flags & ATA_EHI_NO_AUTOPSY)
>  		return;
> @@ -2246,7 +2246,7 @@ static void ata_eh_link_autopsy(struct ata_link *link)
>  		ehc->i.action |= ata_eh_speed_down(dev, eflags, all_err_mask);
>  		trace_ata_eh_link_autopsy(dev, ehc->i.action, all_err_mask);
>  	}
> -	DPRINTK("EXIT\n");
> +	ata_link_dbg(link, "%s: EXIT\n", __func__);
>  }
>  
>  /**
> @@ -3130,7 +3130,7 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  	unsigned long flags;
>  	int rc = 0;
>  
> -	DPRINTK("ENTER\n");
> +	ata_link_dbg(link, "%s: ENTER\n", __func__);
>  
>  	/* For PATA drive side cable detection to work, IDENTIFY must
>  	 * be done backwards such that PDIAG- is released by the slave
> @@ -3245,7 +3245,7 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  
>   err:
>  	*r_failed_dev = dev;
> -	DPRINTK("EXIT rc=%d\n", rc);
> +	ata_link_dbg(link, "%s: EXIT rc=%d\n", __func__, rc);
>  	return rc;
>  }
>  
> @@ -3759,7 +3759,7 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  	int rc, nr_fails;
>  	unsigned long flags, deadline;
>  
> -	DPRINTK("ENTER\n");
> +	ata_port_dbg(ap, "%s: ENTER\n", __func__);
>  
>  	/* prep for recovery */
>  	ata_for_each_link(link, ap, EDGE) {
> @@ -3968,7 +3968,7 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  	if (rc && r_failed_link)
>  		*r_failed_link = link;
>  
> -	DPRINTK("EXIT, rc=%d\n", rc);
> +	ata_port_dbg(ap, "%s: EXIT, rc=%d\n", __func__, rc);
>  	return rc;
>  }
>  
> diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
> index 2ae1799f4992..2f0c5ac1d96d 100644
> --- a/drivers/ata/libata-pmp.c
> +++ b/drivers/ata/libata-pmp.c
> @@ -653,7 +653,7 @@ static int sata_pmp_revalidate(struct ata_device *dev, unsigned int new_class)
>  	u32 *gscr = (void *)ap->sector_buf;
>  	int rc;
>  
> -	DPRINTK("ENTER\n");
> +	ata_dev_dbg(dev, "%s: ENTER\n", __func__);
>  
>  	ata_eh_about_to_do(link, NULL, ATA_EH_REVALIDATE);
>  
> @@ -687,12 +687,12 @@ static int sata_pmp_revalidate(struct ata_device *dev, unsigned int new_class)
>  
>  	ata_eh_done(link, NULL, ATA_EH_REVALIDATE);
>  
> -	DPRINTK("EXIT, rc=0\n");
> +	ata_dev_dbg(dev, "%s: EXIT, rc=0\n", __func__);
>  	return 0;
>  
>   fail:
>  	ata_dev_err(dev, "PMP revalidation failed (errno=%d)\n", rc);
> -	DPRINTK("EXIT, rc=%d\n", rc);
> +	ata_dev_dbg(dev, "%s: EXIT, rc=%d\n", __func__, rc);
>  	return rc;
>  }
>  
> @@ -760,7 +760,7 @@ static int sata_pmp_eh_recover_pmp(struct ata_port *ap,
>  	int detach = 0, rc = 0;
>  	int reval_failed = 0;
>  
> -	DPRINTK("ENTER\n");
> +	ata_port_dbg(ap, "%s: ENTER\n", __func__);
>  
>  	if (dev->flags & ATA_DFLAG_DETACH) {
>  		detach = 1;
> @@ -828,7 +828,7 @@ static int sata_pmp_eh_recover_pmp(struct ata_port *ap,
>  	/* okay, PMP resurrected */
>  	ehc->i.flags = 0;
>  
> -	DPRINTK("EXIT, rc=0\n");
> +	ata_port_dbg(ap, "%s: EXIT, rc=0\n", __func__);
>  	return 0;
>  
>   fail:
> @@ -838,7 +838,7 @@ static int sata_pmp_eh_recover_pmp(struct ata_port *ap,
>  	else
>  		ata_dev_disable(dev);
>  
> -	DPRINTK("EXIT, rc=%d\n", rc);
> +	ata_port_dbg(ap, "%s: EXIT, rc=%d\n", __func__, rc);
>  	return rc;
>  }
>  
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 3d4887d0e84a..9746df3da567 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1769,7 +1769,7 @@ static unsigned int ata_scsi_verify_xlat(struct ata_queued_cmd *qc)
>  		head  = track % dev->heads;
>  		sect  = (u32)block % dev->sectors + 1;
>  
> -		DPRINTK("block %u track %u cyl %u head %u sect %u\n",
> +		ata_dev_dbg(dev, "block %u track %u cyl %u head %u sect %u\n",

Please preserve __func__ printing in the conversion.

>  			(u32)block, track, cyl, head, sect);
>  
>  		/* Check whether the converted CHS can fit.
> @@ -1873,7 +1873,7 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
>  			tf_flags |= ATA_TFLAG_FUA;
>  		break;
>  	default:
> -		DPRINTK("no-byte command\n");
> +		ata_dev_dbg(qc->dev, "no-byte command %x\n", cdb[0]);

ditto

>  		fp = 0;
>  		goto invalid_fld;
>  	}
> @@ -2027,7 +2027,7 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
>  early_finish:
>  	ata_qc_free(qc);
>  	cmd->scsi_done(cmd);
> -	DPRINTK("EXIT - early finish (good or error)\n");
> +	ata_dev_dbg(dev, "%s: EXIT - early finish (good or error)\n", __func__);
>  	return 0;
>  
>  err_did:
> @@ -2035,12 +2035,12 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
>  	cmd->result = (DID_ERROR << 16);
>  	cmd->scsi_done(cmd);
>  err_mem:
> -	DPRINTK("EXIT - internal\n");
> +	ata_dev_dbg(dev, "%s: EXIT - internal\n", __func__);
>  	return 0;
>  
>  defer:
>  	ata_qc_free(qc);
> -	DPRINTK("EXIT - defer\n");
> +	ata_dev_dbg(dev, "%s: EXIT - defer rc %d\n", __func__, rc);
>  	if (rc == ATA_DEFER_LINK)
>  		return SCSI_MLQUEUE_DEVICE_BUSY;
>  	else
> @@ -2807,7 +2807,7 @@ static void atapi_request_sense(struct ata_queued_cmd *qc)
>  	struct ata_port *ap = qc->ap;
>  	struct scsi_cmnd *cmd = qc->scsicmd;
>  
> -	DPRINTK("ATAPI request sense\n");
> +	ata_port_dbg(ap, "ATAPI request sense\n");

ditto

>  	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
>  
> @@ -2848,7 +2848,7 @@ static void atapi_request_sense(struct ata_queued_cmd *qc)
>  
>  	ata_qc_issue(qc);
>  
> -	DPRINTK("EXIT\n");
> +	ata_port_dbg(ap, "%s: EXIT\n", __func__);
>  }
>  
>  /*
> @@ -2956,10 +2956,8 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
>  	qc->complete_fn = atapi_qc_complete;
>  
>  	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> -	if (scmd->sc_data_direction == DMA_TO_DEVICE) {
> +	if (scmd->sc_data_direction == DMA_TO_DEVICE)
>  		qc->tf.flags |= ATA_TFLAG_WRITE;
> -		DPRINTK("direction: write\n");

Please either keep it or document the removal in the patch description.

> -	}
>  
>  	qc->tf.command = ATA_CMD_PACKET;
>  	ata_qc_set_pc_nbytes(qc);
> @@ -4342,7 +4340,7 @@ static inline int __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
>  	return rc;
>  
>   bad_cdb_len:
> -	DPRINTK("bad CDB len=%u, scsi_op=0x%02x, max=%u\n",
> +	ata_dev_dbg(dev, "bad CDB len=%u, scsi_op=0x%02x, max=%u\n",

Please preserve __func__ printing in the conversion.

>  		scmd->cmd_len, scsi_op, dev->cdb_len);
>  	scmd->result = DID_ERROR << 16;
>  	scmd->scsi_done(scmd);
> @@ -4783,7 +4781,7 @@ void ata_scsi_hotplug(struct work_struct *work)
>  	int i;
>  
>  	if (ap->pflags & ATA_PFLAG_UNLOADING) {
> -		DPRINTK("ENTER/EXIT - unloading\n");
> +		ata_port_dbg(ap, "%s: ENTER/EXIT - unloading\n", __func__);
>  		return;
>  	}
>  
> @@ -4808,7 +4806,7 @@ void ata_scsi_hotplug(struct work_struct *work)
>  		msleep(10);
>  #endif
>  
> -	DPRINTK("ENTER\n");
> +	ata_port_dbg(ap, "%s: ENTER\n", __func__);
>  	mutex_lock(&ap->scsi_scan_mutex);
>  
>  	/* Unplug detached devices.  We cannot use link iterator here
> @@ -4824,7 +4822,7 @@ void ata_scsi_hotplug(struct work_struct *work)
>  	ata_scsi_scan_host(ap, 0);
>  
>  	mutex_unlock(&ap->scsi_scan_mutex);
> -	DPRINTK("EXIT\n");
> +	ata_port_dbg(ap, "%s: EXIT\n", __func__);
>  }
>  
>  /**
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index c5ea0fc635e5..3326c3b02c61 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -351,8 +351,8 @@ static void ata_dev_select(struct ata_port *ap, unsigned int device,
>  			   unsigned int wait, unsigned int can_sleep)
>  {
>  	if (ata_msg_probe(ap))
> -		ata_port_info(ap, "ata_dev_select: ENTER, device %u, wait %u\n",
> -			      device, wait);
> +		ata_port_info(ap, "%s: ENTER, device %u, wait %u\n",
> +			      __func__, device, wait);
>  
>  	if (wait)
>  		ata_wait_idle(ap);
> @@ -514,7 +514,7 @@ EXPORT_SYMBOL_GPL(ata_sff_tf_read);
>   */
>  void ata_sff_exec_command(struct ata_port *ap, const struct ata_taskfile *tf)
>  {
> -	DPRINTK("ata%u: cmd 0x%X\n", ap->print_id, tf->command);
> +	ata_port_dbg(ap, "cmd 0x%X\n", tf->command);

Please preserve __func__ printing in the conversion.

>  	iowrite8(tf->command, ap->ioaddr.command_addr);
>  	ata_sff_pause(ap);
> @@ -684,7 +684,8 @@ static void ata_pio_sector(struct ata_queued_cmd *qc)
>  	page = nth_page(page, (offset >> PAGE_SHIFT));
>  	offset %= PAGE_SIZE;
>  
> -	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
> +	ata_port_dbg(ap, "data %s\n",
> +		     qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");

ditto

>  
>  	/* do the actual data transfer */
>  	buf = kmap_atomic(page);
> @@ -745,7 +746,7 @@ static void ata_pio_sectors(struct ata_queued_cmd *qc)
>  static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
>  {
>  	/* send SCSI cdb */
> -	DPRINTK("send cdb\n");
> +	ata_port_dbg(ap, "send cdb %x proto %d\n", qc->cdb[0], qc->tf.protocol);

Please preserve __func__ printing in the conversion.

Please document enhancements in the patch description.

>  	WARN_ON_ONCE(qc->dev->cdb_len < 12);
>  
>  	ap->ops->sff_data_xfer(qc, qc->cdb, qc->dev->cdb_len, 1);
> @@ -815,7 +816,8 @@ static int __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
>  	/* don't cross page boundaries */
>  	count = min(count, (unsigned int)PAGE_SIZE - offset);
>  
> -	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
> +	ata_dev_dbg(dev, "data %s\n",
> +		    qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");

Please preserve __func__ printing in the conversion.

>  	/* do the actual data transfer */
>  	buf = kmap_atomic(page);
> @@ -997,8 +999,8 @@ int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
>  	WARN_ON_ONCE(in_wq != ata_hsm_ok_in_wq(ap, qc));
>  
>  fsm_start:
> -	DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
> -		ap->print_id, qc->tf.protocol, ap->hsm_task_state, status);
> +	ata_port_dbg(ap, "protocol %d task_state %d (dev_stat 0x%X)\n",
> +		qc->tf.protocol, ap->hsm_task_state, status);

ditto

>  	switch (ap->hsm_task_state) {
>  	case HSM_ST_FIRST:
> @@ -1199,8 +1201,8 @@ int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
>  		}
>  
>  		/* no more data to transfer */
> -		DPRINTK("ata%u: dev %u command complete, drv_stat 0x%x\n",
> -			ap->print_id, qc->dev->devno, status);
> +		ata_port_dbg(ap, "dev %u command complete, drv_stat 0x%x\n",
> +			qc->dev->devno, status);

ditto

>  		WARN_ON_ONCE(qc->err_mask & (AC_ERR_DEV | AC_ERR_HSM));
>  
> @@ -1257,7 +1259,7 @@ EXPORT_SYMBOL_GPL(ata_sff_queue_pio_task);
>  
>  void ata_sff_flush_pio_task(struct ata_port *ap)
>  {
> -	DPRINTK("ENTER\n");
> +	ata_port_dbg(ap, "%s: ENTER\n", __func__);
>  
>  	cancel_delayed_work_sync(&ap->sff_pio_task);
>  
> @@ -1951,7 +1953,7 @@ static int ata_bus_softreset(struct ata_port *ap, unsigned int devmask,
>  {
>  	struct ata_ioports *ioaddr = &ap->ioaddr;
>  
> -	DPRINTK("ata%u: bus reset via SRST\n", ap->print_id);
> +	ata_port_dbg(ap, "bus reset via SRST\n");
ditto
 
>  	if (ap->ioaddr.ctl_addr) {
>  		/* software reset.  causes dev0 to be selected */
> @@ -1990,7 +1992,7 @@ int ata_sff_softreset(struct ata_link *link, unsigned int *classes,
>  	int rc;
>  	u8 err;
>  
> -	DPRINTK("ENTER\n");
> +	ata_port_dbg(ap, "%s: ENTER\n", __func__);
>  
>  	/* determine if device 0/1 are present */
>  	if (ata_devchk(ap, 0))
> @@ -2002,7 +2004,7 @@ int ata_sff_softreset(struct ata_link *link, unsigned int *classes,
>  	ap->ops->sff_dev_select(ap, 0);
>  
>  	/* issue bus reset */
> -	DPRINTK("about to softreset, devmask=%x\n", devmask);
> +	ata_port_dbg(ap, "about to softreset, devmask=%x\n", devmask);

ditto

>  	rc = ata_bus_softreset(ap, devmask, deadline);
>  	/* if link is occupied, -ENODEV too is an error */
>  	if (rc && (rc != -ENODEV || sata_scr_valid(link))) {
> @@ -2017,7 +2019,8 @@ int ata_sff_softreset(struct ata_link *link, unsigned int *classes,
>  		classes[1] = ata_sff_dev_classify(&link->device[1],
>  						  devmask & (1 << 1), &err);
>  
> -	DPRINTK("EXIT, classes[0]=%u [1]=%u\n", classes[0], classes[1]);
> +	ata_port_dbg(ap, "%s: EXIT, classes[0]=%u [1]=%u\n",
> +		     __func__, classes[0], classes[1]);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(ata_sff_softreset);
> @@ -2050,7 +2053,7 @@ int sata_sff_hardreset(struct ata_link *link, unsigned int *class,
>  	if (online)
>  		*class = ata_sff_dev_classify(link->device, 1, NULL);
>  
> -	DPRINTK("EXIT, class=%u\n", *class);
> +	ata_link_dbg(link, "%s: EXIT, class=%u\n", __func__, *class);
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(sata_sff_hardreset);
> @@ -2081,7 +2084,7 @@ void ata_sff_postreset(struct ata_link *link, unsigned int *classes)
>  
>  	/* bail out if no device is present */
>  	if (classes[0] == ATA_DEV_NONE && classes[1] == ATA_DEV_NONE) {
> -		DPRINTK("EXIT, no device\n");
> +		ata_link_dbg(link, "%s: EXIT, no device\n", __func__);
>  		return;
>  	}
>  
> @@ -2462,8 +2465,6 @@ static int ata_pci_init_one(struct pci_dev *pdev,
>  	struct ata_host *host = NULL;
>  	int rc;
>  
> -	DPRINTK("ENTER\n");
> -

Please either keep it or document the removal in the patch description.

>  	pi = ata_sff_find_valid_pi(ppi);
>  	if (!pi) {
>  		dev_err(&pdev->dev, "no valid port_info specified\n");

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 15/24] pata_pdc2027x: Replace PDPRINTK() with dynamic debugging

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Use dyndebug to allow the user to enable debugging during runtime.

This patch also converts ordinary printks() to dev_*() helpers and
removes some redundant debug messages. Please remember to also mention
all this in the patch description.

> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/pata_pdc2027x.c | 72 ++++++++++++++++++---------------------------
>  1 file changed, 29 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/ata/pata_pdc2027x.c b/drivers/ata/pata_pdc2027x.c
> index 1a18e675ba9f..8ba41c14e1ee 100644
> --- a/drivers/ata/pata_pdc2027x.c
> +++ b/drivers/ata/pata_pdc2027x.c
> @@ -36,13 +36,6 @@
>  
>  #define DRV_NAME	"pata_pdc2027x"
>  #define DRV_VERSION	"1.0"
> -#undef PDC_DEBUG
> -
> -#ifdef PDC_DEBUG
> -#define PDPRINTK(fmt, args...) printk(KERN_ERR "%s: " fmt, __func__, ## args)
> -#else
> -#define PDPRINTK(fmt, args...)
> -#endif
>  
>  enum {
>  	PDC_MMIO_BAR		= 5,
> @@ -220,11 +213,13 @@ static int pdc2027x_cable_detect(struct ata_port *ap)
>  	if (cgcr & (1 << 26))
>  		goto cbl40;
>  
> -	PDPRINTK("No cable or 80-conductor cable on port %d\n", ap->port_no);
> +	ata_port_dbg(ap, "No cable or 80-conductor cable on port %d\n",
> +		     ap->port_no);

Please preserve __func__ printing in the conversion.

Also please use dev_*() helper instead.

>  
>  	return ATA_CBL_PATA80;
>  cbl40:
> -	printk(KERN_INFO DRV_NAME ": 40-conductor cable detected on port %d\n", ap->port_no);
> +	ata_port_info(ap, "40-conductor cable detected on port %d\n",
> +		      ap->port_no);

please use dev_*() helper instead.

>  	return ATA_CBL_PATA40;
>  }
>  
> @@ -298,17 +293,17 @@ static void pdc2027x_set_piomode(struct ata_port *ap, struct ata_device *adev)
>  	unsigned int pio = adev->pio_mode - XFER_PIO_0;
>  	u32 ctcr0, ctcr1;
>  
> -	PDPRINTK("adev->pio_mode[%X]\n", adev->pio_mode);
> +	ata_port_dbg(ap, "adev->pio_mode[%X]\n", adev->pio_mode);

Please preserve __func__ printing in the conversion.

Also please use dev_*() helper instead.

>  
>  	/* Sanity check */
>  	if (pio > 4) {
> -		printk(KERN_ERR DRV_NAME ": Unknown pio mode [%d] ignored\n", pio);
> +		ata_port_err(ap, "Unknown pio mode [%d] ignored\n", pio);

Please use dev_*() helper instead.

>  		return;
>  
>  	}
>  
>  	/* Set the PIO timing registers using value table for 133MHz */
> -	PDPRINTK("Set pio regs... \n");
> +	ata_port_dbg(ap, "Set pio regs... \n");

Please preserve __func__ printing in the conversion.

Also please use dev_*() helper instead.

>  
>  	ctcr0 = ioread32(dev_mmio(ap, adev, PDC_CTCR0));
>  	ctcr0 &= 0xffff0000;
> @@ -321,9 +316,7 @@ static void pdc2027x_set_piomode(struct ata_port *ap, struct ata_device *adev)
>  	ctcr1 |= (pdc2027x_pio_timing_tbl[pio].value2 << 24);
>  	iowrite32(ctcr1, dev_mmio(ap, adev, PDC_CTCR1));
>  
> -	PDPRINTK("Set pio regs done\n");
> -
> -	PDPRINTK("Set to pio mode[%u] \n", pio);
> +	ata_port_dbg(ap, "Set to pio mode[%u] \n", pio);

ditto

>  }
>  
>  /**
> @@ -356,7 +349,7 @@ static void pdc2027x_set_dmamode(struct ata_port *ap, struct ata_device *adev)
>  			iowrite32(ctcr1 & ~(1 << 7), dev_mmio(ap, adev, PDC_CTCR1));
>  		}
>  
> -		PDPRINTK("Set udma regs... \n");
> +		ata_port_dbg(ap, "Set udma regs... \n");

ditto

>  
>  		ctcr1 = ioread32(dev_mmio(ap, adev, PDC_CTCR1));
>  		ctcr1 &= 0xff000000;
> @@ -365,16 +358,14 @@ static void pdc2027x_set_dmamode(struct ata_port *ap, struct ata_device *adev)
>  			(pdc2027x_udma_timing_tbl[udma_mode].value2 << 16);
>  		iowrite32(ctcr1, dev_mmio(ap, adev, PDC_CTCR1));
>  
> -		PDPRINTK("Set udma regs done\n");
> -
> -		PDPRINTK("Set to udma mode[%u] \n", udma_mode);
> +		ata_port_dbg(ap, "Set to udma mode[%u] \n", udma_mode);

ditto

>  
>  	} else  if ((dma_mode >= XFER_MW_DMA_0) &&
>  		   (dma_mode <= XFER_MW_DMA_2)) {
>  		/* Set the MDMA timing registers with value table for 133MHz */
>  		unsigned int mdma_mode = dma_mode & 0x07;
>  
> -		PDPRINTK("Set mdma regs... \n");
> +		ata_port_dbg(ap, "Set mdma regs... \n");

ditto

>  		ctcr0 = ioread32(dev_mmio(ap, adev, PDC_CTCR0));
>  
>  		ctcr0 &= 0x0000ffff;
> @@ -382,11 +373,10 @@ static void pdc2027x_set_dmamode(struct ata_port *ap, struct ata_device *adev)
>  			(pdc2027x_mdma_timing_tbl[mdma_mode].value1 << 24);
>  
>  		iowrite32(ctcr0, dev_mmio(ap, adev, PDC_CTCR0));
> -		PDPRINTK("Set mdma regs done\n");
>  
> -		PDPRINTK("Set to mdma mode[%u] \n", mdma_mode);
> +		ata_port_dbg(ap, "Set to mdma mode[%u] \n", mdma_mode);

ditto

>  	} else {
> -		printk(KERN_ERR DRV_NAME ": Unknown dma mode [%u] ignored\n", dma_mode);
> +		ata_port_err(ap, "Unknown dma mode [%u] ignored\n", dma_mode);
>  	}
>  }
>  
> @@ -420,7 +410,7 @@ static int pdc2027x_set_mode(struct ata_link *link, struct ata_device **r_failed
>  			ctcr1 |= (1 << 25);
>  			iowrite32(ctcr1, dev_mmio(ap, dev, PDC_CTCR1));
>  
> -			PDPRINTK("Turn on prefetch\n");
> +			ata_dev_dbg(dev, "Turn on prefetch\n");

ditto

>  		} else {
>  			pdc2027x_set_dmamode(ap, dev);
>  		}
> @@ -491,8 +481,8 @@ static long pdc_read_counter(struct ata_host *host)
>  
>  	counter = (bccrh << 15) | bccrl;
>  
> -	PDPRINTK("bccrh [%X] bccrl [%X]\n", bccrh,  bccrl);
> -	PDPRINTK("bccrhv[%X] bccrlv[%X]\n", bccrhv, bccrlv);
> +	dev_dbg(host->dev, "bccrh [%X] bccrl [%X]\n", bccrh,  bccrl);
> +	dev_dbg(host->dev, "bccrhv[%X] bccrlv[%X]\n", bccrhv, bccrlv);

ditto

>  	/*
>  	 * The 30-bit decreasing counter are read by 2 pieces.
> @@ -501,7 +491,7 @@ static long pdc_read_counter(struct ata_host *host)
>  	 */
>  	if (retry && !(bccrh == bccrhv && bccrl >= bccrlv)) {
>  		retry--;
> -		PDPRINTK("rereading counter\n");
> +		dev_dbg(host->dev, "rereading counter\n");

ditto

>  		goto retry;
>  	}
>  
> @@ -526,20 +516,18 @@ static void pdc_adjust_pll(struct ata_host *host, long pll_clock, unsigned int b
>  
>  	/* Sanity check */
>  	if (unlikely(pll_clock_khz < 5000L || pll_clock_khz > 70000L)) {
> -		printk(KERN_ERR DRV_NAME ": Invalid PLL input clock %ldkHz, give up!\n", pll_clock_khz);
> +		dev_err(host->dev, "Invalid PLL input clock %ldkHz, give up!\n", pll_clock_khz);
>  		return;
>  	}
>  
> -#ifdef PDC_DEBUG
> -	PDPRINTK("pout_required is %ld\n", pout_required);
> +	dev_dbg(host->dev, "pout_required is %ld\n", pout_required);

Please preserve __func__ printing in the conversion.


>  	/* Show the current clock value of PLL control register
>  	 * (maybe already configured by the firmware)
>  	 */
>  	pll_ctl = ioread16(mmio_base + PDC_PLL_CTL);
>  
> -	PDPRINTK("pll_ctl[%X]\n", pll_ctl);
> -#endif
> +	dev_dbg(host->dev, "pll_ctl[%X]\n", pll_ctl);

ditto
 
>  	/*
>  	 * Calculate the ratio of F, R and OD
> @@ -558,7 +546,7 @@ static void pdc_adjust_pll(struct ata_host *host, long pll_clock, unsigned int b
>  		R = 0x00;
>  	} else {
>  		/* Invalid ratio */
> -		printk(KERN_ERR DRV_NAME ": Invalid ratio %ld, give up!\n", ratio);
> +		dev_err(host->dev, "Invalid ratio %ld, give up!\n", ratio);
>  		return;
>  	}
>  
> @@ -566,15 +554,15 @@ static void pdc_adjust_pll(struct ata_host *host, long pll_clock, unsigned int b
>  
>  	if (unlikely(F < 0 || F > 127)) {
>  		/* Invalid F */
> -		printk(KERN_ERR DRV_NAME ": F[%d] invalid!\n", F);
> +		dev_err(host->dev, "F[%d] invalid!\n", F);
>  		return;
>  	}
>  
> -	PDPRINTK("F[%d] R[%d] ratio*1000[%ld]\n", F, R, ratio);
> +	dev_dbg(host->dev, "F[%d] R[%d] ratio*1000[%ld]\n", F, R, ratio);

ditto

>  	pll_ctl = (R << 8) | F;
>  
> -	PDPRINTK("Writing pll_ctl[%X]\n", pll_ctl);
> +	dev_dbg(host->dev, "Writing pll_ctl[%X]\n", pll_ctl);

ditto

>  	iowrite16(pll_ctl, mmio_base + PDC_PLL_CTL);
>  	ioread16(mmio_base + PDC_PLL_CTL); /* flush */
> @@ -582,15 +570,13 @@ static void pdc_adjust_pll(struct ata_host *host, long pll_clock, unsigned int b
>  	/* Wait the PLL circuit to be stable */
>  	msleep(30);
>  
> -#ifdef PDC_DEBUG
>  	/*
>  	 *  Show the current clock value of PLL control register
>  	 * (maybe configured by the firmware)
>  	 */
>  	pll_ctl = ioread16(mmio_base + PDC_PLL_CTL);
>  
> -	PDPRINTK("pll_ctl[%X]\n", pll_ctl);
> -#endif
> +	dev_dbg(host->dev, "pll_ctl[%X]\n", pll_ctl);

ditto
 
>  	return;
>  }
> @@ -611,7 +597,7 @@ static long pdc_detect_pll_input_clock(struct ata_host *host)
>  
>  	/* Start the test mode */
>  	scr = ioread32(mmio_base + PDC_SYS_CTL);
> -	PDPRINTK("scr[%X]\n", scr);
> +	dev_dbg(host->dev, "scr[%X]\n", scr);

ditto

>  	iowrite32(scr | (0x01 << 14), mmio_base + PDC_SYS_CTL);
>  	ioread32(mmio_base + PDC_SYS_CTL); /* flush */
>  
> @@ -628,7 +614,7 @@ static long pdc_detect_pll_input_clock(struct ata_host *host)
>  
>  	/* Stop the test mode */
>  	scr = ioread32(mmio_base + PDC_SYS_CTL);
> -	PDPRINTK("scr[%X]\n", scr);
> +	dev_dbg(host->dev, "scr[%X]\n", scr);

ditto

>  	iowrite32(scr & ~(0x01 << 14), mmio_base + PDC_SYS_CTL);
>  	ioread32(mmio_base + PDC_SYS_CTL); /* flush */
>  
> @@ -638,8 +624,8 @@ static long pdc_detect_pll_input_clock(struct ata_host *host)
>  	pll_clock = ((start_count - end_count) & 0x3fffffff) / 100 *
>  		(100000000 / usec_elapsed);
>  
> -	PDPRINTK("start[%ld] end[%ld] \n", start_count, end_count);
> -	PDPRINTK("PLL input clock[%ld]Hz\n", pll_clock);
> +	dev_dbg(host->dev, "start[%ld] end[%ld] \n", start_count, end_count);
> +	dev_dbg(host->dev, "PLL input clock[%ld]Hz\n", pll_clock);

ditto

>  	return pll_clock;
>  }

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 16/24] sata_fsl: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Replace all DPRINTK calls with the ata_XXX_dbg functions.

Please preserve __func__ printing in the conversion.

> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/sata_fsl.c | 85 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 46 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index 4dc528bf8e85..8bb129c87a3f 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -316,9 +316,9 @@ static void fsl_sata_set_irq_coalescing(struct ata_host *host,
>  	intr_coalescing_ticks = ticks;
>  	spin_unlock_irqrestore(&host->lock, flags);
>  
> -	DPRINTK("interrupt coalescing, count = 0x%x, ticks = %x\n",
> +	dev_dbg(host->dev, "interrupt coalescing, count = 0x%x, ticks = %x\n",
>  			intr_coalescing_count, intr_coalescing_ticks);
> -	DPRINTK("ICC register status: (hcr base: 0x%x) = 0x%x\n",
> +	dev_dbg(host->dev, "ICC register status: (hcr base: 0x%x) = 0x%x\n",
>  			hcr_base, ioread32(hcr_base + ICC));
>  }
>  
> @@ -390,18 +390,18 @@ static ssize_t fsl_sata_rx_watermark_store(struct device *dev,
>  	return strlen(buf);
>  }
>  
> -static inline unsigned int sata_fsl_tag(unsigned int tag,
> +static inline unsigned int sata_fsl_tag(struct ata_dev *dev, unsigned int tag,
>  					void __iomem *hcr_base)
>  {
>  	/* We let libATA core do actual (queue) tag allocation */
>  
>  	if (unlikely(tag >= SATA_FSL_QUEUE_DEPTH)) {
> -		DPRINTK("tag %d invalid : out of range\n", tag);
> +		ata_dev_dbg(dev, "tag %d invalid : out of range\n", tag);
>  		return 0;
>  	}
>  
>  	if (unlikely((ioread32(hcr_base + CQ)) & (1 << tag))) {
> -		DPRINTK("tag %d invalid : in use!!\n", tag);
> +		ata_dev_dbg(dev, "tag %d invalid : in use!!\n", tag);
>  		return 0;
>  	}
>  
> @@ -513,7 +513,7 @@ static void sata_fsl_qc_prep(struct ata_queued_cmd *qc)
>  	struct sata_fsl_port_priv *pp = ap->private_data;
>  	struct sata_fsl_host_priv *host_priv = ap->host->private_data;
>  	void __iomem *hcr_base = host_priv->hcr_base;
> -	unsigned int tag = sata_fsl_tag(qc->hw_tag, hcr_base);
> +	unsigned int tag = sata_fsl_tag(qc->dev, qc->hw_tag, hcr_base);
>  	struct command_desc *cd;
>  	u32 desc_info = CMD_DESC_RES | CMD_DESC_SNOOP_ENABLE;
>  	u32 num_prde = 0;
> @@ -828,7 +828,7 @@ static int sata_fsl_hardreset(struct ata_link *link, unsigned int *class,
>  	int i = 0;
>  	unsigned long start_jiffies;
>  
> -	DPRINTK("in xx_hardreset\n");
> +	ata_port_dbg(ap, "in xx_hardreset\n");
>  
>  try_offline_again:
>  	/*
> @@ -855,7 +855,7 @@ static int sata_fsl_hardreset(struct ata_link *link, unsigned int *class,
>  			goto try_offline_again;
>  	}
>  
> -	DPRINTK("hardreset, controller off-lined\n");
> +	ata_port_dbg(port, "hardreset, controller off-lined\n");
>  	VPRINTK("HStatus = 0x%x\n", ioread32(hcr_base + HSTATUS));
>  	VPRINTK("HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
>  
> @@ -885,7 +885,7 @@ static int sata_fsl_hardreset(struct ata_link *link, unsigned int *class,
>  		goto err;
>  	}
>  
> -	DPRINTK("hardreset, controller off-lined & on-lined\n");
> +	ata_port_dbg(ap, "hardreset, controller off-lined & on-lined\n");
>  	VPRINTK("HStatus = 0x%x\n", ioread32(hcr_base + HSTATUS));
>  	VPRINTK("HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
>  
> @@ -944,10 +944,10 @@ static int sata_fsl_softreset(struct ata_link *link, unsigned int *class,
>  	u8 *cfis;
>  	u32 Serror;
>  
> -	DPRINTK("in xx_softreset\n");
> +	ata_link_dbg(link, "in xx_softreset\n");
>  
>  	if (ata_link_offline(link)) {
> -		DPRINTK("PHY reports no device\n");
> +		ata_link_dbg(link, "PHY reports no device\n");
>  		*class = ATA_DEV_NONE;
>  		return 0;
>  	}
> @@ -960,7 +960,7 @@ static int sata_fsl_softreset(struct ata_link *link, unsigned int *class,
>  	 * reached here, we can send a command to the target device
>  	 */
>  
> -	DPRINTK("Sending SRST/device reset\n");
> +	ata_link_dbg(link, "Sending SRST/device reset\n");
>  
>  	ata_tf_init(link->device, &tf);
>  	cfis = (u8 *) &pp->cmdentry->cfis;
> @@ -972,7 +972,7 @@ static int sata_fsl_softreset(struct ata_link *link, unsigned int *class,
>  	tf.ctl |= ATA_SRST;	/* setup SRST bit in taskfile control reg */
>  	ata_tf_to_fis(&tf, pmp, 0, cfis);
>  
> -	DPRINTK("Dumping cfis : 0x%x, 0x%x, 0x%x, 0x%x\n",
> +	ata_port_dbg(ap, "Dumping cfis : 0x%x, 0x%x, 0x%x, 0x%x\n",
>  		cfis[0], cfis[1], cfis[2], cfis[3]);
>  
>  	/*
> @@ -980,7 +980,7 @@ static int sata_fsl_softreset(struct ata_link *link, unsigned int *class,
>  	 * other commands are active on the controller/device
>  	 */
>  
> -	DPRINTK("@Softreset, CQ = 0x%x, CA = 0x%x, CC = 0x%x\n",
> +	ata_port_dbg(ap, "@Softreset, CQ = 0x%x, CA = 0x%x, CC = 0x%x\n",
>  		ioread32(CQ + hcr_base),
>  		ioread32(CA + hcr_base), ioread32(CC + hcr_base));
>  
> @@ -993,15 +993,17 @@ static int sata_fsl_softreset(struct ata_link *link, unsigned int *class,
>  	if (temp & 0x1) {
>  		ata_port_warn(ap, "ATA_SRST issue failed\n");
>  
> -		DPRINTK("Softreset@5000,CQ=0x%x,CA=0x%x,CC=0x%x\n",
> +		ata_port_dbg(ap, "Softreset@5000,CQ=0x%x,CA=0x%x,CC=0x%x\n",
>  			ioread32(CQ + hcr_base),
>  			ioread32(CA + hcr_base), ioread32(CC + hcr_base));
>  
>  		sata_fsl_scr_read(&ap->link, SCR_ERROR, &Serror);
>  
> -		DPRINTK("HStatus = 0x%x\n", ioread32(hcr_base + HSTATUS));
> -		DPRINTK("HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
> -		DPRINTK("Serror = 0x%x\n", Serror);
> +		ata_port_dbg(ap, "HStatus = 0x%x\n",
> +			     ioread32(hcr_base + HSTATUS));
> +		ata_port_dbg(ap, "HControl = 0x%x\n",
> +			     ioread32(hcr_base + HCONTROL));
> +		ata_port_dbg(ap, "Serror = 0x%x\n", Serror);
>  		goto err;
>  	}
>  
> @@ -1033,12 +1035,14 @@ static int sata_fsl_softreset(struct ata_link *link, unsigned int *class,
>  	 */
>  	iowrite32(0x01, CC + hcr_base);	/* We know it will be cmd#0 always */
>  
> -	DPRINTK("SATA FSL : Now checking device signature\n");
> +	ata_link_dbg(link, "SATA FSL : Now checking device signature\n");
>  
>  	*class = ATA_DEV_NONE;
>  
>  	/* Verify if SStatus indicates device presence */
>  	if (ata_link_online(link)) {
> +		char desc[16];
> +

What do we need it for?

>  		/*
>  		 * if we are here, device presence has been detected,
>  		 * 1st D2H FIS would have been received, but sfis in
> @@ -1048,7 +1052,7 @@ static int sata_fsl_softreset(struct ata_link *link, unsigned int *class,
>  
>  		*class = sata_fsl_dev_classify(ap);
>  
> -		DPRINTK("class = %d\n", *class);
> +		ata_link_dbg(link, "class = %d\n", *class);
>  		VPRINTK("ccreg = 0x%x\n", ioread32(hcr_base + CC));
>  		VPRINTK("cereg = 0x%x\n", ioread32(hcr_base + CE));
>  	}
> @@ -1062,7 +1066,7 @@ static int sata_fsl_softreset(struct ata_link *link, unsigned int *class,
>  static void sata_fsl_error_handler(struct ata_port *ap)
>  {
>  
> -	DPRINTK("in xx_error_handler\n");
> +	ata_port_dbg(ap, "in xx_error_handler\n");
>  	sata_pmp_error_handler(ap);
>  
>  }
> @@ -1105,7 +1109,7 @@ static void sata_fsl_error_intr(struct ata_port *ap)
>  	if (unlikely(SError & 0xFFFF0000))
>  		sata_fsl_scr_write(&ap->link, SCR_ERROR, SError);
>  
> -	DPRINTK("error_intr,hStat=0x%x,CE=0x%x,DE =0x%x,SErr=0x%x\n",
> +	ata_port_dbg(ap, "error_intr,hStat=0x%x,CE=0x%x,DE =0x%x,SErr=0x%x\n",
>  		hstatus, cereg, ioread32(hcr_base + DE), SError);
>  
>  	/* handle fatal errors */
> @@ -1122,7 +1126,7 @@ static void sata_fsl_error_intr(struct ata_port *ap)
>  
>  	/* Handle PHYRDY change notification */
>  	if (hstatus & INT_ON_PHYRDY_CHG) {
> -		DPRINTK("SATA FSL: PHYRDY change indication\n");
> +		ata_port_dbg(ap, "SATA FSL: PHYRDY change indication\n");
>  
>  		/* Setup a soft-reset EH action */
>  		ata_ehi_hotplugged(ehi);
> @@ -1143,7 +1147,7 @@ static void sata_fsl_error_intr(struct ata_port *ap)
>  		 */
>  		abort = 1;
>  
> -		DPRINTK("single device error, CE=0x%x, DE=0x%x\n",
> +		ata_port_dbg(ap, "single device error, CE=0x%x, DE=0x%x\n",
>  			ioread32(hcr_base + CE), ioread32(hcr_base + DE));
>  
>  		/* find out the offending link and qc */
> @@ -1248,12 +1252,12 @@ static void sata_fsl_host_intr(struct ata_port *ap)
>  	}
>  
>  	if (unlikely(SError & 0xFFFF0000)) {
> -		DPRINTK("serror @host_intr : 0x%x\n", SError);
> +		ata_port_dbg(ap, "serror @host_intr : 0x%x\n", SError);
>  		sata_fsl_error_intr(ap);
>  	}
>  
>  	if (unlikely(hstatus & status_mask)) {
> -		DPRINTK("error interrupt!!\n");
> +		ata_port_dbg(ap, "error interrupt!!\n");
>  		sata_fsl_error_intr(ap);
>  		return;
>  	}
> @@ -1271,15 +1275,15 @@ static void sata_fsl_host_intr(struct ata_port *ap)
>  		/* clear CC bit, this will also complete the interrupt */
>  		iowrite32(done_mask, hcr_base + CC);
>  
> -		DPRINTK("Status of all queues :\n");
> -		DPRINTK("done_mask/CC = 0x%x, CA = 0x%x, CE=0x%x\n",
> +		ata_port_dbg(ap, "Status of all queues :\n");
> +		ata_port_dbg(ap, "done_mask/CC = 0x%x, CA = 0x%x, CE=0x%x\n",
>  			done_mask, ioread32(hcr_base + CA),
>  			ioread32(hcr_base + CE));
>  
>  		for (i = 0; i < SATA_FSL_QUEUE_DEPTH; i++) {
>  			if (done_mask & (1 << i))
> -				DPRINTK
> -				    ("completing ncq cmd,tag=%d,CC=0x%x,CA=0x%x\n",
> +				ata_port_dbg(ap,
> +				     "completing ncq cmd,tag=%d,CC=0x%x,CA=0x%x\n",
>  				     i, ioread32(hcr_base + CC),
>  				     ioread32(hcr_base + CA));
>  		}
> @@ -1290,7 +1294,7 @@ static void sata_fsl_host_intr(struct ata_port *ap)
>  		iowrite32(1, hcr_base + CC);
>  		qc = ata_qc_from_tag(ap, ATA_TAG_INTERNAL);
>  
> -		DPRINTK("completing non-ncq cmd, CC=0x%x\n",
> +		ata_port_dbg(ap, "completing non-ncq cmd, CC=0x%x\n",
>  			 ioread32(hcr_base + CC));
>  
>  		if (qc) {
> @@ -1298,7 +1302,7 @@ static void sata_fsl_host_intr(struct ata_port *ap)
>  		}
>  	} else {
>  		/* Spurious Interrupt!! */
> -		DPRINTK("spurious interrupt!!, CC = 0x%x\n",
> +		ap_port_dbg(ap, "spurious interrupt!!, CC = 0x%x\n",
>  			ioread32(hcr_base + CC));
>  		iowrite32(done_mask, hcr_base + CC);
>  		return;
> @@ -1318,7 +1322,7 @@ static irqreturn_t sata_fsl_interrupt(int irq, void *dev_instance)
>  	interrupt_enables = ioread32(hcr_base + HSTATUS);
>  	interrupt_enables &= 0x3F;
>  
> -	DPRINTK("interrupt status 0x%x\n", interrupt_enables);
> +	dev_dbg(host->dev, "interrupt status 0x%x\n", interrupt_enables);
>  
>  	if (!interrupt_enables)
>  		return IRQ_NONE;
> @@ -1372,7 +1376,7 @@ static int sata_fsl_init_controller(struct ata_host *host)
>  	iowrite32((temp & ~0x3F), hcr_base + HCONTROL);
>  
>  	/* Disable interrupt coalescing control(icc), for the moment */
> -	DPRINTK("icc = 0x%x\n", ioread32(hcr_base + ICC));
> +	dev_dbg(host->dev, "icc = 0x%x\n", ioread32(hcr_base + ICC));
>  	iowrite32(0x01000000, hcr_base + ICC);
>  
>  	/* clear error registers, SError is cleared by libATA  */
> @@ -1391,8 +1395,8 @@ static int sata_fsl_init_controller(struct ata_host *host)
>  	 * callback, that should also initiate the OOB, COMINIT sequence
>  	 */
>  
> -	DPRINTK("HStatus = 0x%x\n", ioread32(hcr_base + HSTATUS));
> -	DPRINTK("HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
> +	dev_dbg(host->dev, "HStatus = 0x%x\n", ioread32(hcr_base + HSTATUS));
> +	dev_dbg(host->dev, "HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
>  
>  	return 0;
>  }
> @@ -1471,9 +1475,12 @@ static int sata_fsl_probe(struct platform_device *ofdev)
>  		iowrite32(temp | TRANSCFG_RX_WATER_MARK, csr_base + TRANSCFG);
>  	}
>  
> -	DPRINTK("@reset i/o = 0x%x\n", ioread32(csr_base + TRANSCFG));
> -	DPRINTK("sizeof(cmd_desc) = %d\n", sizeof(struct command_desc));
> -	DPRINTK("sizeof(#define cmd_desc) = %d\n", SATA_FSL_CMD_DESC_SIZE);
> +	dev_dbg(&ofdev->dev,
> +		"@reset i/o = 0x%x\n", ioread32(csr_base + TRANSCFG));
> +	dev_dbg(&ofdev->dev,
> +		"sizeof(cmd_desc) = %d\n", sizeof(struct command_desc));
> +	dev_dbg(&ofdev->dev,
> +		"sizeof(#define cmd_desc) = %d\n", SATA_FSL_CMD_DESC_SIZE);
>  
>  	host_priv = kzalloc(sizeof(struct sata_fsl_host_priv), GFP_KERNEL);
>  	if (!host_priv)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 17/24] sata_mv: replace DPRINTK with 'pci_dump' module parameter

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Implement module parameter 'pci_dump' and move the DPRINTK calls
> over to dev_printk().

Please preserve __func__ printing in the conversion.

> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/sata_mv.c | 71 +++++++++++++++++++++++++++------------------------
>  1 file changed, 38 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 73ba8e134ca9..2ede635e4023 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -96,6 +96,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 */
> @@ -1258,47 +1262,49 @@ 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, "%p: %s\n", start + b, linebuf);
>  	}
>  }
> -#endif
> -#if defined(ATA_DEBUG) || defined(CONFIG_PCI)
> +#if 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, "%02x: %s\n", 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;
>  	int start_port, num_ports, p, start_hc, num_hcs, hc;
>  
> +	if (!pci_dump)
> +		return;
> +	dev_printk(KERN_DEBUG, &pdev->dev, "All regs @ PCI error\n");
>  	if (0 > port) {
>  		start_hc = start_port = 0;
>  		num_ports = 8;		/* shld be benign for 4 port devs */
> @@ -1308,31 +1314,31 @@ 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,
> +		   "All registers for port(s) %u-%u:\n", start_port,
> +		   num_ports > 1 ? num_ports - 1 : start_port);
>  
>  	if (NULL != pdev) {
> -		DPRINTK("PCI config space regs:\n");
> +		dev_printk(KERN_DEBUG, &pdev->dev, "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, "PCI regs:\n");
> +	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, "HC regs (HC %i):\n", 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, "EDMA regs (port %i):\n", p);
> +		mv_dump_mem(&pdev->dev, port_base, 0x54);
> +		dev_printk(KERN_DEBUG, &pdev->dev, "SATA regs (port %i):\n", p);
> +		mv_dump_mem(&pdev->dev, port_base+0x300, 0x60);
>  	}
> -#endif
>  }
>  
>  static unsigned int mv_scr_offset(unsigned int sc_reg_in)
> @@ -2969,7 +2975,6 @@ 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));
>  
>  	writelfl(0, mmio + hpriv->irq_cause_offset);
> 

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 18/24] libata: add 'link' argument to ata_dev_classify()

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Add a 'link' argument to ata_dev_classify() so that we can call
> ata_link_dbg() with the correct device and drop the DPRINTK usage.

This patch also:

- does actual ata_dev_classify() conversion to not use DPRINTK()

- adds printing of unknown signatures in ata_dev_classify()

- removes DPRINTK() instance from sata_fsl_softreset()

Please document above changes in the patch description.

Also please:

- preserve __func__ printing in the DPRINTK() conversion

- use dev_dbg() directly instead of ata_link_dbg()

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/ahci.c            |  2 +-
>  drivers/ata/ahci.h            |  2 +-
>  drivers/ata/libahci.c         |  9 +++++----
>  drivers/ata/libata-core.c     | 18 +++++++++++-------
>  drivers/ata/libata-sff.c      |  2 +-
>  drivers/ata/sata_fsl.c        | 11 +++++------
>  drivers/ata/sata_inic162x.c   |  2 +-
>  drivers/ata/sata_sil24.c      |  2 +-
>  drivers/scsi/libsas/sas_ata.c |  3 ++-
>  include/linux/libata.h        |  3 ++-
>  10 files changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 89d9980f3de4..cda9cdd2495b 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -826,7 +826,7 @@ static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
>  	hpriv->start_engine(ap);
>  
>  	if (online)
> -		*class = ahci_dev_classify(ap);
> +		*class = ahci_dev_classify(link);
>  
>  	ata_link_dbg(link, "EXIT, rc=%d, class=%u\n", rc, *class);
>  	return rc;
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index ef356e70e6de..65df818e5e09 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -403,7 +403,7 @@ extern struct ata_port_operations ahci_ops;
>  extern struct ata_port_operations ahci_platform_ops;
>  extern struct ata_port_operations ahci_pmp_retry_srst_ops;
>  
> -unsigned int ahci_dev_classify(struct ata_port *ap);
> +unsigned int ahci_dev_classify(struct ata_link *link);
>  void ahci_fill_cmd_slot(struct ahci_port_priv *pp, unsigned int tag,
>  			u32 opts);
>  void ahci_save_initial_config(struct device *dev,
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index d6c93ce83504..421a40731ed3 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1281,8 +1281,9 @@ static void ahci_dev_config(struct ata_device *dev)
>  	}
>  }
>  
> -unsigned int ahci_dev_classify(struct ata_port *ap)
> +unsigned int ahci_dev_classify(struct ata_link *link)
>  {
> +	struct ata_port *ap = link->ap;
>  	void __iomem *port_mmio = ahci_port_base(ap);
>  	struct ata_taskfile tf;
>  	u32 tmp;
> @@ -1293,7 +1294,7 @@ unsigned int ahci_dev_classify(struct ata_port *ap)
>  	tf.lbal		= (tmp >> 8)	& 0xff;
>  	tf.nsect	= (tmp)		& 0xff;
>  
> -	return ata_dev_classify(&tf);
> +	return ata_dev_classify(link, &tf);
>  }
>  EXPORT_SYMBOL_GPL(ahci_dev_classify);
>  
> @@ -1464,7 +1465,7 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
>  		reason = "device not ready";
>  		goto fail;
>  	} else
> -		*class = ahci_dev_classify(ap);
> +		*class = ahci_dev_classify(link);
>  
>  	/* re-enable FBS if disabled before */
>  	if (fbs_disabled)
> @@ -1574,7 +1575,7 @@ int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
>  	hpriv->start_engine(ap);
>  
>  	if (*online)
> -		*class = ahci_dev_classify(ap);
> +		*class = ahci_dev_classify(link);
>  
>  	ata_link_dbg(link, "%s: EXIT, rc=%d, class=%u\n", __func__, rc, *class);
>  	return rc;
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index f4b5a61b1c63..a36f24de399b 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1046,6 +1046,7 @@ const char *sata_spd_string(unsigned int spd)
>  
>  /**
>   *	ata_dev_classify - determine device type based on ATA-spec signature
> + *	@link: ATA device link for the device to be identified
>   *	@tf: ATA taskfile register set for device to be identified
>   *
>   *	Determine from taskfile register contents whether a device is
> @@ -1059,7 +1060,8 @@ const char *sata_spd_string(unsigned int spd)
>   *	Device type, %ATA_DEV_ATA, %ATA_DEV_ATAPI, %ATA_DEV_PMP,
>   *	%ATA_DEV_ZAC, or %ATA_DEV_UNKNOWN the event of failure.
>   */
> -unsigned int ata_dev_classify(const struct ata_taskfile *tf)
> +unsigned int ata_dev_classify(struct ata_link *link,
> +			      const struct ata_taskfile *tf)
>  {
>  	/* Apple's open source Darwin code hints that some devices only
>  	 * put a proper signature into the LBA mid/high registers,
> @@ -1083,31 +1085,33 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf)
>  	 * ata_dev_read_id().
>  	 */
>  	if ((tf->lbam == 0) && (tf->lbah == 0)) {
> -		DPRINTK("found ATA device by sig\n");
> +		ata_link_dbg(link, "found ATA device by sig\n");
>  		return ATA_DEV_ATA;
>  	}
>  
>  	if ((tf->lbam == 0x14) && (tf->lbah == 0xeb)) {
> -		DPRINTK("found ATAPI device by sig\n");
> +		ata_link_dbg(link, "found ATAPI device by sig\n");
>  		return ATA_DEV_ATAPI;
>  	}
>  
>  	if ((tf->lbam == 0x69) && (tf->lbah == 0x96)) {
> -		DPRINTK("found PMP device by sig\n");
> +		ata_link_dbg(link, "found PMP device by sig\n");
>  		return ATA_DEV_PMP;
>  	}
>  
>  	if ((tf->lbam == 0x3c) && (tf->lbah == 0xc3)) {
> -		DPRINTK("found SEMB device by sig (could be ATA device)\n");
> +		ata_link_dbg(link,
> +			     "found SEMB device by sig (could be ATA device)\n");
>  		return ATA_DEV_SEMB;
>  	}
>  
>  	if ((tf->lbam == 0xcd) && (tf->lbah == 0xab)) {
> -		DPRINTK("found ZAC device by sig\n");
> +		ata_link_dbg(link, "found ZAC device by sig\n");
>  		return ATA_DEV_ZAC;
>  	}
>  
> -	DPRINTK("unknown device\n");
> +	ata_link_dbg(link, "unknown device signature %02x %02x\n",
> +		     tf->lbam, tf->lbah);
>  	return ATA_DEV_UNKNOWN;
>  }
>  
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 3326c3b02c61..48b27f4e52fe 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -1850,7 +1850,7 @@ unsigned int ata_sff_dev_classify(struct ata_device *dev, int present,
>  		return ATA_DEV_NONE;
>  
>  	/* determine if device is ATA or ATAPI */
> -	class = ata_dev_classify(&tf);
> +	class = ata_dev_classify(dev->link, &tf);
>  
>  	if (class == ATA_DEV_UNKNOWN) {
>  		/* If the device failed diagnostic, it's likely to
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index 8bb129c87a3f..6260ea9e180f 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -797,9 +797,9 @@ static void sata_fsl_port_stop(struct ata_port *ap)
>  	kfree(pp);
>  }
>  
> -static unsigned int sata_fsl_dev_classify(struct ata_port *ap)
> +static unsigned int sata_fsl_dev_classify(struct ata_link *link)
>  {
> -	struct sata_fsl_host_priv *host_priv = ap->host->private_data;
> +	struct sata_fsl_host_priv *host_priv = link->ap->host->private_data;
>  	void __iomem *hcr_base = host_priv->hcr_base;
>  	struct ata_taskfile tf;
>  	u32 temp;
> @@ -815,7 +815,7 @@ static unsigned int sata_fsl_dev_classify(struct ata_port *ap)
>  	tf.lbal = (temp >> 8) & 0xff;
>  	tf.nsect = temp & 0xff;
>  
> -	return ata_dev_classify(&tf);
> +	return ata_dev_classify(link, &tf);
>  }
>  
>  static int sata_fsl_hardreset(struct ata_link *link, unsigned int *class,
> @@ -917,7 +917,7 @@ static int sata_fsl_hardreset(struct ata_link *link, unsigned int *class,
>  	} else {
>  		ata_port_info(ap, "Signature Update detected @ %d msecs\n",
>  			      jiffies_to_msecs(jiffies - start_jiffies));
> -		*class = sata_fsl_dev_classify(ap);
> +		*class = sata_fsl_dev_classify(link);
>  		return 0;
>  	}
>  
> @@ -1050,9 +1050,8 @@ static int sata_fsl_softreset(struct ata_link *link, unsigned int *class,
>  		 * would have been updated
>  		 */
>  
> -		*class = sata_fsl_dev_classify(ap);
> +		*class = sata_fsl_dev_classify(link);
>  
> -		ata_link_dbg(link, "class = %d\n", *class);
>  		VPRINTK("ccreg = 0x%x\n", ioread32(hcr_base + CC));
>  		VPRINTK("cereg = 0x%x\n", ioread32(hcr_base + CE));
>  	}
> diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
> index e0bcf9b2dab0..3b2c38623b57 100644
> --- a/drivers/ata/sata_inic162x.c
> +++ b/drivers/ata/sata_inic162x.c
> @@ -649,7 +649,7 @@ static int inic_hardreset(struct ata_link *link, unsigned int *class,
>  		}
>  
>  		inic_tf_read(ap, &tf);
> -		*class = ata_dev_classify(&tf);
> +		*class = ata_dev_classify(link, &tf);
>  	}
>  
>  	return 0;
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index 4b00dce151eb..f041f00342c4 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -687,7 +687,7 @@ static int sil24_softreset(struct ata_link *link, unsigned int *class,
>  	}
>  
>  	sil24_read_tf(ap, 0, &tf);
> -	*class = ata_dev_classify(&tf);
> +	*class = ata_dev_classify(link, &tf);
>  
>  	ata_link_dbg(link, "%s: EXIT, class=%u\n", __func__, *class);
>  	return 0;
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 4f6cdf53e913..06736b2c7a97 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -620,6 +620,7 @@ void sas_ata_task_abort(struct sas_task *task)
>  
>  static int sas_get_ata_command_set(struct domain_device *dev)
>  {
> +	struct ata_port *ap = dev->sata_dev.ap;
>  	struct dev_to_host_fis *fis =
>  		(struct dev_to_host_fis *) dev->frame_rcvd;
>  	struct ata_taskfile tf;
> @@ -629,7 +630,7 @@ static int sas_get_ata_command_set(struct domain_device *dev)
>  
>  	ata_tf_from_fis((const u8 *)fis, &tf);
>  
> -	return ata_dev_classify(&tf);
> +	return ata_dev_classify(&ap->link, &tf);
>  }
>  
>  void sas_probe_sata(struct asd_sas_port *port)
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 7b2f039d3d21..808a96a8cb1d 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1180,7 +1180,8 @@ extern int ata_std_qc_defer(struct ata_queued_cmd *qc);
>  extern void ata_noop_qc_prep(struct ata_queued_cmd *qc);
>  extern void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
>  		 unsigned int n_elem);
> -extern unsigned int ata_dev_classify(const struct ata_taskfile *tf);
> +extern unsigned int ata_dev_classify(struct ata_link *link,
> +				     const struct ata_taskfile *tf);
>  extern void ata_dev_disable(struct ata_device *adev);
>  extern void ata_id_string(const u16 *id, unsigned char *s,
>  			  unsigned int ofs, unsigned int len);
> 

Re: [PATCH 19/24] libata: Use ata_port_printk() in ata_dump_id()

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Use ata_port_printk() in ata_dump_id(), dropping DPRINTK usage there.

Wouldn't it be better to pass and use 'dev' instead of 'ap'
(the function prints device's id block)?

Why does it convert DPRINTK() to ata_port_printk() and not dev_dbg()?

Also please preserve __func__ printing in the conversion.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/libata-core.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index a36f24de399b..914520254215 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1438,9 +1438,10 @@ static int ata_hpa_resize(struct ata_device *dev)
>   *	caller.
>   */
>  
> -static inline void ata_dump_id(const u16 *id)
> +static inline void ata_dump_id(struct ata_port *ap, const u16 *id)
>  {
> -	DPRINTK("49==0x%04x  "
> +	ata_port_printk(ap, KERN_DEBUG,
> +		"49==0x%04x  "
>  		"53==0x%04x  "
>  		"63==0x%04x  "
>  		"64==0x%04x  "
> @@ -1450,7 +1451,8 @@ static inline void ata_dump_id(const u16 *id)
>  		id[63],
>  		id[64],
>  		id[75]);
> -	DPRINTK("80==0x%04x  "
> +	ata_port_printk(ap, KERN_DEBUG,
> +		"80==0x%04x  "
>  		"81==0x%04x  "
>  		"82==0x%04x  "
>  		"83==0x%04x  "
> @@ -1460,7 +1462,8 @@ static inline void ata_dump_id(const u16 *id)
>  		id[82],
>  		id[83],
>  		id[84]);
> -	DPRINTK("88==0x%04x  "
> +	ata_port_printk(ap, KERN_DEBUG,
> +		"88==0x%04x  "
>  		"93==0x%04x\n",
>  		id[88],
>  		id[93]);
> @@ -2542,7 +2545,7 @@ int ata_dev_configure(struct ata_device *dev)
>  	xfer_mask = ata_id_xfermask(id);
>  
>  	if (ata_msg_probe(ap))
> -		ata_dump_id(id);
> +		ata_dump_id(ap, id);
>  
>  	/* SCSI only uses 4-char revisions, dump full 8 chars from ATA */
>  	ata_id_c_string(dev->id, fwrevbuf, ATA_ID_FW_REV,

Re: [PATCH 19/24] libata: Use ata_port_printk() in ata_dump_id()

[Bartlomiej Zolnierkiewicz]

On 1/30/20 12:52 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> On 12/13/18 11:47 AM, Hannes Reinecke wrote:
>> Use ata_port_printk() in ata_dump_id(), dropping DPRINTK usage there.
> 
> Wouldn't it be better to pass and use 'dev' instead of 'ap'
> (the function prints device's id block)?
> 
> Why does it convert DPRINTK() to ata_port_printk() and not dev_dbg()?

s/dev_dbg()/ata_port_dbg()/

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Also please preserve __func__ printing in the conversion.
> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/ata/libata-core.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index a36f24de399b..914520254215 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -1438,9 +1438,10 @@ static int ata_hpa_resize(struct ata_device *dev)
>>   *	caller.
>>   */
>>  
>> -static inline void ata_dump_id(const u16 *id)
>> +static inline void ata_dump_id(struct ata_port *ap, const u16 *id)
>>  {
>> -	DPRINTK("49==0x%04x  "
>> +	ata_port_printk(ap, KERN_DEBUG,
>> +		"49==0x%04x  "
>>  		"53==0x%04x  "
>>  		"63==0x%04x  "
>>  		"64==0x%04x  "
>> @@ -1450,7 +1451,8 @@ static inline void ata_dump_id(const u16 *id)
>>  		id[63],
>>  		id[64],
>>  		id[75]);
>> -	DPRINTK("80==0x%04x  "
>> +	ata_port_printk(ap, KERN_DEBUG,
>> +		"80==0x%04x  "
>>  		"81==0x%04x  "
>>  		"82==0x%04x  "
>>  		"83==0x%04x  "
>> @@ -1460,7 +1462,8 @@ static inline void ata_dump_id(const u16 *id)
>>  		id[82],
>>  		id[83],
>>  		id[84]);
>> -	DPRINTK("88==0x%04x  "
>> +	ata_port_printk(ap, KERN_DEBUG,
>> +		"88==0x%04x  "
>>  		"93==0x%04x\n",
>>  		id[88],
>>  		id[93]);
>> @@ -2542,7 +2545,7 @@ int ata_dev_configure(struct ata_device *dev)
>>  	xfer_mask = ata_id_xfermask(id);
>>  
>>  	if (ata_msg_probe(ap))
>> -		ata_dump_id(id);
>> +		ata_dump_id(ap, id);
>>  
>>  	/* SCSI only uses 4-char revisions, dump full 8 chars from ATA */
>>  	ata_id_c_string(dev->id, fwrevbuf, ATA_ID_FW_REV,

Re: [PATCH 20/24] pata_pdc202xx_old: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Replace all DPRINTK calls with the ata_XXX_dbg functions.

Please preserve __func__ printing in the conversion.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/pata_pdc202xx_old.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/pata_pdc202xx_old.c b/drivers/ata/pata_pdc202xx_old.c
> index 9001991d2830..6d52b1bb1b9e 100644
> --- a/drivers/ata/pata_pdc202xx_old.c
> +++ b/drivers/ata/pata_pdc202xx_old.c
> @@ -37,7 +37,7 @@ static int pdc2026x_cable_detect(struct ata_port *ap)
>  static void pdc202xx_exec_command(struct ata_port *ap,
>  				  const struct ata_taskfile *tf)
>  {
> -	DPRINTK("ata%u: cmd 0x%X\n", ap->print_id, tf->command);
> +	ata_port_dbg(ap, "ata%u: cmd 0x%X\n", ap->print_id, tf->command);
>  
>  	iowrite8(tf->command, ap->ioaddr.command_addr);
>  	ndelay(400);

Re: [PATCH 21/24] libata: remove DPRINTK() macro

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> No users left, remove it.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> ---
>  include/linux/libata.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 808a96a8cb1d..cf62f3bdf4ef 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -62,14 +62,12 @@
>  
>  /* note: prints function name for you */
>  #ifdef ATA_DEBUG
> -#define DPRINTK(fmt, args...) printk(KERN_ERR "%s: " fmt, __func__, ## args)
>  #ifdef ATA_VERBOSE_DEBUG
>  #define VPRINTK(fmt, args...) printk(KERN_ERR "%s: " fmt, __func__, ## args)
>  #else
>  #define VPRINTK(fmt, args...)
>  #endif	/* ATA_VERBOSE_DEBUG */
>  #else
> -#define DPRINTK(fmt, args...)
>  #define VPRINTK(fmt, args...)
>  #endif	/* ATA_DEBUG */

Re: [PATCH 22/24] ata: Remove remaining references to ATA_DEBUG

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> ATA_DEBUG is now only used to switch on additional message levels.
> So move those two levels (ATA_MSG_INFO and ATA_MSG_CTL) over to
> ata_XXX_dbg() and remove the remaining references to ATA_DEBUG.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> ---
>  drivers/ata/libata-core.c |  6 ++----
>  drivers/ata/libata-sff.c  |  3 +--
>  include/linux/libata.h    | 11 ++---------
>  3 files changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 914520254215..598b92d24530 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2464,8 +2464,8 @@ int ata_dev_configure(struct ata_device *dev)
>  	char modelbuf[ATA_ID_PROD_LEN+1];
>  	int rc;
>  
> -	if (!ata_dev_enabled(dev) && ata_msg_info(ap)) {
> -		ata_dev_info(dev, "%s: ENTER/EXIT -- nodev\n", __func__);
> +	if (!ata_dev_enabled(dev)) {
> +		ata_dev_dbg(dev, "%s: ENTER/EXIT -- nodev\n", __func__);
>  		return 0;
>  	}
>  
> @@ -6025,8 +6025,6 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
>  #if defined(ATA_VERBOSE_DEBUG)
>  	/* turn on all debugging levels */
>  	ap->msg_enable = 0x00FF;
> -#elif defined(ATA_DEBUG)
> -	ap->msg_enable = ATA_MSG_DRV | ATA_MSG_INFO | ATA_MSG_CTL | ATA_MSG_WARN | ATA_MSG_ERR;
>  #else
>  	ap->msg_enable = ATA_MSG_DRV | ATA_MSG_ERR | ATA_MSG_WARN;
>  #endif
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 48b27f4e52fe..6587ae4fe0f4 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -1277,8 +1277,7 @@ void ata_sff_flush_pio_task(struct ata_port *ap)
>  
>  	ap->sff_pio_task_link = NULL;
>  
> -	if (ata_msg_ctl(ap))
> -		ata_port_dbg(ap, "%s: EXIT\n", __func__);
> +	ata_port_dbg(ap, "%s: EXIT\n", __func__);
>  }
>  
>  static void ata_sff_pio_task(struct work_struct *work)
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index cf62f3bdf4ef..4b5b174994b5 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -54,22 +54,17 @@
>   * compile-time options: to be removed as soon as all the drivers are
>   * converted to the new debugging mechanism
>   */
> -#undef ATA_DEBUG		/* debugging output */
>  #undef ATA_VERBOSE_DEBUG	/* yet more debugging output */
>  #undef ATA_IRQ_TRAP		/* define to ack screaming irqs */
>  #undef ATA_NDEBUG		/* define to disable quick runtime checks */
>  
>  
>  /* note: prints function name for you */
> -#ifdef ATA_DEBUG
>  #ifdef ATA_VERBOSE_DEBUG
>  #define VPRINTK(fmt, args...) printk(KERN_ERR "%s: " fmt, __func__, ## args)
>  #else
>  #define VPRINTK(fmt, args...)
>  #endif	/* ATA_VERBOSE_DEBUG */
> -#else
> -#define VPRINTK(fmt, args...)
> -#endif	/* ATA_DEBUG */
>  
>  #define BPRINTK(fmt, args...) if (ap->flags & ATA_FLAG_DEBUGMSG) printk(KERN_ERR "%s: " fmt, __func__, ## args)
>  
> @@ -1979,11 +1974,9 @@ static inline u8 ata_wait_idle(struct ata_port *ap)
>  {
>  	u8 status = ata_sff_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
>  
> -#ifdef ATA_DEBUG
>  	if (status != 0xff && (status & (ATA_BUSY | ATA_DRQ)))
> -		ata_port_printk(ap, KERN_DEBUG, "abnormal Status 0x%X\n",
> -				status);
> -#endif
> +		ata_port_dbg(ap, "%s: abnormal Status 0x%X\n",
> +			     __func__, status);
>  
>  	return status;
>  }

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 23/24] libata: drop BPRINTK()

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> No users, drop it.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

This patch is obviously correct and is independent of all other
patches in the series so it should be moved at the beginning of
the patchset (or simply applied now).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> ---
>  include/linux/libata.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 4b5b174994b5..2f992b6ad626 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -66,8 +66,6 @@
>  #define VPRINTK(fmt, args...)
>  #endif	/* ATA_VERBOSE_DEBUG */
>  
> -#define BPRINTK(fmt, args...) if (ap->flags & ATA_FLAG_DEBUGMSG) printk(KERN_ERR "%s: " fmt, __func__, ## args)
> -
>  #define ata_print_version_once(dev, version)			\
>  ({								\
>  	static bool __print_once;				\

Re: [PATCH 24/24] libata.h: Whitespace cleanup

[Bartlomiej Zolnierkiewicz]

On 12/13/18 11:47 AM, Hannes Reinecke wrote:
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

This patch is obviously correct and is independent of all other
patches in the series so it should be moved at the beginning of
the patchset (or simply applied now).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> ---
>  include/linux/libata.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 2f992b6ad626..3ed1b9ce1888 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -602,7 +602,7 @@ struct ata_ioports {
>  
>  struct ata_host {
>  	spinlock_t		lock;
> -	struct device 		*dev;
> +	struct device		*dev;
>  	void __iomem * const	*iomap;
>  	unsigned int		n_ports;
>  	unsigned int		n_tags;			/* nr of NCQ tags */
> @@ -822,7 +822,7 @@ struct ata_port {
>  	/* Flags that change dynamically, protected by ap->lock */
>  	unsigned int		pflags; /* ATA_PFLAG_xxx */
>  	unsigned int		print_id; /* user visible unique port ID */
> -	unsigned int            local_port_no; /* host local port num */
> +	unsigned int		local_port_no; /* host local port num */
>  	unsigned int		port_no; /* 0 based port no. inside the host */
>  
>  #ifdef CONFIG_ATA_SFF
> @@ -857,7 +857,7 @@ struct ata_port {
>  
>  	struct ata_port_stats	stats;
>  	struct ata_host		*host;
> -	struct device 		*dev;
> +	struct device		*dev;
>  	struct device		tdev;
>  
>  	struct mutex		scsi_scan_mutex;
> @@ -1008,7 +1008,7 @@ struct ata_port_info {
>  	unsigned long		mwdma_mask;
>  	unsigned long		udma_mask;
>  	struct ata_port_operations *port_ops;
> -	void 			*private_data;
> +	void			*private_data;
>  };
>  
>  struct ata_timing {
> @@ -1499,7 +1499,7 @@ static inline bool ata_tag_valid(unsigned int tag)
>   * Internal use only, iterate commands ignoring error handling and
>   * status of 'qc'.
>   */
> -#define ata_qc_for_each_raw(ap, qc, tag)					\
> +#define ata_qc_for_each_raw(ap, qc, tag)			\
>  	__ata_qc_for_each(ap, qc, tag, ATA_MAX_QUEUE, __ata_qc_from_tag)
>  
>  /*

Re: [PATCH 01/24] libata: move ata_{port,link,dev}_dbg to dynamic debugging

[Hannes Reinecke]

On 1/30/20 11:42 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> [ added Tejun to Cc: ]
> 
> On 12/13/18 11:46 AM, Hannes Reinecke wrote:
>> Use dev_dbg() for ata_{port,link,dev}_dbg to allow for selective
>> debugging during runtime.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  include/linux/libata.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 38c95d66ab12..7b2f039d3d21 100644
[ .. ]
> 
> While you are at it please remove ata_{port,link,dev}_printk()
> altogether.
> 
> [ Since code in libata-transport.c sets valid device names using
>   dev_set_name() we can simply use generic dev_*() helpers. ]
> 
> Please also note that ata_{link,dev}_printk() differs slightly in PMP
> handling for links and devices names from code in libata-transport.c:
> 
I know.
I hated it, too.
One should try to keep the naming consistent, and these 'link' devices
mess up everything.

> void ata_link_printk(const struct ata_link *link, const char *level,
> 		     const char *fmt, ...)
> ...
> 	if (sata_pmp_attached(link->ap) || link->ap->slave_link)
> 		printk("%sata%u.%02u: %pV",
> 		       level, link->ap->print_id, link->pmp, &vaf);
> 	else
> 		printk("%sata%u: %pV",
> 		       level, link->ap->print_id, &vaf);
> ...
> 
> int ata_tlink_add(struct ata_link *link)
> ...
> 	if (ata_is_host_link(link))
> 		dev_set_name(dev, "link%d", ap->print_id);
>         else
> 		dev_set_name(dev, "link%d.%d", ap->print_id, link->pmp);
> ...
> 
> 
> void ata_dev_printk(const struct ata_device *dev, const char *level,
> 		    const char *fmt, ...)
> ...
> 	printk("%sata%u.%02u: %pV",
> 	       level, dev->link->ap->print_id, dev->link->pmp + dev->devno,
> 	       &vaf);
> ...
> 
> static int ata_tdev_add(struct ata_device *ata_dev)
> ...
> 	if (ata_is_host_link(link))
> 		dev_set_name(dev, "dev%d.%d", ap->print_id,ata_dev->devno);
>         else
> 		dev_set_name(dev, "dev%d.%d.0", ap->print_id, link->pmp);
> ...
> 
> I assume that the code in libata-transport.c is the preferred one but
> I would like Jens or Tejun to confirm this.
> 
I'll go with the libata-transport names; one should really prefix the
subsystem with the names otherwise things really get messy if one tries
to analyse or debug issues.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH 02/24] sata_nv: move DPRINTK to ata debugging

[Hannes Reinecke]

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
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer