| Message ID | 20181216171951.31306-4-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 67d8208fba1324fa0198f9fc58a9edbe09596947 |
| Headers | show |
| Series | [1/8] powerpc: allow NOT_COHERENT_CACHE for amigaone | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 8 lines checked |
On Sunday, December 16, 2018 6:19:46 PM CET Christoph Hellwig wrote: > This function is internal to the DMA API implementation. Instead use the > DMA API to properly unmap. Note that the DMA API usage in this driver > is a disaster and urgently needs some work - it is missing all the unmaps, > seems to do a secondary map where it looks like it should to a unmap > in one place to work around cache coherency and the directions passed in > seem to be partially wrong. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/crypto/amcc/crypto4xx_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/crypto/amcc/crypto4xx_core.c b/drivers/crypto/amcc/crypto4xx_core.c > index 6eaec9ba0f68..63cb6956c948 100644 > --- a/drivers/crypto/amcc/crypto4xx_core.c > +++ b/drivers/crypto/amcc/crypto4xx_core.c > @@ -596,7 +596,7 @@ static void crypto4xx_aead_done(struct crypto4xx_device *dev, > pd->pd_ctl_len.bf.pkt_len, > dst); > } else { > - __dma_sync_page(sg_page(dst), dst->offset, dst->length, > + dma_unmap_page(dev->core_dev->device, pd->dest, dst->length, > DMA_FROM_DEVICE); > } > > Yeap, the crypto4xx is a real piece of work. However, ibm emac network driver is even worse: |/* | * Lack of dma_unmap_???? calls is intentional. | * | * API-correct usage requires additional support state information to be | * maintained for every RX and TX buffer descriptor (BD). Unfortunately, due to | * EMAC design (e.g. TX buffer passed from network stack can be split into | * several BDs, dma_map_single/dma_map_page can be used to map particular BD), | * maintaining such information will add additional overhead. | * Current DMA API implementation for 4xx processors only ensures cache coherency | * and dma_unmap_???? routines are empty and are likely to stay this way. | * I decided to omit dma_unmap_??? calls because I don't want to add additional | * complexity just for the sake of following some abstract API, when it doesn't | * add any real benefit to the driver. I understand that this decision maybe | * controversial, but I really tried to make code API-correct and efficient | * at the same time and didn't come up with code I liked :(. --ebs | */ <https://elixir.bootlin.com/linux/v4.20-rc6/source/drivers/net/ethernet/ibm/emac/core.c#L58> Problem is, I can't really enable the DMA_DEBUG because every PPC4XX/APM82181 device I have is some sort of network appliance. So a proper test would require to fix the emac driver first since DMA_DEBUG will disable itself. Regards, Christian PS: Since it looks like you are located in Germany as well: If you are interested (PM me), I could just mail you a "MyBook Live" NAS Kit (PCB, Shell, Screws). All you would need: a SATA 3,5" HDD and a standard 12V PSU with a 5.5mm plug. I maintain a active OpenWrt port for the device: http://downloads.openwrt.org/snapshots/targets/apm821xx/sata/
On Sun, Dec 16, 2018 at 07:28:32PM +0100, Christian Lamparter wrote: > Yeap, the crypto4xx is a real piece of work. However, ibm emac network driver > is even worse: Oh, fun.
On Sunday, December 16, 2018 6:19:46 PM CET Christoph Hellwig wrote: > This function is internal to the DMA API implementation. Instead use the > DMA API to properly unmap. Note that the DMA API usage in this driver > is a disaster and urgently needs some work - it is missing all the unmaps, > seems to do a secondary map where it looks like it should to a unmap > in one place to work around cache coherency and the directions passed in > seem to be partially wrong. > > Signed-off-by: Christoph Hellwig <hch@lst.de> I've loaded the series (+dir -> direction patch) onto a cross-compiled vanilla 4.20-rc7. I can report that the box didn't crash, though I would have liked to test with DMA_DEBUG. Tested-by: Christian Lamparter <chunkeey@gmail.com> > --- > drivers/crypto/amcc/crypto4xx_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/crypto/amcc/crypto4xx_core.c b/drivers/crypto/amcc/crypto4xx_core.c > index 6eaec9ba0f68..63cb6956c948 100644 > --- a/drivers/crypto/amcc/crypto4xx_core.c > +++ b/drivers/crypto/amcc/crypto4xx_core.c > @@ -596,7 +596,7 @@ static void crypto4xx_aead_done(struct crypto4xx_device *dev, > pd->pd_ctl_len.bf.pkt_len, > dst); > } else { > - __dma_sync_page(sg_page(dst), dst->offset, dst->length, > + dma_unmap_page(dev->core_dev->device, pd->dest, dst->length, > DMA_FROM_DEVICE); > } > >
diff --git a/drivers/crypto/amcc/crypto4xx_core.c b/drivers/crypto/amcc/crypto4xx_core.c index 6eaec9ba0f68..63cb6956c948 100644 --- a/drivers/crypto/amcc/crypto4xx_core.c +++ b/drivers/crypto/amcc/crypto4xx_core.c @@ -596,7 +596,7 @@ static void crypto4xx_aead_done(struct crypto4xx_device *dev, pd->pd_ctl_len.bf.pkt_len, dst); } else { - __dma_sync_page(sg_page(dst), dst->offset, dst->length, + dma_unmap_page(dev->core_dev->device, pd->dest, dst->length, DMA_FROM_DEVICE); }
This function is internal to the DMA API implementation. Instead use the DMA API to properly unmap. Note that the DMA API usage in this driver is a disaster and urgently needs some work - it is missing all the unmaps, seems to do a secondary map where it looks like it should to a unmap in one place to work around cache coherency and the directions passed in seem to be partially wrong. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/crypto/amcc/crypto4xx_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)