Re: libata passthru: support PIO multi commands

2007-06-12 Thread Albert Lee
Alan Cox wrote:
>>ata_scsi_pass_thru() is not executed at ioctl submission time (block 
>>queue submission time), but rather immediately before it is issued to 
>>the drive.  At that point you know the bus is idle, all other commands 
>>have finished executing, and dev->multi_count is fresh not stale.  The 
>>code path goes from ata_scsi_pass_thru() to ata_qc_issue() without 
>>releasing the spinlock, even.
> 
> 
> Think up to user space
> 
> Poorusersapp  set multicount to 4
> 
> Evilproprietarytuningdaemon   set multicount to 8
> 
> Poorusersapp  issue I/O
> 
> at which point an error is indeed best.
> 
> 
>>But the last point is true -- we should error rather than just warn 
>>there, AFAICS.
> 
> 
> Definitely. We've been asked "please do something stupid" and not even in
> a case where the requester may know better.
> 

It looks like the ATA passthru commands contain more information than
what libata needs to execute a command.

e.g. protocol number:
 libata could possibly infer the protocol from the command opcode.

e.g. multi_count:
 libata caches dev->multi_count. Passing multi_count along with
 each passthru command looks useless for libata. 

e.g. t_dir:
 libata could possible infer the direction from the command opcode
 or from the protocol number (e.g. 4: PIO_IN / 5: PIO_OUT).

Due to the redundant info, there is possiblely inconsistency between
the parameters. e.g. t_dir vs protocol. e.g. command vs protocol.

It seems the "redundant" parameters are designed to allow stateless SATL
implementation: The application/passthru command tells the stateless SATL
implementation the protocol and the multi_count, etc. Then SATL just
follows the instruction blindly, even if asked to do something stupid.

Currently libata
 - uses the passthru protocol number blindly
   (even if the application issues a DMA command with wrong PIO protocol.)
 - checks and warns about multi_count
 - ignores t_dir, byte_block and so on.

Maybe we need a strategy to deal with incorrect passed-thru commands?
say,
 - check and reject if something wrong
 - mimimal check and warn/ignore, if it doesn't hurt command execution

--
albert

-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-mm patch] drivers/ata/sata_nv.c: make 3 functions static

2007-06-12 Thread Adrian Bunk
This patch makes 3 needlessly global functions static.

Signed-off-by: Adrian Bunk <[EMAIL PROTECTED]>

---

 drivers/ata/sata_nv.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

--- linux-2.6.22-rc4-mm2/drivers/ata/sata_nv.c.old  2007-06-13 
00:02:18.0 +0200
+++ linux-2.6.22-rc4-mm2/drivers/ata/sata_nv.c  2007-06-13 00:03:12.0 
+0200
@@ -314,7 +314,7 @@
 static int nv_ncqintr_dmasetupfis(struct ata_port *ap);
 static void ncq_clear_singlefis(struct ata_port *ap, u32 val);
 static u32 ncq_ownfisintr_value (struct ata_port *ap);
-void ncq_hotplug(struct ata_port *ap, u32 fis);
+static void ncq_hotplug(struct ata_port *ap, u32 fis);
 static irqreturn_t nv_mcp55_interrupt(int irq, void *dev_instance);
 static int ncq_interrupt(struct ata_port *ap, u32 fis);
 static int nv_scsi_queuecmd(struct scsi_cmnd *cmd,
@@ -1931,7 +1931,7 @@
ncq_clear(ap);
 }
 
-int nv_std_prereset(struct ata_port *ap, unsigned long deadline)
+static int nv_std_prereset(struct ata_port *ap, unsigned long deadline)
 {
struct ata_eh_context *ehc = &ap->eh_context;
 
@@ -2265,7 +2265,7 @@
return 0;
 }
 
