| Message ID | 1270202304.1989.14.camel@edumazet-laptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Fri, 02 Apr 2010 11:58:24 +0200 > [PATCH net-next-2.6] net: illegal_highdma() fix > > Followup to commit 5acbbd428db47b12f137a8a2aa96b3c0a96b744e > (net: change illegal_highdma to use dma_mask) > > If dev->dev.parent is NULL, we should not try to dereference it. > > Dont force inline illegal_highdma() as its pretty big now. > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Applied, thanks for tracking this down. -- 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 Fri, 02 Apr 2010 11:58:24 +0200 Eric Dumazet <eric.dumazet@gmail.com> wrote: > diff --git a/net/core/dev.c b/net/core/dev.c > index e19cdae..c6b5206 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -1801,7 +1801,7 @@ EXPORT_SYMBOL(netdev_rx_csum_fault); > * 2. No high memory really exists on this machine. > */ > > -static inline int illegal_highdma(struct net_device *dev, struct sk_buff *skb) > +static int illegal_highdma(struct net_device *dev, struct sk_buff *skb) > { > #ifdef CONFIG_HIGHMEM > int i; > @@ -1814,6 +1814,8 @@ static inline int illegal_highdma(struct net_device *dev, struct sk_buff *skb) > if (PCI_DMA_BUS_IS_PHYS) { > struct device *pdev = dev->dev.parent; > > + if (!pdev) > + return 0; Sorry about that and thanks for the fix. I think, if pdev is null, returning 1 here is safer since the device doesn't set up dma info properly. Do you know what device hits this bug? You said that you use bnx2 and tg3. Both call SET_NETDEV_DEV with pdev->dev. I tested bnx2 and seems that netdev->dev.parent is set up correctly. -- 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
Le dimanche 04 avril 2010 à 18:16 +0900, FUJITA Tomonori a écrit : > > + return 0; > > Sorry about that and thanks for the fix. > > I think, if pdev is null, returning 1 here is safer since the device > doesn't set up dma info properly. > > Do you know what device hits this bug? You said that you use bnx2 and > tg3. Both call SET_NETDEV_DEV with pdev->dev. I tested bnx2 and seems > that netdev->dev.parent is set up correctly. > -- Might be because of my setup, I suspect I had two reasons to hit the bug : A bonding of eth2 (bnx2) and eth3 (tg3) Then vlans on top of this bond0 When first dev_queue_xmit() was called, it was for a virtual device :) # ip link 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 qdisc noqueue state UNKNOWN link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000 link/ether 00:1e:0b:ec:d3:dc brd ff:ff:ff:ff:ff:ff 3: eth1: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc mq master bond0 state UP qlen 1000 link/ether 00:1e:0b:ec:d3:d2 brd ff:ff:ff:ff:ff:ff 4: eth2: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc hfsc master bond0 state UP qlen 1000 link/ether 00:1e:0b:ec:d3:d2 brd ff:ff:ff:ff:ff:ff 5: eth3: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000 link/ether 00:1e:0b:92:78:51 brd ff:ff:ff:ff:ff:ff 6: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP link/ether 00:1e:0b:ec:d3:d2 brd ff:ff:ff:ff:ff:ff 7: vlan.103@bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 100 link/ether 00:1e:0b:ec:d3:d2 brd ff:ff:ff:ff:ff:ff 8: vlan.825@bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000 link/ether 00:1e:0b:ec:d3:d2 brd ff:ff:ff:ff:ff:ff -- 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 Sun, 04 Apr 2010 11:29:55 +0200 Eric Dumazet <eric.dumazet@gmail.com> wrote: > > I think, if pdev is null, returning 1 here is safer since the device > > doesn't set up dma info properly. > > > > Do you know what device hits this bug? You said that you use bnx2 and > > tg3. Both call SET_NETDEV_DEV with pdev->dev. I tested bnx2 and seems > > that netdev->dev.parent is set up correctly. > > -- > > Might be because of my setup, I suspect I had two reasons to hit the > bug : > > A bonding of eth2 (bnx2) and eth3 (tg3) > > Then vlans on top of this bond0 > > When first dev_queue_xmit() was called, it was for a virtual device :) Thanks! So it's due to bond or vlan (or both). I guess that returning zero here with a null pdev is fine. If we return 1, probably some people would complain about performance regression. Like the block layer does, coping the dma restriction info from the lower devices can solve this problem but I guess that it's over engineering. My original patch doesn't loosen the DMA restriction checking so returning zero shouldn't break anything. If when we fix the usage of NETIF_F_HIGHDMA in each driver and also check the usage of netdev->dev.parent, everything should be fine. -- 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/net/core/dev.c b/net/core/dev.c index e19cdae..c6b5206 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1801,7 +1801,7 @@ EXPORT_SYMBOL(netdev_rx_csum_fault); * 2. No high memory really exists on this machine. */ -static inline int illegal_highdma(struct net_device *dev, struct sk_buff *skb) +static int illegal_highdma(struct net_device *dev, struct sk_buff *skb) { #ifdef CONFIG_HIGHMEM int i; @@ -1814,6 +1814,8 @@ static inline int illegal_highdma(struct net_device *dev, struct sk_buff *skb) if (PCI_DMA_BUS_IS_PHYS) { struct device *pdev = dev->dev.parent; + if (!pdev) + return 0; for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { dma_addr_t addr = page_to_phys(skb_shinfo(skb)->frags[i].page); if (!pdev->dma_mask || addr + PAGE_SIZE - 1 > *pdev->dma_mask)