[PATCH 5/5] sil24: make error_intr less verbose

[PATCH 5/5] sil24: make error_intr less verbose

[Tejun Heo]

sil24_error_intr is too verbose.  Shut it up.

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/sata_sil24.c
===================================================================
--- work.orig/drivers/scsi/sata_sil24.c	2005-11-16 17:05:19.000000000 +0900
+++ work/drivers/scsi/sata_sil24.c	2005-11-16 17:08:05.000000000 +0900
@@ -678,9 +678,9 @@ static void sil24_error_intr(struct ata_
 	if (serror)
 		writel(serror, port + PORT_SERROR);
 
-	printk(KERN_ERR DRV_NAME " ata%u: error interrupt on port%d\n"
-	       "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
-	       ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus, serror);
+	DPRINTK("ata%u: error interrupt on port%d\n"
+		"  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
+		ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus, serror);
 
 	if (cmd_err == PORT_CERR_DEV || cmd_err == PORT_CERR_SDB) {
 		/*

Re: [PATCH 5/5] sil24: make error_intr less verbose

[Jeff Garzik]

Tejun Heo wrote:
> sil24_error_intr is too verbose.  Shut it up.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> Index: work/drivers/scsi/sata_sil24.c
> ===================================================================
> --- work.orig/drivers/scsi/sata_sil24.c	2005-11-16 17:05:19.000000000 +0900
> +++ work/drivers/scsi/sata_sil24.c	2005-11-16 17:08:05.000000000 +0900
> @@ -678,9 +678,9 @@ static void sil24_error_intr(struct ata_
>  	if (serror)
>  		writel(serror, port + PORT_SERROR);
>  
> -	printk(KERN_ERR DRV_NAME " ata%u: error interrupt on port%d\n"
> -	       "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
> -	       ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus, serror);
> +	DPRINTK("ata%u: error interrupt on port%d\n"
> +		"  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
> +		ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus, serror);

Mild NAK.

I am grappling with this on AHCI too :)  This is because ATA_ERR is much 
more common on ATAPI, yes?

My preferred change would be

	if ((class != ATA_DEV_ATAPI) ||
	    (sil24_cmd_err > PORT_CERR_SDB))
		printk()

so that truly uncommon errors are always printed, but users logs are not 
spammed.

Another option is to do

	if (ata_ratelimit())
		printk()

Regards,

	Jeff

Re: [PATCH 5/5] sil24: make error_intr less verbose

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> sil24_error_intr is too verbose.  Shut it up.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> Index: work/drivers/scsi/sata_sil24.c
>> ===================================================================
>> --- work.orig/drivers/scsi/sata_sil24.c    2005-11-16 
>> 17:05:19.000000000 +0900
>> +++ work/drivers/scsi/sata_sil24.c    2005-11-16 17:08:05.000000000 +0900
>> @@ -678,9 +678,9 @@ static void sil24_error_intr(struct ata_
>>      if (serror)
>>          writel(serror, port + PORT_SERROR);
>>  
>> -    printk(KERN_ERR DRV_NAME " ata%u: error interrupt on port%d\n"
>> -           "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
>> -           ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, 
>> sstatus, serror);
>> +    DPRINTK("ata%u: error interrupt on port%d\n"
>> +        "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
>> +        ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus, 
>> serror);
> 
> 
> Mild NAK.
> 
> I am grappling with this on AHCI too :)  This is because ATA_ERR is much 
> more common on ATAPI, yes?

Yeap.

> 
> My preferred change would be
> 
>     if ((class != ATA_DEV_ATAPI) ||
>         (sil24_cmd_err > PORT_CERR_SDB))
>         printk()
> 
> so that truly uncommon errors are always printed, but users logs are not 
> spammed.
> 
> Another option is to do
> 
>     if (ata_ratelimit())
>         printk()
> 

I'll go for the first one.  ATAPI devices can spit a lot of harmless 
errors and I think it's best if users don't see any of those.

-- 
tejun

Re: [PATCH 5/5] sil24: make error_intr less verbose

[Edward Falk]

>> -    printk(KERN_ERR DRV_NAME " ata%u: error interrupt on port%d\n"
>> -           "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
>> -           ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, 
>> sstatus, serror);
>> +    DPRINTK("ata%u: error interrupt on port%d\n"
>> +        "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
>> +        ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus, 
>> serror);
> 
> 
> Mild NAK.

Agreed.  Bear in mind that it's an extrordinarily exceptional 
circumstance for the driver to reach this point.  If this happens, then 
something has truly gone wrong and the message should be logged.

Note that this point in the driver is *not* reached for an ordinary ATA 
errors, but only for the kind of errors that libata can't handle.

	-ed falk

Re: [PATCH 5/5] sil24: make error_intr less verbose

[Albert Lee]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> sil24_error_intr is too verbose.  Shut it up.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> Index: work/drivers/scsi/sata_sil24.c
>> ===================================================================
>> --- work.orig/drivers/scsi/sata_sil24.c    2005-11-16
>> 17:05:19.000000000 +0900
>> +++ work/drivers/scsi/sata_sil24.c    2005-11-16 17:08:05.000000000 +0900
>> @@ -678,9 +678,9 @@ static void sil24_error_intr(struct ata_
>>      if (serror)
>>          writel(serror, port + PORT_SERROR);
>>  
>> -    printk(KERN_ERR DRV_NAME " ata%u: error interrupt on port%d\n"
>> -           "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
>> -           ap->id, ap->port_no, slot_stat, irq_stat, cmd_err,
>> sstatus, serror);
>> +    DPRINTK("ata%u: error interrupt on port%d\n"
>> +        "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
>> +        ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus,
>> serror);
> 
> 
> Mild NAK.
> 
> I am grappling with this on AHCI too :)  This is because ATA_ERR is much
> more common on ATAPI, yes?
> 
> My preferred change would be
> 
>     if ((class != ATA_DEV_ATAPI) ||
>         (sil24_cmd_err > PORT_CERR_SDB))
>         printk()
> 
> so that truly uncommon errors are always printed, but users logs are not
> spammed.
> 
> Another option is to do
> 
>     if (ata_ratelimit())
>         printk()
> 

The "PIO error" messages from ata_pio_error() are also noisy for ATAPI devices.

Ex.
scsi278 : pata_pdc2027x
  Vendor: ASUS      Model: CRW-5232AS        Rev: 1.02
  Type:   CD-ROM                             ANSI SCSI revision: 05
Attached scsi generic sg3 at scsi277, channel 0, id 0, lun 0,  type 5
ata1: PIO error, drv_stat 0x51  <==
  Vendor: BENQ      Model: DVD DD DW1620     Rev: B7T9
  Type:   CD-ROM                             ANSI SCSI revision: 05
ata1: PIO error, drv_stat 0x51  <==
ata1: PIO error, drv_stat 0x51  <==
sr0: scsi3-mmc drive: 40x/40x writer cd/rw xa/form2 cdda tray
Attached scsi CD-ROM sr0 at scsi277, channel 0, id 0, lun 0
ata1: PIO error, drv_stat 0x51  <==
ata1: PIO error, drv_stat 0x51  <==
sr1: scsi3-mmc drive: 40x/40x writer cd/rw xa/form2 cdda tray
Attached scsi CD-ROM sr1 at scsi277, channel 0, id 1, lun 0
Attached scsi generic sg4 at scsi277, channel 0, id 1, lun 0,  type 5
ata1: PIO error, drv_stat 0x51  <==
ata1: PIO error, drv_stat 0x51  <==

Seven "PIO error" messages were logged during module loading simply
because the hot-plug scripts are reading mode page unsupported by the ATAPI devices.
These are not errors that needs much attention.

Could we print out the error only for the non-packet commands?
(as in the attached patch)

Another idea, maybe we should not print out the error message in ata_pio_error().
ata_pio_error() doesn't have enough information to tell trivial errors from severe errors.
How about moving the prink() to the complete functions or EH?

Albert

==========

--- linux/drivers/scsi/libata-core.c	2005-11-17 10:08:02.000000000 +0800
+++ pio_err_msg/drivers/scsi/libata-core.c	2005-11-17 10:26:18.000000000 +0800
@@ -3212,11 +3212,12 @@ static void ata_pio_error(struct ata_por
 {
 	struct ata_queued_cmd *qc;
 
-	printk(KERN_WARNING "ata%u: PIO error\n", ap->id);
-
 	qc = ata_qc_from_tag(ap, ap->active_tag);
 	assert(qc != NULL);
 
+	if (qc->tf.command != ATA_CMD_PACKET)
+		printk(KERN_WARNING "ata%u: PIO error\n", ap->id);
+
 	ap->hsm_task_state = HSM_ST_IDLE;
 
 	ata_poll_qc_complete(qc, AC_ERR_ATA_BUS);