-u32 ncq_valid_dhfisflag(struct nv_port_priv *pp)
+static u32 ncq_valid_dhfisflag(struct nv_port_priv *pp)
 {
u32 valid = (pp->dhfis_flags == pp->qc_active);
 
@@ -2332,7 +2332,7 @@
 #endif
 
 
-void ncq_hotplug(struct ata_port *ap, u32 fis)
+static void ncq_hotplug(struct ata_port *ap, u32 fis)
 {
u32 serror;
struct ata_eh_info *ehi = &ap->eh_info;

-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] Make the IDE DMA timeout modifiable

2007-06-12 Thread Sergei Shtylyov

Bartlomiej Zolnierkiewicz wrote:

On Feb 20, 2007, at 5:44 PM, Bartlomiej Zolnierkiewicz wrote:



On Wednesday 21 February 2007 02:19, Suleiman Souhlal wrote:



It can be changed via /proc/ide/hd?/settings.



Why do we need to change IDE DMA timeout dynamically?



I've used it to test error recovery (for example).



Seems quite useable for developers but I would prefer not to
expose it in production kernels for end users.


   It seems that I have counter example of a customer asking if this timeout 
can be done configurable. :-)
   BTW, why the timeout is so damn long? 2*WAIT_CMD is 20 secs, and if DMA is 
not complete or interrupt pending, it may wait 10 more secs...



Thanks,
Bart


MBR, Sergei
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PROBLEM + PATCH] Sata port disabled by BIOS gets initialized

2007-06-12 Thread Matt Sealey

Tejun Heo wrote:
> Hello,
>
>> If the user decides to disable the port through the BIOS, the driver
>> needs to respect the user's wish to not use the port and carry on.
>> Here the end result is a forceful reinitialization of the port by the
>> driver against the user's wishes.
> 
> Well, currently, the Linux driver policy is to exploit the hardware
> capability to the maximum - e.g. we unlock HPA unconditionally and force
> multi-mode controllers into its best possible mode.  We try hard to
> ignore BIOS imposed settings/limits.

Isn't there a case for speeding up boot and not wasting resources by
respecting BIOS settings in this regard? If you have an 8-port
controller on a board and one disk, forcing all of them enabled
regardless of BIOS settings is just 7 redundant port scans.

It should at least be an option - the default being to open up all
gunports, an option to respect BIOS settings and only use the ones
requested and enabled.. (I can see an edge case where a user disables
a disk in the BIOS to stop another OS from looking at/for it, but
wants the Linux system to boot from it)

-- 
Matt Sealey <[EMAIL PROTECTED]>
Genesi, Manager, Developer Relations
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 2b/3] Expose Power Management Policy option to users

2007-06-12 Thread Kristen Carlson Accardi
Enable link power management for ata drivers

libata drivers can define a function (enable_pm) that will
perform hardware specific actions to enable whatever power
management policy the user set up from the scsi sysfs 
interface if the driver supports it.  This power management 
policy will be activated after all disks have been 
enumerated and intialized.

Signed-off-by:  Kristen Carlson Accardi <[EMAIL PROTECTED]>
Index: 2.6-git/drivers/ata/libata-scsi.c
===
--- 2.6-git.orig/drivers/ata/libata-scsi.c
+++ 2.6-git/drivers/ata/libata-scsi.c
@@ -2890,6 +2890,51 @@ void ata_scsi_simulate(struct ata_device
}
 }
 
