Re: [PATCHv3] 3c59x: fix missing dma_mapping_error check and bad ring refill logic

2018-01-22 Thread Neil Horman 
On Mon, Jan 22, 2018 at 01:27:19AM -0500, tedheadster wrote:
> On Wed, Jan 3, 2018 at 1:44 PM, David Miller  wrote:
> > From: Neil Horman 
> > Date: Wed,  3 Jan 2018 13:09:23 -0500
> >
> >> A few spots in 3c59x missed calls to dma_mapping_error checks, casuing
> >> WARN_ONS to trigger.  Clean those up.  While we're at it, refactor the
> >> refill code a bit so that if skb allocation or dma mapping fails, we
> >> recycle the existing buffer.  This prevents holes in the rx ring, and
> >> makes for much simpler logic
> >>
> >> Note: This is compile only tested.  Ted, if you could run this and
> >> confirm that it continues to work properly, I would appreciate it, as I
> >> currently don't have access to this hardware
> >>
> 
> Neil,
>   I was able to test this patch. I did not get any WARN_ON messages.
> However, I am getting a lot of dropped receive packets; uptime is 11
> minutes and it has already dropped 214 of 743 receive packets.
> 
> Admittedly this is on a slow i486 regression testing system, but the
> drop rate is approximately 30% which seems high even for this system
> because it is on a very quiet switched network.
> 
> I enabled some debugging messages by setting msglvl to 4 and
> recompiling with DYNAMIC_DEBUG=y. I did not see any messages of the
> form "No memory to allocate a sk_buff of size" so that leaves the
> following two cases:
> 
> boomerang_rx()
> ...
> newskb = netdev_alloc_skb_ip_align(dev, PKT_BUF_SZ);
> if (!newskb) {
>   dev->stats.rx_dropped++;
>   goto clear_complete;
>   }
>   newdma = pci_map_single(VORTEX_PCI(vp), newskb->data,
> PKT_BUF_SZ, 
> PCI_DMA_FROMDEVICE)
>   if (dma_mapping_error(_PCI(vp)->dev, newdma)) {
> dev->stats.rx_dropped++;
> consume_skb(newskb);
> goto clear_complete;
>   }
> 
> What shall we do to determine if it is hitting the pci_map_single() or
> netdev_alloc_skb_ip_align() failure?
> 
> - Matthew
> 

Well, I would suggest that you either modify and rebuild the kernel to add a
printk in both of those locations or (if you don't want to go to all that
trouble), write a systemtap script to probe both of those locations and print a
warning if those paths are executed.

That said, while I understand its good for your understanding of the problem,
knowing which cse you hit likely won't help you fix much.  Depending on which
path you hit, it means your either low on ram from which to allocate skbs, or
your out of space in your iommu (i'm guessing your using a software iotlb lib).
Both are pretty limited resources on the system you describe.

Neil

Re: [PATCHv3] 3c59x: fix missing dma_mapping_error check and bad ring refill logic

2018-01-21 Thread tedheadster 
On Wed, Jan 3, 2018 at 1:44 PM, David Miller  wrote:
> From: Neil Horman 
> Date: Wed,  3 Jan 2018 13:09:23 -0500
>
>> A few spots in 3c59x missed calls to dma_mapping_error checks, casuing
>> WARN_ONS to trigger.  Clean those up.  While we're at it, refactor the
>> refill code a bit so that if skb allocation or dma mapping fails, we
>> recycle the existing buffer.  This prevents holes in the rx ring, and
>> makes for much simpler logic
>>
>> Note: This is compile only tested.  Ted, if you could run this and
>> confirm that it continues to work properly, I would appreciate it, as I
>> currently don't have access to this hardware
>>

Neil,
  I was able to test this patch. I did not get any WARN_ON messages.
However, I am getting a lot of dropped receive packets; uptime is 11
minutes and it has already dropped 214 of 743 receive packets.

Admittedly this is on a slow i486 regression testing system, but the
drop rate is approximately 30% which seems high even for this system
because it is on a very quiet switched network.

I enabled some debugging messages by setting msglvl to 4 and
recompiling with DYNAMIC_DEBUG=y. I did not see any messages of the
form "No memory to allocate a sk_buff of size" so that leaves the
following two cases:

boomerang_rx()
...
newskb = netdev_alloc_skb_ip_align(dev, PKT_BUF_SZ);
if (!newskb) {
  dev->stats.rx_dropped++;
  goto clear_complete;
  }
  newdma = pci_map_single(VORTEX_PCI(vp), newskb->data,
PKT_BUF_SZ, PCI_DMA_FROMDEVICE)
  if (dma_mapping_error(_PCI(vp)->dev, newdma)) {
dev->stats.rx_dropped++;
consume_skb(newskb);
goto clear_complete;
  }

What shall we do to determine if it is hitting the pci_map_single() or
netdev_alloc_skb_ip_align() failure?

- Matthew

Re: [PATCHv3] 3c59x: fix missing dma_mapping_error check and bad ring refill logic

2018-01-03 Thread David Miller 
From: Neil Horman 
Date: Wed,  3 Jan 2018 13:09:23 -0500

> A few spots in 3c59x missed calls to dma_mapping_error checks, casuing
> WARN_ONS to trigger.  Clean those up.  While we're at it, refactor the
> refill code a bit so that if skb allocation or dma mapping fails, we
> recycle the existing buffer.  This prevents holes in the rx ring, and
> makes for much simpler logic
> 
> Note: This is compile only tested.  Ted, if you could run this and
> confirm that it continues to work properly, I would appreciate it, as I
> currently don't have access to this hardware
> 
> Signed-off-by: Neil Horman 
> CC: Steffen Klassert 
> CC: "David S. Miller" 
> Reported-by: tedheads...@gmail.com
> 
> ---
> Change notes:
> 
> v2)
> * Fixed tx path to free skb on mapping error
> * Refactored rx path to recycle skbs on allocation/mapping error
> * Used refactoring to remove oom timer and dirty_rx index
> 
> v3)
> * Removed unused variable that was causing a warning

