Re: [patch 29/30] sata_nv: don't read shadow registers when in ADMA mode

Re: [patch 29/30] sata_nv: don't read shadow registers when in ADMA mode

[Robert Hancock]

Jeff Garzik wrote:
> akpm@linux-foundation.org wrote:
>> From: Robert Hancock <hancockr@shaw.ca>
>>
>> Reading from the ATA shadow registers while we are in ADMA mode may cause
>> undefined behavior.  Don't read the ATA status register when completing
>> commands for this reason, it shouldn't be needed as the controller will
>> notify us if the command failed.  Also, don't allow commands with result
>> taskfile requested to execute in ADMA mode, since that requires accessing
>> the shadow registers.  We also still need to override tf_read since 
>> libata
>> will read the result taskfile on a command failure, and we need to go 
>> into
>> port register mode before allowing this.
>>
>> Signed-off-by: Robert Hancock <hancockr@shaw.ca>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> ---
>>
>>  drivers/ata/sata_nv.c |   33 ++++++++++++++++++++-------------
>>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> Robert, do you think this should be pushed into #upstream-fixes 
> (2.6.21-rc)?
> 
> I lean towards "yes", since it is a needed-by-hardware fix, but I also 
> am interested in testing feedback since it is so late in the 2.6.21-rc 
> game.

I would lean toward that as well, but it would be good to get some 
testing from some sata_nv ADMA users to make sure it doesn't do anything 
funny for them..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: [patch 29/30] sata_nv: don't read shadow registers when in ADMA mode

[Alistair John Strachan]

On Friday 09 March 2007 14:27, you wrote:
> Jeff Garzik wrote:
> > akpm@linux-foundation.org wrote:
> >> From: Robert Hancock <hancockr@shaw.ca>
> >>
> >> Reading from the ATA shadow registers while we are in ADMA mode may
> >> cause undefined behavior.  Don't read the ATA status register when
> >> completing commands for this reason, it shouldn't be needed as the
> >> controller will notify us if the command failed.  Also, don't allow
> >> commands with result taskfile requested to execute in ADMA mode, since
> >> that requires accessing the shadow registers.  We also still need to
> >> override tf_read since libata
> >> will read the result taskfile on a command failure, and we need to go
> >> into
> >> port register mode before allowing this.
> >>
> >> Signed-off-by: Robert Hancock <hancockr@shaw.ca>
> >> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> >> ---
> >>
> >>  drivers/ata/sata_nv.c |   33 ++++++++++++++++++++-------------
> >>  1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > Robert, do you think this should be pushed into #upstream-fixes
> > (2.6.21-rc)?
> >
> > I lean towards "yes", since it is a needed-by-hardware fix, but I also
> > am interested in testing feedback since it is so late in the 2.6.21-rc
> > game.
>
> I would lean toward that as well, but it would be good to get some
> testing from some sata_nv ADMA users to make sure it doesn't do anything
> funny for them..

Since I've been a bit of a problem case this time, I'd be happy to test it.

Can I assume that I can apply the patch you sent to Jeff "[PATCH] sata_nv: 
revert use of notifiers for now", and apply this one, to -rc3, and then be 
able to usefully test?

-- 
Cheers,
Alistair.

Final year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: [patch 29/30] sata_nv: don't read shadow registers when in ADMA mode

[Robert Hancock]

Alistair John Strachan wrote:
>>> I lean towards "yes", since it is a needed-by-hardware fix, but I also
>>> am interested in testing feedback since it is so late in the 2.6.21-rc
>>> game.
>> I would lean toward that as well, but it would be good to get some
>> testing from some sata_nv ADMA users to make sure it doesn't do anything
>> funny for them..
> 
> Since I've been a bit of a problem case this time, I'd be happy to test it.
> 
> Can I assume that I can apply the patch you sent to Jeff "[PATCH] sata_nv: 
> revert use of notifiers for now", and apply this one, to -rc3, and then be 
> able to usefully test?

Yes, you should be able to.

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/