+int ata_scsi_set_link_pm_policy(struct Scsi_Host *shost,
+   enum scsi_host_link_pm policy)
+{
+   struct ata_port *ap = ata_shost_to_port(shost);
+   int rc = -EINVAL;
+   int i;
+
+   /*
+* make sure no broken devices are on this port,
+* and that all devices support interface power
+* management
+*/
+   for (i = 0; i < ATA_MAX_DEVICES; i++) {
+   struct ata_device *dev = &ap->device[i];
+
+   /* only check drives which exist */
+   if (!ata_dev_enabled(dev))
+   continue;
+
+   /*
+* do we need to handle the case where we've hotplugged
+* a broken drive (since hotplug and ALPM are mutually
+* exclusive) ?
+*
+* If so, if we detect a broken drive on a port with
+* alpm already enabled, then we should reset the policy
+* to off for the entire port.
+*/
+   if ((dev->horkage & ATA_HORKAGE_ALPM) ||
+   !(dev->flags & ATA_DFLAG_IPM)) {
+   ata_dev_printk(dev, KERN_ERR,
+   "Unable to set Link PM policy\n");
+   ap->pm_policy = SHOST_MAX_PERFORMANCE;
+   }
+   }
+
+   if (ap->ops->enable_pm)
+   rc = ap->ops->enable_pm(ap, policy);
+
+   if (!rc)
+   shost->shost_link_pm_policy = ap->pm_policy;
+   return rc;
+}
+EXPORT_SYMBOL_GPL(ata_scsi_set_link_pm_policy);
+
 int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
 {
int i, rc;
@@ -2912,7 +2957,7 @@ int ata_scsi_add_hosts(struct ata_host *
shost->max_lun = 1;
shost->max_channel = 1;
shost->max_cmd_len = 16;
-
+   shost->shost_link_pm_policy = ap->pm_policy;
rc = scsi_add_host(ap->scsi_host, ap->host->dev);
if (rc)
goto err_add;
Index: 2.6-git/include/linux/libata.h
===
--- 2.6-git.orig/include/linux/libata.h
+++ 2.6-git/include/linux/libata.h
@@ -136,6 +136,7 @@ enum {
ATA_DFLAG_CDB_INTR  = (1 << 2), /* device asserts INTRQ when ready 
for CDB */
ATA_DFLAG_NCQ   = (1 << 3), /* device supports NCQ */
ATA_DFLAG_FLUSH_EXT = (1 << 4), /* do FLUSH_EXT instead of FLUSH */
+   ATA_DFLAG_IPM   = (1 << 6), /* device supports interface PM */
ATA_DFLAG_CFG_MASK  = (1 << 8) - 1,
 
ATA_DFLAG_PIO   = (1 << 8), /* device limited to PIO mode */
@@ -300,6 +301,7 @@ enum {
ATA_HORKAGE_NONCQ   = (1 << 2), /* Don't use NCQ */
ATA_HORKAGE_MAX_SEC_128 = (1 << 3), /* Limit max sects to 128 */
ATA_HORKAGE_DMA_RW_ONLY = (1 << 4), /* ATAPI DMA for RW only */
+   ATA_HORKAGE_ALPM= (1 << 5), /* ALPM problems */
 };
 
 enum hsm_task_states {
@@ -548,6 +550,7 @@ struct ata_port {
 
pm_message_tpm_mesg;
int *pm_result;
+   enum scsi_host_link_pm  pm_policy;
 
void*private_data;
 
@@ -607,7 +610,7 @@ struct ata_port_operations {
 
int (*port_suspend) (struct ata_port *ap, pm_message_t mesg);
int (*port_resume) (struct ata_port *ap);
-
+   int (*enable_pm) (struct ata_port *ap, enum scsi_host_link_pm policy);
int (*port_start) (struct ata_port *ap);
void (*port_stop) (struct ata_port *ap);
 
@@ -812,7 +815,7 @@ extern int ata_cable_40wire(struct ata_p
 extern int ata_cable_80wire(struct ata_port *ap);
 extern int ata_cable_sata(struct ata_port *ap);
 extern int ata_cable_unknown(struct ata_port *ap);
-
+extern int ata_scsi_set_link_pm_policy(struct Scsi_Host *shost, enum 
scsi_host_link_pm);
 /*
  * Timing helpers
  */
Index: 2.6-git/drivers/ata/libata-core.c
===
--- 2.6-git.orig/drivers/ata/libata-core.c
+++ 2.6-git/drivers/ata/libata-core.c
@@ -2019,6 +2019,9 @@ int ata_dev_configure(struct ata_device 
if (dev->flags & ATA_DFLAG_LBA48)
dev->max_sectors = ATA_MAX_SEC

Re: [patch 2a/3] Expose Power Management Policy option to users

2007-06-12 Thread Kristen Carlson Accardi
Expose Power Management Policy option to users

This patch will modify the scsi subsystem to allow
users to set a power management policy for the link.

The scsi subsystem will create a new sysfs file for each
host in /sys/class/scsi_host called "link_power_management_policy".
This file can have 3 possible values:

Value   Meaning
---
min_power   User wishes the link to conserve power as much as
possible, even at the cost of some performance

max_performance User wants priority to be on performance, not power
savings

medium_powerUser wants power savings, with less performance cost
than min_power (but less power savings as well).


Signed-off-by:  Kristen Carlson Accardi <[EMAIL PROTECTED]>
Index: 2.6-git/drivers/scsi/hosts.c
===
--- 2.6-git.orig/drivers/scsi/hosts.c
+++ 2.6-git/drivers/scsi/hosts.c
@@ -54,6 +54,25 @@ static struct class shost_class = {
 };
 
 /**
+ * scsi_host_set_link_pm - Change the link power management policy
+ * @shost: scsi host to change the policy of.
+ * @policy:policy to change to.
+ *
+ * Returns zero if successful or an error if the requested
+ * transition is illegal.
+ **/
+int scsi_host_set_link_pm(struct Scsi_Host *shost,
+   enum scsi_host_link_pm policy)
+{
+   struct scsi_host_template *sht = shost->hostt;
+   if (sht->set_link_pm_policy)
+   sht->set_link_pm_policy(shost, policy);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(scsi_host_set_link_pm);
+
+/**
  * scsi_host_set_state - Take the given host through the host
  * state model.
  * @shost: scsi host to change the state of.
Index: 2.6-git/drivers/scsi/scsi_sysfs.c
===
--- 2.6-git.orig/drivers/scsi/scsi_sysfs.c
+++ 2.6-git/drivers/scsi/scsi_sysfs.c
@@ -189,6 +189,74 @@ show_shost_state(struct class_device *cl
 
 static CLASS_DEVICE_ATTR(state, S_IRUGO | S_IWUSR, show_shost_state, 
store_shost_state);
 
+static const struct {
+   enum scsi_host_link_pm  value;
+   char*name;
+} shost_link_pm_policy[] = {
+   { SHOST_NOT_AVAILABLE, "max_performance" },
+   { SHOST_MIN_POWER, "min_power" },
+   { SHOST_MAX_PERFORMANCE, "max_performance" },
+   { SHOST_MEDIUM_POWER, "medium_power" },
+};
+
+const char *scsi_host_link_pm_policy(enum scsi_host_link_pm policy)
+{
+   int i;
+   char *name = NULL;
+
+   for (i = 0; i < ARRAY_SIZE(shost_link_pm_policy); i++) {
+   if (shost_link_pm_policy[i].value == policy) {
+   name = shost_link_pm_policy[i].name;
+   break;
+   }
+   }
+   return name;
+}
+
+static ssize_t store_link_pm_policy(struct class_device *class_dev,
+   const char *buf, size_t count)
+{
+   struct Scsi_Host *shost = class_to_shost(class_dev);
+   enum scsi_host_link_pm policy = 0;
+   int i;
+
+   /*
+* we are skipping array location 0 on purpose - this
+* is because a value of SHOST_NOT_AVAILABLE is displayed
+* to the user as max_performance, but when the user
+* writes "max_performance", they actually want the
+* value to match SHOST_MAX_PERFORMANCE.
+*/
+   for (i = 1; i < ARRAY_SIZE(shost_link_pm_policy); i++) {
+   const int len = strlen(shost_link_pm_policy[i].name);
+   if (strncmp(shost_link_pm_policy[i].name, buf, len) == 0 &&
+  buf[len] == '\n') {
+   policy = shost_link_pm_policy[i].value;
+   break;
+   }
+   }
+   if (!policy)
+   return -EINVAL;
+
+   if (scsi_host_set_link_pm(shost, policy))
+   return -EINVAL;
+   return count;
+}
+static ssize_t
+show_link_pm_policy(struct class_device *class_dev, char *buf)
+{
+   struct Scsi_Host *shost = class_to_shost(class_dev);
+   const char *policy =
+   scsi_host_link_pm_policy(shost->shost_link_pm_policy);
+
+   if (!policy)
+   return -EINVAL;
+
+   return snprintf(buf, 23, "%s\n", policy);
+}
+static CLASS_DEVICE_ATTR(link_power_management_policy, S_IRUGO | S_IWUSR,
+   show_link_pm_policy, store_link_pm_policy);
+
 shost_rd_attr(unique_id, "%u\n");
 shost_rd_attr(host_busy, "%hu\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
@@ -207,6 +275,7 @@ static struct class_device_attribute *sc
&class_device_attr_proc_name,
&class_device_attr_scan,
&class_device_attr_state,
+   &class_device_attr_link_power_management_policy,
NULL
 };
 
Index: 2.6-git/include/scsi/scsi_host.h
===
--- 2.6-git.orig/include/scsi/scsi_host.h
+++ 2.6-git/include/scsi/scsi_host.h
@

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

2007-06-12 Thread Kristen Carlson Accardi
On Tue, 12 Jun 2007 11:46:56 -0400
Jeff Garzik <[EMAIL PROTECTED]> wrote:

> Matthew Garrett wrote:
> > On Tue, Jun 12, 2007 at 11:17:14AM -0300, Henrique de Moraes Holschuh wrote:
> >> On Tue, 12 Jun 2007, Matthew Garrett wrote:
> >>> Laptop bays are designed to deal with hotplugging PATA - I don't think 
> >>> this is too much of an issue :)
> >> The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
> >> cards use usb2.0 and pci-e hotplug...
> > 
> > Yes, but they'll also send an ACPI interrupt even if the SATA host 
> > controller doesn't - it's part of the spec for bays.
> 
> Regardless, having a laptop does not imply having a docking bay.
> 
>   Jeff
> 

For bay devices, we can use ACPI just like we do now.  For non-bay devices,
we can implement hotplug via polling when ALPM is enabled.  In my experience
most laptop vendors implement extra drive as either PATA in a dock station, 
USB in a dock station, or a bay device either on the dock station, or 
on the laptop itself.


Kristen
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

2007-06-12 Thread Jeff Garzik

Matthew Garrett wrote:
Excluding the corner case of an Expresscard SATA controller (where I 
suspect you'd want different policy), I doubt there are any cases where 
you have a laptop with hotplug capabilities without it being implemented 
as an ACPI bay.


Cardbus card.

Jeff



-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: libata passthru: support PIO multi commands

2007-06-12 Thread Alan Cox
> ata_scsi_pass_thru() is not executed at ioctl submission time (block 
> queue submission time), but rather immediately before it is issued to 
> the drive.  At that point you know the bus is idle, all other commands 
> have finished executing, and dev->multi_count is fresh not stale.  The 
> code path goes from ata_scsi_pass_thru() to ata_qc_issue() without 
> releasing the spinlock, even.

Think up to user space

Poorusersappset multicount to 4

Evilproprietarytuningdaemon set multicount to 8

Poorusersappissue I/O

at which point an error is indeed best.

> But the last point is true -- we should error rather than just warn 
> there, AFAICS.

Definitely. We've been asked "please do something stupid" and not even in
a case where the requester may know better.

Alan
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 0/3] AHCI Link Power Management

2007-06-12 Thread Kristen Carlson Accardi
On Tue, 12 Jun 2007 13:40:15 +0900
Tejun Heo <[EMAIL PROTECTED]> wrote:

> I don't think the end result will vary in any significant way.  My
> biggest argument for sw implementation is it can be used for other
> controllers.

What I had in mind when I created the new port operation "enable_pm"
was that other controllers (besides the ahci controller) could define their
own method of enabling power management.  Maybe for non-ahci controllers this
is a software based solution which uses generic SATA dipm/hipm stuff and 
polling.  

See patch 2/3 of this series for the implementation of this.

Kristen
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

2007-06-12 Thread Matthew Garrett
On Tue, Jun 12, 2007 at 11:46:56AM -0400, Jeff Garzik wrote:
> Matthew Garrett wrote:
> >On Tue, Jun 12, 2007 at 11:17:14AM -0300, Henrique de Moraes Holschuh 
> >wrote:
> >>On Tue, 12 Jun 2007, Matthew Garrett wrote:
> >>>Laptop bays are designed to deal with hotplugging PATA - I don't think 
> >>>this is too much of an issue :)
> >>The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
> >>cards use usb2.0 and pci-e hotplug...
> >
> >Yes, but they'll also send an ACPI interrupt even if the SATA host 
> >controller doesn't - it's part of the spec for bays.
> 
> Regardless, having a laptop does not imply having a docking bay.

Excluding the corner case of an Expresscard SATA controller (where I 
suspect you'd want different policy), I doubt there are any cases where 
you have a laptop with hotplug capabilities without it being implemented 
as an ACPI bay.

-- 
Matthew Garrett | [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

2007-06-12 Thread Matthew Garrett
On Wed, Jun 13, 2007 at 12:45:21AM +0900, Tejun Heo wrote:
> Matthew Garrett wrote:
> > Yes, but they'll also send an ACPI interrupt even if the SATA host 
> > controller doesn't - it's part of the spec for bays.
> 
> Does the spec mandate that the ACPI interrupt shouldn't depend on SATA
> phy status?  I don't think vendors are likely to implement separate
> mechanism when SATA phy status can do the job fine.

I suspect that the spec allows them to do that, but think that it's 
unlikely to actually happen in most cases. Bear in mind that Windows 
doesn't tend to drive the hardware in AHCI mode, and that their 
implementation is likely to be very similar to the implementation they 
used for PATA devices.

-- 
Matthew Garrett | [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

2007-06-12 Thread Jeff Garzik

Matthew Garrett wrote:

On Tue, Jun 12, 2007 at 11:17:14AM -0300, Henrique de Moraes Holschuh wrote:

On Tue, 12 Jun 2007, Matthew Garrett wrote:
Laptop bays are designed to deal with hotplugging PATA - I don't think 
this is too much of an issue :)

The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
cards use usb2.0 and pci-e hotplug...


Yes, but they'll also send an ACPI interrupt even if the SATA host 
controller doesn't - it's part of the spec for bays.


Regardless, having a laptop does not imply having a docking bay.

Jeff



-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 0/3] AHCI Link Power Management

2007-06-12 Thread Kristen Carlson Accardi
On Tue, 12 Jun 2007 00:43:12 -0400
Jeff Garzik <[EMAIL PROTECTED]> wrote:

> We will do AHCI link PM -- presuming that I can be convinced that it 
> does not repeatedly park the hard drive heads, or something similarly 
> annoying on PATA<->SATA bridges and similar setups.
> 
> IF it works as advertised -- a big if considering all the AHCI silicon 
> implementations out there -- we definitely want to use it.
> 
>   Jeff
> 

I understand that this is a concern of yours based on some experience you
had with earlier controllers.  In general, this behavior would be considered
incorrect - link power management should not translate to disk parking, even
on PATA->SATA brigdes, and if it does, then that's completely broken.  That 
said, I would believe you if you said broken hardware exists, and when you 
get specific examples of it, you can add it to the blacklist for this feature.
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

2007-06-12 Thread Tejun Heo
Matthew Garrett wrote:
> On Tue, Jun 12, 2007 at 11:17:14AM -0300, Henrique de Moraes Holschuh wrote:
>> On Tue, 12 Jun 2007, Matthew Garrett wrote:
>>> Laptop bays are designed to deal with hotplugging PATA - I don't think 
>>> this is too much of an issue :)
>> The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
>> cards use usb2.0 and pci-e hotplug...
> 
> Yes, but they'll also send an ACPI interrupt even if the SATA host 
> controller doesn't - it's part of the spec for bays.

Does the spec mandate that the ACPI interrupt shouldn't depend on SATA
phy status?  I don't think vendors are likely to implement separate
mechanism when SATA phy status can do the job fine.

-- 
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

2007-06-12 Thread Matthew Garrett
On Tue, Jun 12, 2007 at 11:17:14AM -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 12 Jun 2007, Matthew Garrett wrote:
> > Laptop bays are designed to deal with hotplugging PATA - I don't think 
> > this is too much of an issue :)
> 
> The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
> cards use usb2.0 and pci-e hotplug...

Yes, but they'll also send an ACPI interrupt even if the SATA host 
controller doesn't - it's part of the spec for bays.

-- 
Matthew Garrett | [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: libata passthru: support PIO multi commands

2007-06-12 Thread Jeff Garzik

Alan Cox wrote:

+   if (multi_count != dev->multi_count)
+   ata_dev_printk(dev, KERN_WARNING,
+  "invalid multi_count %u ignored\n",
+  multi_count);
+   }

What limits log spamming here ?

Intelligence of the user with privs?


Not ever SG_IO call requires priviledges.


All calls relevant to this thread require CAP_SYS_RAWIO.



Issue a multi count sized write. Acidentally collide with another program
which mucks up the multiwrite count (for once it probably won't be HAL
making a mess ;)) and send data. The state of the disk at the point a
short multiwrite terminates is undefined, it may have written some of the
sectors, it may have blanked them all (Optical media) etc. Far better
just to error the invalid command - especially as in some cases without
Mark's fixes for DRQ cleanup on error it may even hang in one direction.


ata_scsi_pass_thru() is not executed at ioctl submission time (block 
queue submission time), but rather immediately before it is issued to 
the drive.  At that point you know the bus is idle, all other commands 
have finished executing, and dev->multi_count is fresh not stale.  The 
code path goes from ata_scsi_pass_thru() to ata_qc_issue() without 
releasing the spinlock, even.


But the last point is true -- we should error rather than just warn 
there, AFAICS.


Jeff


-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

2007-06-12 Thread Henrique de Moraes Holschuh
On Tue, 12 Jun 2007, Matthew Garrett wrote:
> On Tue, Jun 12, 2007 at 09:18:19AM -0300, Henrique de Moraes Holschuh wrote:
> > On Tue, 12 Jun 2007, Matthew Garrett wrote:
> > > 
> > > On laptops, I suspect that we'll probably get an ACPI interrupt even if 
> > > the AHCI hotplug pathway can't manage.
> > 
> > As long as we don't crash the drive or AHCI controller because we hotplugged
> > it in a way it didn't like (like trying to hotplug a ICH5R would cause).
> 
> Laptop bays are designed to deal with hotplugging PATA - I don't think 
> this is too much of an issue :)

The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
cards use usb2.0 and pci-e hotplug...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

2007-06-12 Thread Matthew Garrett
On Tue, Jun 12, 2007 at 09:18:19AM -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 12 Jun 2007, Matthew Garrett wrote:
> > 
> > On laptops, I suspect that we'll probably get an ACPI interrupt even if 
> > the AHCI hotplug pathway can't manage.
> 
> As long as we don't crash the drive or AHCI controller because we hotplugged
> it in a way it didn't like (like trying to hotplug a ICH5R would cause).

Laptop bays are designed to deal with hotplugging PATA - I don't think 
this is too much of an issue :)

-- 
Matthew Garrett | [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] ide: convert ide_find_best_mode() users to use ide_max_dma_mode()

2007-06-12 Thread Sergei Shtylyov

Bartlomiej Zolnierkiewicz wrote:

ide-timing.h:



* remove handling of DMA modes from ide_find_best_mode() and rename it to
  ide_find_best_pio_mode()



* drop no longer needed "map" argument from ide_find_best_pio_mode()
  and delete needless ->id check



* remove no longer needed XFER_SWDMA and XFER_UDMA* defines



au1xxx-ide.c:



* use ide_max_dma_mode() instead of ide_find_best_mode()



* remove needless CONFIG_BLK_DEV_IDE_AU1XXX_MDMA2_DBDMA #ifdef



amd74xx.c:

* store UDMA masks in amd_ide_chip[] and while at it make "base" field
  to be u8 instead of unsigned long

* convert the driver to use UDMA masks from amd_ide_chip[]

* use ide_max_dma_mode() and ide_find_best_pio_mode() instead
  of ide_find_best_mode()

* delete stale comment from amd74xx_ide_dma_check()

* remove no longer needed AMD_UDMA* defines

via82cxxx.c:



* remove unused DISPLAY_VIA_TIMINGS define



* store UDMA masks in via_isa_bridges[] and while at it make "flags" field
  to be u8 instead of u16



* convert the driver to use UDMA masks from via_isa_bridges[]



* use ide_max_dma_mode() and ide_find_best_pio_mode() instead
  of ide_find_best_mode()



* remove no longer needed VIA_UDMA* defines



pmac.c:



* use ide_max_dma_mode() instead of ide_find_best_mode()



There should be no functionality changes caused by this patch.


   I'm afraid this hasn't been accomplished yet. ;-)


Signed-off-by: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]>



