Skip to contentSkip to conversation

libata: scsi error handling, encore

PATCH In review Douglas Gilbert 5 participants 12 messages Oct 9, 2005 – Oct 19, 2005

Subsystems (2)
SCSI libata
lore
Share & tools
lore

Douglas Gilbert

author
#0 lore raw

cover libata: scsi error handling, encore

This is a series of patches that are equivalent to one sent on 2005/09/19:
"[PATCH] libata: scsi error handling, lk 2.6.14-rc1"
http://marc.theaimsgroup.com/?l=linux-scsi&m=112711945709949&w=2

The aim is to build more general sense buffers for SCSI errors detected in the SCSI ATA translation layer in libata. These patches are against Jeff's libata-dev git repository, upstream branch.
Patch 1: adds ata_scsi_set_sense() extern declaration and
definition
Patch 2: switch error processing in libata-scsi.c to use
ata_scsi_set_sense()
Patch 3: remove static inline definitions in libata.h that
are no longer used after patch 2

Signed-off-by Douglas Gilbert <[email protected]>

Doug Gilbert

#1 lore raw
Quoted 16 lines · Douglas Gilbert

This is a series of patches that are equivalent to one sent on 2005/09/19:
"[PATCH] libata: scsi error handling, lk 2.6.14-rc1"
http://marc.theaimsgroup.com/?l=linux-scsi&m=112711945709949&w=2

The aim is to build more general sense buffers for SCSI errors detected in the SCSI ATA translation layer in libata. These patches are against Jeff's libata-dev git repository, upstream branch.
Patch 1: adds ata_scsi_set_sense() extern declaration and
definition
Patch 2: switch error processing in libata-scsi.c to use
ata_scsi_set_sense()
Patch 3: remove static inline definitions in libata.h that
are no longer used after patch 2

Quoted 1 line
Signed-off-by Douglas Gilbert <[email protected]>

First and foremost, applied all three patches. Thanks for your patience. 'upstream' branch will reflect your changes as soon as master.kernel.org propagates to the public mirrors.

The patches had to be hand-applied, for a few reasons, so I have some follow-up notes.

1) It would be nice if you could avoid using MIME attachments for patches, but rather, include them inline.

2) Your email subject should be descriptive and unique, since it is copied verbatim into the Linux Kernel changelog by Linus's patch merging tool, git-applymbox. e.g.

[patch 1/3] libata scsi EH: add ata_scsi_set_sense() [patch 2/3] libata scsi EH: upgrade EH using ata_scsi_set_sense() [patch 3/3] libata scsi EH: remove now-unused helpers

See http://linux.yyz.us/patch-format.html for more info.

3) The following change should have been in a separate patch, since it was largely unrelated to use of ata_scsi_set_sense() -- it changes behavior, and its easy for a reviewer to miss this behavior change, if it is buried deep inside a pile of unrelated changes:

Quoted 2 lines

- yield an error for mode sense requests for saved

    values [sat-r06]

But that's just a note for the future. As I stated, I applied your patches as-is... The optimal, reviewer-friendly transformation IMO would have been:

[patch 1/3] libata scsi EH: add ata_scsi_set_sense() [patch 2/3] libata scsi EH: upgrade EH using ata_scsi_set_sense() [patch 3/3] libata scsi EH: return err for MODE SENSE saved vals
[and obviously, patch #3 depends on patch #2]

4) I excised the following chunk from patch #2, before applying:

Review line 1633

Quoted 8 lines
(quoted diff) +1 -1
@@ -1572,7 +1628,7 @@
 		 * time).  We need to issue REQUEST SENSE some other
 		 * way, to avoid completing the command twice.
 		 */
-		cmd->result = SAM_STAT_CHECK_CONDITION;
+		cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
 
 		qc->scsidone(cmd);

We don't yet have sense at this point; the code above largely serves as a trigger to a SCSI EH kthread, which will wake up and issue REQUEST SENSE for us. Its a bit of a weird setup, and I'm also working in this area, so I simply removed the above quoted change from your patch, which was applied otherwise unaltered.

Thanks,

Jeff

#2 lore raw
Quoted 10 lines · Jeff Garzik

4) I excised the following chunk from patch #2, before applying:

Quoted 7 lines
(quoted diff) +1 -1
@@ -1572,7 +1628,7 @@
 		 * time).  We need to issue REQUEST SENSE some other
 		 * way, to avoid completing the command twice.
 		 */
