Hack to fix not working spindown over Firewire

Hack to fix not working spindown over Firewire

[Tino Keitel]

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

Hi folks,

I like the manage_start_stop feature of the SD driver to spin down the
hard disks (SATA and USB) during suspend. However, it didn't work with
my Firewire hard disk.

After some research I found out that I can spin down the disk with
sg_start --pc=2, and spin it up with sg_start --pc=1. I adopted this
into sd_start_stop_device() with the vendor name of the device
hardcoded (see the attached patch) and now my Firewire hard disk spins
down on suspend and spins up on resume.

Is there any chance to get this behaviour without such ugly changes to
the kernel? I had to make it conditional by checking the device as
otherwise the SATA disk reports an error.

Regards,
Tino

[-- Attachment #2: start_stop_unit_set_power_condition_v2.diff --]
[-- Type: text/x-diff, Size: 592 bytes --]

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3cea17d..4d7dbd6 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1788,8 +1788,16 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
 	struct scsi_device *sdp = sdkp->device;
 	int res;
 
-	if (start)
+	if (start) {
 		cmd[4] |= 1;	/* START */
+		if(!strncmp(sdp->vendor, "WDC WD32", 8)) {
+			cmd[4] |= (1 << 4); /* power condition */
+		}
+	} else {
+		if(!strncmp(sdp->vendor, "WDC WD32", 8)) {
+			cmd[4] |= (2 << 4); /* power condition */
+		}
+	}
 
 	if (!scsi_device_online(sdp))
 		return -ENODEV;

Re: Hack to fix not working spindown over Firewire

[Stefan Richter]

Tino Keitel wrote:
> I like the manage_start_stop feature of the SD driver to spin down the
> hard disks (SATA and USB) during suspend. However, it didn't work with
> my Firewire hard disk.
> 
> After some research I found out that I can spin down the disk with
> sg_start --pc=2, and spin it up with sg_start --pc=1. I adopted this
> into sd_start_stop_device() with the vendor name of the device
> hardcoded (see the attached patch) and now my Firewire hard disk spins
> down on suspend and spins up on resume.
> 
> Is there any chance to get this behaviour without such ugly changes to
> the kernel? I had to make it conditional by checking the device as
> otherwise the SATA disk reports an error.
[...]
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1788,8 +1788,16 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
>  	struct scsi_device *sdp = sdkp->device;
>  	int res;
>  
> -	if (start)
> +	if (start) {
>  		cmd[4] |= 1;	/* START */
> +		if(!strncmp(sdp->vendor, "WDC WD32", 8)) {
> +			cmd[4] |= (1 << 4); /* power condition */
> +		}
> +	} else {
> +		if(!strncmp(sdp->vendor, "WDC WD32", 8)) {
> +			cmd[4] |= (2 << 4); /* power condition */
> +		}
> +	}
>  
>  	if (!scsi_device_online(sdp))
>  		return -ENODEV;

For LKML:  There was previous discussion on linux1394-user and LSML.
http://marc.info/?t=120720546800001
The enclosure is a Raidsonic ICY Box.

Responsible for the spindown behavior is the IDE-to-FireWire bridge
controller which is a Prolific PL-3507, and its firmware which is
different from the USB firmware.  Also, there are different firmwares in
the wild for PL-3507.

The sdp->vendor string is assembled by the firmware from data gathered
from the IDE device.  It is therefore not suitable to detect affected
devices.

The traditional way to handle things like this is:

   - Implement this as a workaround in sd.

   - Make this workaround conditional on a quirks flag in
     struct scsi_device.

   - Switch this flag on in a place where it can be reliably
     detected that this device needs it.  There is an interface
     to do this in userspace.  But in this particular case, the
     best place is the sbp2 driver (ond firewire-sbp2 driver),
     because it can detect PL-3507 firmwares by means of firmware
     identifiers.

Tino, to find the firmware marker you can for example:
# echo 0x1000 > /sys/module/sbp2/parameters/workarounds
Then plug in the disk and
# dmesg | grep sbp2

On the other hand, perhaps we can enable the workaround unconditional
for all firewire disks (switch in the flag in sbp2 and firewire-sbp2
without looking at the device) or for all RBC disks (switch on the flag
in sd.c if sdev->type == TYPE_RBC).

RBC clause 5.4.1 states that support of the following POWER CONDITION
values in the START STOP UNIT cdb is mandatory:
0 = No change in power condition
1 = Place device in Active condition
2 = Place device in Idle condition
3 = Place device in Standby condition
5 = Place device in Sleep condition
There is also optional support for 7 = Device Control, which is
uninteresting for our purposes.  According to the description of Active,
Idle, Standby, Sleep in RBC, we do most certainly want code 1 on resume,
and rather 3 than 2 on suspend. 5 instead of 3 or 2 might even be better
from the POV of energy consumption but a "device reset may be required
before access to the device is allowed" which I'd rather not deal with.

Tino, does the scsi stack log the device as "Direct-Access-RBC" after it
was plugged in?

Note to self:  Test sg_start --pc={1,3} with all the various SBP-2
bridges I have access to...
-- 
Stefan Richter
-=====-==--- -=-- ====-
http://arcgraph.de/sr/

Re: Hack to fix not working spindown over Firewire

[Stefan Richter]

I wrote:
> Tino Keitel wrote:
>> @@ -1788,8 +1788,16 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
>>  	struct scsi_device *sdp = sdkp->device;
>>  	int res;
>>  
>> -	if (start)
>> +	if (start) {
>>  		cmd[4] |= 1;	/* START */
>> +		if(!strncmp(sdp->vendor, "WDC WD32", 8)) {
>> +			cmd[4] |= (1 << 4); /* power condition */
>> +		}
>> +	} else {
>> +		if(!strncmp(sdp->vendor, "WDC WD32", 8)) {
>> +			cmd[4] |= (2 << 4); /* power condition */
>> +		}
>> +	}
>>  
>>  	if (!scsi_device_online(sdp))
>>  		return -ENODEV;
> 
[...]
> The traditional way to handle things like this is:
> 
>    - Implement this as a workaround in sd.
> 
>    - Make this workaround conditional on a quirks flag in
>      struct scsi_device.
> 
>    - Switch this flag on in a place where it can be reliably
>      detected that this device needs it.  There is an interface
>      to do this in userspace.  But in this particular case, the
>      best place is the sbp2 driver (ond firewire-sbp2 driver),
>      because it can detect PL-3507 firmwares by means of firmware
>      identifiers.
[...]
> On the other hand, perhaps we can enable the workaround unconditional
> for all firewire disks (switch in the flag in sbp2 and firewire-sbp2
> without looking at the device) or for all RBC disks (switch on the flag
> in sd.c if sdev->type == TYPE_RBC).

PS:
If it works for all RBC devices (it should according to the spec...) &&
your ICY Box exposes itself as RBC device (it should do so), then we
don't need any changes to struct scsi_device and to sbp2/ firewire-sbp2.
-- 
Stefan Richter
-=====-==--- -=-- ====-
http://arcgraph.de/sr/

Re: Hack to fix not working spindown over Firewire

[Tino Keitel]

On Wed, Apr 30, 2008 at 15:31:41 +0200, Stefan Richter wrote:

[...]

> Tino, to find the firmware marker you can for example:
> # echo 0x1000 > /sys/module/sbp2/parameters/workarounds
> Then plug in the disk and
> # dmesg | grep sbp2

I used firewire_sbp2. I hope that's ok.

firewire_sbp2: Please notify linux1394-devel@lists.sourceforge.net if
you need the workarounds parameter for fw1.0
firewire_sbp2: Workarounds for fw1.0: 0x100 (firmware_revision
0x012804, model_id 0x000001)
firewire_sbp2: fw1.0: logged in to LUN 0000 (0 retries)

[...]

> uninteresting for our purposes.  According to the description of Active,
> Idle, Standby, Sleep in RBC, we do most certainly want code 1 on resume,
> and rather 3 than 2 on suspend. 5 instead of 3 or 2 might even be better

FYI, sg_start --pc=3 also spins down the drive here.

> from the POV of energy consumption but a "device reset may be required
> before access to the device is allowed" which I'd rather not deal with.
> 
> Tino, does the scsi stack log the device as "Direct-Access-RBC" after it
> was plugged in?

Yes:

scsi 7:0:0:0: Direct-Access-RBC WDC WD32 00JB-00KFA0           PQ: 0 ANSI: 4

> 
> Note to self:  Test sg_start --pc={1,3} with all the various SBP-2
> bridges I have access to...

Thanks for your effort.

Regards,
Tino

Re: Hack to fix not working spindown over Firewire

[Stefan Richter]

On  7 May, Tino Keitel wrote at LKML and LSML:

[PL-3507 in Raidsonic Icy Box needs power condition 2 or 3 in the START
STOP UNIT command in order to spin down]

> On Wed, Apr 30, 2008 at 15:31:41 +0200, Stefan Richter wrote:
>> According to the description of Active,
>> Idle, Standby, Sleep in RBC, we do most certainly want code 1 on resume,
>> and rather 3 than 2 on suspend. 5 instead of 3 or 2 might even be better
> 
> FYI, sg_start --pc=3 also spins down the drive here.
> 
>> from the POV of energy consumption but a "device reset may be required
>> before access to the device is allowed" which I'd rather not deal with.
>> 
>> Tino, does the scsi stack log the device as "Direct-Access-RBC" after it
>> was plugged in?
> 
> Yes:
> 
> scsi 7:0:0:0: Direct-Access-RBC WDC WD32 00JB-00KFA0           PQ: 0 ANSI: 4


I tested a few FireWire disks now.

1) FireWire to IDE or to SATA chipset
2) start bit is honored:
   "sg_start --stop" stops the motor and "sg_start" restarts it
