[PATCH]: libata-scsi: Don't start hotplug work queue if hotplug is disabled

[PATCH]: libata-scsi: Don't start hotplug work queue if hotplug is disabled

[Chunbo Luo]

Hi

When ata_scsi_scan_host() scan device failed, it will start a work queue
unconditionally. This may cause some noisy messages. This patch fix this
problem.


---
commit 6c686a6814a805782c7ab48ad89352ee309e8c32
Author: Chunbo Luo <chunbo.luo@windriver.com>
Date:   Fri Jun 27 09:56:52 2008 +0800

    libata-scsi: Don't start hotplug work queue if hotplug is disabled
    
    Previously, queue_delayed_work() was started unconditionally, but if
    ATA_PFLAG_SCSI_HOTPLUG is not set and we do this, the work queue may
    cause multiple messages like this:
    
    ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
    
    So now,it is only started when the hotplug flag is actually set.
    
    Signed-off-by: Chunbo Luo <chunbo.luo@windriver.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 57a4364..7ed5bb3 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3108,8 +3108,9 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
                                "                  switching to async\n");
        }
 
-       queue_delayed_work(ata_aux_wq, &ap->hotplug_task,
-                          round_jiffies_relative(HZ));
+       if (ap->pflags & ATA_PFLAG_SCSI_HOTPLUG)
+               queue_delayed_work(ata_aux_wq, &ap->hotplug_task,
+                                  round_jiffies_relative(HZ));
 }
 
 /**
---


Best Regards
Chunbo

Re: [PATCH]: libata-scsi: Don't start hotplug work queue if hotplug is disabled

[Tejun Heo]

Chunbo Luo wrote:
> Hi
> 
> When ata_scsi_scan_host() scan device failed, it will start a work queue
> unconditionally. This may cause some noisy messages. This patch fix this
> problem.

ATA_PFLAG_SCSI_HOTPLUG doesn't mean that hotplug is enabled.  It
indicates that EH needs to schedule a kick at SCSI's ass and make it
scan for discovered devices && work running on ata_aux_wq testing it
doesn't mean anything.  There's no synchronization.

>     Previously, queue_delayed_work() was started unconditionally, but if
>     ATA_PFLAG_SCSI_HOTPLUG is not set and we do this, the work queue may
>     cause multiple messages like this:
>     
>     ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.

And this message has nothing to do with SCSI.  It's printed by libata
proper for several drivers which don't support ATAPI.  Did the patch
really kill the warning messages?  Can you please post full boot log
from 2.6.26 w/o the patch?

Andrew, please drop the patch from -mm.  Thanks.

-- 
tejun

Re: [PATCH]: libata-scsi: Don't start hotplug work queue if hotplug is disabled

[Chunbo Luo]

On Fri, 2008-07-18 at 22:33 +0900, Tejun Heo wrote:
> Chunbo Luo wrote:
> > Hi
> > 
> > When ata_scsi_scan_host() scan device failed, it will start a work queue
> > unconditionally. This may cause some noisy messages. This patch fix this
> > problem.
> 
> ATA_PFLAG_SCSI_HOTPLUG doesn't mean that hotplug is enabled.  It
> indicates that EH needs to schedule a kick at SCSI's ass and make it
> scan for discovered devices && work running on ata_aux_wq testing it
> doesn't mean anything.  There's no synchronization.
> 
> >     Previously, queue_delayed_work() was started unconditionally, but if
> >     ATA_PFLAG_SCSI_HOTPLUG is not set and we do this, the work queue may
> >     cause multiple messages like this:
> >     
> >     ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
> 
> And this message has nothing to do with SCSI.  It's printed by libata
> proper for several drivers which don't support ATAPI.  Did the patch
> really kill the warning messages?  Can you please post full boot log
> from 2.6.26 w/o the patch?

If the drivers don't support ATAPI, the hotplug task cannot find linked device,
and it will start the hotplug task again. Which will print messages continously 
like this.

----------
ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
...
------------

We should find a way to avoid this kind of noisy warning messages.

This patch cannot kill the warning messages, but it can avoid the
duplicate warning messages.




> 
> Andrew, please drop the patch from -mm.  Thanks.
> 

Re: [PATCH]: libata-scsi: Don't start hotplug work queue if hotplug is disabled

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

Chunbo Luo wrote:
>> And this message has nothing to do with SCSI.  It's printed by libata
>> proper for several drivers which don't support ATAPI.  Did the patch
>> really kill the warning messages?  Can you please post full boot log
>> from 2.6.26 w/o the patch?
> 
> If the drivers don't support ATAPI, the hotplug task cannot find linked device,
> and it will start the hotplug task again. Which will print messages continously 
> like this.
> 
> ----------
> ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
> ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
> ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
> ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
> ...
> ------------
> 
> We should find a way to avoid this kind of noisy warning messages.
> 
> This patch cannot kill the warning messages, but it can avoid the
> duplicate warning messages.

Hmmm... yeah, indeed.  That check is located at rather strange place.
Can you please test the attached patch?

Thanks.

-- 
tejun

[-- Attachment #2: update-atapi-disable.patch --]
[-- Type: text/x-patch, Size: 3283 bytes --]

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9bef1a8..9cd04f6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -120,7 +120,7 @@ static char ata_force_param_buf[PAGE_SIZE] __initdata;
 module_param_string(force, ata_force_param_buf, sizeof(ata_force_param_buf), 0);
 MODULE_PARM_DESC(force, "Force ATA configurations including cable type, link speed and transfer mode (see Documentation/kernel-parameters.txt for details)");
 
-int atapi_enabled = 1;
+static int atapi_enabled = 1;
 module_param(atapi_enabled, int, 0444);
 MODULE_PARM_DESC(atapi_enabled, "Enable discovery of ATAPI devices (0=off, 1=on)");
 
@@ -2142,6 +2142,16 @@ int ata_dev_configure(struct ata_device *dev)
 		return 0;
 	}
 
+	if ((!atapi_enabled || (ap->flags & ATA_FLAG_NO_ATAPI)) &&
+	    dev->class == ATA_DEV_ATAPI) {
+		ata_dev_printk(dev, KERN_WARNING,
+			"WARNING: ATAPI is %s, device ignored.\n",
+			atapi_enabled ? "not supported with this driver"
+				      : "disabled");
+		ata_dev_disable(dev);
+		return 0;
+	}
+
 	/* let ACPI work its magic */
 	rc = ata_acpi_on_devcfg(dev);
 	if (rc)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 479c29e..4627774 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2550,36 +2550,6 @@ static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
 }
 
 /**
- *	ata_scsi_dev_enabled - determine if device is enabled
- *	@dev: ATA device
- *
- *	Determine if commands should be sent to the specified device.
- *
- *	LOCKING:
- *	spin_lock_irqsave(host lock)
- *
- *	RETURNS:
- *	0 if commands are not allowed / 1 if commands are allowed
- */
-
-static int ata_scsi_dev_enabled(struct ata_device *dev)
-{
-	if (unlikely(!ata_dev_enabled(dev)))
-		return 0;
-
-	if (!atapi_enabled || (dev->link->ap->flags & ATA_FLAG_NO_ATAPI)) {
-		if (unlikely(dev->class == ATA_DEV_ATAPI)) {
-			ata_dev_printk(dev, KERN_WARNING,
-				       "WARNING: ATAPI is %s, device ignored.\n",
-				       atapi_enabled ? "not supported with this driver" : "disabled");
-			return 0;
-		}
-	}
-
-	return 1;
-}
-
-/**
  *	ata_scsi_find_dev - lookup ata_device from scsi_cmnd
  *	@ap: ATA port to which the device is attached
  *	@scsidev: SCSI device from which we derive the ATA device
@@ -2600,7 +2570,7 @@ ata_scsi_find_dev(struct ata_port *ap, const struct scsi_device *scsidev)
 {
 	struct ata_device *dev = __ata_scsi_find_dev(ap, scsidev);
 
-	if (unlikely(!dev || !ata_scsi_dev_enabled(dev)))
+	if (unlikely(!dev || !ata_dev_enabled(dev)))
 		return NULL;
 
 	return dev;
@@ -3621,7 +3591,7 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *),
 
 	ata_scsi_dump_cdb(ap, cmd);
 
-	if (likely(ata_scsi_dev_enabled(ap->link.device)))
+	if (likely(ata_dev_enabled(ap->link.device)))
 		rc = __ata_scsi_queuecmd(cmd, done, ap->link.device);
 	else {
 		cmd->result = (DID_BAD_TARGET << 16);
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index f6f9c28..ade5c75 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -66,7 +66,6 @@ enum {
 
 extern unsigned int ata_print_id;
 extern struct workqueue_struct *ata_aux_wq;
-extern int atapi_enabled;
 extern int atapi_passthru16;
 extern int libata_fua;
 extern int libata_noacpi;

Re: [PATCH]: libata-scsi: Don't start hotplug work queue if hotplug is disabled

[Chunbo Luo]

On Mon, 2008-07-21 at 13:40 +0900, Tejun Heo wrote:
> Chunbo Luo wrote:
> >> And this message has nothing to do with SCSI.  It's printed by libata
> >> proper for several drivers which don't support ATAPI.  Did the patch
> >> really kill the warning messages?  Can you please post full boot log
> >> from 2.6.26 w/o the patch?
> > 
> > If the drivers don't support ATAPI, the hotplug task cannot find linked device,
> > and it will start the hotplug task again. Which will print messages continously 
> > like this.
> > 
> > ----------
> > ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
> > ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
> > ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
> > ata3.00: WARNING: ATAPI is not supported with this driver, device ignored.
> > ...
> > ------------
> > 
> > We should find a way to avoid this kind of noisy warning messages.
> > 
> > This patch cannot kill the warning messages, but it can avoid the
> > duplicate warning messages.
> 
> Hmmm... yeah, indeed.  That check is located at rather strange place.
> Can you please test the attached patch?

I tested the attached patch on mpc7448hpc2 ,it worked. It can avoid the
multiple warning messages.

Thanks!
Chunbo
> 
> Thanks.
>