Date:	Sun, 28 Jul 2013 21:22:52 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Neil Horman <nhorman@...driver.com>
CC:	Eric Dumazet <eric.dumazet@...il.com>,
	David Miller <davem@...emloft.net>,
	<luis.henriques@...onical.com>, <netdev@...r.kernel.org>,
	<jcliburn@...il.com>, <stable@...r.kernel.org>
Subject: Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling
 rx ring

On Sun, 2013-07-28 at 14:53 -0400, Neil Horman wrote:
> On Sun, Jul 28, 2013 at 09:15:54AM -0700, Eric Dumazet wrote:
> > On Sun, 2013-07-28 at 06:44 -0400, Neil Horman wrote:
> > > On Sat, Jul 27, 2013 at 08:02:05PM -0700, David Miller wrote:
> > > > From: Eric Dumazet <eric.dumazet@...il.com>
> > > > Date: Sat, 27 Jul 2013 16:59:43 -0700
> > > > 
> > > > > If a hardware needs frame being in a single 4K page, its driver must do
> > > > > its own allocation, or add appropriate aligning logic.
> > > > 
> > > > Whatever the cause the original patch in this thread is not the
> > > > correct fix and I've thus reverted it.
> > > > 
> > > 
> > > So what exactly is the consensus here?  We can certainly modify the patch to
> > > ensure allocation on a 4k boundary, and within a unique 4k page, but thats just
> > > a guess at the problem, and the Qualcomm people have been silent on the issue
> > > for weeks now.
> > > 
> > 
> > Thats the guess yes, but since MTU can be more than 4096 bytes, it could
> > be something else, some kind of DMA problem.

Oh well, there goes that theory.

> Right, thats exactly my concern, its a guess, one which infers problems of its
> own.  I'll certainly code up a patch to test, sure, given what we've discussed
> here, but I'm not sure it leaves us in any better position than we were in with
> my origional patch.
> 
> > I wonder why rx_buffer_len includes VLAN_HLEN as this nic provides
> > accelerated vlan.
> > 
> I expect its an overestimation of sizing requirements in the rx ring.  The
> set_features method for the driver doesn't re-allocate the rx ring buffers if
> the vlan tag accleration feature is disabled, so I suspect that they keep that
> extra space in place in the event that its ever turned off.
> 
> > drivers/net/ethernet/atheros/atl1c/atl1c_main.c uses :
> >  hw->dmar_block = atl1c_dma_req_1024;
> > 
> > while drivers/net/ethernet/atheros/atlx/atl1.c uses :
> >  hw->dmar_block = atl1_dma_req_256;
> > 
> Another question for the Atheros people I suppose.  Although, atl1c takes this
> value through several other contortions in atl1c_configure_tx, in which it seems
> to allow the value to be either 2, or 3 (capped by the min_t call, and floored
> by the check against DEVICE_CTRL_MAXRRS_MIN).  By all rights the setting in the
> atlx driver would be considered invalid in the atl1c driver (if such
> comparisons can be drawn).
[...]

The value programmed as the Max Read Request Size should be irrelevant
to memory writes (i.e. RX DMA).  Memory write transactions must be
limited by the Max Payload Size and must not cross 4K boundaries.  I
would expect PCIe devices to split linear DMA transfers into suitably
sized transactions automatically, modulo bugs of course.

Since we know lengths > 4K work, perhaps it would be worth testing with
the fragment cache size reduced to 16K?  The driver would never
previously have used RX buffers crossing 16K boundaries, except if SLOB
was used (and that's an unlikely combination).

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists