* 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).