3) power condition field is honored:
   "sg_start --pc=3" stops the motor and "sg_start --pc=1 restarts it
4) peripheral device type claimed by the firmware
5) vendor and model of the enclosure


1)           2)     3)     4)    5)
bridge       start  pc     PDT   enclosure
--------------------------------------------------------------
INIC-2430    yes    yes    SBC   AVLAB 2.5" Drive Kit
INIC-2430    yes    yes    SBC   IOI FWB-IDE01AB, 1 HDD
INIC-2430    yes    yes    SBC   IOI FWB-IDE01AB, 2 HDDs, JBOD

OXFW911      yes    yes    RBC   MacPower Icecube
OXUF922      yes    yes    RBC   MacPower Icecube 800+
OXFW912      yes    yes    RBC   MacPower Igloo
OXFW912      yes    yes    RBC   VulcanTech DualDrive 2nd gen.
OXUF924DSB   yes    yes    SBC   MacPower Taurus, 1 HDD
OXUF924DSB   no     no     SBC   MacPower Taurus, 2 HDDs, JBOD

PL-3507      no     no     RBC   MacPower Prefect II

TSB42AA9     no     yes    SBC   DViCO MomoBay CX-1
TSB42AA9A    yes    yes    RBC   DViCO MomoBay FX-3A


General remarks:

Several disks don't require an "sg_start" or "sg_start --pc=1" to spin
up again; they do so on next access.  Some even ignore "sg_start --pc=1"
and do not start the motor until the next access (e.g. read access).  I
did not test systematically whether "sg_start" or "sg_start --pc=1" are
required at all.  (Proper support of "sg_start --pc=1" is beneficial in
so far as the high-latency operation which spin-up is happens when it is
expected, not sometime later.)

