On 07/12/2016 06:33 PM, Jiang, Dave wrote: > On Tue, 2016-07-12 at 11:57 +0000, Lars-Peter Clausen wrote: >> On 07/12/2016 01:45 AM, Dave Jiang wrote: >>> Adding error reporting infrastructure though the DMA cookie. >>> An error status will be set in the cookie if the descriptor is >>> returned from driver as failed in order to notify upper layer of >>> the status. >>> >>> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx> >> >> Hi, >> >> Thanks for working on this. But I'm afraid that this particular >> approach >> will not work. The idea behind the cookies is that they are stateless >> except >> for the cookie value itself and do not require any lifetime >> management. The >> cookie values are monotonically incrementing which means you can >> easily >> check whether a transfer is completed by comparing its value to the >> last >> completed transfer. No additional state is required. >> >> What you are doing in this patch is conceptionally the same as you >> did in >> the first version of your patch. You assign additional state as well >> as >> lifetime management to the cookie embedded in the >> dma_async_tx_descriptor >> struct. The lifetime of this new cookie struct is the same as the >> tx_descriptor it belong to though and all your interfaces work on a >> struct >> dma_async_tx_descriptor as well. So in essence this is not any >> different >> compared to embedding the error code in the tx_descriptor itself. In >> addition this patch series and this patch in particular introduce a >> fair >> amount of memory leaks since not everybody is calling >> dma_cookie_free(). >> >> So the same review points of version 1 of the patch still apply. >> >> My personal preference for error reporting is to add status field to >> the >> complete callback. This state struct should contain error contentions >> but >> also other status information like the number of bytes transferred >> for the >> descriptor. > > Thanks for the feedback. That sounds reasonable. Let me work on that. > > So to clarify, what you are saying is to modify this callback[1] to add > an additional reporting struct: > > typedef void (*dma_async_tx_callback)(void *dma_async_param, struct > dma_result *result); > > And let the upper layer allocate the memory for that report struct and > provide it to the DMA driver to store the results in? I'm not sure about the details yet, but yes something like that. The report struct should probably not be allocated by the higher layers. The report struct should only be valid during the invocation of the callback function. If a client needs the data outside of the callback function it should make a copy. This means that the report struct might simply be allocated on the stack in the driver, the driver initializes it, passed it to the callback and once the callback has returned the data is discarded again. For a transitional period it might make sense to have two callbacks one with the result parameter, the other without. Otherwise we'd have to update all users at the same time. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
