Patchwork [4/5] libata/sff: Use ops->bmdma_stop instead of ata_bmdma_stop()

login
register
mail settings
Submitter Benjamin Herrenschmidt
Date Dec. 1, 2009, 7:08 a.m.
Message ID <20091201070833.CC084B7BD9@ozlabs.org>
Download mbox | patch
Permalink /patch/39887/
State Accepted
Commit f0353813afe784330622596ff141e7525ccd99de
Headers show

Comments

Benjamin Herrenschmidt - Dec. 1, 2009, 7:08 a.m.
In libata-sff, ata_sff_post_internal_cmd() directly calls ata_bmdma_stop()
instead of ap->ops->bmdma_stop(). This can be a problem for controllers
that use their own bmdma_stop for which the generic sff one isn't suitable

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

 drivers/ata/libata-sff.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Tejun Heo - Dec. 1, 2009, 7:25 a.m.
On 12/01/2009 04:08 PM, Benjamin Herrenschmidt wrote:
> In libata-sff, ata_sff_post_internal_cmd() directly calls ata_bmdma_stop()
> instead of ap->ops->bmdma_stop(). This can be a problem for controllers
> that use their own bmdma_stop for which the generic sff one isn't suitable
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Oh... that's a scary bug lurking around.  Thanks for catching it.

Acked-by: Tejun Heo <tj@kernel.org>
Benjamin Herrenschmidt - Dec. 1, 2009, 7:29 a.m.
On Tue, 2009-12-01 at 16:25 +0900, Tejun Heo wrote:
> On 12/01/2009 04:08 PM, Benjamin Herrenschmidt wrote:
> > In libata-sff, ata_sff_post_internal_cmd() directly calls ata_bmdma_stop()
> > instead of ap->ops->bmdma_stop(). This can be a problem for controllers
> > that use their own bmdma_stop for which the generic sff one isn't suitable
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Oh... that's a scary bug lurking around.  Thanks for catching it.
> 
> Acked-by: Tejun Heo <tj@kernel.org>

Feel free to pick that one up earlier if you want (ie for 2.6.32), as
long as we manage to get it in in 2.6.33 -before- I push powerpc-next to
Linus, I'm happy :-) (or we can have it in both trees).

Cheers,
Ben.
Tejun Heo - Dec. 1, 2009, 7:33 a.m.
On 12/01/2009 04:29 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2009-12-01 at 16:25 +0900, Tejun Heo wrote:
>> On 12/01/2009 04:08 PM, Benjamin Herrenschmidt wrote:
>>> In libata-sff, ata_sff_post_internal_cmd() directly calls ata_bmdma_stop()
>>> instead of ap->ops->bmdma_stop(). This can be a problem for controllers
>>> that use their own bmdma_stop for which the generic sff one isn't suitable
>>>
>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>
>> Oh... that's a scary bug lurking around.  Thanks for catching it.
>>
>> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Feel free to pick that one up earlier if you want (ie for 2.6.32), as
> long as we manage to get it in in 2.6.33 -before- I push powerpc-next to
> Linus, I'm happy :-) (or we can have it in both trees).

As nasty as the bug might be, given that we haven't had too much
problem with that, I think it would be best to give it some time to be
tested before releasing it to users.  It causes behavior differences
for all bmdma drivers which implement custom bmdma_stop.  It's an
apparent bug fix but well the nasty ones are always apparent bug
fixes, right?  :-)

Thanks.
Benjamin Herrenschmidt - Dec. 1, 2009, 7:57 a.m.
On Tue, 2009-12-01 at 16:33 +0900, Tejun Heo wrote:
> > Feel free to pick that one up earlier if you want (ie for 2.6.32),
> as
> > long as we manage to get it in in 2.6.33 -before- I push
> powerpc-next to
> > Linus, I'm happy :-) (or we can have it in both trees).
> 
> As nasty as the bug might be, given that we haven't had too much
> problem with that, I think it would be best to give it some time to be
> tested before releasing it to users.  It causes behavior differences
> for all bmdma drivers which implement custom bmdma_stop.  It's an
> apparent bug fix but well the nasty ones are always apparent bug
> fixes, right?  :-) 

Yeah, I agree. I wouldn't be comfortable pushing it into .32 to be
honest after a quick grep of how many drivers actually use that hook and
considering that no problem was reported so far...

I'll merge it with the rest of the powerpc stuff during the .33 merge
window if you are ok with that.

Cheers,
Ben.

Patch

--- linux-work.orig/drivers/ata/libata-sff.c	2009-12-01 17:48:27.000000000 +1100
+++ linux-work/drivers/ata/libata-sff.c	2009-12-01 17:48:48.000000000 +1100
@@ -2384,7 +2384,7 @@  void ata_sff_post_internal_cmd(struct at
 	ap->hsm_task_state = HSM_ST_IDLE;
 
 	if (ap->ioaddr.bmdma_addr)
-		ata_bmdma_stop(qc);
+		ap->ops->bmdma_stop(qc);
 
 	spin_unlock_irqrestore(ap->lock, flags);
 }