The firmwares of CX-1, FX-3A, AVLAB, and Prefect II implement
auto-spin-down after some time of inactivity (different from firmware to
firmware) and auto-spin-up on subsequent access.

Remarks on individual enclosures/ firmwares:

MacPower Taurus
---------------
I could not test RAID0 and RAID1 due to lack of spare disks.

MacPower Prefect II
-------------------
This is a 5.25" enclosure, i.e. primarily meant for CD/DVD-ROM/R/W.  I
tested it with a HDD here.  The firmware is from MacPower, not from
Prolific.  Power conditions 2 or 5 don't spin the disk down either.

MomoBay CX-1
------------
"sg_start --stop" stops the motor, but "sg_start" fails with unit
attention.  Subsequent read commands after "sg_start --stop" time out.
I.e. "sg_start --stop" causes CX-1 to go offline.  It needs to be
power-cycled after that.

MomoBay FX-3A
-------------
"sg_start --stop" works.  "sg_start" ends with unit attention, but
starts the motor.  Subsequent read commands succeed.


Conclusions so far:

  - Some PL-3507 based devices, notably those with a later firmware from
    Prolific, spin down if power condition 2 or 3 is set.  Others don't.

  - The old and in several ways quirky DViCO CX-1 must not be exposed to
    START STOP UNIT with start flag off.

  - There is alas no correlation between the PDT and support for either
    variant of spin down/ spin up.

I hope to test another PL-3507 device and an Apple Mac in target mode
soon.  I will then post a patch for the sbp2/scsi/sd stack based on
Tino's patch and the current findings which switches all SBP-2 HDDs to
using the power condition field in sd_start_stop_device().

I also have two devices which probably contain old SYM13FWxxx bridges.
Alas I can't test them with HDDs because they are sealed CD-RW
enclosures which cannot be opened without damage.
-- 
Stefan Richter
-=====-==--- -=-= -=--=
http://arcgraph.de/sr/

Re: Clean patch to fix not working spindown over Firewire

[Tino Keitel]

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

On Wed, Apr 30, 2008 at 15:36:36 +0200, Stefan Richter wrote:

[...]

> PS:
> If it works for all RBC devices (it should according to the spec...) &&
> your ICY Box exposes itself as RBC device (it should do so), then we
> don't need any changes to struct scsi_device and to sbp2/ firewire-sbp2.

Do you mean something like in the attached (untested) patch?

Regards,
Tino

[-- Attachment #2: start_stop_unit_set_power_condition_v3.diff --]
[-- Type: text/x-diff, Size: 566 bytes --]

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 01cefbb..6b927f6 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1787,8 +1787,13 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
 	struct scsi_device *sdp = sdkp->device;
 	int res;
 
-	if (start)
+	if (start) {
 		cmd[4] |= 1;	/* START */
+		/* active power condition */
+		cmd[4] |= (sdp->type == TYPE_RBC) ? (1 << 4) : 0; 
+	} else
+		/* standby power condition */
+		cmd[4] |= (sdp->type == TYPE_RBC) ? (3 << 4) : 0 ; 
 
 	if (!scsi_device_online(sdp))
 		return -ENODEV;

Re: Clean patch to fix not working spindown over Firewire

[Stefan Richter]

On  9 May, Tino Keitel wrote:
> On Wed, Apr 30, 2008 at 15:36:36 +0200, Stefan Richter wrote:
>> If it works for all RBC devices (it should according to the spec...) &&
>> your ICY Box exposes itself as RBC device (it should do so), then we
>> don't need any changes to struct scsi_device and to sbp2/ firewire-sbp2.
> 
> Do you mean something like in the attached (untested) patch?
> 
> Regards,
> Tino
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 01cefbb..6b927f6 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1787,8 +1787,13 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
>  	struct scsi_device *sdp = sdkp->device;
>  	int res;
>  
> -	if (start)
> +	if (start) {
>  		cmd[4] |= 1;	/* START */
> +		/* active power condition */
> +		cmd[4] |= (sdp->type == TYPE_RBC) ? (1 << 4) : 0; 
> +	} else
> +		/* standby power condition */
> +		cmd[4] |= (sdp->type == TYPE_RBC) ? (3 << 4) : 0 ; 
>  
>  	if (!scsi_device_online(sdp))
>  		return -ENODEV;

Yes, that's what I meant.  Would you test it on your disks and, if it
works, repost to LSML with your Signed-off-by line?
(See linux/Documentation/SubmittingPatches.)

Or write it as

 	if (start)
 		cmd[4] |= 1;	/* START */
+
+	if (sdp->type == TYPE_RBC)
+		cmd[4] |= start ? 1 << 4 : 3 << 4;	/* POWER CONDITION */


Alas the test for TYPE_RBC is not ideal.  As I wrote in my other mail,
there are some SBP-2 disks which pose as TYPE_DISK.  It's not a big
problem though because almost all of them also work with POWER
CONDITION = 0.  The only exception is DViCO Momobay CX-1 which goes
belly-up POWER CONDITION = 0 and START = 0.  Maybe I should simply
overwrite the CX-1's sdp->type by TYPE_RBC.  I suspect that this is what
it implements anyway.  The firmwares of the later Momobay models FX-3A
and IIRC CX-2 do so.
-- 
Stefan Richter
-=====-==--- -=-= -=--=
http://arcgraph.de/sr/

Re: Clean patch to fix not working spindown over Firewire

[Stefan Richter]

> On  9 May, Tino Keitel wrote:
>> On Wed, Apr 30, 2008 at 15:36:36 +0200, Stefan Richter wrote:
>>> If it works for all RBC devices (it should according to the spec...) &&
>>> your ICY Box exposes itself as RBC device (it should do so), then we
>>> don't need any changes to struct scsi_device and to sbp2/ firewire-sbp2.
>> 
>> Do you mean something like in the attached (untested) patch?
>> 
>> Regards,
>> Tino
>> 
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 01cefbb..6b927f6 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -1787,8 +1787,13 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
>>  	struct scsi_device *sdp = sdkp->device;
>>  	int res;
>>  
>> -	if (start)
>> +	if (start) {
>>  		cmd[4] |= 1;	/* START */
>> +		/* active power condition */
>> +		cmd[4] |= (sdp->type == TYPE_RBC) ? (1 << 4) : 0; 
>> +	} else
>> +		/* standby power condition */
>> +		cmd[4] |= (sdp->type == TYPE_RBC) ? (3 << 4) : 0 ; 
>>  

PS:
The power conditions should be supported by many more devices besides
RBC devices.  Only SAT (and hence libata) and Linux' "File-backed USB
Storage Gadget" (drivers/usb/gadget/file_storage.c) have objections
against power condition.

But it's less intrusive if we enable it only for RBC for now because we
now have a good idea about how well it is supported by them.
-- 
Stefan Richter
-=====-==--- -=-= -=--=
http://arcgraph.de/sr/

Re: Hack to fix not working spindown over Firewire

[Stefan Richter]

I wrote:
> 1) FireWire to IDE or to SATA chipset
> 2) start bit is honored:
>    "sg_start --stop" stops the motor and "sg_start" restarts it
> 3) power condition field is honored:
>    "sg_start --pc=3" stops the motor and "sg_start --pc=1 restarts it
> 4) peripheral device type claimed by the firmware
> 5) vendor and model of the enclosure
> 
> 
> 1)           2)     3)     4)    5)
> bridge       start  pc     PDT   enclosure
> --------------------------------------------------------------
> INIC-2430    yes    yes    SBC   AVLAB 2.5" Drive Kit
> INIC-2430    yes    yes    SBC   IOI FWB-IDE01AB, 1 HDD
> INIC-2430    yes    yes    SBC   IOI FWB-IDE01AB, 2 HDDs, JBOD
> 
> OXFW911      yes    yes    RBC   MacPower Icecube
> OXUF922      yes    yes    RBC   MacPower Icecube 800+
> OXFW912      yes    yes    RBC   MacPower Igloo
> OXFW912      yes    yes    RBC   VulcanTech DualDrive 2nd gen.
> OXUF924DSB   yes    yes    SBC   MacPower Taurus, 1 HDD
> OXUF924DSB   no     no     SBC   MacPower Taurus, 2 HDDs, JBOD
> 
> PL-3507      no     no     RBC   MacPower Prefect II ¹

  PL-3507      no     yes    RBC   Mapower MAP-KC21C1 ²