Index: b/drivers/ide/mips/au1xxx-ide.c
===
--- a/drivers/ide/mips/au1xxx-ide.c
+++ b/drivers/ide/mips/au1xxx-ide.c
@@ -381,9 +381,7 @@ static int auide_dma_setup(ide_drive_t *
 
 static int auide_dma_check(ide_drive_t *drive)

 {
-   u8 speed;
-
-#ifdef CONFIG_BLK_DEV_IDE_AU1XXX_MDMA2_DBDMA
+   u8 speed = ide_max_dma_mode(drive);
 
 	if( dbdma_init_done == 0 ){


   Ugh, coding style isnn't kosher here. :-)


auide_hwif.white_list = ide_in_drive_list(drive->id,
@@ -394,7 +392,6 @@ static int auide_dma_check(ide_drive_t *
auide_ddma_init(&auide_hwif);
dbdma_init_done = 1;
}
-#endif
 
 	/* Is the drive in our DMA black list? */
 
@@ -409,8 +406,6 @@ static int auide_dma_check(ide_drive_t *

else
drive->using_dma = 1;
 
-	speed = ide_find_best_mode(drive, XFER_PIO | XFER_MWDMA);

-   
if (drive->autodma && (speed & XFER_MODE) != XFER_PIO)


   ide_max_dma_mode() returns 0 if DMA not available, so that if should have 
looked this way:


if (drive->autodma && speed)


return 0;


   But is this really equivalent? Why there's no fallback call to 
ide_find_best_pio_mode() like in other cases?


MBR, Sergei
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

2007-06-12 Thread Henrique de Moraes Holschuh
On Tue, 12 Jun 2007, Matthew Garrett wrote:
> On Mon, Jun 11, 2007 at 08:59:46PM -0700, Arjan van de Ven wrote:
> > that's a temporary shortcoming; even with these power savings you can 
> > do hotplug as long as you're willing to poll for it at a reasonable 
> > interval and are willing to wait the time between polls for a hotplug 
> > to take effect..
> 
> On laptops, I suspect that we'll probably get an ACPI interrupt even if 
> the AHCI hotplug pathway can't manage.

As long as we don't crash the drive or AHCI controller because we hotplugged
it in a way it didn't like (like trying to hotplug a ICH5R would cause).

It is not like 1Hz would not be fast enough for laptop bays and docks, so
the polling frequency would not be a problem.  I still prefer when levels
and on/off are kept separate, but if it won't stand in the way of hotplug, I
certainly don't care enough to bother.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

2007-06-12 Thread Matthew Garrett
On Mon, Jun 11, 2007 at 08:59:46PM -0700, Arjan van de Ven wrote:

> that's a temporary shortcoming; even with these power savings you can 
> do hotplug as long as you're willing to poll for it at a reasonable 
> interval and are willing to wait the time between polls for a hotplug 
> to take effect..

On laptops, I suspect that we'll probably get an ACPI interrupt even if 
the AHCI hotplug pathway can't manage.

-- 
Matthew Garrett | [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html