-		cmd->result = SAM_STAT_CHECK_CONDITION;
+		cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
Quoted 8 lines

Review line 1633

Quoted 1 line
(quoted diff) +0 -0
 		qc->scsidone(cmd);

We don't yet have sense at this point; the code above largely serves as a trigger to a SCSI EH kthread, which will wake up and issue REQUEST SENSE for us. Its a bit of a weird setup, and I'm also working in this area, so I simply removed the above quoted change from your patch, which was applied otherwise unaltered.

If libata-scsi aspires to become SATL*, it needs to implement autosense (which has been the norm since 2002 and everyone has already forgotten this word, since it is the norm).

E.g. If you look at the SAS Code, you have more than enough information to generate it (see sas_task.h::struct ata_task_resp).

Luben
* libata-scsi would need a _lot_ of changes to become SATL. Would it be more efficient to start from a clean slate (drivers/scsi/satl/satl.c) or change libata-scsi beyond recognition? What is the political stance on this?

Signature · 3 lines
http://linux.adaptec.com/sas/
Disclaimer: Opinions stated in this email are my own, not of my employer.
For inquiries write to: [email protected] or [email protected].
#3 lore raw
Quoted 4 lines · Luben Tuikov

* libata-scsi would need a _lot_ of changes to become SATL. Would it be more efficient to start from a clean slate (drivers/scsi/satl/satl.c) or change libata-scsi beyond recognition? What is the political stance on this?

We don't need multiple ATA<->SCSI simulators in the kernel.

Jeff

author
#4 lore raw
In reply to Jeff Garzik
Quoted 18 lines · Jeff Garzik
Quoted 16 lines · Douglas Gilbert

This is a series of patches that are equivalent to one sent on 2005/09/19:
"[PATCH] libata: scsi error handling, lk 2.6.14-rc1"
http://marc.theaimsgroup.com/?l=linux-scsi&m=112711945709949&w=2

The aim is to build more general sense buffers for SCSI errors detected in the SCSI ATA translation layer in libata. These patches are against Jeff's libata-dev git repository, upstream branch.
Patch 1: adds ata_scsi_set_sense() extern declaration and
definition
Patch 2: switch error processing in libata-scsi.c to use
ata_scsi_set_sense()
Patch 3: remove static inline definitions in libata.h that
are no longer used after patch 2

Quoted 6 lines
Quoted 1 line
Signed-off-by Douglas Gilbert <[email protected]>

First and foremost, applied all three patches. Thanks for your patience. 'upstream' branch will reflect your changes as soon as master.kernel.org propagates to the public mirrors.

Jeff,
Six days have passed and "git pull ..../libata-dev" still does not reflect these changes. Am I doing something wrong? Should I wait before submitting more changes?

Doug Gilbert

#5 lore raw
Quoted 3 lines · Douglas Gilbert

Six days have passed and "git pull ..../libata-dev" still does not reflect these changes. Am I doing something wrong? Should I wait before submitting more changes?

Your changes are committed to the 'upstream' branch. Maybe you need to do
git pull $url upstream
?

Jeff

author
#6 lore raw
Quoted 9 lines · Jeff Garzik
Quoted 3 lines · Douglas Gilbert

Six days have passed and "git pull ..../libata-dev" still does not reflect these changes. Am I doing something wrong? Should I wait before submitting more changes?

Your changes are committed to the 'upstream' branch. Maybe you need to do
git pull $url upstream

Jeff,
Thanks, indeed I do. Obviously the fact the 'upstream' was the active branch in the destination directory was not sufficient. So do I need to do both
'git pull $url' and 'git pull $url upstream' or is the latter sufficient?

"git" does seems an appropriate name :-)

Doug Gilbert

#7 lore raw
Quoted 21 lines · Douglas Gilbert
Quoted 10 lines · Jeff Garzik
Quoted 3 lines · Douglas Gilbert

Six days have passed and "git pull ..../libata-dev" still does not reflect these changes. Am I doing something wrong? Should I wait before submitting more changes?

Your changes are committed to the 'upstream' branch. Maybe you need to do
git pull $url upstream

Jeff,
Thanks, indeed I do. Obviously the fact the 'upstream' was the active branch in the destination directory was not sufficient. So do I need to do both
'git pull $url' and 'git pull $url upstream' or is the latter sufficient?