> TSB42AA9     no     yes    SBC   DViCO MomoBay CX-1
> TSB42AA9A    yes    yes    RBC   DViCO MomoBay FX-3A

  EFI firmware no     no     RBC   Apple Mac mini x86 in target mode ³


¹) As mentioned, with firmware from MacPower.

²) Same firmware_revision as with Tino's disk, i.e. apparently firmware
   from Prolific.  Same findings as Tino's:
   "sg_start --stop" is accepted but does not stop the motor.
   "sg_start --pc=3" or "=2" stop the motor.
   "sg_start --pc=1" or a read command restarts the motor.
   "sg_start" does not restart the motor.

³) "sg_start --stop" is accepted but does not stop the motor.
   "sg_start --pc=3" or "=1" fails with "invalid field in cdb".
   This is of course against the spec, because RBC includes power
   condition.

Moreover, when OS X shuts down, it apparently sends START STOP UNIT
without power condition to FireWire disks.  Therefore we also should
rather _not_ use power condition per default.

I will send a patch which uses the power condition field only for
PL-3507 and DViCO CX-1.
-- 
Stefan Richter
-=====-==--- -=-= -=-=-
http://arcgraph.de/sr/

[PATCH 0/5] SCSI and FireWire: fix/add START STOP UNIT for SBP-2 disks

[Stefan Richter]

This series contains:

Patch 1/5...3/5:  Some SBP-2 disks don't work as expected if the power
condition field in the START STOP UNIT command is not set.  They either
don't spin down on start=0, or they become unresponsive after it.  These
patches selectively adds power condition values for known affected
devices.  We can't do so across the board because certain other SBP-2
devices have been found to not support power condition (even though it
is mandatory as per RBC).

These three patches should IMO go into mainline during the current -rc
phase.

Patch 4/5...5/5:  Enable scsi_device.manage_start_stop for all SBP-2
devices (except if logged in non-exclusively).  This lets FireWire disks
spin down on suspend, driver unbinding, or system shutdown.

These two patches are post 2.6.26 material.

James, how do we handle patch 1/5, assumed that this one is OK?  Do you
route it through scsi-rc-fixes (or scsi-misc if really necessary), or do
you make an exception and let it slip through linux1394-2.6.git?  The
patch applies to plain 2.6.25 as well as to current scsi-misc without
conflicts.

 drivers/firewire/fw-sbp2.c |   25 +++++++++++++++++++++++--
 drivers/ieee1394/sbp2.c    |   22 ++++++++++++++++++++--
 drivers/ieee1394/sbp2.h    |    1 +
 drivers/scsi/sd.c          |    5 +++++
 include/scsi/scsi_device.h |    1 +
 5 files changed, 50 insertions(+), 4 deletions(-)

Stefan Richter (5):
      scsi: sd: optionally set power condition in START STOP UNIT
      firewire: fw-sbp2: fix spindown for PL-3507 and TSB42AA9 firmwares
      ieee1394: sbp2: fix spindown for PL-3507 and TSB42AA9 firmwares
      firewire: fw-sbp2: spin disks down on suspend and shutdown
      ieee1394: sbp2: spin disks down on suspend and shutdown
