Re: [PATCH 05/42] libata: move __func__ into ata_{port,link,dev}_dbg() helper

Re: [PATCH 05/42] libata: move __func__ into ata_{port,link,dev}_dbg() helper

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Move the __func__ argument into the ata_{port,link,dev}_dbg()
> helper and drop the explicit argument from the caller.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/libata-acpi.c | 14 ++++++--------
>  drivers/ata/libata-core.c |  9 ++++-----
>  drivers/ata/libata-sff.c  |  2 +-
>  include/linux/libata.h    |  6 +++---
>  4 files changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 9a7c25252e50..d5bcf5718fd3 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -419,8 +419,7 @@ static int ata_dev_get_GTF(struct ata_device *dev, struct ata_acpi_gtf **gtf)
>  	output.pointer = NULL;	/* ACPI-CA sets this; save/free it later */
>  
>  	if (ata_msg_probe(ap))
> -		ata_dev_dbg(dev, "%s: ENTER: port#: %d\n",
> -			    __func__, ap->port_no);
> +		ata_dev_dbg(dev, "ENTER: port#: %d\n", ap->port_no);
>  
>  	/* _GTF has no input parameters */
>  	status = acpi_evaluate_object(ata_dev_acpi_handle(dev), "_GTF", NULL,
> @@ -438,8 +437,7 @@ static int ata_dev_get_GTF(struct ata_device *dev, struct ata_acpi_gtf **gtf)
>  
>  	if (!output.length || !output.pointer) {
>  		if (ata_msg_probe(ap))
> -			ata_dev_dbg(dev, "%s: Run _GTF: length or ptr is NULL (0x%llx, 0x%p)\n",
> -				    __func__,
> +			ata_dev_dbg(dev, "Run _GTF: length or ptr is NULL (0x%llx, 0x%p)\n",
>  				    (unsigned long long)output.length,
>  				    output.pointer);
>  		rc = -EINVAL;
> @@ -465,8 +463,8 @@ static int ata_dev_get_GTF(struct ata_device *dev, struct ata_acpi_gtf **gtf)
>  	if (gtf) {
>  		*gtf = (void *)out_obj->buffer.pointer;
>  		if (ata_msg_probe(ap))
> -			ata_dev_dbg(dev, "%s: returning gtf=%p, gtf_count=%d\n",
> -				    __func__, *gtf, rc);
> +			ata_dev_dbg(dev, "returning gtf=%p, gtf_count=%d\n",
> +				    *gtf, rc);
>  	}
>  	return rc;
>  
> @@ -780,8 +778,8 @@ static int ata_acpi_push_id(struct ata_device *dev)
>  	union acpi_object in_params[1];
>  
>  	if (ata_msg_probe(ap))
> -		ata_dev_dbg(dev, "%s: ix = %d, port#: %d\n",
> -			    __func__, dev->devno, ap->port_no);
> +		ata_dev_dbg(dev, "ix = %d, port#: %d\n",
> +			    dev->devno, ap->port_no);
>  
>  	/* Give the drive Identify data to the drive via the _SDD method */
>  	/* _SDD: set up input parameters */
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 5bf6e4da218a..3a8af0fef540 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1846,7 +1846,7 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>  	int rc;
>  
>  	if (ata_msg_ctl(ap))
> -		ata_dev_dbg(dev, "%s: ENTER\n", __func__);
> +		ata_dev_dbg(dev, "ENTER\n");
>  
>  retry:
>  	ata_tf_init(dev, &tf);
> @@ -2447,7 +2447,7 @@ int ata_dev_configure(struct ata_device *dev)
>  	}
>  
>  	if (ata_msg_probe(ap))
> -		ata_dev_dbg(dev, "%s: ENTER\n", __func__);
> +		ata_dev_dbg(dev, "ENTER\n");
>  
>  	/* set horkage */
>  	dev->horkage |= ata_dev_blacklisted(dev);
> @@ -2498,9 +2498,8 @@ int ata_dev_configure(struct ata_device *dev)
>  	/* print device capabilities */
>  	if (ata_msg_probe(ap))
>  		ata_dev_dbg(dev,
> -			    "%s: cfg 49:%04x 82:%04x 83:%04x 84:%04x "
> +			    "cfg 49:%04x 82:%04x 83:%04x 84:%04x "
>  			    "85:%04x 86:%04x 87:%04x 88:%04x\n",
> -			    __func__,
>  			    id[49], id[82], id[83], id[84],
>  			    id[85], id[86], id[87], id[88]);
>  
> @@ -2767,7 +2766,7 @@ int ata_dev_configure(struct ata_device *dev)
>  
>  err_out_nosup:
>  	if (ata_msg_probe(ap))
> -		ata_dev_dbg(dev, "%s: EXIT, err\n", __func__);
> +		ata_dev_dbg(dev, "EXIT, err\n");
>  	return rc;
>  }
>  
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 038db94216a9..277398427e4e 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -1266,7 +1266,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, "EXIT\n");
>  }
>  
>  static void ata_sff_pio_task(struct work_struct *work)
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 6b1ffb78a410..508f501095c9 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1424,7 +1424,7 @@ static inline int sata_srst_pmp(struct ata_link *link)
>  #define ata_port_info(ap, fmt, ...)				\
>  	dev_info(&ap->tdev, fmt, ##__VA_ARGS__)
>  #define ata_port_dbg(ap, fmt, ...)				\
> -	dev_dbg(&ap->tdev, fmt, ##__VA_ARGS__)
> +	dev_dbg(&ap->tdev, "%s: " fmt, __func__, ##__VA_ARGS__)
>  
>  #define ata_link_err(link, fmt, ...)				\
>  	dev_err(&link->tdev, fmt, ##__VA_ARGS__)
> @@ -1435,7 +1435,7 @@ static inline int sata_srst_pmp(struct ata_link *link)
>  #define ata_link_info(link, fmt, ...)				\
>  	dev_info(&link->tdev, fmt, ##__VA_ARGS__)
>  #define ata_link_dbg(link, fmt, ...)				\
> -	dev_dbg(&link->tdev, fmt, ##__VA_ARGS__)
> +	dev_dbg(&link->tdev, "%s: " fmt, __func__, ##__VA_ARGS__)
>  
>  #define ata_dev_err(dev, fmt, ...)				\
>  	dev_err(&dev->tdev, fmt, ##__VA_ARGS__)
> @@ -1446,7 +1446,7 @@ static inline int sata_srst_pmp(struct ata_link *link)
>  #define ata_dev_info(dev, fmt, ...)				\
>  	dev_info(&dev->tdev, fmt, ##__VA_ARGS__)
>  #define ata_dev_dbg(dev, fmt, ...)				\
> -	dev_dbg(&dev->tdev, fmt, ##__VA_ARGS__)
> +	dev_dbg(&dev->tdev, "%s: " fmt, __func__, ##__VA_ARGS__)
>  
>  void ata_print_version(const struct device *dev, const char *version);
>  

Re: [PATCH 09/42] libata: Add ata_port_classify() helper

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Add an ata_port_classify() helper to print out the results from
> the device classification and remove the debugging statements
> from ata_dev_classify(). Also provide a mapping ata_dev_class_string()
> to provide a string representation for those instances calling
> ata_dev_classify() directly.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/libahci.c       |  2 +-
>  drivers/ata/libata-core.c   | 67 ++++++++++++++++++++++++++++++++++-----------
>  drivers/ata/libata-sff.c    |  5 ++++
>  drivers/ata/sata_fsl.c      |  2 +-
>  drivers/ata/sata_inic162x.c |  2 +-
>  drivers/ata/sata_sil24.c    |  2 +-
>  include/linux/libata.h      |  3 ++
>  7 files changed, 63 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 4055071f213f..c1bc973ecc16 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1276,7 +1276,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_port_classify(ap, &tf);
>  }
>  EXPORT_SYMBOL_GPL(ahci_dev_classify);
>  
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index a9d47a746f77..17abc52ce41e 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1025,6 +1025,27 @@ const char *sata_spd_string(unsigned int spd)
>  	return spd_str[spd - 1];
>  }
>  
> +const char *ata_dev_class_string(unsigned int class)
> +{
> +	static const char * const class_str[] = {
> +		"unknown",
> +		"ATA",
> +		"ATA (unsupported)",
> +		"ATAPI",
> +		"ATAPI (unsupported",
> +		"PMP",
> +		"PMP (unsupported)",
> +		"SEMB",
> +		"SEMB (unsupported)",
> +		"ZAC",
> +		"ZAC (unsupported)",
> +		"none",
> +	};
> +	if (class == 0 || (class - 1) >= ARRAY_SIZE(class_str))
> +		return "unknown";
> +	return class_str[class - 1];
> +}
> +
>  /**
>   *	ata_dev_classify - determine device type based on ATA-spec signature
>   *	@tf: ATA taskfile register set for device to be identified
> @@ -1063,35 +1084,48 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf)
>  	 * SEMB signature.  This is worked around in
>  	 * ata_dev_read_id().
>  	 */
> -	if ((tf->lbam == 0) && (tf->lbah == 0)) {
> -		DPRINTK("found ATA device by sig\n");
> +	if ((tf->lbam == 0) && (tf->lbah == 0))
>  		return ATA_DEV_ATA;
> -	}
>  
> -	if ((tf->lbam == 0x14) && (tf->lbah == 0xeb)) {
> -		DPRINTK("found ATAPI device by sig\n");
> +	if ((tf->lbam == 0x14) && (tf->lbah == 0xeb))
>  		return ATA_DEV_ATAPI;
> -	}
>  
> -	if ((tf->lbam == 0x69) && (tf->lbah == 0x96)) {
> -		DPRINTK("found PMP device by sig\n");
> +	if ((tf->lbam == 0x69) && (tf->lbah == 0x96))
>  		return ATA_DEV_PMP;
> -	}
>  
> -	if ((tf->lbam == 0x3c) && (tf->lbah == 0xc3)) {
> -		DPRINTK("found SEMB device by sig (could be ATA device)\n");
> +	if ((tf->lbam == 0x3c) && (tf->lbah == 0xc3))
>  		return ATA_DEV_SEMB;
> -	}
>  
> -	if ((tf->lbam == 0xcd) && (tf->lbah == 0xab)) {
> -		DPRINTK("found ZAC device by sig\n");
> +	if ((tf->lbam == 0xcd) && (tf->lbah == 0xab))
>  		return ATA_DEV_ZAC;
> -	}
>  
> -	DPRINTK("unknown device\n");
>  	return ATA_DEV_UNKNOWN;
>  }
>  
> +/**
> + *	ata_port_classify - determine device type based on ATA-spec signature
> + *	@ap: ATA port device on which the classification should be run
> + *	@tf: ATA taskfile register set for device to be identified
> + *
> + *	A wrapper around ata_dev_classify() to provide additional logging
> + *
> + *	RETURNS:
> + *	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_port_classify(struct ata_port *ap,
> +			      const struct ata_taskfile *tf)
> +{
> +	unsigned int class = ata_dev_classify(tf);
> +
> +	if (class != ATA_DEV_UNKNOWN)
> +		ata_port_dbg(ap, "found %s device by sig\n",
> +			     ata_dev_class_string(class));
> +	else
> +		ata_port_dbg(ap, "found unknown device\n");
> +	return class;
> +}
> +
>  /**
>   *	ata_id_string - Convert IDENTIFY DEVICE page into string
>   *	@id: IDENTIFY DEVICE results we will examine
> @@ -7302,6 +7336,7 @@ EXPORT_SYMBOL_GPL(sata_link_hardreset);
>  EXPORT_SYMBOL_GPL(sata_std_hardreset);
>  EXPORT_SYMBOL_GPL(ata_std_postreset);
>  EXPORT_SYMBOL_GPL(ata_dev_classify);
> +EXPORT_SYMBOL_GPL(ata_port_classify);
>  EXPORT_SYMBOL_GPL(ata_dev_pair);
>  EXPORT_SYMBOL_GPL(ata_ratelimit);
>  EXPORT_SYMBOL_GPL(ata_msleep);
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 9fbe67f73197..939cda91c56d 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -1839,6 +1839,11 @@ unsigned int ata_sff_dev_classify(struct ata_device *dev, int present,
>  
>  	/* determine if device is ATA or ATAPI */
>  	class = ata_dev_classify(&tf);
> +	if (class != ATA_DEV_UNKNOWN)
> +		ata_dev_dbg(dev, "found %s device by sig\n",
> +			     ata_dev_class_string(class));
> +	else
> +		ata_dev_dbg(dev, "found unknown device\n");
>  
>  	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 d55ee244d693..0864c4fafa39 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -812,7 +812,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_port_classify(ap, &tf);
>  }
>  
>  static int sata_fsl_hardreset(struct ata_link *link, unsigned int *class,
> diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
> index a6b76cc12a66..12f189f2ab1e 100644
> --- a/drivers/ata/sata_inic162x.c
> +++ b/drivers/ata/sata_inic162x.c
> @@ -657,7 +657,7 @@ static int inic_hardreset(struct ata_link *link, unsigned int *class,
>  		}
>  
>  		inic_tf_read(ap, &tf);
> -		*class = ata_dev_classify(&tf);
> +		*class = ata_port_classify(ap, &tf);
>  	}
>  
>  	return 0;
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index 560070d4f1d0..2373cf5d8d14 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -677,7 +677,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_port_classify(ap, &tf);
>  
>  	DPRINTK("EXIT, class=%u\n", *class);
>  	return 0;
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 508f501095c9..0b784c298f97 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1173,7 +1173,10 @@ extern int ata_std_qc_defer(struct ata_queued_cmd *qc);
>  extern enum ata_completion_errors 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 const char *ata_dev_class_string(unsigned int class);
>  extern unsigned int ata_dev_classify(const struct ata_taskfile *tf);
> +extern unsigned int ata_port_classify(struct ata_port *ap,
> +				      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 10/42] libata: move ata_dump_id() to dynamic debugging

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Use ata_dev_dbg() to print out the information in ata_dump_id()
> and remove the ata_msg_probe() conditional.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/libata-core.c | 38 ++++++++++----------------------------
>  1 file changed, 10 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 17abc52ce41e..1987886140b6 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1440,6 +1440,7 @@ static int ata_hpa_resize(struct ata_device *dev)
>  
>  /**
>   *	ata_dump_id - IDENTIFY DEVICE info debugging output
> + *	@dev: device from which the information is fetched
>   *	@id: IDENTIFY DEVICE page to dump
>   *
>   *	Dump selected 16-bit words from the given IDENTIFY DEVICE
> @@ -1449,32 +1450,14 @@ static int ata_hpa_resize(struct ata_device *dev)
>   *	caller.
>   */
>  
> -static inline void ata_dump_id(const u16 *id)
> -{
> -	DPRINTK("49==0x%04x  "
> -		"53==0x%04x  "
> -		"63==0x%04x  "
> -		"64==0x%04x  "
> -		"75==0x%04x  \n",
> -		id[49],
> -		id[53],
> -		id[63],
> -		id[64],
> -		id[75]);
> -	DPRINTK("80==0x%04x  "
> -		"81==0x%04x  "
> -		"82==0x%04x  "
> -		"83==0x%04x  "
> -		"84==0x%04x  \n",
> -		id[80],
> -		id[81],
> -		id[82],
> -		id[83],
> -		id[84]);
> -	DPRINTK("88==0x%04x  "
> -		"93==0x%04x\n",
> -		id[88],
> -		id[93]);
> +static inline void ata_dump_id(struct ata_device *dev, const u16 *id)
> +{
> +	ata_dev_dbg(dev,
> +		"49==0x%04x  53==0x%04x  63==0x%04x  64==0x%04x  75==0x%04x\n"
> +		"80==0x%04x  81==0x%04x  82==0x%04x  83==0x%04x  84==0x%04x\n"
> +		"88==0x%04x  93==0x%04x\n",
> +		id[49], id[53], id[63], id[64], id[75], id[80],
> +		id[81], id[82], id[83], id[84], id[88], id[93]);
>  }
>  
>  /**
> @@ -2551,8 +2534,7 @@ int ata_dev_configure(struct ata_device *dev)
>  	/* find max transfer mode; for printk only */
>  	xfer_mask = ata_id_xfermask(id);
>  
> -	if (ata_msg_probe(ap))
> -		ata_dump_id(id);
> +	ata_dump_id(dev, 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 12/42] sata_mv: replace DPRINTK with 'pci_dump' module parameter

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Implement module parameter 'pci_dump' and move the DPRINTK calls
> over to dev_printk().
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/sata_mv.c | 88 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 49 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index d7228f8e9297..1eb93976af8d 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -83,6 +83,10 @@ module_param(irq_coalescing_usecs, int, S_IRUGO);
>  MODULE_PARM_DESC(irq_coalescing_usecs,
>  		 "IRQ coalescing time threshold in usecs");
>  
> +static int pci_dump;
> +module_param(pci_dump, int, S_IRUGO);
> +MODULE_PARM_DESC(pci_dump, "Enable dumping of PCI registers on error");
> +
>  enum {
>  	/* BAR's are enumerated in terms of pci_resource_start() terms */
>  	MV_PRIMARY_BAR		= 0,	/* offset 0x10: memory space */
> @@ -1245,42 +1249,43 @@ static int mv_stop_edma(struct ata_port *ap)
>  	return err;
>  }
>  
> -#ifdef ATA_DEBUG
> -static void mv_dump_mem(void __iomem *start, unsigned bytes)
> +static void mv_dump_mem(struct device *dev, void __iomem *start, unsigned bytes)
>  {
> -	int b, w;
> +	int b, w, o;
> +	unsigned char linebuf[38];
> +
>  	for (b = 0; b < bytes; ) {
> -		DPRINTK("%p: ", start + b);
> -		for (w = 0; b < bytes && w < 4; w++) {
> -			printk("%08x ", readl(start + b));
> +		for (w = 0, o = 0; b < bytes && w < 4; w++) {
> +			o += snprintf(linebuf + o, 38 - o,
> +				      "%08x ", readl(start + b));
>  			b += sizeof(u32);
>  		}
> -		printk("\n");
> +		dev_printk(KERN_DEBUG, dev, "%s: %p: %s\n",
> +			   __func__, start + b, linebuf);
>  	}
>  }
> -#endif
> -#if defined(ATA_DEBUG) || defined(CONFIG_PCI)
> +
>  static void mv_dump_pci_cfg(struct pci_dev *pdev, unsigned bytes)
>  {
> -#ifdef ATA_DEBUG
> -	int b, w;
> +	int b, w, o;
>  	u32 dw;
> +	unsigned char linebuf[38];
> +
>  	for (b = 0; b < bytes; ) {
> -		DPRINTK("%02x: ", b);
> -		for (w = 0; b < bytes && w < 4; w++) {
> +		for (w = 0, o = 0; b < bytes && w < 4; w++) {
>  			(void) pci_read_config_dword(pdev, b, &dw);
> -			printk("%08x ", dw);
> +			o += snprintf(linebuf + o, 38 - o,
> +				      "%08x ", dw);
>  			b += sizeof(u32);
>  		}
> -		printk("\n");
> +		dev_printk(KERN_DEBUG, &pdev->dev, "%s: %02x: %s\n",
> +			   __func__, b, linebuf);
>  	}
> -#endif
>  }
> -#endif
> +
>  static void mv_dump_all_regs(void __iomem *mmio_base, int port,
>  			     struct pci_dev *pdev)
>  {
> -#ifdef ATA_DEBUG
>  	void __iomem *hc_base = mv_hc_base(mmio_base,
>  					   port >> MV_PORT_HC_SHIFT);
>  	void __iomem *port_base;
> @@ -1295,31 +1300,34 @@ static void mv_dump_all_regs(void __iomem *mmio_base, int port,
>  		start_port = port;
>  		num_ports = num_hcs = 1;
>  	}
> -	DPRINTK("All registers for port(s) %u-%u:\n", start_port,
> -		num_ports > 1 ? num_ports - 1 : start_port);
> +	dev_printk(KERN_DEBUG, &pdev->dev,
> +		   "%s: All registers for port(s) %u-%u:\n", __func__,
> +		   start_port, num_ports > 1 ? num_ports - 1 : start_port);
>  
> -	if (NULL != pdev) {
> -		DPRINTK("PCI config space regs:\n");
> -		mv_dump_pci_cfg(pdev, 0x68);
> -	}
> -	DPRINTK("PCI regs:\n");
> -	mv_dump_mem(mmio_base+0xc00, 0x3c);
> -	mv_dump_mem(mmio_base+0xd00, 0x34);
> -	mv_dump_mem(mmio_base+0xf00, 0x4);
> -	mv_dump_mem(mmio_base+0x1d00, 0x6c);
> +	dev_printk(KERN_DEBUG, &pdev->dev,
> +		   "%s: PCI config space regs:\n", __func__);
> +	mv_dump_pci_cfg(pdev, 0x68);
> +
> +	dev_printk(KERN_DEBUG, &pdev->dev, "%s: PCI regs:\n", __func__);
> +	mv_dump_mem(&pdev->dev, mmio_base+0xc00, 0x3c);
> +	mv_dump_mem(&pdev->dev, mmio_base+0xd00, 0x34);
> +	mv_dump_mem(&pdev->dev, mmio_base+0xf00, 0x4);
> +	mv_dump_mem(&pdev->dev, mmio_base+0x1d00, 0x6c);
>  	for (hc = start_hc; hc < start_hc + num_hcs; hc++) {
>  		hc_base = mv_hc_base(mmio_base, hc);
> -		DPRINTK("HC regs (HC %i):\n", hc);
> -		mv_dump_mem(hc_base, 0x1c);
> +		dev_printk(KERN_DEBUG, &pdev->dev, "%s: HC regs (HC %i):\n",
> +			   __func__, hc);
> +		mv_dump_mem(&pdev->dev, hc_base, 0x1c);
>  	}
>  	for (p = start_port; p < start_port + num_ports; p++) {
>  		port_base = mv_port_base(mmio_base, p);
> -		DPRINTK("EDMA regs (port %i):\n", p);
> -		mv_dump_mem(port_base, 0x54);
> -		DPRINTK("SATA regs (port %i):\n", p);
> -		mv_dump_mem(port_base+0x300, 0x60);
> +		dev_printk(KERN_DEBUG, &pdev->dev, "%s: EDMA regs (port %i):\n",
> +			   __func__, p);
> +		mv_dump_mem(&pdev->dev, port_base, 0x54);
> +		dev_printk(KERN_DEBUG, &pdev->dev, "%s: SATA regs (port %i):\n",
> +			   __func__, p);
> +		mv_dump_mem(&pdev->dev, port_base+0x300, 0x60);
>  	}
> -#endif
>  }
>  
>  static unsigned int mv_scr_offset(unsigned int sc_reg_in)
> @@ -2958,9 +2966,11 @@ static int mv_pci_error(struct ata_host *host, void __iomem *mmio)
>  
>  	dev_err(host->dev, "PCI ERROR; PCI IRQ cause=0x%08x\n", err_cause);
>  
> -	DPRINTK("All regs @ PCI error\n");
> -	mv_dump_all_regs(mmio, -1, to_pci_dev(host->dev));
> -
> +	if (pci_dump) {
> +		dev_printk(KERN_DEBUG, host->dev, "%s: All regs @ PCI error\n",
> +			   __func__);
> +		mv_dump_all_regs(mmio, -1, to_pci_dev(host->dev));
> +	}
>  	writelfl(0, mmio + hpriv->irq_cause_offset);
>  
>  	for (i = 0; i < host->n_ports; i++) {
> 

Re: [PATCH 35/42] sata_nv: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:54 AM, Hannes Reinecke wrote:
> Replace all DPRINTK calls with the ata_XXX_dbg functions.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/sata_nv.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index f7dd32679c8e..0bdf18625d95 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -2094,7 +2094,7 @@ static int nv_swncq_sdbfis(struct ata_port *ap)
>  	ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask);
>  
>  	if (!ap->qc_active) {
> -		DPRINTK("over\n");
> +		ata_port_dbg(ap, "over\n");
>  		nv_swncq_pp_reinit(ap);
>  		return 0;
>  	}
> @@ -2109,10 +2109,10 @@ static int nv_swncq_sdbfis(struct ata_port *ap)
>  		 */
>  		lack_dhfis = 1;
>  
> -	DPRINTK("id 0x%x QC: qc_active 0x%x,"
> +	ata_port_dbg(ap, "QC: qc_active 0x%llx,"
>  		"SWNCQ:qc_active 0x%X defer_bits %X "
>  		"dhfis 0x%X dmafis 0x%X last_issue_tag %x\n",
> -		ap->print_id, ap->qc_active, pp->qc_active,
> +		ap->qc_active, pp->qc_active,
>  		pp->defer_queue.defer_bits, pp->dhfis_bits,
>  		pp->dmafis_bits, pp->last_issue_tag);
>  
> @@ -2154,7 +2154,7 @@ static void nv_swncq_dmafis(struct ata_port *ap)
>  	__ata_bmdma_stop(ap);
>  	tag = nv_swncq_tag(ap);
>  
> -	DPRINTK("dma setup tag 0x%x\n", tag);
> +	ata_port_dbg(ap, "dma setup tag 0x%x\n", tag);
>  	qc = ata_qc_from_tag(ap, tag);
>  
>  	if (unlikely(!qc))
> @@ -2222,9 +2222,9 @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis)
>  
>  	if (fis & NV_SWNCQ_IRQ_SDBFIS) {
>  		pp->ncq_flags |= ncq_saw_sdb;
> -		DPRINTK("id 0x%x SWNCQ: qc_active 0x%X "
> +		ata_port_dbg(ap, "SWNCQ: qc_active 0x%X "
>  			"dhfis 0x%X dmafis 0x%X sactive 0x%X\n",
> -			ap->print_id, pp->qc_active, pp->dhfis_bits,
> +			pp->qc_active, pp->dhfis_bits,
>  			pp->dmafis_bits, readl(pp->sactive_block));
>  		if (nv_swncq_sdbfis(ap) < 0)
>  			goto irq_error;
> @@ -2250,7 +2250,7 @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis)
>  				goto irq_exit;
>  
>  			if (pp->defer_queue.defer_bits) {
> -				DPRINTK("send next command\n");
> +				ata_port_dbg(ap, "send next command\n");
>  				qc = nv_swncq_qc_from_dq(ap);
>  				nv_swncq_issue_atacmd(ap, qc);
>  			}
> 

Re: [PATCH 15/42] libata: add reset tracepoints

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> To follow the flow of control we should be using tracepoints, as
> they will tie in with the actual I/O flow and deliver a better
> overview about what it happening.
> This patch adds tracepoints for hard reset, soft reset, and postreset
> and adds them in the libata-eh control flow.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/libata-eh.c       | 21 ++++++++--
>  include/trace/events/libata.h | 96 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 114 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 3bfd9da58473..ef3576eb5874 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -2787,12 +2787,19 @@ int ata_eh_reset(struct ata_link *link, int classify,
>  
>  		/* mark that this EH session started with reset */
>  		ehc->last_reset = jiffies;
> -		if (reset == hardreset)
> +		if (reset == hardreset) {
>  			ehc->i.flags |= ATA_EHI_DID_HARDRESET;
> -		else
> +			trace_ata_link_hardreset_begin(link, classes, deadline);
> +		} else {
>  			ehc->i.flags |= ATA_EHI_DID_SOFTRESET;
> +			trace_ata_link_softreset_begin(link, classes, deadline);
> +		}
>  
>  		rc = ata_do_reset(link, reset, classes, deadline, true);
> +		if (reset == hardreset)
> +			trace_ata_link_hardreset_end(link, classes, rc);
> +		else
> +			trace_ata_link_softreset_end(link, classes, rc);
>  		if (rc && rc != -EAGAIN) {
>  			failed_link = link;
>  			goto fail;
> @@ -2806,8 +2813,11 @@ int ata_eh_reset(struct ata_link *link, int classify,
>  				ata_link_info(slave, "hard resetting link\n");
>  
>  			ata_eh_about_to_do(slave, NULL, ATA_EH_RESET);
> +			trace_ata_slave_hardreset_begin(slave, classes,
> +							deadline);
>  			tmp = ata_do_reset(slave, reset, classes, deadline,
>  					   false);
> +			trace_ata_slave_hardreset_end(slave, classes, tmp);
>  			switch (tmp) {
>  			case -EAGAIN:
>  				rc = -EAGAIN;
> @@ -2834,7 +2844,9 @@ int ata_eh_reset(struct ata_link *link, int classify,
>  			}
>  
>  			ata_eh_about_to_do(link, NULL, ATA_EH_RESET);
> +			trace_ata_link_softreset_begin(link, classes, deadline);
>  			rc = ata_do_reset(link, reset, classes, deadline, true);
> +			trace_ata_link_softreset_end(link, classes, rc);
>  			if (rc) {
>  				failed_link = link;
>  				goto fail;
> @@ -2888,8 +2900,11 @@ int ata_eh_reset(struct ata_link *link, int classify,
>  	 */
>  	if (postreset) {
>  		postreset(link, classes);
> -		if (slave)
> +		trace_ata_link_postreset(link, classes, rc);
> +		if (slave) {
>  			postreset(slave, classes);
> +			trace_ata_slave_postreset(slave, classes, rc);
> +		}
>  	}
>  
>  	/*
> diff --git a/include/trace/events/libata.h b/include/trace/events/libata.h
> index ab69434e2329..ec2a350d1aca 100644
> --- a/include/trace/events/libata.h
> +++ b/include/trace/events/libata.h
> @@ -132,6 +132,22 @@
>  		ata_protocol_name(ATAPI_PROT_PIO),	\
>  		ata_protocol_name(ATAPI_PROT_DMA))
>  
> +#define ata_class_name(class)	{ class, #class }
> +#define show_class_name(val)				\
> +	__print_symbolic(val,				\
> +		ata_class_name(ATA_DEV_UNKNOWN),	\
> +		ata_class_name(ATA_DEV_ATA),		\
> +		ata_class_name(ATA_DEV_ATA_UNSUP),	\
> +		ata_class_name(ATA_DEV_ATAPI),		\
> +		ata_class_name(ATA_DEV_ATAPI_UNSUP),	\
> +		ata_class_name(ATA_DEV_PMP),		\
> +		ata_class_name(ATA_DEV_PMP_UNSUP),	\
> +		ata_class_name(ATA_DEV_SEMB),		\
> +		ata_class_name(ATA_DEV_SEMB_UNSUP),	\
> +		ata_class_name(ATA_DEV_ZAC),		\
> +		ata_class_name(ATA_DEV_ZAC_UNSUP),	\
> +		ata_class_name(ATA_DEV_NONE))
> +
>  const char *libata_trace_parse_status(struct trace_seq*, unsigned char);
>  #define __parse_status(s) libata_trace_parse_status(p, s)
>  
> @@ -329,6 +345,86 @@ TRACE_EVENT(ata_eh_link_autopsy_qc,
>  		  __parse_eh_err_mask(__entry->eh_err_mask))
>  );
>  
> +DECLARE_EVENT_CLASS(ata_link_reset_begin_template,
> +
> +	TP_PROTO(struct ata_link *link, unsigned int *class, unsigned long deadline),
> +
> +	TP_ARGS(link, class, deadline),
> +
> +	TP_STRUCT__entry(
> +		__field( unsigned int,	ata_port )
> +		__array( unsigned int,	class, 2 )
> +		__field( unsigned long,	deadline )
> +	),
> +
> +	TP_fast_assign(
> +		__entry->ata_port	= link->ap->print_id;
> +		memcpy(__entry->class, class, 2);
> +		__entry->deadline	= deadline;
> +	),
> +
> +	TP_printk("ata_port=%u deadline=%lu classes=[%s,%s]",
> +		  __entry->ata_port, __entry->deadline,
> +		  show_class_name(__entry->class[0]),
> +		  show_class_name(__entry->class[1]))
> +);
> +
> +DEFINE_EVENT(ata_link_reset_begin_template, ata_link_hardreset_begin,
> +	     TP_PROTO(struct ata_link *link, unsigned int *class, unsigned long deadline),
> +	     TP_ARGS(link, class, deadline));
> +
> +DEFINE_EVENT(ata_link_reset_begin_template, ata_slave_hardreset_begin,
> +	     TP_PROTO(struct ata_link *link, unsigned int *class, unsigned long deadline),
> +	     TP_ARGS(link, class, deadline));
> +
> +DEFINE_EVENT(ata_link_reset_begin_template, ata_link_softreset_begin,
> +	     TP_PROTO(struct ata_link *link, unsigned int *class, unsigned long deadline),
> +	     TP_ARGS(link, class, deadline));
> +
> +DECLARE_EVENT_CLASS(ata_link_reset_end_template,
> +
> +	TP_PROTO(struct ata_link *link, unsigned int *class, int rc),
> +
> +	TP_ARGS(link, class, rc),
> +
> +	TP_STRUCT__entry(
> +		__field( unsigned int,	ata_port )
> +		__array( unsigned int,	class, 2 )
> +		__field( int,		rc	)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->ata_port	= link->ap->print_id;
> +		memcpy(__entry->class, class, 2);
> +		__entry->rc		= rc;
> +	),
> +
> +	TP_printk("ata_port=%u rc=%d class=[%s,%s]",
> +		  __entry->ata_port, __entry->rc,
> +		  show_class_name(__entry->class[0]),
> +		  show_class_name(__entry->class[1]))
> +);
> +
> +DEFINE_EVENT(ata_link_reset_end_template, ata_link_hardreset_end,
> +	     TP_PROTO(struct ata_link *link, unsigned int *class, int rc),
> +	     TP_ARGS(link, class, rc));
> +
> +DEFINE_EVENT(ata_link_reset_end_template, ata_slave_hardreset_end,
> +	     TP_PROTO(struct ata_link *link, unsigned int *class, int rc),
> +	     TP_ARGS(link, class, rc));
> +
> +DEFINE_EVENT(ata_link_reset_end_template, ata_link_softreset_end,
> +	     TP_PROTO(struct ata_link *link, unsigned int *class, int rc),
> +	     TP_ARGS(link, class, rc));
> +
> +DEFINE_EVENT(ata_link_reset_end_template, ata_link_postreset,
> +	     TP_PROTO(struct ata_link *link, unsigned int *class, int rc),
> +	     TP_ARGS(link, class, rc));
> +
> +DEFINE_EVENT(ata_link_reset_end_template, ata_slave_postreset,
> +	     TP_PROTO(struct ata_link *link, unsigned int *class, int rc),
> +	     TP_ARGS(link, class, rc));
> +
>  #endif /*  _TRACE_LIBATA_H */
>  
>  /* This part must be outside protection */
> 

Re: [PATCH 17/42] libata: tracepoints for bus-master DMA

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Add tracepoints for bus-master DMA and taskfile related functions.
> That allows us to drop the relevant DPRINTK() calls.

..DPRINTK() & VPRINTK() calls.

Also please see comments below (from v1 review).

> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/ata/libata-core.c     |  5 +++
>  drivers/ata/libata-sff.c      | 23 +++++++---
>  include/trace/events/libata.h | 97 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 120 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 329cc587eeab..695974d0c634 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -7383,3 +7383,8 @@ EXPORT_SYMBOL_GPL(ata_cable_ignore);
>  EXPORT_SYMBOL_GPL(ata_cable_sata);
>  EXPORT_SYMBOL_GPL(ata_host_get);
>  EXPORT_SYMBOL_GPL(ata_host_put);
> +
> +EXPORT_TRACEPOINT_SYMBOL_GPL(ata_tf_load);
> +EXPORT_TRACEPOINT_SYMBOL_GPL(ata_exec_command);
> +EXPORT_TRACEPOINT_SYMBOL_GPL(ata_bmdma_setup);
> +EXPORT_TRACEPOINT_SYMBOL_GPL(ata_bmdma_start);
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 25c10f3eef83..e2d1504f7562 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -22,7 +22,7 @@
>  #include <linux/module.h>
>  #include <linux/libata.h>
>  #include <linux/highmem.h>
> -
> +#include <trace/events/libata.h>
>  #include "libata.h"
>  
>  static struct workqueue_struct *ata_sff_wq;
> @@ -509,6 +509,7 @@ EXPORT_SYMBOL_GPL(ata_sff_exec_command);
>   *	ata_tf_to_host - issue ATA taskfile to host controller
>   *	@ap: port to which command is being issued
>   *	@tf: ATA taskfile register set
> + *	@tag: tag of the associated command
>   *
>   *	Issues ATA taskfile register set to ATA host controller,
>   *	with proper synchronization with interrupt handler and
> @@ -518,9 +519,12 @@ EXPORT_SYMBOL_GPL(ata_sff_exec_command);
>   *	spin_lock_irqsave(host lock)
>   */
>  static inline void ata_tf_to_host(struct ata_port *ap,
> -				  const struct ata_taskfile *tf)
> +				  const struct ata_taskfile *tf,
> +				  unsigned int tag)
>  {
> +	trace_ata_tf_load(ap, tf);
>  	ap->ops->sff_tf_load(ap, tf);
> +	trace_ata_exec_command(ap, tf, tag);
>  	ap->ops->sff_exec_command(ap, tf);
>  }
>  
> @@ -753,6 +757,7 @@ static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
>  	case ATAPI_PROT_DMA:
>  		ap->hsm_task_state = HSM_ST_LAST;
>  		/* initiate bmdma */
> +		trace_ata_bmdma_start(ap, &qc->tf, qc->tag);
>  		ap->ops->bmdma_start(qc);
>  		break;
>  #endif /* CONFIG_ATA_BMDMA */
> @@ -1361,7 +1366,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
>  		if (qc->tf.flags & ATA_TFLAG_POLLING)
>  			ata_qc_set_polling(qc);
>  
> -		ata_tf_to_host(ap, &qc->tf);
> +		ata_tf_to_host(ap, &qc->tf, qc->tag);
>  		ap->hsm_task_state = HSM_ST_LAST;
>  
>  		if (qc->tf.flags & ATA_TFLAG_POLLING)
> @@ -1373,7 +1378,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
>  		if (qc->tf.flags & ATA_TFLAG_POLLING)
>  			ata_qc_set_polling(qc);
>  
> -		ata_tf_to_host(ap, &qc->tf);
> +		ata_tf_to_host(ap, &qc->tf, qc->tag);
>  
>  		if (qc->tf.flags & ATA_TFLAG_WRITE) {
>  			/* PIO data out protocol */
> @@ -1403,7 +1408,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
>  		if (qc->tf.flags & ATA_TFLAG_POLLING)
>  			ata_qc_set_polling(qc);
>  
> -		ata_tf_to_host(ap, &qc->tf);
> +		ata_tf_to_host(ap, &qc->tf, qc->tag);
>  
>  		ap->hsm_task_state = HSM_ST_FIRST;
>  
> @@ -2735,8 +2740,11 @@ unsigned int ata_bmdma_qc_issue(struct ata_queued_cmd *qc)
>  	case ATA_PROT_DMA:
>  		WARN_ON_ONCE(qc->tf.flags & ATA_TFLAG_POLLING);
>  
> +		trace_ata_tf_load(ap, &qc->tf);
>  		ap->ops->sff_tf_load(ap, &qc->tf);  /* load tf registers */
> +		trace_ata_bmdma_setup(ap, &qc->tf, qc->tag);
>  		ap->ops->bmdma_setup(qc);	    /* set up bmdma */
> +		trace_ata_bmdma_start(ap, &qc->tf, qc->tag);
>  		ap->ops->bmdma_start(qc);	    /* initiate bmdma */
>  		ap->hsm_task_state = HSM_ST_LAST;
>  		break;
> @@ -2744,7 +2752,9 @@ unsigned int ata_bmdma_qc_issue(struct ata_queued_cmd *qc)
>  	case ATAPI_PROT_DMA:
>  		WARN_ON_ONCE(qc->tf.flags & ATA_TFLAG_POLLING);
>  
> +		trace_ata_tf_load(ap, &qc->tf);
>  		ap->ops->sff_tf_load(ap, &qc->tf);  /* load tf registers */
> +		trace_ata_bmdma_setup(ap, &qc->tf, qc->tag);
>  		ap->ops->bmdma_setup(qc);	    /* set up bmdma */
>  		ap->hsm_task_state = HSM_ST_FIRST;
>  
> @@ -2792,6 +2802,7 @@ unsigned int ata_bmdma_port_intr(struct ata_port *ap, struct ata_queued_cmd *qc)
>  			return ata_sff_idle_irq(ap);
>  
>  		/* before we do anything else, clear DMA-Start bit */
> +		trace_ata_bmdma_stop(ap, &qc->tf, qc->tag);
>  		ap->ops->bmdma_stop(qc);
>  		bmdma_stopped = true;
>  
> @@ -2871,6 +2882,7 @@ void ata_bmdma_error_handler(struct ata_port *ap)
>  			thaw = true;
>  		}
>  
> +		trace_ata_bmdma_stop(ap, &qc->tf, qc->tag);
>  		ap->ops->bmdma_stop(qc);
>  
>  		/* if we're gonna thaw, make sure IRQ is clear */
> @@ -2904,6 +2916,7 @@ void ata_bmdma_post_internal_cmd(struct ata_queued_cmd *qc)
>  
>  	if (ata_is_dma(qc->tf.protocol)) {
>  		spin_lock_irqsave(ap->lock, flags);
> +		trace_ata_bmdma_stop(ap, &qc->tf, qc->tag);
>  		ap->ops->bmdma_stop(qc);
>  		spin_unlock_irqrestore(ap->lock, flags);
>  	}
> diff --git a/include/trace/events/libata.h b/include/trace/events/libata.h
> index ec2a350d1aca..f2cd6daa6b2d 100644
> --- a/include/trace/events/libata.h
> +++ b/include/trace/events/libata.h
> @@ -291,6 +291,103 @@ DEFINE_EVENT(ata_qc_complete_template, ata_qc_complete_done,
>  	     TP_PROTO(struct ata_queued_cmd *qc),
>  	     TP_ARGS(qc));
>  
> +TRACE_EVENT(ata_tf_load,
> +
> +	TP_PROTO(struct ata_port *ap, const struct ata_taskfile *tf),
> +
> +	TP_ARGS(ap, tf),
> +
> +	TP_STRUCT__entry(
> +		__field( unsigned int,	ata_port )
> +		__field( unsigned char,	cmd	)
> +		__field( unsigned char,	dev	)
> +		__field( unsigned char,	lbal	)
> +		__field( unsigned char,	lbam	)
> +		__field( unsigned char,	lbah	)
> +		__field( unsigned char,	nsect	)
> +		__field( unsigned char,	feature	)
> +		__field( unsigned char,	hob_lbal )
> +		__field( unsigned char,	hob_lbam )
> +		__field( unsigned char,	hob_lbah )
> +		__field( unsigned char,	hob_nsect )
> +		__field( unsigned char,	hob_feature )
> +		__field( unsigned char,	proto	)
> +		__field( unsigned long,	flags	)
> +	),

'flags' field doesn't seem to be used?

> +	TP_fast_assign(
> +		__entry->ata_port	= ap->print_id;
> +		__entry->proto		= tf->protocol;
> +		__entry->cmd		= tf->command;
> +		__entry->dev		= tf->device;
> +		__entry->lbal		= tf->lbal;
> +		__entry->lbam		= tf->lbam;
> +		__entry->lbah		= tf->lbah;
> +		__entry->hob_lbal	= tf->hob_lbal;
> +		__entry->hob_lbam	= tf->hob_lbam;
> +		__entry->hob_lbah	= tf->hob_lbah;
> +		__entry->feature	= tf->feature;
> +		__entry->hob_feature	= tf->hob_feature;
> +		__entry->nsect		= tf->nsect;
> +		__entry->hob_nsect	= tf->hob_nsect;
> +	),
> +
> +	TP_printk("ata_port=%u proto=%s cmd=%s%s " \
> +		  " tf=(%02x/%02x:%02x:%02x:%02x:%02x/%02x:%02x:%02x:%02x:%02x/%02x)",
> +		  __entry->ata_port,
> +		  show_protocol_name(__entry->proto),
> +		  show_opcode_name(__entry->cmd),
> +		  __parse_subcmd(__entry->cmd, __entry->feature, __entry->hob_nsect),
> +		  __entry->cmd, __entry->feature, __entry->nsect,
> +		  __entry->lbal, __entry->lbam, __entry->lbah,
> +		  __entry->hob_feature, __entry->hob_nsect,
> +		  __entry->hob_lbal, __entry->hob_lbam, __entry->hob_lbah,
> +		  __entry->dev)
> +);
> +
> +DECLARE_EVENT_CLASS(ata_exec_command_template,
> +
> +	TP_PROTO(struct ata_port *ap, const struct ata_taskfile *tf, unsigned int tag),
> +
> +	TP_ARGS(ap, tf, tag),
> +
> +	TP_STRUCT__entry(
> +		__field( unsigned int,	ata_port )
> +		__field( unsigned int,	tag	)
> +		__field( unsigned char,	cmd	)
> +		__field( unsigned char,	proto	)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->ata_port	= ap->print_id;
> +		__entry->tag		= tag;
> +		__entry->proto		= tf->protocol;
> +		__entry->cmd		= tf->command;
> +	),
> +
> +	TP_printk("ata_port=%u cmd=%s%s tag=%d",
> +		  __entry->ata_port,
> +		  show_protocol_name(__entry->proto),
> +		  show_opcode_name(__entry->cmd),
> +		  __entry->tag)

Please keep both new events consistent regarding 'proto' and
'cmd' fields printing (add "proto=%s" in this event or rework
printing in ata_tf_load one).

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

> +);
> +
> +DEFINE_EVENT(ata_exec_command_template, ata_exec_command,
> +	     TP_PROTO(struct ata_port *ap, const struct ata_taskfile *tf, unsigned int tag),
> +	     TP_ARGS(ap, tf, tag));
> +
> +DEFINE_EVENT(ata_exec_command_template, ata_bmdma_setup,
> +	     TP_PROTO(struct ata_port *ap, const struct ata_taskfile *tf, unsigned int tag),
> +	     TP_ARGS(ap, tf, tag));
> +
> +DEFINE_EVENT(ata_exec_command_template, ata_bmdma_start,
> +	     TP_PROTO(struct ata_port *ap, const struct ata_taskfile *tf, unsigned int tag),
> +	     TP_ARGS(ap, tf, tag));
> +
> +DEFINE_EVENT(ata_exec_command_template, ata_bmdma_stop,
> +	     TP_PROTO(struct ata_port *ap, const struct ata_taskfile *tf, unsigned int tag),
> +	     TP_ARGS(ap, tf, tag));
> +
>  TRACE_EVENT(ata_eh_link_autopsy,
>  
>  	TP_PROTO(struct ata_device *dev, unsigned int eh_action, unsigned int eh_err_mask),
> 

Re: [PATCH 18/42] libata: drop DPRINTK calls for bus-master DMA

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Bus-master DMA is now logged with generic tracepoints, so we can
> drop the DPRINTK() calls here.

..drop DPRINTK(), VPRINTK() and dev_dbg() calls here.

Also please replace "DPRINTK" with "debugging" in the patch summary.

With the above fixes:

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

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

> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/ata/libata-sff.c        | 18 +-----------------
>  drivers/ata/pata_octeon_cf.c    | 10 ++--------
>  drivers/ata/pata_pdc202xx_old.c |  2 --
>  drivers/ata/pata_sil680.c       |  1 -
>  drivers/ata/sata_dwc_460ex.c    | 18 ++++--------------
>  drivers/ata/sata_rcar.c         |  2 --
>  6 files changed, 7 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index e2d1504f7562..1078b621d47b 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -413,12 +413,6 @@ void ata_sff_tf_load(struct ata_port *ap, const struct ata_taskfile *tf)
>  		iowrite8(tf->hob_lbal, ioaddr->lbal_addr);
>  		iowrite8(tf->hob_lbam, ioaddr->lbam_addr);
>  		iowrite8(tf->hob_lbah, ioaddr->lbah_addr);
> -		VPRINTK("hob: feat 0x%X nsect 0x%X, lba 0x%X 0x%X 0x%X\n",
> -			tf->hob_feature,
> -			tf->hob_nsect,
> -			tf->hob_lbal,
> -			tf->hob_lbam,
> -			tf->hob_lbah);
>  	}
>  
>  	if (is_addr) {
> @@ -427,18 +421,10 @@ void ata_sff_tf_load(struct ata_port *ap, const struct ata_taskfile *tf)
>  		iowrite8(tf->lbal, ioaddr->lbal_addr);
>  		iowrite8(tf->lbam, ioaddr->lbam_addr);
>  		iowrite8(tf->lbah, ioaddr->lbah_addr);
> -		VPRINTK("feat 0x%X nsect 0x%X lba 0x%X 0x%X 0x%X\n",
> -			tf->feature,
> -			tf->nsect,
> -			tf->lbal,
> -			tf->lbam,
> -			tf->lbah);
>  	}
>  
> -	if (tf->flags & ATA_TFLAG_DEVICE) {
> +	if (tf->flags & ATA_TFLAG_DEVICE)
>  		iowrite8(tf->device, ioaddr->device_addr);
> -		VPRINTK("device 0x%X\n", tf->device);
> -	}
>  
>  	ata_wait_idle(ap);
>  }
> @@ -498,8 +484,6 @@ 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);
> -
>  	iowrite8(tf->command, ap->ioaddr.command_addr);
>  	ata_sff_pause(ap);
>  }
> diff --git a/drivers/ata/pata_octeon_cf.c b/drivers/ata/pata_octeon_cf.c
> index 7c87168a1932..9b66552efbd2 100644
> --- a/drivers/ata/pata_octeon_cf.c
> +++ b/drivers/ata/pata_octeon_cf.c
> @@ -514,20 +514,14 @@ static void octeon_cf_exec_command16(struct ata_port *ap,
>  {
>  	/* The base of the registers is at ioaddr.data_addr. */
>  	void __iomem *base = ap->ioaddr.data_addr;
> -	u16 blob;
> +	u16 blob = 0;
>  
> -	if (tf->flags & ATA_TFLAG_DEVICE) {
> -		VPRINTK("device 0x%X\n", tf->device);
> +	if (tf->flags & ATA_TFLAG_DEVICE)
>  		blob = tf->device;
> -	} else {
> -		blob = 0;
> -	}
>  
> -	DPRINTK("ata%u: cmd 0x%X\n", ap->print_id, tf->command);
>  	blob |= (tf->command << 8);
>  	__raw_writew(blob, base + 6);
>  
> -
>  	ata_wait_idle(ap);
>  }
>  
> diff --git a/drivers/ata/pata_pdc202xx_old.c b/drivers/ata/pata_pdc202xx_old.c
> index 378ed9ea97e9..3778270e762f 100644
> --- a/drivers/ata/pata_pdc202xx_old.c
> +++ b/drivers/ata/pata_pdc202xx_old.c
> @@ -38,8 +38,6 @@ 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);
> -
>  	iowrite8(tf->command, ap->ioaddr.command_addr);
>  	ndelay(400);
>  }
> diff --git a/drivers/ata/pata_sil680.c b/drivers/ata/pata_sil680.c
> index 7ab9aea3b630..42ea13dd4ace 100644
> --- a/drivers/ata/pata_sil680.c
> +++ b/drivers/ata/pata_sil680.c
> @@ -211,7 +211,6 @@ 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);
>  	iowrite8(tf->command, ap->ioaddr.command_addr);
>  	ioread8(ap->ioaddr.bmdma_addr + ATA_DMA_CMD);
>  }
> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 9dcef6ac643b..ef07d4a03627 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -970,9 +970,6 @@ static void sata_dwc_exec_command_by_tag(struct ata_port *ap,
>  {
>  	struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
>  
> -	dev_dbg(ap->dev, "%s cmd(0x%02x): %s tag=%d\n", __func__, tf->command,
> -		ata_get_cmd_descript(tf->command), tag);
> -
>  	hsdevp->cmd_issued[tag] = cmd_issued;
>  
>  	/*
> @@ -995,12 +992,9 @@ static void sata_dwc_bmdma_setup(struct ata_queued_cmd *qc)
>  {
>  	u8 tag = qc->hw_tag;
>  
> -	if (ata_is_ncq(qc->tf.protocol)) {
> -		dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x tag=%d\n",
> -			__func__, qc->ap->link.sactive, tag);
> -	} else {
> +	if (!ata_is_ncq(qc->tf.protocol))
>  		tag = 0;
> -	}
> +
>  	sata_dwc_bmdma_setup_by_tag(qc, tag);
>  }
>  
> @@ -1057,13 +1051,9 @@ static void sata_dwc_bmdma_start(struct ata_queued_cmd *qc)
>  {
>  	u8 tag = qc->hw_tag;
>  
> -	if (ata_is_ncq(qc->tf.protocol)) {
> -		dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x tag=%d\n",
> -			__func__, qc->ap->link.sactive, tag);
> -	} else {
> +	if (!ata_is_ncq(qc->tf.protocol))
>  		tag = 0;
> -	}
> -	dev_dbg(qc->ap->dev, "%s\n", __func__);
> +
>  	sata_dwc_bmdma_start_by_tag(qc, tag);
>  }
>  
> diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
> index 1b42be234761..0925a0564cc5 100644
> --- a/drivers/ata/sata_rcar.c
> +++ b/drivers/ata/sata_rcar.c
> @@ -436,8 +436,6 @@ 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);
> -
>  	iowrite32(tf->command, ap->ioaddr.command_addr);
>  	ata_sff_pause(ap);
>  }
> 

Re: [PATCH 19/42] pata_octeon_cf: add bmdma tracepoints and drop DPRINTK() calls

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Add missing bmdma tracepoints and drop the now pointless
> DPRINTK() calls.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/pata_octeon_cf.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/pata_octeon_cf.c b/drivers/ata/pata_octeon_cf.c
> index 9b66552efbd2..62646dbc9d71 100644
> --- a/drivers/ata/pata_octeon_cf.c
> +++ b/drivers/ata/pata_octeon_cf.c
> @@ -19,7 +19,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <scsi/scsi_host.h>
> -
> +#include <trace/events/libata.h>
>  #include <asm/byteorder.h>
>  #include <asm/octeon/octeon.h>
>  
> @@ -535,12 +535,10 @@ 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");
>  	/* issue r/w command */
>  	qc->cursg = qc->sg;
>  	cf_port->dma_finished = 0;
>  	ap->ops->sff_exec_command(ap, &qc->tf);
> -	DPRINTK("EXIT\n");
>  }
>  
>  /**
> @@ -792,8 +790,11 @@ static unsigned int octeon_cf_qc_issue(struct ata_queued_cmd *qc)
>  	case ATA_PROT_DMA:
>  		WARN_ON(qc->tf.flags & ATA_TFLAG_POLLING);
>  
> +		trace_ata_tf_load(ap, &qc->tf);
>  		ap->ops->sff_tf_load(ap, &qc->tf);  /* load tf registers */
> +		trace_ata_bmdma_setup(ap, &qc->tf, qc->tag);
>  		octeon_cf_dma_setup(qc);	    /* set up dma */
> +		trace_ata_bmdma_start(ap, &qc->tf, qc->tag);
>  		octeon_cf_dma_start(qc);	    /* initiate dma */
>  		ap->hsm_task_state = HSM_ST_LAST;
>  		break;
> 

Re: [PATCH 20/42] pata_arasan_cf: use generic tracepoints

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> For completeness add generic tracepoints for bus-master DMA.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/pata_arasan_cf.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/ata/pata_arasan_cf.c b/drivers/ata/pata_arasan_cf.c
> index e9cf31f38450..f066c47f8d6e 100644
> --- a/drivers/ata/pata_arasan_cf.c
> +++ b/drivers/ata/pata_arasan_cf.c
> @@ -39,6 +39,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
> +#include <trace/events/libata.h>
>  
>  #define DRIVER_NAME	"arasan_cf"
>  #define TIMEOUT		msecs_to_jiffies(3000)
> @@ -703,9 +704,11 @@ static unsigned int arasan_cf_qc_issue(struct ata_queued_cmd *qc)
>  	case ATA_PROT_DMA:
>  		WARN_ON_ONCE(qc->tf.flags & ATA_TFLAG_POLLING);
>  
> +		trace_ata_tf_load(ap, &qc->tf);
>  		ap->ops->sff_tf_load(ap, &qc->tf);
>  		acdev->dma_status = 0;
>  		acdev->qc = qc;
> +		trace_ata_bmdma_start(ap, &qc->tf, qc->tag)
>  		arasan_cf_dma_start(acdev);
>  		ap->hsm_task_state = HSM_ST_LAST;
>  		break;
> 

Re: [PATCH 21/42] sata_dwc_460ex: Use generic tracepoints and drop dev_dbg() statements

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Add generic tracepoints and drop the now obsolete logging statements.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Please update patch summary line for consistency with other patches
to be just "sata_dwc_460ex: use generic tracepoints" (moreover this
patch removes also dev_vdbg() statements).

With the patch summary line fixed:

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

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

> ---
>  drivers/ata/sata_dwc_460ex.c | 39 ++++-----------------------------------
>  1 file changed, 4 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index ef07d4a03627..a29cd81b7811 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -34,6 +34,7 @@
>  #include <linux/phy/phy.h>
>  #include <linux/libata.h>
>  #include <linux/slab.h>
> +#include <trace/events/libata.h>
>  
>  #include "libata.h"
>  
> @@ -311,21 +312,6 @@ static const char *get_dma_dir_descript(int dma_dir)
>  	}
>  }
>  
> -static void sata_dwc_tf_dump(struct ata_port *ap, struct ata_taskfile *tf)
> -{
> -	dev_vdbg(ap->dev,
> -		"taskfile cmd: 0x%02x protocol: %s flags: 0x%lx device: %x\n",
> -		tf->command, get_prot_descript(tf->protocol), tf->flags,
> -		tf->device);
> -	dev_vdbg(ap->dev,
> -		"feature: 0x%02x nsect: 0x%x lbal: 0x%x lbam: 0x%x lbah: 0x%x\n",
> -		tf->feature, tf->nsect, tf->lbal, tf->lbam, tf->lbah);
> -	dev_vdbg(ap->dev,
> -		"hob_feature: 0x%02x hob_nsect: 0x%x hob_lbal: 0x%x hob_lbam: 0x%x hob_lbah: 0x%x\n",
> -		tf->hob_feature, tf->hob_nsect, tf->hob_lbal, tf->hob_lbam,
> -		tf->hob_lbah);
> -}
> -
>  static void dma_dwc_xfer_done(void *hsdev_instance)
>  {
>  	unsigned long flags;
> @@ -548,6 +534,7 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
>  		 * active tag.  It is the tag that matches the command about to
>  		 * be completed.
>  		 */
> +		trace_ata_bmdma_start(ap, &qc->tf, tag);
>  		qc->ap->link.active_tag = tag;
>  		sata_dwc_bmdma_start_by_tag(qc, tag);
>  
> @@ -1021,12 +1008,6 @@ static void sata_dwc_bmdma_start_by_tag(struct ata_queued_cmd *qc, u8 tag)
>  		start_dma = 0;
>  	}
>  
> -	dev_dbg(ap->dev,
> -		"%s qc=%p tag: %x cmd: 0x%02x dma_dir: %s start_dma? %x\n",
> -		__func__, qc, tag, qc->tf.command,
> -		get_dma_dir_descript(qc->dma_dir), start_dma);
> -	sata_dwc_tf_dump(ap, &qc->tf);
> -
>  	if (start_dma) {
>  		sata_dwc_scr_read(&ap->link, SCR_ERROR, &reg);
>  		if (reg & SATA_DWC_SERROR_ERR_BITS) {
> @@ -1064,16 +1045,6 @@ static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
>  	struct ata_port *ap = qc->ap;
>  	struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
>  
> -#ifdef DEBUG_NCQ
> -	if (qc->hw_tag > 0 || ap->link.sactive > 1)
> -		dev_info(ap->dev,
> -			 "%s ap id=%d cmd(0x%02x)=%s qc tag=%d prot=%s ap active_tag=0x%08x ap sactive=0x%08x\n",
> -			 __func__, ap->print_id, qc->tf.command,
> -			 ata_get_cmd_descript(qc->tf.command),
> -			 qc->hw_tag, get_prot_descript(qc->tf.protocol),
> -			 ap->link.active_tag, ap->link.sactive);
> -#endif
> -
>  	if (!ata_is_ncq(qc->tf.protocol))
>  		tag = 0;
>  
> @@ -1090,11 +1061,9 @@ static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
>  		sactive |= (0x00000001 << tag);
>  		sata_dwc_scr_write(&ap->link, SCR_ACTIVE, sactive);
>  
> -		dev_dbg(qc->ap->dev,
> -			"%s: tag=%d ap->link.sactive = 0x%08x sactive=0x%08x\n",
> -			__func__, tag, qc->ap->link.sactive, sactive);
> -
> +		trace_ata_tf_load(ap, &qc->tf);
>  		ap->ops->sff_tf_load(ap, &qc->tf);
> +		trace_ata_exec_command(qp, &qc->tf, tag);
>  		sata_dwc_exec_command_by_tag(ap, &qc->tf, tag,
>  					     SATA_DWC_CMD_ISSUED_PEND);
>  	} else {
> 

Re: [PATCH 22/42] sata_nv: use generic tracepoints

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Remove logging messages covered by generic tracepoints and add
> generic tracepoints for internal calls.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/sata_nv.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index 8639f66706a3..f7dd32679c8e 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -31,6 +31,7 @@
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_device.h>
>  #include <linux/libata.h>
> +#include <trace/events/libata.h>
>  
>  #define DRV_NAME			"sata_nv"
>  #define DRV_VERSION			"3.5"
> @@ -1428,8 +1429,6 @@ 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);
> -
>  	return 0;
>  }
>  
> @@ -2007,19 +2006,17 @@ static unsigned int nv_swncq_issue_atacmd(struct ata_port *ap,
>  	if (qc == NULL)
>  		return 0;
>  
> -	DPRINTK("Enter\n");
> -
>  	writel((1 << qc->hw_tag), pp->sactive_block);
>  	pp->last_issue_tag = qc->hw_tag;
>  	pp->dhfis_bits &= ~(1 << qc->hw_tag);
>  	pp->dmafis_bits &= ~(1 << qc->hw_tag);
>  	pp->qc_active |= (0x1 << qc->hw_tag);
>  
> +	trace_ata_tf_load(ap, &qc->tf);
>  	ap->ops->sff_tf_load(ap, &qc->tf);	 /* load tf registers */
> +	trace_ata_exec_command(ap, &qc->tf, qc->hw_tag);
>  	ap->ops->sff_exec_command(ap, &qc->tf);
>  
> -	DPRINTK("Issued tag %u\n", qc->hw_tag);
> -
>  	return 0;
>  }
>  
> @@ -2031,8 +2028,6 @@ 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");
> -
>  	if (!pp->qc_active)
>  		nv_swncq_issue_atacmd(ap, qc);
>  	else
> 

Re: [PATCH 24/42] libata-sff: drop DPRINTK for the HSM state machine

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> The information is now logged with tracepoints, so the DPRINTK
> calls can be dropped.

The addition of trace_*() statements should be in patch #23 and
this patch should contain only the removal of no longer needed
DPRINTK() instances.

[ Just like it has been done in reset tracepoints conversion. ]

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

> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/ata/libata-sff.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 1078b621d47b..f6a54b574b25 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -660,7 +660,7 @@ 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");
> +	trace_ata_sff_pio_transfer_data(qc, offset, qc->sect_size);
>  
>  	/* do the actual data transfer */
>  	buf = kmap_atomic(page);
> @@ -723,7 +723,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");
> +	trace_atapi_send_cdb(qc, 0, qc->dev->cdb_len);
>  	WARN_ON_ONCE(qc->dev->cdb_len < 12);
>  
>  	ap->ops->sff_data_xfer(qc, qc->cdb, qc->dev->cdb_len, 1);
> @@ -794,7 +794,7 @@ 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");
> +	trace_atapi_pio_transfer_data(qc, offset, count);
>  
>  	/* do the actual data transfer */
>  	buf = kmap_atomic(page);
> @@ -976,8 +976,7 @@ 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);
> +	trace_ata_sff_hsm_state(qc, status);
>  
>  	switch (ap->hsm_task_state) {
>  	case HSM_ST_FIRST:
> @@ -1178,8 +1177,7 @@ 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);
> +		trace_ata_sff_hsm_command_complete(qc, status);
>  
>  		WARN_ON_ONCE(qc->err_mask & (AC_ERR_DEV | AC_ERR_HSM));
>  

Re: [PATCH 24/42] libata-sff: drop DPRINTK for the HSM state machine

[Bartlomiej Zolnierkiewicz]

On 3/2/20 6:57 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> On 2/13/20 10:53 AM, Hannes Reinecke wrote:
>> The information is now logged with tracepoints, so the DPRINTK
>> calls can be dropped.
> 
> The addition of trace_*() statements should be in patch #23 and
> this patch should contain only the removal of no longer needed
> DPRINTK() instances.
> 
> [ Just like it has been done in reset tracepoints conversion. ]

Alternatively:

(since here are only few DPRINTK() instances, unlike the reset
tracepoints conversion)

please merge this patch into patch #23 (just like it was in
the previous iteration, sorry for the confusion).

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

> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/ata/libata-sff.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
>> index 1078b621d47b..f6a54b574b25 100644
>> --- a/drivers/ata/libata-sff.c
>> +++ b/drivers/ata/libata-sff.c
>> @@ -660,7 +660,7 @@ 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");
>> +	trace_ata_sff_pio_transfer_data(qc, offset, qc->sect_size);
>>  
>>  	/* do the actual data transfer */
>>  	buf = kmap_atomic(page);
>> @@ -723,7 +723,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");
>> +	trace_atapi_send_cdb(qc, 0, qc->dev->cdb_len);
>>  	WARN_ON_ONCE(qc->dev->cdb_len < 12);
>>  
>>  	ap->ops->sff_data_xfer(qc, qc->cdb, qc->dev->cdb_len, 1);
>> @@ -794,7 +794,7 @@ 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");
>> +	trace_atapi_pio_transfer_data(qc, offset, count);
>>  
>>  	/* do the actual data transfer */
>>  	buf = kmap_atomic(page);
>> @@ -976,8 +976,7 @@ 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);
>> +	trace_ata_sff_hsm_state(qc, status);
>>  
>>  	switch (ap->hsm_task_state) {
>>  	case HSM_ST_FIRST:
>> @@ -1178,8 +1177,7 @@ 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);
>> +		trace_ata_sff_hsm_command_complete(qc, status);
>>  
>>  		WARN_ON_ONCE(qc->err_mask & (AC_ERR_DEV | AC_ERR_HSM));
>>  

Re: [PATCH 27/42] libata: add tracepoints for ATA error handling

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Add tracepoints for ATA error handling.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/libata-eh.c       | 10 +++++---
>  include/trace/events/libata.h | 60 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 67 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index ef3576eb5874..a068b5370aac 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -933,7 +933,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");
> +	trace_ata_std_sched_eh(ap);
>  }
>  EXPORT_SYMBOL_GPL(ata_std_sched_eh);
>  
> @@ -1060,7 +1060,7 @@ static void __ata_port_freeze(struct ata_port *ap)
>  
>  	ap->pflags |= ATA_PFLAG_FROZEN;
>  
> -	DPRINTK("ata%u port frozen\n", ap->print_id);
> +	trace_ata_port_freeze(ap);
>  }
>  
>  /**
> @@ -1208,7 +1208,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);
> +	trace_ata_port_thaw(ap);
>  }
>  
>  static void ata_eh_scsidone(struct scsi_cmnd *scmd)
> @@ -1347,6 +1347,8 @@ void ata_eh_about_to_do(struct ata_link *link, struct ata_device *dev,
>  	struct ata_eh_context *ehc = &link->eh_context;
>  	unsigned long flags;
>  
> +	trace_ata_eh_about_to_do(link, dev ? dev->devno : 0, action);
> +
>  	spin_lock_irqsave(ap->lock, flags);
>  
>  	ata_eh_clear_action(link, dev, ehi, action);
> @@ -1377,6 +1379,8 @@ void ata_eh_done(struct ata_link *link, struct ata_device *dev,
>  {
>  	struct ata_eh_context *ehc = &link->eh_context;
>  
> +	trace_ata_eh_done(link, dev ? dev->devno : 0, action);
> +
>  	ata_eh_clear_action(link, dev, &ehc->i, action);
>  }
>  
> diff --git a/include/trace/events/libata.h b/include/trace/events/libata.h
> index bebde8a72b9c..35e84ff735b0 100644
> --- a/include/trace/events/libata.h
> +++ b/include/trace/events/libata.h
> @@ -454,6 +454,37 @@ TRACE_EVENT(ata_eh_link_autopsy_qc,
>  		  __parse_eh_err_mask(__entry->eh_err_mask))
>  );
>  
> +DECLARE_EVENT_CLASS(ata_eh_action_template,
> +
> +	TP_PROTO(struct ata_link *link, unsigned int devno, unsigned int eh_action),
> +
> +	TP_ARGS(link, devno, eh_action),
> +
> +	TP_STRUCT__entry(
> +		__field( unsigned int,	ata_port )
> +		__field( unsigned int,	ata_dev	)
> +		__field( unsigned int,	eh_action )
> +	),
> +
> +	TP_fast_assign(
> +		__entry->ata_port	= link->ap->print_id;
> +		__entry->ata_dev	= link->pmp + devno;
> +		__entry->eh_action	= eh_action;
> +	),
> +
> +	TP_printk("ata_port=%u ata_dev=%u eh_action=%s",
> +		  __entry->ata_port, __entry->ata_dev,
> +		  __parse_eh_action(__entry->eh_action))
> +);
> +
> +DEFINE_EVENT(ata_eh_action_template, ata_eh_about_to_do,
> +	     TP_PROTO(struct ata_link *link, unsigned int devno, unsigned int eh_action),
> +	     TP_ARGS(link, devno, eh_action));
> +
> +DEFINE_EVENT(ata_eh_action_template, ata_eh_done,
> +	     TP_PROTO(struct ata_link *link, unsigned int devno, unsigned int eh_action),
> +	     TP_ARGS(link, devno, eh_action));
> +
>  DECLARE_EVENT_CLASS(ata_link_reset_begin_template,
>  
>  	TP_PROTO(struct ata_link *link, unsigned int *class, unsigned long deadline),
> @@ -534,6 +565,35 @@ DEFINE_EVENT(ata_link_reset_end_template, ata_slave_postreset,
>  	     TP_PROTO(struct ata_link *link, unsigned int *class, int rc),
>  	     TP_ARGS(link, class, rc));
>  
> +DECLARE_EVENT_CLASS(ata_port_eh_begin_template,
> +
> +	TP_PROTO(struct ata_port *ap),
> +
> +	TP_ARGS(ap),
> +
> +	TP_STRUCT__entry(
> +		__field( unsigned int,	ata_port )
> +	),
> +
> +	TP_fast_assign(
> +		__entry->ata_port	= ap->print_id;
> +	),
> +
> +	TP_printk("ata_port=%u", __entry->ata_port)
> +);
> +
> +DEFINE_EVENT(ata_port_eh_begin_template, ata_std_sched_eh,
> +	     TP_PROTO(struct ata_port *ap),
> +	     TP_ARGS(ap));
> +
> +DEFINE_EVENT(ata_port_eh_begin_template, ata_port_freeze,
> +	     TP_PROTO(struct ata_port *ap),
> +	     TP_ARGS(ap));
> +
> +DEFINE_EVENT(ata_port_eh_begin_template, ata_port_thaw,
> +	     TP_PROTO(struct ata_port *ap),
> +	     TP_ARGS(ap));
> +
>  DECLARE_EVENT_CLASS(ata_sff_hsm_template,
>  
>  	TP_PROTO(struct ata_queued_cmd *qc, unsigned char status),
> 

Re: [PATCH 25/42] libata-sff: add tracepoints for ata_sff_flush_pio_task()

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Replace DPRINTK calls with tracepoints.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/libata-sff.c      |  5 +----
>  include/trace/events/libata.h | 25 +++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index f6a54b574b25..ffe633f13f55 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -1234,7 +1234,7 @@ EXPORT_SYMBOL_GPL(ata_sff_queue_pio_task);
>  
>  void ata_sff_flush_pio_task(struct ata_port *ap)
>  {
> -	DPRINTK("ENTER\n");
> +	trace_ata_sff_flush_pio_task(ap);
>  
>  	cancel_delayed_work_sync(&ap->sff_pio_task);
>  
> @@ -1251,9 +1251,6 @@ void ata_sff_flush_pio_task(struct ata_port *ap)
>  	spin_unlock_irq(ap->lock);
>  
>  	ap->sff_pio_task_link = NULL;
> -
> -	if (ata_msg_ctl(ap))
> -		ata_port_dbg(ap, "EXIT\n");
>  }
>  
>  static void ata_sff_pio_task(struct work_struct *work)
> diff --git a/include/trace/events/libata.h b/include/trace/events/libata.h
> index a4200ef943f3..bebde8a72b9c 100644
> --- a/include/trace/events/libata.h
> +++ b/include/trace/events/libata.h
> @@ -617,6 +617,31 @@ DEFINE_EVENT(ata_transfer_data_template, atapi_send_cdb,
>  	     TP_PROTO(struct ata_queued_cmd *qc, unsigned int offset, unsigned int count),
>  	     TP_ARGS(qc, offset, count));
>  
> +DECLARE_EVENT_CLASS(ata_sff_template,
> +
> +	TP_PROTO(struct ata_port *ap),
> +
> +	TP_ARGS(ap),
> +
> +	TP_STRUCT__entry(
> +		__field( unsigned int,	ata_port )
> +		__field( unsigned char,	hsm_state )
> +	),
> +
> +	TP_fast_assign(
> +		__entry->ata_port	= ap->print_id;
> +		__entry->hsm_state	= ap->hsm_task_state;
> +	),
> +
> +	TP_printk("ata_port=%u task_state=%s",
> +		  __entry->ata_port,
> +		  show_sff_hsm_state_name(__entry->hsm_state))
> +);
> +
> +DEFINE_EVENT(ata_sff_template, ata_sff_flush_pio_task,
> +	     TP_PROTO(struct ata_port *ap),
> +	     TP_ARGS(ap));
> +
>  #endif /*  _TRACE_LIBATA_H */
>  
>  /* This part must be outside protection */
> 

Re: [PATCH 34/42] pata_pdc2027x: Replace PDPRINTK() with standard ata logging

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:54 AM, Hannes Reinecke wrote:
> Use standard ata logging macros instead of the hand-crafted
> PDPRINTK and remove duplicate logging messages.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/pata_pdc2027x.c | 81 ++++++++++++++++++++-------------------------
>  1 file changed, 36 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/ata/pata_pdc2027x.c b/drivers/ata/pata_pdc2027x.c
> index de834fbb6dfe..bf0ccc901179 100644
> --- a/drivers/ata/pata_pdc2027x.c
> +++ b/drivers/ata/pata_pdc2027x.c
> @@ -30,13 +30,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,
> @@ -214,11 +207,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);
>  
>  	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);
>  	return ATA_CBL_PATA40;
>  }
>  
> @@ -292,17 +287,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);
>  
>  	/* 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);
>  		return;
>  
>  	}
>  
>  	/* Set the PIO timing registers using value table for 133MHz */
> -	PDPRINTK("Set pio regs... \n");
> +	ata_port_dbg(ap, "Set pio regs...\n");
>  
>  	ctcr0 = ioread32(dev_mmio(ap, adev, PDC_CTCR0));
>  	ctcr0 &= 0xffff0000;
> @@ -315,9 +310,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);
>  }
>  
>  /**
> @@ -350,7 +343,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");
>  
>  		ctcr1 = ioread32(dev_mmio(ap, adev, PDC_CTCR1));
>  		ctcr1 &= 0xff000000;
> @@ -359,16 +352,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);
>  
>  	} 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");
>  		ctcr0 = ioread32(dev_mmio(ap, adev, PDC_CTCR0));
>  
>  		ctcr0 &= 0x0000ffff;
> @@ -376,11 +367,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);
>  	} 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);
>  	}
>  }
>  
> @@ -414,7 +404,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");
>  		} else {
>  			pdc2027x_set_dmamode(ap, dev);
>  		}
> @@ -485,8 +475,10 @@ 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, "%s: bccrh [%X] bccrl [%X]\n",
> +		__func__, bccrh,  bccrl);
> +	dev_dbg(host->dev, "%s: bccrhv[%X] bccrlv[%X]\n",
> +		__func__, bccrhv, bccrlv);
>  
>  	/*
>  	 * The 30-bit decreasing counter are read by 2 pieces.
> @@ -495,7 +487,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, "%s: rereading counter\n", __func__);
>  		goto retry;
>  	}
>  
> @@ -520,20 +512,21 @@ 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, "%s: pout_required is %ld\n",
> +		__func__, pout_required);
>  
>  	/* 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, "%s: pll_ctl[%X]\n", __func__, pll_ctl);
>  
>  	/*
>  	 * Calculate the ratio of F, R and OD
> @@ -552,7 +545,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;
>  	}
>  
> @@ -560,15 +553,16 @@ 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, "%s: F[%d] R[%d] ratio*1000[%ld]\n",
> +		__func__, F, R, ratio);
>  
>  	pll_ctl = (R << 8) | F;
>  
> -	PDPRINTK("Writing pll_ctl[%X]\n", pll_ctl);
> +	dev_dbg(host->dev, "%s: Writing pll_ctl[%X]\n", __func__, pll_ctl);
>  
>  	iowrite16(pll_ctl, mmio_base + PDC_PLL_CTL);
>  	ioread16(mmio_base + PDC_PLL_CTL); /* flush */
> @@ -576,15 +570,12 @@ 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, "%s: pll_ctl[%X]\n", __func__,
> +		ioread16(mmio_base + PDC_PLL_CTL));
>  
>  	return;
>  }
> @@ -605,7 +596,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, "%s: scr[%X]\n", __func__, scr);
>  	iowrite32(scr | (0x01 << 14), mmio_base + PDC_SYS_CTL);
>  	ioread32(mmio_base + PDC_SYS_CTL); /* flush */
>  
> @@ -622,7 +613,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, "%s: scr[%X]\n", __func__, scr);
>  	iowrite32(scr & ~(0x01 << 14), mmio_base + PDC_SYS_CTL);
>  	ioread32(mmio_base + PDC_SYS_CTL); /* flush */
>  
> @@ -632,8 +623,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, "%s: start[%ld] end[%ld] PLL input clock[%ld]HZ\n",
> +		__func__, start_count, end_count, pll_clock);
>  
>  	return pll_clock;
>  }
> 

