Re: [PATCH 1/9] libata: change debugging macros/adjust dbg levels

[Tejun Heo <htejun@gmail.com>]

Hello,

Generally looks good to me.  Just a few comments.

Borislav Petkov wrote:
> change debugging macros ata_(port|dev)_printk and adjust debugging levels
> 
> Signed-off-by: <petkov@math.uni-muenster.de>
> 
> 
> --- libata-dev/include/linux/libata.h	2006-06-29 17:17:50.000000000 +0200
> +++ libata-dev/include/linux/libata.h.new	2006-06-28 17:23:30.000000000 +0200
> @@ -67,24 +67,26 @@
>  #define HAVE_LIBATA_MSG 1
>  
>  enum {
> -	ATA_MSG_DRV	= 0x0001,
> -	ATA_MSG_INFO	= 0x0002,
> -	ATA_MSG_PROBE	= 0x0004,
> -	ATA_MSG_WARN	= 0x0008,
> -	ATA_MSG_MALLOC	= 0x0010,
> -	ATA_MSG_CTL	= 0x0020,
> -	ATA_MSG_INTR	= 0x0040,
> -	ATA_MSG_ERR	= 0x0080,
> -};
> -
> -#define ata_msg_drv(p)    ((p)->msg_enable & ATA_MSG_DRV)
> -#define ata_msg_info(p)   ((p)->msg_enable & ATA_MSG_INFO)
> -#define ata_msg_probe(p)  ((p)->msg_enable & ATA_MSG_PROBE)
> -#define ata_msg_warn(p)   ((p)->msg_enable & ATA_MSG_WARN)
> -#define ata_msg_malloc(p) ((p)->msg_enable & ATA_MSG_MALLOC)
> -#define ata_msg_ctl(p)    ((p)->msg_enable & ATA_MSG_CTL)
> -#define ata_msg_intr(p)   ((p)->msg_enable & ATA_MSG_INTR)
> -#define ata_msg_err(p)    ((p)->msg_enable & ATA_MSG_ERR)
> +	ATA_MSG_ERR	= 0x0001,
> +	ATA_MSG_WARN	= 0x0002,
> +	ATA_MSG_DRV	= 0x0004,

Comment for ATA_MSG_DRV please.  Also, I'm not sure we need separate DRV 
from INFO.

> +	ATA_MSG_INFO	= 0x0008, /* revalidation messages, EH progress */
> +	ATA_MSG_VDEBUG	= 0x0010, /* verbose hot path */
> +	ATA_MSG_CMD	= 0x0020, /* issue / completion */
> +	ATA_MSG_SG	= 0x0040, /* SG map/unmap handling */
> +	ATA_MSG_TRACE	= 0x0080, /* function tracing, e.g. enter/exit */
> +};
> +
> +const char *__ata_msg_lvs[] = {
> +	[ATA_MSG_ERR]	 = KERN_ERR,
> +	[ATA_MSG_WARN]	 = KERN_WARNING,
> +	[ATA_MSG_DRV]	 = KERN_INFO,
> +	[ATA_MSG_INFO]	 = KERN_INFO,
> +	[ATA_MSG_VDEBUG] = KERN_DEBUG,
> +	[ATA_MSG_CMD]	 = KERN_DEBUG,
> +	[ATA_MSG_SG]	 = KERN_DEBUG,
> +	[ATA_MSG_TRACE]	 = KERN_DEBUG
> +};

Please declare the array here and define it in libata-core.c.

>  static inline u32 ata_msg_init(int dval, int default_msg_enable_bits)
>  {
> @@ -805,11 +807,16 @@ extern void ata_do_eh(struct ata_port *a
>  /*
>   * printk helpers
>   */
> -#define ata_port_printk(ap, lv, fmt, args...) \
> -	printk(lv"ata%u: "fmt, (ap)->id , ##args)
> +#define ata_port_printk(ap, lv, fmt, args...) do { \
> +	if (unlikely((ap)->msg_enable & (0xFF & (lv)))) \
                                          ^^^^why?

> +		printk("%sata%u: "fmt, __ata_msg_lvs[lv], (ap)->id , ##args); \
> +} while (0)
>  
> -#define ata_dev_printk(dev, lv, fmt, args...) \
> -	printk(lv"ata%u.%02u: "fmt, (dev)->ap->id, (dev)->devno , ##args)
> +#define ata_dev_printk(dev, lv, fmt, args...) do { \
> +	if (unlikely((dev->ap)->msg_enable & (0xFF & (lv)))) \
                                               ^^^^ditto

> +		printk("%sata%u.%02u: "fmt, __ata_msg_lvs[lv], \
> +				(dev)->ap->id, (dev)->devno , ##args); \
> +} while (0)
>  
>  /*
>   * ata_eh_info helpers
> @@ -997,9 +1004,8 @@ static inline u8 ata_wait_idle(struct at
>  
>  	if (status & (ATA_BUSY | ATA_DRQ)) {
>  		unsigned long l = ap->ioaddr.status_addr;
> -		if (ata_msg_warn(ap))
> -			printk(KERN_WARNING "ATA: abnormal status 0x%X on port 0x%lX\n",
> -				status, l);
> +		ata_port_printk(ap, ATA_MSG_WARN, 
> +			"ATA: abnormal status 0x%X on port 0x%lX\n", status, l);
                          ^^^^^ you can drop the header.

-- 
tejun