-- 
Stefan Richter
-=====-==--- -=-= -=-==
http://arcgraph.de/sr/

[PATCH 1/5] scsi: sd: optionally set power condition in START STOP UNIT

[Stefan Richter]

Adds a new scsi_device flag, start_stop_pwr_cond:  If enabled, the sd
driver will not send plain START STOP UNIT commands but ones with the
power condition field set to 3 (standby) or 1 (active) respectively.

Some FireWire disk firmwares do not stop the motor if power condition is
zero.  Or worse, the become unresponsive after a START STOP UNIT with
power condition = 0 and start = 0.

http://lkml.org/lkml/2008/4/29/704

This patch only adds the necessary code to sd_mod but doesn't activate
it.  Follow-up patches to the FireWire drivers will add detection of
affected devices and enable the code for them.

I did not add power condition values to scsi_error.c::scsi_eh_try_stu()
for now.  The three firmwares which suffer from above mentioned problems
do not need START STOP UNIT in the error handler, and they are not
adversely affected by START STOP UNIT with power condition = 0 and start
= 1 (like scsi_eh_try_stu() sends it if scsi_device.allow_restart is
enabled).

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/scsi/sd.c          |    5 +++++
 include/scsi/scsi_device.h |    1 +
 2 files changed, 6 insertions(+)

Index: linux/drivers/scsi/sd.c
===================================================================
--- linux.orig/drivers/scsi/sd.c
+++ linux/drivers/scsi/sd.c
@@ -1115,6 +1115,8 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 				cmd[1] = 1;	/* Return immediately */
 				memset((void *) &cmd[2], 0, 8);
 				cmd[4] = 1;	/* Start spin cycle */
+				if (sdkp->device->start_stop_pwr_cond)
+					cmd[4] |= 1 << 4;
 				scsi_execute_req(sdkp->device, cmd, DMA_NONE,
 						 NULL, 0, &sshdr,
 						 SD_TIMEOUT, SD_MAX_RETRIES);