Re: [PATCH 36/42] sata_fsl: move DPRINTK to ata debugging

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:54 AM, Hannes Reinecke wrote:
> Replace all DPRINTK calls with the standard logging functions.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/ata/sata_fsl.c | 90 ++++++++++++++++++++++++++------------------------
>  1 file changed, 46 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index 5d48e1d223fa..51627da79ffc 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -311,10 +311,12 @@ 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",
> -			intr_coalescing_count, intr_coalescing_ticks);
> -	DPRINTK("ICC register status: (hcr base: 0x%x) = 0x%x\n",
> -			hcr_base, ioread32(hcr_base + ICC));
> +	dev_dbg(host->dev,
> +		"%s: interrupt coalescing, count = 0x%x, ticks = %x\n",
> +		__func__, intr_coalescing_count, intr_coalescing_ticks);
> +	dev_dbg(host->dev,
> +		"%s: ICC register status: (hcr base: 0x%x) = 0x%x\n",
> +		__func__, hcr_base, ioread32(hcr_base + ICC));
>  }
>  
>  static ssize_t fsl_sata_intr_coalescing_show(struct device *dev,
> @@ -385,18 +387,19 @@ 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_port *ap,
> +					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_port_dbg(ap, "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_port_dbg(ap, "tag %d invalid : in use!!\n", tag);
>  		return 0;
>  	}
>  
> @@ -508,7 +511,7 @@ static enum ata_completion_errors 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(ap, qc->hw_tag, hcr_base);
>  	struct command_desc *cd;
>  	u32 desc_info = CMD_DESC_RES | CMD_DESC_SNOOP_ENABLE;
>  	u32 num_prde = 0;
> @@ -557,7 +560,7 @@ static unsigned int sata_fsl_qc_issue(struct ata_queued_cmd *qc)
>  	struct ata_port *ap = qc->ap;
>  	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(ap, qc->hw_tag, hcr_base);
>  
>  	VPRINTK("xx_qc_issue called,CQ=0x%x,CA=0x%x,CE=0x%x,CC=0x%x\n",
>  		ioread32(CQ + hcr_base),
> @@ -586,7 +589,7 @@ static bool sata_fsl_qc_fill_rtf(struct ata_queued_cmd *qc)
>  	struct sata_fsl_port_priv *pp = qc->ap->private_data;
>  	struct sata_fsl_host_priv *host_priv = qc->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->ap, qc->hw_tag, hcr_base);
>  	struct command_desc *cd;
>  
>  	cd = pp->cmdentry + tag;
> @@ -850,7 +853,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(ap, "hardreset, controller off-lined\n");
>  	VPRINTK("HStatus = 0x%x\n", ioread32(hcr_base + HSTATUS));
>  	VPRINTK("HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
>  
> @@ -880,7 +883,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, "controller off-lined & on-lined\n");
>  	VPRINTK("HStatus = 0x%x\n", ioread32(hcr_base + HSTATUS));
>  	VPRINTK("HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
>  
> @@ -962,7 +965,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]);
>  
>  	/*
> @@ -970,7 +973,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, "CQ = 0x%x, CA = 0x%x, CC = 0x%x\n",
>  		ioread32(CQ + hcr_base),
>  		ioread32(CA + hcr_base), ioread32(CC + hcr_base));
>  
> @@ -983,15 +986,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 HControl = 0x%x Serror = 0x%x\n",
> +			     ioread32(hcr_base + HSTATUS),
> +			     ioread32(hcr_base + HCONTROL),
> +			     Serror);
>  		goto err;
>  	}
>  
> @@ -1049,7 +1054,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, "ENTER\n");
>  	sata_pmp_error_handler(ap);
>  
>  }
> @@ -1092,7 +1097,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, "hStat=0x%x,CE=0x%x,DE =0x%x,SErr=0x%x\n",
>  		hstatus, cereg, ioread32(hcr_base + DE), SError);
>  
>  	/* handle fatal errors */
> @@ -1109,7 +1114,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, "PHYRDY change indication\n");
>  
>  		/* Setup a soft-reset EH action */
>  		ata_ehi_hotplugged(ehi);
> @@ -1130,7 +1135,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 */
> @@ -1235,12 +1240,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;
>  	}
> @@ -1258,17 +1263,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",
> -			done_mask, ioread32(hcr_base + CA),
> -			ioread32(hcr_base + CE));
> +		ata_port_dbg(ap, "Status of all queues: 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",
> -				     i, ioread32(hcr_base + CC),
> -				     ioread32(hcr_base + CA));
> +				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));
>  		}
>  		ata_qc_complete_multiple(ap, ata_qc_get_active(ap) ^ done_mask);
>  		return;
> @@ -1277,16 +1280,16 @@ 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",
> -			 ioread32(hcr_base + CC));
> +		ata_port_dbg(ap, "completing non-ncq cmd, CC=0x%x\n",
> +			     ioread32(hcr_base + CC));
>  
>  		if (qc) {
>  			ata_qc_complete(qc);
>  		}
>  	} else {
>  		/* Spurious Interrupt!! */
> -		DPRINTK("spurious interrupt!!, CC = 0x%x\n",
> -			ioread32(hcr_base + CC));
> +		ata_port_dbg(ap, "spurious interrupt!!, CC = 0x%x\n",
> +			     ioread32(hcr_base + CC));
>  		iowrite32(done_mask, hcr_base + CC);
>  		return;
>  	}
> @@ -1305,7 +1308,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);
> +	ata_port_dbg(ap, "interrupt status 0x%x\n", interrupt_enables);
>  
>  	if (!interrupt_enables)
>  		return IRQ_NONE;
> @@ -1359,14 +1362,14 @@ 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));
> +	ata_port_dbg(ap, "icc = 0x%x\n", ioread32(hcr_base + ICC));
>  	iowrite32(0x01000000, hcr_base + ICC);
>  
>  	/* clear error registers, SError is cleared by libATA  */
>  	iowrite32(0x00000FFFF, hcr_base + CE);
>  	iowrite32(0x00000FFFF, hcr_base + DE);
>  
> - 	/*
> +x	/*

This needs to be removed.

Otherwise the patch looks fine:

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

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

>  	 * reset the number of command complete bits which will cause the
>  	 * interrupt to be signaled
>  	 */
> @@ -1378,8 +1381,9 @@ 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));
> +	ata_port_dbg(ap, "HStatus = 0x%x HControl = 0x%x\n",
> +		     ioread32(hcr_base + HSTATUS),
> +		     ioread32(hcr_base + HCONTROL));
>  
>  	return 0;
>  }
> @@ -1458,9 +1462,7 @@ 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);
> +	ata_port_dbg(ap, "@reset i/o = 0x%x\n", ioread32(csr_base + TRANSCFG));
>  
>  	host_priv = kzalloc(sizeof(struct sata_fsl_host_priv), GFP_KERNEL);
>  	if (!host_priv)
> 

Re: [PATCH 11/42] libata: sanitize ATA_HORKAGE_DUMP_ID

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> As ata_dev_dbg() is now using dynamic debugging ATA_HORKAGE_DUMP_ID
> will now print out the raw IDENTIFY data without a header unless
> explicitly enable via dyndebug.
> So move the logging level up to INFO and have the header printed
> always.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/libata-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 1987886140b6..40e327e49905 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1944,10 +1944,10 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>  	}
>  
>  	if (dev->horkage & ATA_HORKAGE_DUMP_ID) {
> -		ata_dev_dbg(dev, "dumping IDENTIFY data, "
> +		ata_dev_info(dev, "dumping IDENTIFY data, "
>  			    "class=%d may_fallback=%d tried_spinup=%d\n",
>  			    class, may_fallback, tried_spinup);
> -		print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET,
> +		print_hex_dump(KERN_INFO, "", DUMP_PREFIX_OFFSET,
>  			       16, 2, id, ATA_ID_WORDS * sizeof(*id), true);
>  	}
>  
> 

Re: [PATCH 13/42] sata_mv: kill 'port' argument in mv_dump_all_regs()

[Bartlomiej Zolnierkiewicz]

On 2/13/20 10:53 AM, Hannes Reinecke wrote:
> Always '-1', so drop it and simplify the function.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

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

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

> ---
>  drivers/ata/sata_mv.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 1eb93976af8d..940b7f2e9105 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -1283,23 +1283,13 @@ static void mv_dump_pci_cfg(struct pci_dev *pdev, unsigned bytes)
>  	}
>  }
>  
> -static void mv_dump_all_regs(void __iomem *mmio_base, int port,
> -			     struct pci_dev *pdev)
> +static void mv_dump_all_regs(void __iomem *mmio_base, struct pci_dev *pdev)
>  {
> -	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 (0 > port) {
> -		start_hc = start_port = 0;
> -		num_ports = 8;		/* shld be benign for 4 port devs */
> -		num_hcs = 2;
> -	} else {
> -		start_hc = port >> MV_PORT_HC_SHIFT;
> -		start_port = port;
> -		num_ports = num_hcs = 1;
> -	}
> +	start_hc = start_port = 0;
> +	num_ports = 8;		/* shld be benign for 4 port devs */
> +	num_hcs = 2;
>  	dev_printk(KERN_DEBUG, &pdev->dev,
>  		   "%s: All registers for port(s) %u-%u:\n", __func__,
>  		   start_port, num_ports > 1 ? num_ports - 1 : start_port);
> @@ -1314,13 +1304,13 @@ static void mv_dump_all_regs(void __iomem *mmio_base, int port,
>  	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);
> +		void __iomem *hc_base = mv_hc_base(mmio_base, hc);
>  		dev_printk(KERN_DEBUG, &pdev->dev, "%s: HC regs (HC %i):\n",
>  			   __func__, hc);
>  		mv_dump_mem(&pdev->dev, hc_base, 0x1c);
>  	}
>  	for (p = start_port; p < start_port + num_ports; p++) {
> -		port_base = mv_port_base(mmio_base, p);
> +		void __iomem *port_base = mv_port_base(mmio_base, p);
>  		dev_printk(KERN_DEBUG, &pdev->dev, "%s: EDMA regs (port %i):\n",
>  			   __func__, p);
>  		mv_dump_mem(&pdev->dev, port_base, 0x54);
> @@ -2969,7 +2959,7 @@ static int mv_pci_error(struct ata_host *host, void __iomem *mmio)
>  	if (pci_dump) {
>  		dev_printk(KERN_DEBUG, host->dev, "%s: All regs @ PCI error\n",
>  			   __func__);
> -		mv_dump_all_regs(mmio, -1, to_pci_dev(host->dev));
> +		mv_dump_all_regs(mmio, to_pci_dev(host->dev));
>  	}
>  	writelfl(0, mmio + hpriv->irq_cause_offset);
>  
> 

Re: [PATCH 17/42] libata: tracepoints for bus-master DMA

[Hannes Reinecke]

On 3/2/20 6:24 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> On 2/13/20 10:53 AM, Hannes Reinecke wrote:
>> Add tracepoints for bus-master DMA and taskfile related functions.
>> That allows us to drop the relevant DPRINTK() calls.
> 
> ..DPRINTK() & VPRINTK() calls.
> 
> Also please see comments below (from v1 review).
> 
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/ata/libata-core.c     |  5 +++
>>  drivers/ata/libata-sff.c      | 23 +++++++---
>>  include/trace/events/libata.h | 97 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 120 insertions(+), 5 deletions(-)
>>
[ .. ]
>> +	TP_STRUCT__entry(
>> +		__field( unsigned int,	ata_port )
>> +		__field( unsigned char,	cmd	)
>> +		__field( unsigned char,	dev	)
>> +		__field( unsigned char,	lbal	)
>> +		__field( unsigned char,	lbam	)
>> +		__field( unsigned char,	lbah	)
>> +		__field( unsigned char,	nsect	)
>> +		__field( unsigned char,	feature	)
>> +		__field( unsigned char,	hob_lbal )
>> +		__field( unsigned char,	hob_lbam )
>> +		__field( unsigned char,	hob_lbah )
>> +		__field( unsigned char,	hob_nsect )
>> +		__field( unsigned char,	hob_feature )
>> +		__field( unsigned char,	proto	)
>> +		__field( unsigned long,	flags	)
>> +	),
> 
> 'flags' field doesn't seem to be used?
> 
Indeed.

[ .. ]
>> +	TP_printk("ata_port=%u cmd=%s%s tag=%d",
>> +		  __entry->ata_port,
>> +		  show_protocol_name(__entry->proto),
>> +		  show_opcode_name(__entry->cmd),
>> +		  __entry->tag)
> 
> Please keep both new events consistent regarding 'proto' and
> 'cmd' fields printing (add "proto=%s" in this event or rework
> printing in ata_tf_load one).
> right, will be doing so in the next round.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
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