Quoted 4 lines

"git" does seems an appropriate name :-)

Doug Gilbert

Hi, Doug,

I use the following script.

$ cat ~/bin/git-sync.sh
#!/bin/sh

if [ ! -r .git/origin ]; then
echo "not in git repo" 2>&1
exit 1
fi

rsync -avz --ignore-existing $(cat .git/origin)/ .git/ rsync -avz $(cat .git/origin)/refs/ .git/refs/

After syncing, you need to do 'git-read-tree -m upstream'
'git-checkout-cache -q -f -u -a'

Signature · 1 line
tejun
#8 lore raw
Quoted 42 lines · Tejun Heo
Quoted 23 lines · Douglas Gilbert
Quoted 11 lines · Jeff Garzik
Quoted 3 lines · Douglas Gilbert

Six days have passed and "git pull ..../libata-dev" still does not reflect these changes. Am I doing something wrong? Should I wait before submitting more changes?

Your changes are committed to the 'upstream' branch. Maybe you need to do
git pull $url upstream

Jeff,
Thanks, indeed I do. Obviously the fact the 'upstream' was the active branch in the destination directory was not sufficient. So do I need to do both
'git pull $url' and 'git pull $url upstream' or is the latter sufficient?

Quoted 4 lines

"git" does seems an appropriate name :-)

Doug Gilbert

Hi, Doug,

I use the following script.

$ cat ~/bin/git-sync.sh
#!/bin/sh

if [ ! -r .git/origin ]; then
echo "not in git repo" 2>&1
exit 1
fi

Quoted 6 lines

rsync -avz --ignore-existing $(cat .git/origin)/ .git/ rsync -avz $(cat .git/origin)/refs/ .git/refs/

After syncing, you need to do 'git-read-tree -m upstream'
'git-checkout-cache -q -f -u -a'

Oops, and my .git/origin contains url of the repository.

$ cat .git/origin
rsync://rsync.kernel.org/pub/scm/linux/kernel/git/jgarzik/libata-dev.git
Signature · 1 line
tejun
#9 lore raw
In reply to Tejun Heo
Quoted 8 lines · Tejun Heo
$ cat ~/bin/git-sync.sh
#!/bin/sh

if [ ! -r .git/origin ]; then
echo "not in git repo" 2>&1
exit 1
fi

Quoted 5 lines

rsync -avz --ignore-existing $(cat .git/origin)/ .git/ rsync -avz $(cat .git/origin)/refs/ .git/refs/

After syncing, you need to do 'git-read-tree -m upstream'
'git-checkout-cache -q -f -u -a'

In the fast-moving world of git, this is a bit outdated. You should use

git checkout -f upstream

to check out a specific branch.

Jeff

#10 lore raw
Quoted 18 lines · Jeff Garzik
Quoted 8 lines · Tejun Heo
$ cat ~/bin/git-sync.sh
#!/bin/sh

if [ ! -r .git/origin ]; then
echo "not in git repo" 2>&1
exit 1
fi

Quoted 5 lines

rsync -avz --ignore-existing $(cat .git/origin)/ .git/ rsync -avz $(cat .git/origin)/refs/ .git/refs/

After syncing, you need to do 'git-read-tree -m upstream'
'git-checkout-cache -q -f -u -a'

Quoted 6 lines

In the fast-moving world of git, this is a bit outdated. You should use

git checkout -f upstream

to check out a specific branch.

Oh.. there was the '-f' switch. I always hit 'xxx changed' message
without the option and have gotten into the habit of using read-tree/checkout-cache after a while. Thanks for the tip.

Signature · 1 line
tejun
#11 lore raw
In reply to Jeff Garzik
Quoted 7 lines · Jeff Garzik
Quoted 3 lines · Douglas Gilbert

Six days have passed and "git pull ..../libata-dev" still does not reflect these changes. Am I doing something wrong? Should I wait before submitting more changes?

Quoted 7 lines

Your changes are committed to the 'upstream' branch. Maybe you need to do
git pull $url upstream
?

Jeff

I can see Doug's patch like ata_scsi_set_sense() in the libata-dev upstream branch.

I've also revised the CHS follow-up patch to be based on Doug's patch. e.g. http://marc.theaimsgroup.com/?l=linux-ide&m=112910068215803&w=2
(commit head edea3ab58f8edd5f72d31f891ab4f34382e97e3a).

Albert