Applied.

[PATCHv3] 3c59x: fix missing dma_mapping_error check and bad ring refill logic

2018-01-03 Thread Neil Horman 
A few spots in 3c59x missed calls to dma_mapping_error checks, casuing
WARN_ONS to trigger.  Clean those up.  While we're at it, refactor the
refill code a bit so that if skb allocation or dma mapping fails, we
recycle the existing buffer.  This prevents holes in the rx ring, and
makes for much simpler logic

Note: This is compile only tested.  Ted, if you could run this and
confirm that it continues to work properly, I would appreciate it, as I
currently don't have access to this hardware

Signed-off-by: Neil Horman 
CC: Steffen Klassert 
CC: "David S. Miller" 
Reported-by: tedheads...@gmail.com

---
Change notes:

v2)
* Fixed tx path to free skb on mapping error
* Refactored rx path to recycle skbs on allocation/mapping error
* Used refactoring to remove oom timer and dirty_rx index

v3)
* Removed unused variable that was causing a warning
---
 drivers/net/ethernet/3com/3c59x.c | 90 +--
 1 file changed, 38 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/3com/3c59x.c 
b/drivers/net/ethernet/3com/3c59x.c
index f4e13a7014bd..36c8950dbd2d 100644
--- a/drivers/net/ethernet/3com/3c59x.c
+++ b/drivers/net/ethernet/3com/3c59x.c
@@ -602,7 +602,7 @@ struct vortex_private {
struct sk_buff* rx_skbuff[RX_RING_SIZE];
struct sk_buff* tx_skbuff[TX_RING_SIZE];
unsigned int cur_rx, cur_tx;/* The next free ring entry */
-   unsigned int dirty_rx, dirty_tx;/* The ring entries to be 
free()ed. */
+   unsigned int dirty_tx;  /* The ring entries to be free()ed. */
struct vortex_extra_stats xstats;   /* NIC-specific extra stats */
struct sk_buff *tx_skb; /* Packet being eaten 
by bus master ctrl.  */
dma_addr_t tx_skb_dma;  /* Allocated DMA 
address for bus master ctrl DMA.   */
@@ -618,7 +618,6 @@ struct vortex_private {
 
/* The remainder are related to chip state, mostly media selection. */
struct timer_list timer;/* Media selection 
timer. */
-   struct timer_list rx_oom_timer; /* Rx skb allocation retry 
timer */
int options;/* 
User-settable misc. driver options. */
unsigned int media_override:4,  /* Passed-in media type. */
default_media:4,/* Read from 
the EEPROM/Wn3_Config. */
@@ -760,7 +759,6 @@ static void mdio_sync(struct vortex_private *vp, int bits);
 static int mdio_read(struct net_device *dev, int phy_id, int location);
 static void mdio_write(struct net_device *vp, int phy_id, int location, int 
value);
 static void vortex_timer(struct timer_list *t);
-static void rx_oom_timer(struct timer_list *t);
 static netdev_tx_t vortex_start_xmit(struct sk_buff *skb,
 struct net_device *dev);
 static netdev_tx_t boomerang_start_xmit(struct sk_buff *skb,
@@ -1601,7 +1599,6 @@ vortex_up(struct net_device *dev)
 
timer_setup(>timer, vortex_timer, 0);
mod_timer(>timer, RUN_AT(media_tbl[dev->if_port].wait));
-   timer_setup(>rx_oom_timer, rx_oom_timer, 0);
 
if (vortex_debug > 1)
pr_debug("%s: Initial media type %s.\n",
@@ -1676,7 +1673,7 @@ vortex_up(struct net_device *dev)
window_write16(vp, 0x0040, 4, Wn4_NetDiag);
 
if (vp->full_bus_master_rx) { /* Boomerang bus master. */
-   vp->cur_rx = vp->dirty_rx = 0;
+   vp->cur_rx = 0;
/* Initialize the RxEarly register as recommended. */
iowrite16(SetRxThreshold + (1536>>2), ioaddr + EL3_CMD);
iowrite32(0x0020, ioaddr + PktStatus);
@@ -1729,6 +1726,7 @@ vortex_open(struct net_device *dev)
struct vortex_private *vp = netdev_priv(dev);
int i;
int retval;
+   dma_addr_t dma;
 
/* Use the now-standard shared IRQ implementation. */
if ((retval = request_irq(dev->irq, vp->full_bus_master_rx ?
@@ -1753,7 +1751,11 @@ vortex_open(struct net_device *dev)
break;  /* Bad news!  */
 
skb_reserve(skb, NET_IP_ALIGN); /* Align IP on 16 byte 
boundaries */
-   vp->rx_ring[i].addr = 
cpu_to_le32(pci_map_single(VORTEX_PCI(vp), skb->data, PKT_BUF_SZ, 
PCI_DMA_FROMDEVICE));
+   dma = pci_map_single(VORTEX_PCI(vp), skb->data,
+PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
+   if (dma_mapping_error(_PCI(vp)->dev, dma))
+   break;
+   vp->rx_ring[i].addr = cpu_to_le32(dma);
}
if (i != RX_RING_SIZE) {
pr_emerg("%s: no memory for rx ring\n", dev->name);
@@ -2067,6 +2069,12 @@ vortex_start_xmit(struct sk_buff *skb,