Re: [PATCH] libata-core.c: restore configuration boot messages in ata_dev_configure()

[Tejun Heo <htejun@gmail.com>]

Borislav Petkov wrote:
> On Tue, Jun 27, 2006 at 09:25:26PM +0900, Tejun Heo wrote:
> 
> Hi, 
>> IMHO, this should be done in the following steps.
>>
>> * define ATA_MSG_* and ata_msg_* macros which map 1:1 to the current 
>> message levels.
> 
> How about we pin down these final dbg levels to the following (both proposals 
> merged):
> 
> ATA_MSG_ERR
> ATA_MSG_WARNING
> ATA_MSG_DRV ("standard" driver info, initial cfg messages)
> 
> ATA_MSG_INFO maybe) /* revalidation messages, EH progress, more verbose msgs,
>                        feats */
> ATA_MSG_VDEBUG  /* verbose hot path */, by the way, which are those hotpaths?

I don't know but left VDEBUG()s after CMD/SG/TRACE conversion should be 
a good start.

> ATA_MSG_CMD     /* issue / completion */
> ATA_MSG_SG      /* SG map/unmap handling */
> ATA_MGS_TRACE   /* function enter/exit */

Looks okay to me.

>> * make ata_*_prink()s use ATA_MSG_* instead of KERN_* and embed 
>> ata_msg_enable() into ata_*_printk()s.  Convert ALL ata_*_printk()s and 
>> convertible DEBUG/VDEBUG()s in single sweep - it doesn't have to be a 
>> single patch but post them together.  These conversions touch a lot of 
>> places and other patches have to be regenerated afterward.
> 
> then do something like
> 
> #define ata_(ap|dev)_printk((ap|dev), lv, fmt, args...) \
>     if (ata_msg_err(ap)) \
>         printk(KERN_ERR"ata%u: "fmt, ...); \
>     else if (ata_msg_warn(ap)) \
>         printk(KERN_WARNING"ata%u: "fmt, ...); \
>         .

Actually, how about...

enum {
	ATA_MSG_ERR,
	ATA_MSG_WARNING,
	...
};

const char *__ata_msg_lvs[] = {
	[ATA_MSG_ERR]		= KERN_ERR,
	[ATA_MSG_WARNING]	= KERN_WARNING,
	...
};

#define ata_port_printk(ap, lv, fmt, args...) do { \
	if (unlikely((ap)->msg_enable & (1 << (lv))))
		printk(__ata_msg_lvs[lv]"ata%u: "fmt, (ap)->id , #args);
} while (0)

Note that regardless of message level, the msg_enable is unlikely to hit 
in hot patch, so the 'unlikely()' hint.

> and then call them like so:
> 
> ata_dev_printk(dev, ATA_MSG_ERR, "Error!%d", i);
> 
> and so on. This looks pretty compact to me, no?

Yeap, the call looks good.

Thanks.

-- 
tejun