Re: [PATCH 3/8] crypto4xx_core: don't abuse __dma_sync_page

From: Christian Lamparter <chunkeey@gmail.com>
To: Christoph Hellwig <hch@lst.de>
Cc: David Miller <davem@davemloft.net>,
	Paul Mackerras <paulus@samba.org>,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 3/8] crypto4xx_core: don't abuse __dma_sync_page
Date: Sun, 16 Dec 2018 19:28:32 +0100	[thread overview]
Message-ID: <2753567.GDuzzzdScT@debian64> (raw)
In-Reply-To: <20181216171951.31306-4-hch@lst.de>

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/