Re: [PATCH 0/3] dmaengine: Stear users towards dma_request_slave_chan()

[Peter Ujfalusi]

Hi Andy,

On 03/02/2020 13.16, Andy Shevchenko wrote:
> On Mon, Feb 3, 2020 at 12:59 PM Peter Ujfalusi <peter.ujfalusi@xxxxxx> wrote:
>> On 03/02/2020 12.37, Andy Shevchenko wrote:
>>> On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@xxxxxx> wrote:
> 
>>>> Advise users of dma_request_slave_channel() and
>>>> dma_request_slave_channel_compat() to move to dma_request_slave_chan()
>>>
>>> How? There are legacy ARM boards you have to care / remove before.
>>> DMAengine subsystem makes a p*s off decisions
>>
>> The dma_slave_map support is added few years back for the legacy ARM
>> boards, because we do care.
>> daVinci, OMAP1, pxa, s3cx4xx and even m68k/coldfire moved over.
> 
> Then why simple not to convert (start converting) those few drivers to
> new API and simple remove the old one?

_if_ the dma_request_slave_channel_compat() is really falling back after
dma_request_chan() - this is what it calls first, then it is not a
simple convert to a new API.
1. The arch needs to define the dma_slave_map for the SoC.
2. The dma driver needs few lines to add it to DMAengine core (+pdata
change).
After this the _compat() can be replaced with dma_request_chan()

In most cases I do not have access to documentation and boards to test.

Looking at the output of 'git grep dma_request_slave_channel_compat':
drivers/mmc/host/renesas_sdhi_sys_dmac.c
drivers/spi/spi-pxa2xx-dma.c
drivers/spi/spi-rspi.c
drivers/spi/spi-sh-msiof.c
drivers/tty/serial/8250/8250_dma.c

>From these rspi boots only in DT and I'm not sure about the others.
8250_dma is a tricky one as it is used as generic DMA layer for many arch.

>> Imho it is confusing to have 4+ APIs to do the same thing, but in a
>> slightly different way.
> 
> It was always an excuse by authors "that too many drivers to convert..."

Sure, but before you accuse anyone with neglect, can you check the git
log for 'dma_request_slave_channel' in the commit messages?

>>> without taking care of
>>> (I'm talking now about dma release callback, for example) end users.
>>
>> I have been converting users in the background, but the _compat() is a
>> bit more problematic as I need to maintainers of those legacy platforms
>> to craft the map. If they care.
> 
> Why not to remove them and don't punish users of new drivers / platforms?

The _compat() can not be removed, but we just don't know who really
needs the fallback after dma_request_chan().
With the print the hope is that users will come forward and we can help
migrate or address their specific needs.

>> Obviously the APIs are not going to be removed if we have a single user
>> and if there is clearly a need for something the _compat() was doing and
>> it can not be done via the dma_slave_map, then rest assured there will
>> be a clean API to achieve just that.
>>
>>> They will be scary for no reason.
>>
>> There is a reason: to clean up the API to make it non confusing for the
>> users.
> 
> No, it's a reason when you first take care of existing users and
> decide to obsolete an API followed by removal few releases later.

I'm fine to drop the pr_info() and the __deprecated mark for
dma_request_slave_channel.

> But
> I see no reason to keep such APIs at all, so, instead of this
> *wonderful* messages perhaps somebody should do better work?

To touch the _compat() variant one needs to have access to the
documentation of the SoC on which the code falls back. It is not a
matter of sloppy/poor/ignorant/etc work attitude.
I have kept clear on touching those few drivers using it [1] as I don't
have documentation.

>> New drivers should not use the old API i new code and developers tend to
>> pick the API they use after a quick 'git grep dma_request_' and see what
>> the majority is using.
> 
> Isn't it a point to do better review rather than scary end users?

Sure, but we rarely CCd on new client drivers for DMAengine API usage.

[1] fwiw, there are drivers using dma_request_channel() and by the look
their use is either _compat() or the dma_request_chan_by_mask() style
and some even have a twist here and there...

- PÃter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki