| Message ID | 20121201124239.GB3914@neptun |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)