| Message ID | 20200203101806.2441-1-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | dmaengine: Stear users towards dma_request_slave_chan() | expand |
On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > dma_request_slave_channel_reason() no longer have user in mainline, it > can be removed. > > 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 without taking care of (I'm talking now about dma release callback, for example) end users. They will be scary for no reason.
Hi Andy, On 03/02/2020 12.37, Andy Shevchenko wrote: > On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > >> dma_request_slave_channel_reason() no longer have user in mainline, it >> can be removed. >> >> 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. Imho it is confusing to have 4+ APIs to do the same thing, but in a slightly different way. > 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. 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. 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. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Mon, Feb 3, 2020 at 12:59 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > On 03/02/2020 12.37, Andy Shevchenko wrote: > > On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> 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? > 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..." > > 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? > 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. But I see no reason to keep such APIs at all, so, instead of this *wonderful* messages perhaps somebody should do better work? > 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?
Hi Andy, On 03/02/2020 13.16, Andy Shevchenko wrote: > On Mon, Feb 3, 2020 at 12:59 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >> On 03/02/2020 12.37, Andy Shevchenko wrote: >>> On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> 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
On Mon, Feb 3, 2020 at 2:09 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > On 03/02/2020 13.16, Andy Shevchenko wrote: > > On Mon, Feb 3, 2020 at 12:59 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > >> On 03/02/2020 12.37, Andy Shevchenko wrote: > >>> On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> 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. So, why everyone with the boards outside of your scope has to suffer? ... > >> 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? It's good people, and you, are taking care of it, but make user suffer because somebody wants to call for *developers* is a bad idea. And it happened not first time in the DMA engine subsystem. ... > > 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() Yes, please do. > and the __deprecated mark for > dma_request_slave_channel. This OTOH is for *developers* and its scope won't scary people. So, it's more or less fine. > > 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. Linux Next allows people to `git grep -n dma_slave_DO_NOT_CALL` and tell developers. Isn't it the job good maintainer can do? > [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... Bottom line, if you need to tell *developers* about something, please don't bother *end users*.
Hi Peter, On Mon, Feb 3, 2020 at 1:49 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > On 03/02/2020 13.16, Andy Shevchenko wrote: > > On Mon, Feb 3, 2020 at 12:59 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > >> On 03/02/2020 12.37, Andy Shevchenko wrote: > >>> On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> 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. Both rspi and sh-msiof have users on legacy SH (i.e. without DT): arch/sh/kernel/cpu/sh4a/setup-sh7757.c: .name = "rspi", arch/sh/boards/mach-ecovec24/setup.c: .name = "spi_sh_msiof", The former seems to be used with drivers/dma/sh/shdmac.c: arch/sh/include/cpu-sh4/cpu/sh7757.h: SHDMA_SLAVE_RSPI_TX, arch/sh/include/cpu-sh4/cpu/sh7757.h: SHDMA_SLAVE_RSPI_RX, arch/sh/kernel/cpu/sh4a/setup-sh7757.c: .slave_id = SHDMA_SLAVE_RSPI_TX, arch/sh/kernel/cpu/sh4a/setup-sh7757.c: .slave_id = SHDMA_SLAVE_RSPI_RX, but I have no idea if it still works (and no hardware). The latter doesn't seem to be used with DMA on ecovec24/sh7724, so probably we can just drop the _compat() from sh-sh-msiof.c. BTW, we no longer support non-legacy DMA in drivers/tty/serial/sh-sci.c (DMA was completely broken in that driver when I added DT suppport), but it seems it was used at some point on various SH, cfr. e.g. arch/sh/kernel/cpu/sh4a/setup-sh7724.c still setting up the channels. Which might be a motivation for just dropping _compat() from spi-rspi.c, too? ;-) Anyone who cares for DMA on SuperH? Gr{oetje,eeting}s, Geert
On 2/3/20 2:32 PM, Geert Uytterhoeven wrote: > Both rspi and sh-msiof have users on legacy SH (i.e. without DT): FWIW, there is a patch set by Yoshinori Sato to add device tree support for classical SuperH hardware. It was never merged, unfortunately :(. > Anyone who cares for DMA on SuperH? What is DMA used for on SuperH? Wouldn't dropping it cut support for essential hardware features? Adrian
Hi Adrian, On Mon, Feb 3, 2020 at 9:21 PM John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote: > On 2/3/20 2:32 PM, Geert Uytterhoeven wrote: > > Both rspi and sh-msiof have users on legacy SH (i.e. without DT): > > FWIW, there is a patch set by Yoshinori Sato to add device tree support > for classical SuperH hardware. It was never merged, unfortunately :(. True. > > Anyone who cares for DMA on SuperH? > > What is DMA used for on SuperH? Wouldn't dropping it cut support for > essential hardware features? It may make a few things slower. Does any of your SuperH boards use DMA? Anything interesting in /proc or /sys w.r.t. DMA? Gr{oetje,eeting}s, Geert
Hi Geert! On 2/3/20 9:34 PM, Geert Uytterhoeven wrote: > On Mon, Feb 3, 2020 at 9:21 PM John Paul Adrian Glaubitz > <glaubitz@physik.fu-berlin.de> wrote: >> On 2/3/20 2:32 PM, Geert Uytterhoeven wrote: >>> Both rspi and sh-msiof have users on legacy SH (i.e. without DT): >> >> FWIW, there is a patch set by Yoshinori Sato to add device tree support >> for classical SuperH hardware. It was never merged, unfortunately :(. > > True. Would it be possible to keep DMA support if device tree support was added for SuperH? I think Rich eventually wanted to merge the patches, there were just some minor issues with them. It would be great if we could still get them merged. I would be happy to help with the testing. >>> Anyone who cares for DMA on SuperH? >> >> What is DMA used for on SuperH? Wouldn't dropping it cut support for >> essential hardware features? > > It may make a few things slower. > > Does any of your SuperH boards use DMA? > Anything interesting in /proc or /sys w.r.t. DMA? I have: root@tirpitz:/sys> find . -iname "*dma*" ./bus/dma ./bus/dma/devices/dma0 ./bus/dma/devices/dma1 ./bus/dma/devices/dma2 ./bus/dma/devices/dma3 ./bus/dma/devices/dma4 ./bus/dma/devices/dma5 ./bus/dma/devices/dma6 ./bus/dma/devices/dma7 ./bus/dma/devices/dma8 ./bus/dma/devices/dma9 ./bus/dma/devices/dma10 ./bus/dma/devices/dma11 ./bus/platform/devices/sh_dmac ./bus/platform/devices/sh-dma-engine.0 ./bus/platform/devices/sh-dma-engine.1 ./devices/pci0000:00/0000:00:00.0/consistent_dma_mask_bits ./devices/pci0000:00/0000:00:00.0/dma_mask_bits ./devices/pci0000:00/0000:00:01.0/ata1/host0/scsi_host/host0/unchecked_isa_dma ./devices/pci0000:00/0000:00:01.0/ata1/link1/dev1.0/ata_device/dev1.0/dma_mode ./devices/pci0000:00/0000:00:01.0/ata2/host1/scsi_host/host1/unchecked_isa_dma ./devices/pci0000:00/0000:00:01.0/ata2/link2/dev2.0/ata_device/dev2.0/dma_mode ./devices/pci0000:00/0000:00:01.0/consistent_dma_mask_bits ./devices/pci0000:00/0000:00:01.0/dma_mask_bits ./devices/system/dma ./devices/system/dma/dma0 ./devices/system/dma/dma1 ./devices/system/dma/dma2 ./devices/system/dma/dma3 ./devices/system/dma/dma4 ./devices/system/dma/dma5 ./devices/system/dma/dma6 root@tirpitz:~> find /proc -iname "*dma*" /proc/dma /proc/irq/52/DMAC Address Error1 /proc/irq/33/DMAC Address Error0 /proc/sys/fs/nfs/idmap_cache_timeout find: ‘/proc/5703’: No such file or directory find: ‘/proc/5704’: No such file or directory find: ‘/proc/5705’: No such file or directory root@tirpitz:~> cat /proc/dma root@tirpitz:~> find /sys -iname "*dma*" /sys/bus/dma /sys/bus/dma/devices/dma0 /sys/bus/dma/devices/dma1 /sys/bus/dma/devices/dma2 /sys/bus/dma/devices/dma3 /sys/bus/dma/devices/dma4 /sys/bus/dma/devices/dma5 /sys/bus/dma/devices/dma6 /sys/bus/dma/devices/dma7 /sys/bus/dma/devices/dma8 /sys/bus/dma/devices/dma9 /sys/bus/dma/devices/dma10 /sys/bus/dma/devices/dma11 /sys/bus/platform/devices/sh_dmac /sys/bus/platform/devices/sh-dma-engine.0 /sys/bus/platform/devices/sh-dma-engine.1 /sys/devices/pci0000:00/0000:00:00.0/consistent_dma_mask_bits /sys/devices/pci0000:00/0000:00:00.0/dma_mask_bits /sys/devices/pci0000:00/0000:00:01.0/ata1/host0/scsi_host/host0/unchecked_isa_dma /sys/devices/pci0000:00/0000:00:01.0/ata1/link1/dev1.0/ata_device/dev1.0/dma_mode /sys/devices/pci0000:00/0000:00:01.0/ata2/host1/scsi_host/host1/unchecked_isa_dma /sys/devices/pci0000:00/0000:00:01.0/ata2/link2/dev2.0/ata_device/dev2.0/dma_mode /sys/devices/pci0000:00/0000:00:01.0/consistent_dma_mask_bits /sys/devices/pci0000:00/0000:00:01.0/dma_mask_bits /sys/devices/system/dma /sys/devices/system/dma/dma0 /sys/devices/system/dma/dma1 /sys/devices/system/dma/dma2 /sys/devices/system/dma/dma3 /sys/devices/system/dma/dma4 /sys/devices/system/dma/dma5 /sys/devices/system/dma/dma6 /sys/devices/system/dma/dma7 /sys/devices/system/dma/dma8 /sys/devices/system/dma/dma9 /sys/devices/system/dma/dma10 /sys/devices/system/dma/dma11 /sys/devices/virtual/misc/cpu_dma_latency /sys/devices/platform/sh_dmac /sys/devices/platform/sh_dmac/dma0 /sys/devices/platform/sh_dmac/dma1 /sys/devices/platform/sh_dmac/dma2 /sys/devices/platform/sh_dmac/dma3 /sys/devices/platform/sh_dmac/dma4 /sys/devices/platform/sh_dmac/dma5 /sys/devices/platform/sh_dmac/dma6 /sys/devices/platform/sh_dmac/dma7 /sys/devices/platform/sh_dmac/dma8 /sys/devices/platform/sh_dmac/dma9 /sys/devices/platform/sh_dmac/dma10 /sys/devices/platform/sh_dmac/dma11 /sys/devices/platform/r8a66597_hcd/usb1/1-1/1-1:1.0/host2/scsi_host/host2/unchecked_isa_dma /sys/devices/platform/sh-dma-engine.0 /sys/devices/platform/sh-dma-engine.1 /sys/class/misc/cpu_dma_latency /sys/module/nfs/parameters/nfs4_disable_idmapping /sys/module/nfs/parameters/nfs_idmap_cache_timeout /sys/module/libata/parameters/dma /sys/module/libata/parameters/atapi_dmadir root@tirpitz:~> On my SH-7785LCR. Adrian
On 03-02-20, 12:37, Andy Shevchenko wrote: > On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > > > dma_request_slave_channel_reason() no longer have user in mainline, it > > can be removed. > > > > 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 without taking care of > (I'm talking now about dma release callback, for example) end users. Can you elaborate issue you are seeing with dma_release callback?
Hi Geert, Adrian, On 03/02/2020 22.34, Geert Uytterhoeven wrote: > Hi Adrian, > > On Mon, Feb 3, 2020 at 9:21 PM John Paul Adrian Glaubitz > <glaubitz@physik.fu-berlin.de> wrote: >> On 2/3/20 2:32 PM, Geert Uytterhoeven wrote: >>> Both rspi and sh-msiof have users on legacy SH (i.e. without DT): >> >> FWIW, there is a patch set by Yoshinori Sato to add device tree support >> for classical SuperH hardware. It was never merged, unfortunately :(. > > True. > >>> Anyone who cares for DMA on SuperH? >> >> What is DMA used for on SuperH? Wouldn't dropping it cut support for >> essential hardware features? > > It may make a few things slower. I would not drop DMA support but I would suggest to add dma_slave_map for non DT boot so the _compat() can be dropped. Imho on lower spec SoC (and I believe SuperH is) the DMA makes big difference offloading data movement from the CPU. > Does any of your SuperH boards use DMA? > Anything interesting in /proc or /sys w.r.t. DMA? > > Gr{oetje,eeting}s, > > Geert > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Peter, On Tue, Feb 4, 2020 at 7:52 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > On 03/02/2020 22.34, Geert Uytterhoeven wrote: > > On Mon, Feb 3, 2020 at 9:21 PM John Paul Adrian Glaubitz > > <glaubitz@physik.fu-berlin.de> wrote: > >> On 2/3/20 2:32 PM, Geert Uytterhoeven wrote: > >>> Both rspi and sh-msiof have users on legacy SH (i.e. without DT): > >> > >> FWIW, there is a patch set by Yoshinori Sato to add device tree support > >> for classical SuperH hardware. It was never merged, unfortunately :(. > > > > True. > > > >>> Anyone who cares for DMA on SuperH? > >> > >> What is DMA used for on SuperH? Wouldn't dropping it cut support for > >> essential hardware features? > > > > It may make a few things slower. > > I would not drop DMA support but I would suggest to add dma_slave_map > for non DT boot so the _compat() can be dropped. Which is similar in spirit to gpiod_lookup and clk_register_clkdev(), right? > Imho on lower spec SoC (and I believe SuperH is) the DMA makes big > difference offloading data movement from the CPU. Assumed it is actually used... Gr{oetje,eeting}s, Geert
Hi Adrian, On Mon, Feb 3, 2020 at 10:26 PM John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote: > On 2/3/20 9:34 PM, Geert Uytterhoeven wrote: > > On Mon, Feb 3, 2020 at 9:21 PM John Paul Adrian Glaubitz > > <glaubitz@physik.fu-berlin.de> wrote: > >> On 2/3/20 2:32 PM, Geert Uytterhoeven wrote: > >>> Both rspi and sh-msiof have users on legacy SH (i.e. without DT): > >> > >> FWIW, there is a patch set by Yoshinori Sato to add device tree support > >> for classical SuperH hardware. It was never merged, unfortunately :(. > > > > True. > > Would it be possible to keep DMA support if device tree support was > added for SuperH? I think Rich eventually wanted to merge the patches, > there were just some minor issues with them. Adding DT support would definitely make things easier, but would be a lot of work. However, using dma_slave_map would be an alternative. > >>> Anyone who cares for DMA on SuperH? > >> > >> What is DMA used for on SuperH? Wouldn't dropping it cut support for > >> essential hardware features? > > > > It may make a few things slower. > > > > Does any of your SuperH boards use DMA? > > Anything interesting in /proc or /sys w.r.t. DMA? > > I have: > > root@tirpitz:/sys> find . -iname "*dma*" > ./bus/dma > ./bus/dma/devices/dma0 > ./bus/dma/devices/dma1 > ./bus/dma/devices/dma2 > ./bus/dma/devices/dma3 > ./bus/dma/devices/dma4 > ./bus/dma/devices/dma5 > ./bus/dma/devices/dma6 > ./bus/dma/devices/dma7 > ./bus/dma/devices/dma8 > ./bus/dma/devices/dma9 > ./bus/dma/devices/dma10 > ./bus/dma/devices/dma11 > ./bus/platform/devices/sh_dmac > ./bus/platform/devices/sh-dma-engine.0 > ./bus/platform/devices/sh-dma-engine.1 So you do have the two dmaengines... > On my SH-7785LCR. ... but are they actually used? git grep -E "(SHDMA|sh_dmae_slave_config)" -- "arch/sh/*7785*" doesn't come up with any matches, so I don't think any sh7785 platform is wired to use DMA (yet), only sh7757 and sh772[234]. To double-check: With current upstream, you can look for "slave" symlinks in sysfs. With older kernels, you can look at the interrupt counts for the DMACs in /proc/interrupts. Gr{oetje,eeting}s, Geert
Hi Geert, On 04/02/2020 10.01, Geert Uytterhoeven wrote: > Hi Peter, > > On Tue, Feb 4, 2020 at 7:52 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >> On 03/02/2020 22.34, Geert Uytterhoeven wrote: >>> On Mon, Feb 3, 2020 at 9:21 PM John Paul Adrian Glaubitz >>> <glaubitz@physik.fu-berlin.de> wrote: >>>> On 2/3/20 2:32 PM, Geert Uytterhoeven wrote: >>>>> Both rspi and sh-msiof have users on legacy SH (i.e. without DT): >>>> >>>> FWIW, there is a patch set by Yoshinori Sato to add device tree support >>>> for classical SuperH hardware. It was never merged, unfortunately :(. >>> >>> True. >>> >>>>> Anyone who cares for DMA on SuperH? >>>> >>>> What is DMA used for on SuperH? Wouldn't dropping it cut support for >>>> essential hardware features? >>> >>> It may make a few things slower. >> >> I would not drop DMA support but I would suggest to add dma_slave_map >> for non DT boot so the _compat() can be dropped. > > Which is similar in spirit to gpiod_lookup and clk_register_clkdev(), > right? Yes, it is similar: /* OMAP730, OMAP850 */ static const struct dma_slave_map omap7xx_sdma_map[] = { { "omap-mcbsp.1", "tx", SDMA_FILTER_PARAM(8) }, { "omap-mcbsp.1", "rx", SDMA_FILTER_PARAM(9) }, { "omap-mcbsp.2", "tx", SDMA_FILTER_PARAM(10) }, { "omap-mcbsp.2", "rx", SDMA_FILTER_PARAM(11) }, { "mmci-omap.0", "tx", SDMA_FILTER_PARAM(21) }, { "mmci-omap.0", "rx", SDMA_FILTER_PARAM(22) }, { "omap_udc", "rx0", SDMA_FILTER_PARAM(26) }, { "omap_udc", "rx1", SDMA_FILTER_PARAM(27) }, { "omap_udc", "rx2", SDMA_FILTER_PARAM(28) }, { "omap_udc", "tx0", SDMA_FILTER_PARAM(29) }, { "omap_udc", "tx1", SDMA_FILTER_PARAM(30) }, { "omap_udc", "tx2", SDMA_FILTER_PARAM(31) }, }; "device name", "channel name", "parameter for filter" The in the DMA driver (omap-dma.c): od->ddev.filter.map = od->plat->slave_map; od->ddev.filter.mapcnt = od->plat->slavecnt; od->ddev.filter.fn = omap_dma_filter_fn; When things are converted the filter function no longer needs to be exported, it is local to the DMA driver. > >> Imho on lower spec SoC (and I believe SuperH is) the DMA makes big >> difference offloading data movement from the CPU. > > Assumed it is actually used... Right, imho (again) we should not decide if given SoC needs it or not. It is up to the drivers to use it or not, but with the dma_slave_map there is no difference between DT or legacy boot handling towards DMA. > Gr{oetje,eeting}s, > > Geert > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 04/02/2020 10.15, Peter Ujfalusi wrote: > Hi Geert, > > On 04/02/2020 10.01, Geert Uytterhoeven wrote: >> Hi Peter, >> >> On Tue, Feb 4, 2020 at 7:52 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >>> On 03/02/2020 22.34, Geert Uytterhoeven wrote: >>>> On Mon, Feb 3, 2020 at 9:21 PM John Paul Adrian Glaubitz >>>> <glaubitz@physik.fu-berlin.de> wrote: >>>>> On 2/3/20 2:32 PM, Geert Uytterhoeven wrote: >>>>>> Both rspi and sh-msiof have users on legacy SH (i.e. without DT): >>>>> >>>>> FWIW, there is a patch set by Yoshinori Sato to add device tree support >>>>> for classical SuperH hardware. It was never merged, unfortunately :(. >>>> >>>> True. >>>> >>>>>> Anyone who cares for DMA on SuperH? >>>>> >>>>> What is DMA used for on SuperH? Wouldn't dropping it cut support for >>>>> essential hardware features? >>>> >>>> It may make a few things slower. >>> >>> I would not drop DMA support but I would suggest to add dma_slave_map >>> for non DT boot so the _compat() can be dropped. >> >> Which is similar in spirit to gpiod_lookup and clk_register_clkdev(), >> right? > > Yes, it is similar: > > /* OMAP730, OMAP850 */ > static const struct dma_slave_map omap7xx_sdma_map[] = { > { "omap-mcbsp.1", "tx", SDMA_FILTER_PARAM(8) }, > { "omap-mcbsp.1", "rx", SDMA_FILTER_PARAM(9) }, > { "omap-mcbsp.2", "tx", SDMA_FILTER_PARAM(10) }, > { "omap-mcbsp.2", "rx", SDMA_FILTER_PARAM(11) }, > { "mmci-omap.0", "tx", SDMA_FILTER_PARAM(21) }, > { "mmci-omap.0", "rx", SDMA_FILTER_PARAM(22) }, > { "omap_udc", "rx0", SDMA_FILTER_PARAM(26) }, > { "omap_udc", "rx1", SDMA_FILTER_PARAM(27) }, > { "omap_udc", "rx2", SDMA_FILTER_PARAM(28) }, > { "omap_udc", "tx0", SDMA_FILTER_PARAM(29) }, > { "omap_udc", "tx1", SDMA_FILTER_PARAM(30) }, > { "omap_udc", "tx2", SDMA_FILTER_PARAM(31) }, > }; > > "device name", "channel name", "parameter for filter" fwiw for EDMA on daVinci we have these (for da850): static const struct dma_slave_map da850_edma0_map[] = { { "davinci-mcasp.0", "rx", EDMA_FILTER_PARAM(0, 0) }, { "davinci-mcasp.0", "tx", EDMA_FILTER_PARAM(0, 1) }, { "davinci-mcbsp.0", "rx", EDMA_FILTER_PARAM(0, 2) }, { "davinci-mcbsp.0", "tx", EDMA_FILTER_PARAM(0, 3) }, { "davinci-mcbsp.1", "rx", EDMA_FILTER_PARAM(0, 4) }, { "davinci-mcbsp.1", "tx", EDMA_FILTER_PARAM(0, 5) }, { "spi_davinci.0", "rx", EDMA_FILTER_PARAM(0, 14) }, { "spi_davinci.0", "tx", EDMA_FILTER_PARAM(0, 15) }, { "da830-mmc.0", "rx", EDMA_FILTER_PARAM(0, 16) }, { "da830-mmc.0", "tx", EDMA_FILTER_PARAM(0, 17) }, { "spi_davinci.1", "rx", EDMA_FILTER_PARAM(0, 18) }, { "spi_davinci.1", "tx", EDMA_FILTER_PARAM(0, 19) }, }; static const struct dma_slave_map da850_edma1_map[] = { { "da830-mmc.1", "rx", EDMA_FILTER_PARAM(1, 28) }, { "da830-mmc.1", "tx", EDMA_FILTER_PARAM(1, 29) }, }; and in the DMA driver: ecc->dma_slave.filter.map = info->slave_map; ecc->dma_slave.filter.mapcnt = info->slavecnt; ecc->dma_slave.filter.fn = edma_filter_fn; When I added the dma_slave_map support it was done in a way that it could be used by anyone having the same issue as we were facing with legacy daVinci and OMAP1 boards (no DT support in sight). Drivers do not need to care about legacy/DT boot. > > The in the DMA driver (omap-dma.c): > od->ddev.filter.map = od->plat->slave_map; > od->ddev.filter.mapcnt = od->plat->slavecnt; > od->ddev.filter.fn = omap_dma_filter_fn; > > When things are converted the filter function no longer needs to be > exported, it is local to the DMA driver. > >> >>> Imho on lower spec SoC (and I believe SuperH is) the DMA makes big >>> difference offloading data movement from the CPU. >> >> Assumed it is actually used... > > Right, imho (again) we should not decide if given SoC needs it or not. > It is up to the drivers to use it or not, but with the dma_slave_map > there is no difference between DT or legacy boot handling towards DMA. > >> Gr{oetje,eeting}s, >> >> Geert >> > > - Péter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 2/4/20 2:01 AM, Geert Uytterhoeven wrote: > Hi Peter, > > On Tue, Feb 4, 2020 at 7:52 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >> On 03/02/2020 22.34, Geert Uytterhoeven wrote: >>> On Mon, Feb 3, 2020 at 9:21 PM John Paul Adrian Glaubitz >>> <glaubitz@physik.fu-berlin.de> wrote: >>>> On 2/3/20 2:32 PM, Geert Uytterhoeven wrote: >>>>> Both rspi and sh-msiof have users on legacy SH (i.e. without DT): >>>> >>>> FWIW, there is a patch set by Yoshinori Sato to add device tree support >>>> for classical SuperH hardware. It was never merged, unfortunately :(. >>> >>> True. >>> >>>>> Anyone who cares for DMA on SuperH? >>>> >>>> What is DMA used for on SuperH? Wouldn't dropping it cut support for >>>> essential hardware features? >>> >>> It may make a few things slower. The j-core stuff has DMA but we haven't hooked it up to dmaengine yet. (It's on the todo list but pretty far down.) I fought with dmaengine in a 7760 board in 2018, and got it to run its tests but the ship deadline arrived before I got the ethernet working with it. I found the documentation fairly impenetrable, is there a good primer on what's _current_ for new implementations? (I had similar questions for gpio. It's easy to google for "here's how you did it in 2010"...) >> I would not drop DMA support but I would suggest to add dma_slave_map >> for non DT boot so the _compat() can be dropped. > > Which is similar in spirit to gpiod_lookup and clk_register_clkdev(), > right? > >> Imho on lower spec SoC (and I believe SuperH is) the DMA makes big >> difference offloading data movement from the CPU. > > Assumed it is actually used... The turtle boards need it USB, ethernet, and sdcard, but Rich Felker hasn't finished the j32 port yet (we just got him the updated docs last month) and the existing implementation is nommu so the things that are using it are reaching around behind the OS's back... Rob
Hi Rob, On Tue, Feb 4, 2020 at 10:12 AM Rob Landley <rob@landley.net> wrote: > On 2/4/20 2:01 AM, Geert Uytterhoeven wrote: > > On Tue, Feb 4, 2020 at 7:52 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > >> On 03/02/2020 22.34, Geert Uytterhoeven wrote: > >>> On Mon, Feb 3, 2020 at 9:21 PM John Paul Adrian Glaubitz > >>> <glaubitz@physik.fu-berlin.de> wrote: > >>>> On 2/3/20 2:32 PM, Geert Uytterhoeven wrote: > >>>>> Both rspi and sh-msiof have users on legacy SH (i.e. without DT): > >>>> > >>>> FWIW, there is a patch set by Yoshinori Sato to add device tree support > >>>> for classical SuperH hardware. It was never merged, unfortunately :(. > >>> > >>> True. > >>> > >>>>> Anyone who cares for DMA on SuperH? > >>>> > >>>> What is DMA used for on SuperH? Wouldn't dropping it cut support for > >>>> essential hardware features? > >>> > >>> It may make a few things slower. > > The j-core stuff has DMA but we haven't hooked it up to dmaengine yet. (It's on > the todo list but pretty far down.) And would use DT. Hence the issue is not applicable to j-core. > The turtle boards need it USB, ethernet, and sdcard, but Rich Felker hasn't > finished the j32 port yet (we just got him the updated docs last month) and the > existing implementation is nommu so the things that are using it are reaching > around behind the OS's back... Is j32 the (rebranded) j4? Gr{oetje,eeting}s, Geert
On 2/4/20 3:27 AM, Geert Uytterhoeven wrote: > Hi Rob, > > On Tue, Feb 4, 2020 at 10:12 AM Rob Landley <rob@landley.net> wrote: >> On 2/4/20 2:01 AM, Geert Uytterhoeven wrote: >>> On Tue, Feb 4, 2020 at 7:52 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >>>> On 03/02/2020 22.34, Geert Uytterhoeven wrote: >>>>> On Mon, Feb 3, 2020 at 9:21 PM John Paul Adrian Glaubitz >>>>> <glaubitz@physik.fu-berlin.de> wrote: >>>>>> On 2/3/20 2:32 PM, Geert Uytterhoeven wrote: >>>>>>> Both rspi and sh-msiof have users on legacy SH (i.e. without DT): >>>>>> >>>>>> FWIW, there is a patch set by Yoshinori Sato to add device tree support >>>>>> for classical SuperH hardware. It was never merged, unfortunately :(. The problem with converting everything else to j-core has always been regression testing. I have 2 pieces of classing sh4 hardware, and one classing sh2 board, and none of them are currently on the same continent I am. :) (I've got 3 different j-core boards, though. And am currently in the same room as 2 others. :) >>>>> True. >>>>> >>>>>>> Anyone who cares for DMA on SuperH? >>>>>> >>>>>> What is DMA used for on SuperH? Wouldn't dropping it cut support for >>>>>> essential hardware features? >>>>> >>>>> It may make a few things slower. >> >> The j-core stuff has DMA but we haven't hooked it up to dmaengine yet. (It's on >> the todo list but pretty far down.) > > And would use DT. Hence the issue is not applicable to j-core. Indeed. The last classic sh4 board I used did not have DMA hooked up in Linux, and trying to make current kernels work hit the fact that the device tree people kept breaking various pre-device tree APIs, and my patches to fix them were met with "you should convert everything to device tree rather than fix this obvious regression we caused" and thus never went upstream. (Johnson Controls shipped them locally and I handed the patches over to the lawyers to do the license compliance tarball before I left. And of course I posted the fixes to this list...) It's nice that at least this time somebody is _asking_ before breaking non-device-tree support for stuff that used to work... >> The turtle boards need it USB, ethernet, and sdcard, but Rich Felker hasn't >> finished the j32 port yet (we just got him the updated docs last month) and the >> existing implementation is nommu so the things that are using it are reaching >> around behind the OS's back... > > Is j32 the (rebranded) j4? Yes. Rob
On Tue, Feb 4, 2020 at 8:21 AM Vinod Koul <vkoul@kernel.org> wrote: > > On 03-02-20, 12:37, Andy Shevchenko wrote: > > On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > > > > > dma_request_slave_channel_reason() no longer have user in mainline, it > > > can be removed. > > > > > > 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 without taking care of > > (I'm talking now about dma release callback, for example) end users. > > Can you elaborate issue you are seeing with dma_release callback? [ 7.980381] intel-lpss 0000:00:1e.3: WARN: Device release is not defined so it is not safe to unbind this driver while in use It's not limited to that driver, but actually all I'm maintaining. Users are not happy!
On 04-02-20, 13:21, Andy Shevchenko wrote: > On Tue, Feb 4, 2020 at 8:21 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > On 03-02-20, 12:37, Andy Shevchenko wrote: > > > On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > > > > > > > dma_request_slave_channel_reason() no longer have user in mainline, it > > > > can be removed. > > > > > > > > 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 without taking care of > > > (I'm talking now about dma release callback, for example) end users. > > > > Can you elaborate issue you are seeing with dma_release callback? > > > [ 7.980381] intel-lpss 0000:00:1e.3: WARN: Device release is not > defined so it is not safe to unbind this driver while in use Yes that is expected but is not valid in your case. Anyway this will be turned off before the release. > It's not limited to that driver, but actually all I'm maintaining. > > Users are not happy! > > -- > With Best Regards, > Andy Shevchenko
Vinod, On 05/02/2020 6.43, Vinod Koul wrote: > On 04-02-20, 13:21, Andy Shevchenko wrote: >> On Tue, Feb 4, 2020 at 8:21 AM Vinod Koul <vkoul@kernel.org> wrote: >>> >>> On 03-02-20, 12:37, Andy Shevchenko wrote: >>>> On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >>>> >>>>> dma_request_slave_channel_reason() no longer have user in mainline, it >>>>> can be removed. >>>>> >>>>> 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 without taking care of >>>> (I'm talking now about dma release callback, for example) end users. >>> >>> Can you elaborate issue you are seeing with dma_release callback? >> >> >> [ 7.980381] intel-lpss 0000:00:1e.3: WARN: Device release is not >> defined so it is not safe to unbind this driver while in use > > Yes that is expected but is not valid in your case. In which case it is valid? > Anyway this will be turned off before the release. Looking at the commit which added it and I still don't get the point. If any of the channel is in use then we should not allow the DMA driver to go away at all. Imho there should be a function to check if we can proceed with the .remove of the driver and fail it if any of the channels are in use. Hrm, base/dd.c __device_release_driver() does not check the .remove's return value, so it can not fail. What is expected if the .remove returns with OK but we still have channels in use? After the remove all sorts of things got yanked which might makes the still in use channels cause issues down the road. I'm curious why it is a good thing to remotely try to support unbind when the driver is in use. It is like one wants to support ext4 removal even when your rootfs is ext4. I think krefing the DMA driver for channel request/release is just fine, if user wants to break the system we should not assist... >> It's not limited to that driver, but actually all I'm maintaining. >> >> Users are not happy! >> >> -- >> With Best Regards, >> Andy Shevchenko > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Peter, On 05-02-20, 10:10, Peter Ujfalusi wrote: > Vinod, > > On 05/02/2020 6.43, Vinod Koul wrote: > > On 04-02-20, 13:21, Andy Shevchenko wrote: > >> On Tue, Feb 4, 2020 at 8:21 AM Vinod Koul <vkoul@kernel.org> wrote: > >>> > >>> On 03-02-20, 12:37, Andy Shevchenko wrote: > >>>> On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > >>>> > >>>>> dma_request_slave_channel_reason() no longer have user in mainline, it > >>>>> can be removed. > >>>>> > >>>>> 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 without taking care of > >>>> (I'm talking now about dma release callback, for example) end users. > >>> > >>> Can you elaborate issue you are seeing with dma_release callback? > >> > >> > >> [ 7.980381] intel-lpss 0000:00:1e.3: WARN: Device release is not > >> defined so it is not safe to unbind this driver while in use > > > > Yes that is expected but is not valid in your case. > > In which case it is valid? It is valid for cases where device can be hotplugged. We didnt handle that very well earlier. TBH we never had a reason as most of the embedded cases that is not really doable. > > Anyway this will be turned off before the release. > > Looking at the commit which added it and I still don't get the point. > If any of the channel is in use then we should not allow the DMA driver > to go away at all. Not really, if the device is already gone, we cant do much about it. We have to handle that gracefully rather than oopsing The important part is that the device is gone. Think about a device on PCI card which is yanked off or a USB device unplugged. Device is already gone, you can't communicate with it anymore. So all we can do is handle the condition and exit, hence the new method to let driver know. > Imho there should be a function to check if we can proceed with the > .remove of the driver and fail it if any of the channels are in use. > > Hrm, base/dd.c __device_release_driver() does not check the .remove's > return value, so it can not fail. > > What is expected if the .remove returns with OK but we still have > channels in use? > > After the remove all sorts of things got yanked which might makes the > still in use channels cause issues down the road. > > I'm curious why it is a good thing to remotely try to support unbind > when the driver is in use. > It is like one wants to support ext4 removal even when your rootfs is ext4. > > I think krefing the DMA driver for channel request/release is just fine, > if user wants to break the system we should not assist... > > >> It's not limited to that driver, but actually all I'm maintaining. > >> > >> Users are not happy! > >> > >> -- > >> With Best Regards, > >> Andy Shevchenko > > > > - Péter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Vinod, On 05/02/2020 13.31, Vinod Koul wrote: >> Looking at the commit which added it and I still don't get the point. >> If any of the channel is in use then we should not allow the DMA driver >> to go away at all. > > Not really, if the device is already gone, we cant do much about it. We > have to handle that gracefully rather than oopsing Ah, I have not thought about that. True. > The important part is that the device is gone. Think about a device on > PCI card which is yanked off or a USB device unplugged. Device is > already gone, you can't communicate with it anymore. So all we can do is > handle the condition and exit, hence the new method to let driver know. But for most devices this is not applicable, I also wondered what should I do in order to silence the print. Just add an empty device_release? >> Imho there should be a function to check if we can proceed with the >> .remove of the driver and fail it if any of the channels are in use. >> >> Hrm, base/dd.c __device_release_driver() does not check the .remove's >> return value, so it can not fail. >> >> What is expected if the .remove returns with OK but we still have >> channels in use? >> >> After the remove all sorts of things got yanked which might makes the >> still in use channels cause issues down the road. >> >> I'm curious why it is a good thing to remotely try to support unbind >> when the driver is in use. >> It is like one wants to support ext4 removal even when your rootfs is ext4. >> >> I think krefing the DMA driver for channel request/release is just fine, >> if user wants to break the system we should not assist... >> >>>> It's not limited to that driver, but actually all I'm maintaining. >>>> >>>> Users are not happy! >>>> >>>> -- >>>> With Best Regards, >>>> Andy Shevchenko >>> >> >> - Péter >> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 05-02-20, 13:56, Peter Ujfalusi wrote: > Hi Vinod, > > On 05/02/2020 13.31, Vinod Koul wrote: > >> Looking at the commit which added it and I still don't get the point. > >> If any of the channel is in use then we should not allow the DMA driver > >> to go away at all. > > > > Not really, if the device is already gone, we cant do much about it. We > > have to handle that gracefully rather than oopsing > > Ah, I have not thought about that. True. > > > The important part is that the device is gone. Think about a device on > > PCI card which is yanked off or a USB device unplugged. Device is > > already gone, you can't communicate with it anymore. So all we can do is > > handle the condition and exit, hence the new method to let driver know. > > But for most devices this is not applicable, I also wondered what should > I do in order to silence the print. Just add an empty device_release? I will send a patch removing this before we hit release :) so nothing to be done unless you have a hotpluggable device then would be good to add this. Thanks
Hi, dma_request_slave_channel_reason() no longer have user in mainline, it can be removed. Advise users of dma_request_slave_channel() and dma_request_slave_channel_compat() to move to dma_request_slave_chan() Regards, Peter --- Peter Ujfalusi (3): dmaengine: Remove unused define for (dma_request_slave_channel_reason)() dmaengine: Mark dma_request_slave_channel() deprecated dmaengine: Encourage dma_request_slave_channel_compat() users to migrate drivers/dma/dmaengine.c | 18 ------------------ include/linux/dmaengine.h | 28 ++++++++++++++++++---------- 2 files changed, 18 insertions(+), 28 deletions(-)