| Message ID | 20200819065555.1802761-23-hch@lst.de |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Tue, Sep 01, 2020 at 05:22:09PM +0200, Thomas Bogendoerfer wrote: > On Wed, Aug 19, 2020 at 08:55:49AM +0200, Christoph Hellwig wrote: > > Use the proper modern API to transfer cache ownership for incoherent DMA. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > drivers/net/ethernet/seeq/sgiseeq.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/seeq/sgiseeq.c b/drivers/net/ethernet/seeq/sgiseeq.c > > index 39599bbb5d45b6..f91dae16d69a19 100644 > > --- a/drivers/net/ethernet/seeq/sgiseeq.c > > +++ b/drivers/net/ethernet/seeq/sgiseeq.c > > @@ -112,14 +112,18 @@ struct sgiseeq_private { > > > > static inline void dma_sync_desc_cpu(struct net_device *dev, void *addr) > > { > > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > > - DMA_FROM_DEVICE); > > + struct sgiseeq_private *sp = netdev_priv(dev); > > + > > + dma_sync_single_for_cpu(dev->dev.parent, VIRT_TO_DMA(sp, addr), > > + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); > > } > > > > static inline void dma_sync_desc_dev(struct net_device *dev, void *addr) > > { > > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > > - DMA_TO_DEVICE); > > + struct sgiseeq_private *sp = netdev_priv(dev); > > + > > + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), > > + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); > > } > > this breaks ethernet on IP22 completely, but I haven't figured out why, yet. the problem is that dma_sync_single_for_cpu() doesn't flush anything for IP22, because it only flushes for CPUs which do speculation. So either MIPS arch_sync_dma_for_cpu() should always flush or sgiseeq needs to use a different sync funktion, when it wants to re-read descriptors from memory. Thomas.
On Tue, Sep 01, 2020 at 07:12:41PM +0200, Thomas Bogendoerfer wrote: > On Tue, Sep 01, 2020 at 05:22:09PM +0200, Thomas Bogendoerfer wrote: > > On Wed, Aug 19, 2020 at 08:55:49AM +0200, Christoph Hellwig wrote: > > > Use the proper modern API to transfer cache ownership for incoherent DMA. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > --- > > > drivers/net/ethernet/seeq/sgiseeq.c | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/seeq/sgiseeq.c b/drivers/net/ethernet/seeq/sgiseeq.c > > > index 39599bbb5d45b6..f91dae16d69a19 100644 > > > --- a/drivers/net/ethernet/seeq/sgiseeq.c > > > +++ b/drivers/net/ethernet/seeq/sgiseeq.c > > > @@ -112,14 +112,18 @@ struct sgiseeq_private { > > > > > > static inline void dma_sync_desc_cpu(struct net_device *dev, void *addr) > > > { > > > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > > > - DMA_FROM_DEVICE); > > > + struct sgiseeq_private *sp = netdev_priv(dev); > > > + > > > + dma_sync_single_for_cpu(dev->dev.parent, VIRT_TO_DMA(sp, addr), > > > + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); > > > } > > > > > > static inline void dma_sync_desc_dev(struct net_device *dev, void *addr) > > > { > > > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > > > - DMA_TO_DEVICE); > > > + struct sgiseeq_private *sp = netdev_priv(dev); > > > + > > > + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), > > > + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); > > > } > > > > this breaks ethernet on IP22 completely, but I haven't figured out why, yet. > > the problem is that dma_sync_single_for_cpu() doesn't flush anything > for IP22, because it only flushes for CPUs which do speculation. So > either MIPS arch_sync_dma_for_cpu() should always flush or sgiseeq > needs to use a different sync funktion, when it wants to re-read descriptors > from memory. Well, if IP22 doesn't speculate (which I'm pretty sure is the case), dma_sync_single_for_cpu should indeeed be a no-op. But then there also shouldn't be anything in the cache, as the previous dma_sync_single_for_device should have invalidated it. So it seems like we are missing one (or more) ownership transfers to the device. I'll try to look at the the ownership management in a little more detail tomorrow.
On Tue, Sep 01, 2020 at 07:38:10PM +0200, Thomas Bogendoerfer wrote: > On Tue, Sep 01, 2020 at 07:16:27PM +0200, Christoph Hellwig wrote: > > Well, if IP22 doesn't speculate (which I'm pretty sure is the case), > > dma_sync_single_for_cpu should indeeed be a no-op. But then there > > also shouldn't be anything in the cache, as the previous > > dma_sync_single_for_device should have invalidated it. So it seems like > > we are missing one (or more) ownership transfers to the device. I'll > > try to look at the the ownership management in a little more detail > > tomorrow. > > this is the problem: > > /* Always check for received packets. */ > sgiseeq_rx(dev, sp, hregs, sregs); > > so the driver will look at the rx descriptor on every interrupt, so > we cache the rx descriptor on the first interrupt and if there was > $no rx packet, we will only see it, if cache line gets flushed for > some other reason. kick_tx() does a busy loop checking tx descriptors, > with just sync_desc_cpu... the patch below fixes the problem. Thomas. diff --git a/drivers/net/ethernet/seeq/sgiseeq.c b/drivers/net/ethernet/seeq/sgiseeq.c index 8507ff242014..876e3700a0e4 100644 --- a/drivers/net/ethernet/seeq/sgiseeq.c +++ b/drivers/net/ethernet/seeq/sgiseeq.c @@ -112,14 +112,18 @@ struct sgiseeq_private { static inline void dma_sync_desc_cpu(struct net_device *dev, void *addr) { - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), - DMA_FROM_DEVICE); + struct sgiseeq_private *sp = netdev_priv(dev); + + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), + sizeof(struct sgiseeq_rx_desc), DMA_FROM_DEVICE); } static inline void dma_sync_desc_dev(struct net_device *dev, void *addr) { - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), - DMA_TO_DEVICE); + struct sgiseeq_private *sp = netdev_priv(dev); + + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), + sizeof(struct sgiseeq_rx_desc), DMA_TO_DEVICE); }
On Tue, Sep 01, 2020 at 07:38:10PM +0200, Thomas Bogendoerfer wrote: > this is the problem: > > /* Always check for received packets. */ > sgiseeq_rx(dev, sp, hregs, sregs); > > so the driver will look at the rx descriptor on every interrupt, so > we cache the rx descriptor on the first interrupt and if there was > $no rx packet, we will only see it, if cache line gets flushed for > some other reason. That means a transfer back to device ownership is missing after a (negative) check. > kick_tx() does a busy loop checking tx descriptors, > with just sync_desc_cpu... > > Thomas. > > -- > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a > good idea. [ RFC1925, 2.3 ] ---end quoted text---
diff --git a/drivers/net/ethernet/seeq/sgiseeq.c b/drivers/net/ethernet/seeq/sgiseeq.c index 39599bbb5d45b6..f91dae16d69a19 100644 --- a/drivers/net/ethernet/seeq/sgiseeq.c +++ b/drivers/net/ethernet/seeq/sgiseeq.c @@ -112,14 +112,18 @@ struct sgiseeq_private { static inline void dma_sync_desc_cpu(struct net_device *dev, void *addr) { - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), - DMA_FROM_DEVICE); + struct sgiseeq_private *sp = netdev_priv(dev); + + dma_sync_single_for_cpu(dev->dev.parent, VIRT_TO_DMA(sp, addr), + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); } static inline void dma_sync_desc_dev(struct net_device *dev, void *addr) { - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), - DMA_TO_DEVICE); + struct sgiseeq_private *sp = netdev_priv(dev); + + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); } static inline void hpc3_eth_reset(struct hpc3_ethregs *hregs)
Use the proper modern API to transfer cache ownership for incoherent DMA. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/net/ethernet/seeq/sgiseeq.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)