sky2: Correct mistakenly switched read/write sequence

sky2: Correct mistakenly switched read/write sequence

[Lino Sanfilippo]

In sky2_all_down() the order of the read()/write() access to B0_IMSK seems to
be mistakenly switched. The original intention was obviously to avoid PCI write
posting.
This patch fixes the order.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/net/ethernet/marvell/sky2.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index 78946fe..b20d2fd 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -3481,8 +3481,8 @@ static void sky2_all_down(struct sky2_hw *hw)
 	int i;
 
 	if (hw->flags & SKY2_HW_IRQ_SETUP) {
-		sky2_read32(hw, B0_IMSK);
 		sky2_write32(hw, B0_IMSK, 0);
+		sky2_read32(hw, B0_IMSK);
 
 		synchronize_irq(hw->pdev->irq);
 		napi_disable(&hw->napi);
-- 
1.7.9.5

Re: sky2: Correct mistakenly switched read/write sequence

[David Miller]

From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Date: Sat, 1 Dec 2012 13:42:39 +0100

> In sky2_all_down() the order of the read()/write() access to B0_IMSK seems to
> be mistakenly switched. The original intention was obviously to avoid PCI write
> posting.
> This patch fixes the order.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

I would say that no such intention exists at all.

The read is there because a long time ago the result as used
to compute an 'imask' variable.

Please see commit:

commit d72ff8fa7f8b344382963721f842256825c4660b
Author: Mike McCormack <mikem@ring3k.org>
Date:   Thu May 13 06:12:51 2010 +0000

    sky2: Refactor down/up code out of sky2_restart()
    
    Code to bring down all sky2 interfaces and bring it up
    again can be reused in sky2_suspend and sky2_resume.
    
    Factor the code to bring the interfaces down into
    sky2_all_down and the up code into sky2_all_up.
    
    Signed-off-by: Mike McCormack <mikem@ring3k.org>
    Acked-by: Stephen Hemminger <shemminger@vyatta.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Re: sky2: Correct mistakenly switched read/write sequence

[Lino Sanfilippo]

On Sat, Dec 01, 2012 at 12:27:01PM -0500, David Miller wrote:
> 
> > In sky2_all_down() the order of the read()/write() access to B0_IMSK seems to
> > be mistakenly switched. The original intention was obviously to avoid PCI write
> > posting.
> > This patch fixes the order.
> > 
> > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> 
> I would say that no such intention exists at all.
> 
> The read is there because a long time ago the result as used
> to compute an 'imask' variable.
> 
> Please see commit:
> 
> commit d72ff8fa7f8b344382963721f842256825c4660b

I see. Indeed I was not aware of the history that led to the recent code, so
thanks for pointing that out.

However, since Stephen thinks the patch is useful nevertheless:
@Stephen please feel free to adjust the commit message as you think would make 
the most sense.

Regards,
Lino

Re: sky2: Correct mistakenly switched read/write sequence

[Stephen Hemminger]

On Sat, 1 Dec 2012 13:42:39 +0100
Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote:

> In sky2_all_down() the order of the read()/write() access to B0_IMSK seems to
> be mistakenly switched. The original intention was obviously to avoid PCI write
> posting.
> This patch fixes the order.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

You are both right. David is correct in that the original code here
was quite different and was doing save/restore of irq mask.
That changed as driver evolved to only bring up IRQ if device was
brought up. At which point the read of irq mask was a left over call.

Lino is correct, it makes sense to do read after write to ensure PCI posting.
So I agree with the patch, but would like the commit message changed
slightly.