netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* sky2: Correct mistakenly switched read/write sequence
@ 2012-12-01 12:42 Lino Sanfilippo
  2012-12-01 17:27 ` David Miller
  2012-12-01 18:36 ` Stephen Hemminger
  0 siblings, 2 replies; 4+ messages in thread
From: Lino Sanfilippo @ 2012-12-01 12:42 UTC (permalink / raw)
  To: shemminger, mlindner; +Cc: davem, netdev

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: sky2: Correct mistakenly switched read/write sequence
  2012-12-01 12:42 sky2: Correct mistakenly switched read/write sequence Lino Sanfilippo
@ 2012-12-01 17:27 ` David Miller
  2012-12-01 20:06   ` Lino Sanfilippo
  2012-12-01 18:36 ` Stephen Hemminger
  1 sibling, 1 reply; 4+ messages in thread
From: David Miller @ 2012-12-01 17:27 UTC (permalink / raw)
  To: LinoSanfilippo; +Cc: shemminger, mlindner, netdev

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>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: sky2: Correct mistakenly switched read/write sequence
  2012-12-01 12:42 sky2: Correct mistakenly switched read/write sequence Lino Sanfilippo
  2012-12-01 17:27 ` David Miller
@ 2012-12-01 18:36 ` Stephen Hemminger
  1 sibling, 0 replies; 4+ messages in thread
From: Stephen Hemminger @ 2012-12-01 18:36 UTC (permalink / raw)
  To: Lino Sanfilippo; +Cc: mlindner, davem, netdev

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: sky2: Correct mistakenly switched read/write sequence
  2012-12-01 17:27 ` David Miller
@ 2012-12-01 20:06   ` Lino Sanfilippo
  0 siblings, 0 replies; 4+ messages in thread
From: Lino Sanfilippo @ 2012-12-01 20:06 UTC (permalink / raw)
  To: David Miller; +Cc: LinoSanfilippo, shemminger, mlindner, netdev

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2012-12-01 20:06 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-01 12:42 sky2: Correct mistakenly switched read/write sequence Lino Sanfilippo
2012-12-01 17:27 ` David Miller
2012-12-01 20:06   ` Lino Sanfilippo
2012-12-01 18:36 ` Stephen Hemminger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).