[PATCH 0/3] libata: scsi error handling, encore

[PATCH 0/3] libata: scsi error handling, encore

[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

Signed-off-by: Douglas Gilbert <dougg@torque.net>

Doug Gilbert

Re: [PATCH 0/3] libata: scsi error handling, encore

[Jeff Garzik]

Douglas Gilbert wrote:
> 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 <dougg@torque.net>

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:

>   - 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:

> @@ -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

Re: [PATCH 0/3] libata: scsi error handling, encore

[Luben Tuikov]

On 10/09/05 09:23, Jeff Garzik wrote:
> 4) I excised the following chunk from patch #2, before applying:
> 
> 
>>@@ -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.

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?

-- 
http://linux.adaptec.com/sas/
Disclaimer: Opinions stated in this email are my own, not of my employer.
For inquiries write to: luben_tuikov@adaptec.com or ltuikov@yahoo.com.

Re: [PATCH 0/3] libata: scsi error handling, encore

[Jeff Garzik]

Luben Tuikov wrote:
> * 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

Re: [PATCH 0/3] libata: scsi error handling, encore

[Douglas Gilbert]

Jeff Garzik wrote:
> Douglas Gilbert wrote:
> 
>> 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 <dougg@torque.net>
> 
> 
> 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

Re: [PATCH 0/3] libata: scsi error handling, encore

[Jeff Garzik]

Douglas Gilbert wrote:
> 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

Re: [PATCH 0/3] libata: scsi error handling, encore

[Douglas Gilbert]

Jeff Garzik wrote:
> Douglas Gilbert wrote:
> 
>> 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

Re: [PATCH 0/3] libata: scsi error handling, encore

[Tejun Heo]

Douglas Gilbert wrote:
> Jeff Garzik wrote:
> 
>>Douglas Gilbert wrote:
>>
>>
>>>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
> 

  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'

-- 
tejun

Re: [PATCH 0/3] libata: scsi error handling, encore

[Tejun Heo]

Tejun Heo wrote:
> Douglas Gilbert wrote:
> 
>> Jeff Garzik wrote:
>>
>>> Douglas Gilbert wrote:
>>>
>>>
>>>> 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
>>
> 
>  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'
> 

  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

-- 
tejun

Re: [PATCH 0/3] libata: scsi error handling, encore

[Albert Lee]

Jeff Garzik wrote:
> Douglas Gilbert wrote:
> 
>> 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
> 
> 

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

Re: [PATCH 0/3] libata: scsi error handling, encore

[Jeff Garzik]

Tejun Heo wrote:
> $ 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'


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

Re: [PATCH 0/3] libata: scsi error handling, encore

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> $ 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'
> 
> 
> 
> 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.

-- 
tejun