@@ -1781,6 +1783,9 @@ static int sd_start_stop_device(struct s
 	if (start)
 		cmd[4] |= 1;	/* START */
 
+	if (sdp->start_stop_pwr_cond)
+		cmd[4] |= start ? 1 << 4 : 3 << 4;
+
 	if (!scsi_device_online(sdp))
 		return -ENODEV;
 
Index: linux/include/scsi/scsi_device.h
===================================================================
--- linux.orig/include/scsi/scsi_device.h
+++ linux/include/scsi/scsi_device.h
@@ -134,6 +134,7 @@ struct scsi_device {
 	unsigned no_start_on_add:1;	/* do not issue start on add */
 	unsigned allow_restart:1; /* issue START_UNIT in error handler */
 	unsigned manage_start_stop:1;	/* Let HLD (sd) manage start/stop */
+	unsigned start_stop_pwr_cond:1;	/* Set power cond. in START_STOP_UNIT */
 	unsigned no_uld_attach:1; /* disable connecting to upper level drivers */
 	unsigned select_no_atn:1;
 	unsigned fix_capacity:1;	/* READ_CAPACITY is too high by 1 */

-- 
Stefan Richter
-=====-==--- -=-= -=-==
http://arcgraph.de/sr/

[PATCH 2/5] firewire: fw-sbp2: fix spindown for PL-3507 and TSB42AA9 firmwares

[Stefan Richter]

Reported by Tino Keitel:  PL-3507 with firmware from Prolific does not
spin down the disk on START STOP UNIT with power condition = 0 and start
= 0.  It does however work with power condition = 2 or 3.

Also found while investigating this:  DViCO Momobay CX-1 and FX-3A (TI
TSB42AA9/A based) become unresponsive after START STOP UNIT with power
condition = 0 and start = 0.  They stay responsive if power condition is
set when stopping the motor.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-sbp2.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -86,6 +86,11 @@ MODULE_PARM_DESC(exclusive_login, "Exclu
  * - delay inquiry
  *   Wait extra SBP2_INQUIRY_DELAY seconds after login before SCSI inquiry.
  *
+ * - power condition
+ *   Set the power condition field in the START STOP UNIT commands sent by
+ *   sd_mod on suspend, resume, and shutdown (if manage_start_stop is on).
+ *   Some disks need this to spin down or to resume properly.
+ *
  * - override internal blacklist
  *   Instead of adding to the built-in blacklist, use only the workarounds
  *   specified in the module load parameter.
@@ -97,6 +102,7 @@ MODULE_PARM_DESC(exclusive_login, "Exclu
 #define SBP2_WORKAROUND_FIX_CAPACITY	0x8
 #define SBP2_WORKAROUND_DELAY_INQUIRY	0x10
 #define SBP2_INQUIRY_DELAY		12
+#define SBP2_WORKAROUND_POWER_CONDITION	0x20
 #define SBP2_WORKAROUND_OVERRIDE	0x100
 
 static int sbp2_param_workarounds;
@@ -107,6 +113,8 @@ MODULE_PARM_DESC(workarounds, "Work arou
 	", skip mode page 8 = "   __stringify(SBP2_WORKAROUND_MODE_SENSE_8)
 	", fix capacity = "       __stringify(SBP2_WORKAROUND_FIX_CAPACITY)
 	", delay inquiry = "      __stringify(SBP2_WORKAROUND_DELAY_INQUIRY)
+	", set power condition in start stop unit = "
+				  __stringify(SBP2_WORKAROUND_POWER_CONDITION)
 	", override internal blacklist = " __stringify(SBP2_WORKAROUND_OVERRIDE)
 	", or a combination)");
 
@@ -310,18 +318,25 @@ static const struct {
 		.firmware_revision	= 0x002800,
 		.model			= 0x001010,
 		.workarounds		= SBP2_WORKAROUND_INQUIRY_36 |
-					  SBP2_WORKAROUND_MODE_SENSE_8,
+					  SBP2_WORKAROUND_MODE_SENSE_8 |
+					  SBP2_WORKAROUND_POWER_CONDITION,
 	},
 	/* DViCO Momobay FX-3A with TSB42AA9A bridge */ {
 		.firmware_revision	= 0x002800,
 		.model			= 0x000000,
-		.workarounds		= SBP2_WORKAROUND_DELAY_INQUIRY,
+		.workarounds		= SBP2_WORKAROUND_DELAY_INQUIRY |
+					  SBP2_WORKAROUND_POWER_CONDITION,
 	},
 	/* Initio bridges, actually only needed for some older ones */ {
 		.firmware_revision	= 0x000200,
 		.model			= ~0,
 		.workarounds		= SBP2_WORKAROUND_INQUIRY_36,
 	},
+	/* PL-3507 bridge with Prolific firmware */ {
+		.firmware_revision	= 0x012800,
+		.model			= ~0,
+		.workarounds		= SBP2_WORKAROUND_POWER_CONDITION,
+	},
 	/* Symbios bridge */ {
 		.firmware_revision	= 0xa0b800,
 		.model			= ~0,
@@ -1539,6 +1554,9 @@ static int sbp2_scsi_slave_configure(str
 	if (lu->tgt->workarounds & SBP2_WORKAROUND_FIX_CAPACITY)
 		sdev->fix_capacity = 1;
 
+	if (lu->tgt->workarounds & SBP2_WORKAROUND_POWER_CONDITION)
+		sdev->start_stop_pwr_cond = 1;
+
 	if (lu->tgt->workarounds & SBP2_WORKAROUND_128K_MAX_TRANS)
 		blk_queue_max_sectors(sdev->request_queue, 128 * 1024 / 512);
 

-- 
Stefan Richter
-=====-==--- -=-= -=-==
http://arcgraph.de/sr/

[PATCH 3/5] ieee1394: sbp2: fix spindown for PL-3507 and TSB42AA9 firmwares

[Stefan Richter]

Reported by Tino Keitel:  PL-3507 with firmware from Prolific does not
spin down the disk on START STOP UNIT with power condition = 0 and start
= 0.  It does however work with power condition = 2 or 3.

Also found while investigating this:  DViCO Momobay CX-1 and FX-3A (TI
TSB42AA9/A based) become unresponsive after START STOP UNIT with power
condition = 0 and start = 0.  They stay responsive if power condition is
set when stopping the motor.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/sbp2.c |   20 ++++++++++++++++++--
 drivers/ieee1394/sbp2.h |    1 +
 2 files changed, 19 insertions(+), 2 deletions(-)

Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c
+++ linux/drivers/ieee1394/sbp2.c
@@ -186,6 +186,11 @@ MODULE_PARM_DESC(exclusive_login, "Exclu
  * - delay inquiry
  *   Wait extra SBP2_INQUIRY_DELAY seconds after login before SCSI inquiry.
  *
+ * - power condition
+ *   Set the power condition field in the START STOP UNIT commands sent by
+ *   sd_mod on suspend, resume, and shutdown (if manage_start_stop is on).
+ *   Some disks need this to spin down or to resume properly.
+ *
  * - override internal blacklist
  *   Instead of adding to the built-in blacklist, use only the workarounds
  *   specified in the module load parameter.
@@ -199,6 +204,8 @@ MODULE_PARM_DESC(workarounds, "Work arou
 	", skip mode page 8 = "   __stringify(SBP2_WORKAROUND_MODE_SENSE_8)
 	", fix capacity = "       __stringify(SBP2_WORKAROUND_FIX_CAPACITY)
 	", delay inquiry = "      __stringify(SBP2_WORKAROUND_DELAY_INQUIRY)
+	", set power condition in start stop unit = "
+				  __stringify(SBP2_WORKAROUND_POWER_CONDITION)
 	", override internal blacklist = " __stringify(SBP2_WORKAROUND_OVERRIDE)
 	", or a combination)");
 
@@ -359,18 +366,25 @@ static const struct {
 		.firmware_revision	= 0x002800,
 		.model_id		= 0x001010,
 		.workarounds		= SBP2_WORKAROUND_INQUIRY_36 |
-					  SBP2_WORKAROUND_MODE_SENSE_8,
+					  SBP2_WORKAROUND_MODE_SENSE_8 |
+					  SBP2_WORKAROUND_POWER_CONDITION,
 	},
 	/* DViCO Momobay FX-3A with TSB42AA9A bridge */ {
 		.firmware_revision	= 0x002800,
 		.model_id		= 0x000000,
-		.workarounds		= SBP2_WORKAROUND_DELAY_INQUIRY,
+		.workarounds		= SBP2_WORKAROUND_DELAY_INQUIRY |
+					  SBP2_WORKAROUND_POWER_CONDITION,
 	},
 	/* Initio bridges, actually only needed for some older ones */ {
 		.firmware_revision	= 0x000200,
 		.model_id		= SBP2_ROM_VALUE_WILDCARD,
 		.workarounds		= SBP2_WORKAROUND_INQUIRY_36,
 	},
+	/* PL-3507 bridge with Prolific firmware */ {
+		.firmware_revision	= 0x012800,
+		.model_id		= SBP2_ROM_VALUE_WILDCARD,
+		.workarounds		= SBP2_WORKAROUND_POWER_CONDITION,
+	},
 	/* Symbios bridge */ {
 		.firmware_revision	= 0xa0b800,
 		.model_id		= SBP2_ROM_VALUE_WILDCARD,
@@ -2002,6 +2016,8 @@ static int sbp2scsi_slave_configure(stru
 		sdev->skip_ms_page_8 = 1;
 	if (lu->workarounds & SBP2_WORKAROUND_FIX_CAPACITY)
 		sdev->fix_capacity = 1;
+	if (lu->workarounds & SBP2_WORKAROUND_POWER_CONDITION)
+		sdev->start_stop_pwr_cond = 1;
 	if (lu->workarounds & SBP2_WORKAROUND_128K_MAX_TRANS)
 		blk_queue_max_sectors(sdev->request_queue, 128 * 1024 / 512);
 	return 0;
Index: linux/drivers/ieee1394/sbp2.h
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.h
+++ linux/drivers/ieee1394/sbp2.h
@@ -345,6 +345,7 @@ enum sbp2lu_state_types {
 #define SBP2_WORKAROUND_FIX_CAPACITY	0x8
 #define SBP2_WORKAROUND_DELAY_INQUIRY	0x10
 #define SBP2_INQUIRY_DELAY		12
+#define SBP2_WORKAROUND_POWER_CONDITION	0x20
 #define SBP2_WORKAROUND_OVERRIDE	0x100
 
 #endif /* SBP2_H */

-- 
Stefan Richter
-=====-==--- -=-= -=-==
http://arcgraph.de/sr/

[PATCH 4/5] firewire: fw-sbp2: spin disks down on suspend and shutdown

[Stefan Richter]

This instructs sd_mod to send START STOP UNIT on suspend and resume,
and on driver unbinding or unloading (including when the system is shut
down).

We don't do this though if multiple initiators may log in to the target.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-sbp2.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -1544,6 +1544,9 @@ static int sbp2_scsi_slave_configure(str
 
 	sdev->use_10_for_rw = 1;
 
+	if (sbp2_param_exclusive_login)
+		sdev->manage_start_stop = 1;
+
 	if (sdev->type == TYPE_ROM)
 		sdev->use_10_for_ms = 1;
 

-- 
Stefan Richter
-=====-==--- -=-= -=-==
http://arcgraph.de/sr/

[PATCH 5/5] ieee1394: sbp2: spin disks down on suspend and shutdown

[Stefan Richter]

This instructs sd_mod to send START STOP UNIT on suspend and resume,
and on driver unbinding or unloading (including when the system is shut
down).

We don't do this though if multiple initiators may log in to the target.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/sbp2.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c
+++ linux/drivers/ieee1394/sbp2.c
@@ -2009,6 +2009,8 @@ static int sbp2scsi_slave_configure(stru
 
 	sdev->use_10_for_rw = 1;
 
+	if (sbp2_exclusive_login)
+		sdev->manage_start_stop = 1;
 	if (sdev->type == TYPE_ROM)
 		sdev->use_10_for_ms = 1;
 	if (sdev->type == TYPE_DISK &&

-- 
Stefan Richter
-=====-==--- -=-= -=-==
http://arcgraph.de/sr/

Re: Hack to fix not working spindown over Firewire

[Stefan Richter]

I wrote:
> 1) FireWire to IDE or to SATA chipset
> 2) start bit is honored:
>    "sg_start --stop" stops the motor and "sg_start" restarts it
> 3) power condition field is honored:
>    "sg_start --pc=3" stops the motor and "sg_start --pc=1 restarts it
> 4) peripheral device type claimed by the firmware
> 5) vendor and model of the enclosure
> 
> 
> 1)           2)     3)     4)    5)
> bridge       start  pc     PDT   enclosure
> --------------------------------------------------------------
...
> TSB42AA9     no     yes    SBC   DViCO MomoBay CX-1
> TSB42AA9A    yes    yes    RBC   DViCO MomoBay FX-3A

Correction:  While the FX-3A stops the motor if START STOP UNIT with an
equivalent to "sg_start --stop", it does not work correctly anymore
after it.  Certain commands after that will hang the firmware up;
notably those sent when sd_mod is probing the disk.  I.e. FX-3A fails
not exactly as CX-1, but similarly.
-- 
Stefan Richter
-=====-==--- -=-= -=-==
http://arcgraph.de/sr/

Re: [PATCH 0/5] SCSI and FireWire: fix/add START STOP UNIT for SBP-2 disks

[Tino Keitel]

Hi,

I applied all patches, and tried it on my system with a SATA, USB and
Firewire disk. All disks spun down at suspend, and spun up again at
resume.

Regards,
Tino

Re: [PATCH 0/5] SCSI and FireWire: fix/add START STOP UNIT for SBP-2 disks

[Stefan Richter]

Tino Keitel wrote:
> I applied all patches, and tried it on my system with a SATA, USB and
> Firewire disk. All disks spun down at suspend, and spun up again at
> resume.

So can I add "Tested-by: Tino Keitel <tino.keitel@gmx.de>" to the
changelog of patches 1/5, 2/5, 4/5 (the patches affecting sd and
fw-sbp2)?

Thanks for testing, and for finding in the first place that we need the
power conditions field.
-- 
Stefan Richter
-=====-==--- -=-= =----
http://arcgraph.de/sr/

Re: [PATCH 0/5] SCSI and FireWire: fix/add START STOP UNIT for SBP-2 disks

[Tino Keitel]

On Fri, May 16, 2008 at 19:43:43 +0200, Stefan Richter wrote:
> Tino Keitel wrote:
> > I applied all patches, and tried it on my system with a SATA, USB and
> > Firewire disk. All disks spun down at suspend, and spun up again at
> > resume.
> 
> So can I add "Tested-by: Tino Keitel <tino.keitel@gmx.de>" to the
> changelog of patches 1/5, 2/5, 4/5 (the patches affecting sd and
> fw-sbp2)?

Yes.

Now I need to figure out how to get the Firewire disk to spin down
using HAL. It looks like HAL just uses /usr/bin/eject, which just sends
a START STOP UNIT command without power conditions set, so the kernel
can't do anything here.

Regards,
Tino

Re: [PATCH 0/5] SCSI and FireWire: fix/add START STOP UNIT for SBP-2 disks

[Stefan Richter]

Tino Keitel wrote:
> Now I need to figure out how to get the Firewire disk to spin down
> using HAL. It looks like HAL just uses /usr/bin/eject, which just sends
> a START STOP UNIT command without power conditions set, so the kernel
> can't do anything here.

What if you teach hal to unbind sd from the respective scsi_device?
"echo -n 123:0:0:0 > /sys/bus/scsi/drivers/sd/unbind" is a manual way to 
do it.  And the "bind" file does the reverse.

Or replace /usr/bin/eject by a shell script of yours which calls 
sg_start when you need it to...
-- 
Stefan Richter
-=====-==--- -=-= =--=-
http://arcgraph.de/sr/

Re: [PATCH 0/5] SCSI and FireWire: fix/add START STOP UNIT for SBP-2 disks

[Stefan Richter]

>> Tino Keitel wrote:
>>> I applied all patches, and tried it on my system with a SATA, USB and
>>> Firewire disk. All disks spun down at suspend, and spun up again at
>>> resume.

OK, I now added your Tested-by.  I also added a short comment to the
change in sd_start_stop_device(), as quoted below, and enqueued this
stuff in

    git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6.git sbp2-spindown

I also made these changes visible in the branch of linux1394-2.6.git
which is pulled into -next.  Shortlog and diffstat is

Stefan Richter (5):
      scsi: sd: optionally set power condition in START STOP UNIT
      firewire: fw-sbp2: fix spindown for PL-3507 and TSB42AA9 firmwares
      ieee1394: sbp2: fix spindown for PL-3507 and TSB42AA9 firmwares
      firewire: fw-sbp2: spin disks down on suspend and shutdown
      ieee1394: sbp2: spin disks down on suspend and shutdown

 drivers/firewire/fw-sbp2.c |   25 +++++++++++++++++++++++--
 drivers/ieee1394/sbp2.c    |   22 ++++++++++++++++++++--
 drivers/ieee1394/sbp2.h    |    1 +
 drivers/scsi/sd.c          |    5 +++++
 include/scsi/scsi_device.h |    1 +
 5 files changed, 50 insertions(+), 4 deletions(-)

James, do you agree to the scsi_device.h + sd.c patch?  And will you
pick it up eventually or should I keep it?  I would submit this and the
depending 4 FireWire patches after 2.6.26 was released --- although the
first 3 patches could also go to Linus before 2.6.26.

Here is the updated first patch:


Date: Sun, 11 May 2008 00:34:07 +0200 (CEST)
From: Stefan Richter <stefanr@s5r6.in-berlin.de>
Subject: scsi: sd: optionally set power condition in START STOP UNIT

Adds a new scsi_device flag, start_stop_pwr_cond:  If enabled, the sd
driver will not send plain START STOP UNIT commands but ones with the
power condition field set to 3 (standby) or 1 (active) respectively.

Some FireWire disk firmwares do not stop the motor if power condition is
zero.  Or worse, they become unresponsive after a START STOP UNIT with
power condition = 0 and start = 0.

http://lkml.org/lkml/2008/4/29/704

This patch only adds the necessary code to sd_mod but doesn't activate
it.  Follow-up patches to the FireWire drivers will add detection of
affected devices and enable the code for them.

I did not add power condition values to scsi_error.c::scsi_eh_try_stu()
for now.  The three firmwares which suffer from above mentioned problems
do not need START STOP UNIT in the error handler, and they are not
adversely affected by START STOP UNIT with power condition = 0 and start
= 1 (like scsi_eh_try_stu() sends it if scsi_device.allow_restart is
enabled).

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Tested-by: Tino Keitel <tino.keitel@gmx.de>
---
 drivers/scsi/sd.c          |    5 +++++
 include/scsi/scsi_device.h |    1 +
 2 files changed, 6 insertions(+)

Index: linux/drivers/scsi/sd.c
===================================================================
--- linux.orig/drivers/scsi/sd.c
+++ linux/drivers/scsi/sd.c
@@ -1115,6 +1115,8 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 				cmd[1] = 1;	/* Return immediately */
 				memset((void *) &cmd[2], 0, 8);
 				cmd[4] = 1;	/* Start spin cycle */
+				if (sdkp->device->start_stop_pwr_cond)
+					cmd[4] |= 1 << 4;
 				scsi_execute_req(sdkp->device, cmd, DMA_NONE,
 						 NULL, 0, &sshdr,
 						 SD_TIMEOUT, SD_MAX_RETRIES);
@@ -1781,6 +1783,9 @@ static int sd_start_stop_device(struct s
 	if (start)
 		cmd[4] |= 1;	/* START */
 
+	if (sdp->start_stop_pwr_cond)
+		cmd[4] |= start ? 1 << 4 : 3 << 4;	/* Active or Standby */
+
 	if (!scsi_device_online(sdp))
 		return -ENODEV;
 
Index: linux/include/scsi/scsi_device.h
===================================================================
--- linux.orig/include/scsi/scsi_device.h
+++ linux/include/scsi/scsi_device.h
@@ -134,6 +134,7 @@ struct scsi_device {
 	unsigned no_start_on_add:1;	/* do not issue start on add */
 	unsigned allow_restart:1; /* issue START_UNIT in error handler */
 	unsigned manage_start_stop:1;	/* Let HLD (sd) manage start/stop */
+	unsigned start_stop_pwr_cond:1;	/* Set power cond. in START_STOP_UNIT */
 	unsigned no_uld_attach:1; /* disable connecting to upper level drivers */
 	unsigned select_no_atn:1;
 	unsigned fix_capacity:1;	/* READ_CAPACITY is too high by 1 */

-- 
Stefan Richter
-=====-==--- -=-= =--==
http://arcgraph.de/sr/