Message ID | m3tyzytun6.fsf@intrepid.localdomain |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
----- "Krzysztof Halasa" <khc@pm.waw.pl> wrote: > David Miller <davem@davemloft.net> writes: > > > I think going down the road of trying to use the flexible mode is a > > dead end. I doubt any other OS driver is using it, and that means > > that there are likely many other errata hiding in the bushes which > you > > will unearth by trying to use this new descriptor mode. And it > won't > > show up when you test it, it will show up when some random person > in > > some remote data center somewhere updates their kernel, and they > won't > > send us a bug report, they'll replace their card or downgrade their > > kernel instead. > > Well, I'm afraid it's a possible scenario. I won't touch the flexible > mode unless Intel folks tell me it's safe. > OTOH it seems it was used by less common software, at least in the > 82557-9 times. Not sure about Windows driver. > (It seems the simplified mode was meant for Linux-alike "1 buffer per > packet" approach while the flexible mode was to support systems using > mbuf-like structures). > > > Just make the driver use consistent buffers for RX, and when > packets > > arrive an SKB is allocated and the packet data is copied into the > SKB > >>From the consistent buffer. > > That would be a performance hit, wouldn't it? Especially in older > machines, where e100 was typically installed. I think it should be > the > last resort. > > > And for 2.6.31-rcX we probably have to simply revert your change. > > Unfortunately it seems that reverting will not fix operation > completely > on a system with swiotlb (checking the descriptor status isn't the > only > racy operation which depends on the cache behaviour, though it's the > most frequent). And it will break all non-coherent archs again, they > will either need to use the patch or still stick to (already removed) > eepro100.c > > I looked at the eepro100.c sources. It uses BIDIR mapping the same > way > as e100.c does, but then syncs using FROM_DEVICE/TO_DEVICE instead of > e100's always-BIDIR. I think the same in e100.c would work on all > platforms (though still violating the DMA API a bit). Perhaps we > should > do that instead? > > Walt, can you check if 2.6.30.5 with the following patch applied > still > breaks e100 with the 6 GB of RAM, please? TIA. > (This patch makes swiotlb aware that the CPU didn't alter the buffer, > though I don't know if swiotlb will use this info). > > diff --git a/drivers/net/e100.c b/drivers/net/e100.c > index 014dfb6..53e8252 100644 > --- a/drivers/net/e100.c > +++ b/drivers/net/e100.c > @@ -1764,7 +1764,7 @@ static int e100_rx_indicate(struct nic *nic, > struct rx *rx, > nic->ru_running = RU_SUSPENDED; > pci_dma_sync_single_for_device(nic->pdev, rx->dma_addr, > sizeof(struct rfd), > - PCI_DMA_BIDIRECTIONAL); > + PCI_DMA_FROMDEVICE); > return -ENODATA; > } > > -- > Krzysztof Halasa Krzystof, I've tested this under 2.6.31-rc7 (which was also broken to begin with) and this appears to have fixed it. It's a fairly limited test at this point, as I've just been downloading a large file for the past 15 mins. or so, but this was normally enough to have multiple stoppages. Do you still need 2.6.30.5 tested as well? -Walt -- 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/e100.c b/drivers/net/e100.c index 014dfb6..53e8252 100644 --- a/drivers/net/e100.c +++ b/drivers/net/e100.c @@ -1764,7 +1764,7 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx, nic->ru_running = RU_SUSPENDED; pci_dma_sync_single_for_device(nic->pdev, rx->dma_addr, sizeof(struct rfd), - PCI_DMA_BIDIRECTIONAL); + PCI_DMA_FROMDEVICE); return -ENODATA; }