| Message ID | 20240802075100.6475-1-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | dmaengine: dw: Fix src/dst addr width misconfig | expand |
On Fri, Aug 2, 2024 at 9:51 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > The main goal of this series is to fix the data disappearance in case of > the DW UART handled by the DW AHB DMA engine. The problem happens on a > portion of the data received when the pre-initialized DEV_TO_MEM > DMA-transfer is paused and then disabled. The data just hangs up in the > DMA-engine FIFO and isn't flushed out to the memory on the DMA-channel > suspension (see the second commit log for details). On a way to find the > denoted problem fix it was discovered that the driver doesn't verify the > peripheral device address width specified by a client driver, which in its > turn if unsupported or undefined value passed may cause DMA-transfer being > misconfigured. It's fixed in the first patch of the series. > > In addition to that three cleanup patches follow the fixes described above > in order to make the DWC-engine configuration procedure more coherent. > First one simplifies the CTL_LO register setup methods. Second and third > patches simplify the max-burst calculation procedure and unify it with the > rest of the verification methods. Please see the patches log for more > details. > > Final patch is another cleanup which unifies the status variables naming > in the driver. Acked-by: Andy Shevchenko <andy@kernel.org>
Hi Andy, On Sat, Aug 03, 2024 at 09:29:54PM +0200, Andy Shevchenko wrote: > On Fri, Aug 2, 2024 at 9:51 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > The main goal of this series is to fix the data disappearance in case of > > the DW UART handled by the DW AHB DMA engine. The problem happens on a > > portion of the data received when the pre-initialized DEV_TO_MEM > > DMA-transfer is paused and then disabled. The data just hangs up in the > > DMA-engine FIFO and isn't flushed out to the memory on the DMA-channel > > suspension (see the second commit log for details). On a way to find the > > denoted problem fix it was discovered that the driver doesn't verify the > > peripheral device address width specified by a client driver, which in its > > turn if unsupported or undefined value passed may cause DMA-transfer being > > misconfigured. It's fixed in the first patch of the series. > > > > In addition to that three cleanup patches follow the fixes described above > > in order to make the DWC-engine configuration procedure more coherent. > > First one simplifies the CTL_LO register setup methods. Second and third > > patches simplify the max-burst calculation procedure and unify it with the > > rest of the verification methods. Please see the patches log for more > > details. > > > > Final patch is another cleanup which unifies the status variables naming > > in the driver. > > Acked-by: Andy Shevchenko <andy@kernel.org> Awesome! Thanks. -Serge(y) > > -- > With Best Regards, > Andy Shevchenko
On Fri, 02 Aug 2024 10:50:45 +0300, Serge Semin wrote: > The main goal of this series is to fix the data disappearance in case of > the DW UART handled by the DW AHB DMA engine. The problem happens on a > portion of the data received when the pre-initialized DEV_TO_MEM > DMA-transfer is paused and then disabled. The data just hangs up in the > DMA-engine FIFO and isn't flushed out to the memory on the DMA-channel > suspension (see the second commit log for details). On a way to find the > denoted problem fix it was discovered that the driver doesn't verify the > peripheral device address width specified by a client driver, which in its > turn if unsupported or undefined value passed may cause DMA-transfer being > misconfigured. It's fixed in the first patch of the series. > > [...] Applied, thanks! [1/6] dmaengine: dw: Add peripheral bus width verification commit: b336268dde75cb09bd795cb24893d52152a9191f [2/6] dmaengine: dw: Add memory bus width verification commit: d04b21bfa1c50a2ade4816cab6fdc91827b346b1 [3/6] dmaengine: dw: Simplify prepare CTL_LO methods commit: 1fd6fe89055e6dbb4be8f16b8dcab8602e3603d6 [4/6] dmaengine: dw: Define encode_maxburst() above prepare_ctllo() callbacks commit: 3acb301d33749a8974e61ecda16a5f5441fc9628 [5/6] dmaengine: dw: Simplify max-burst calculation procedure commit: d8fa0802f63502c0409d02c6b701d51841a6f1bd [6/6] dmaengine: dw: Unify ret-val local variables naming commit: 2ebc36b9581df31eed271e5de61fc8a8b66dbc56 Best regards,
On Fri, 02 Aug 2024 10:50:45 +0300, Serge Semin wrote: > The main goal of this series is to fix the data disappearance in case of > the DW UART handled by the DW AHB DMA engine. The problem happens on a > portion of the data received when the pre-initialized DEV_TO_MEM > DMA-transfer is paused and then disabled. The data just hangs up in the > DMA-engine FIFO and isn't flushed out to the memory on the DMA-channel > suspension (see the second commit log for details). On a way to find the > denoted problem fix it was discovered that the driver doesn't verify the > peripheral device address width specified by a client driver, which in its > turn if unsupported or undefined value passed may cause DMA-transfer being > misconfigured. It's fixed in the first patch of the series. > > [...] Applied, thanks! [1/6] dmaengine: dw: Add peripheral bus width verification commit: e2c97d200ac3558e6c34258c96a01a0b9472292f [2/6] dmaengine: dw: Add memory bus width verification commit: 5bb11aedb5309c232967ce490d7b826536f697c0 [3/6] dmaengine: dw: Simplify prepare CTL_LO methods commit: d34e8466c63389ef250c380cd615826afb2a049c [4/6] dmaengine: dw: Define encode_maxburst() above prepare_ctllo() callbacks commit: 2f87a9671ed532fc088ef0b05e350637afdf001a [5/6] dmaengine: dw: Simplify max-burst calculation procedure commit: 17d4353413a4d931e89b2c37106acae4a0972ad8 [6/6] dmaengine: dw: Unify ret-val local variables naming commit: 3f92ee7c4a4e2319d510eb2ddcfdd3105b235f0e Best regards,
On Mon, Aug 05, 2024 at 03:25:35PM +0300, Serge Semin wrote: > On Sat, Aug 03, 2024 at 09:29:54PM +0200, Andy Shevchenko wrote: > > On Fri, Aug 2, 2024 at 9:51 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > The main goal of this series is to fix the data disappearance in case of > > > the DW UART handled by the DW AHB DMA engine. The problem happens on a > > > portion of the data received when the pre-initialized DEV_TO_MEM > > > DMA-transfer is paused and then disabled. The data just hangs up in the > > > DMA-engine FIFO and isn't flushed out to the memory on the DMA-channel > > > suspension (see the second commit log for details). On a way to find the > > > denoted problem fix it was discovered that the driver doesn't verify the > > > peripheral device address width specified by a client driver, which in its > > > turn if unsupported or undefined value passed may cause DMA-transfer being > > > misconfigured. It's fixed in the first patch of the series. > > > > > > In addition to that three cleanup patches follow the fixes described above > > > in order to make the DWC-engine configuration procedure more coherent. > > > First one simplifies the CTL_LO register setup methods. Second and third > > > patches simplify the max-burst calculation procedure and unify it with the > > > rest of the verification methods. Please see the patches log for more > > > details. > > > > > > Final patch is another cleanup which unifies the status variables naming > > > in the driver. > > > > Acked-by: Andy Shevchenko <andy@kernel.org> > > Awesome! Thanks. Not really :-) This series broke iDMA32 + SPI PXA2xx on Intel Merrifield. I haven't had time to investigate further, but rolling back all patches helps. +Cc: Ferry who might also test and maybe investigate as he reported the issue to me initially.
Hi Andy On Sat, Sep 14, 2024 at 09:50:48PM +0300, Andy Shevchenko wrote: > On Mon, Aug 05, 2024 at 03:25:35PM +0300, Serge Semin wrote: > > On Sat, Aug 03, 2024 at 09:29:54PM +0200, Andy Shevchenko wrote: > > > On Fri, Aug 2, 2024 at 9:51 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > > > The main goal of this series is to fix the data disappearance in case of > > > > the DW UART handled by the DW AHB DMA engine. The problem happens on a > > > > portion of the data received when the pre-initialized DEV_TO_MEM > > > > DMA-transfer is paused and then disabled. The data just hangs up in the > > > > DMA-engine FIFO and isn't flushed out to the memory on the DMA-channel > > > > suspension (see the second commit log for details). On a way to find the > > > > denoted problem fix it was discovered that the driver doesn't verify the > > > > peripheral device address width specified by a client driver, which in its > > > > turn if unsupported or undefined value passed may cause DMA-transfer being > > > > misconfigured. It's fixed in the first patch of the series. > > > > > > > > In addition to that three cleanup patches follow the fixes described above > > > > in order to make the DWC-engine configuration procedure more coherent. > > > > First one simplifies the CTL_LO register setup methods. Second and third > > > > patches simplify the max-burst calculation procedure and unify it with the > > > > rest of the verification methods. Please see the patches log for more > > > > details. > > > > > > > > Final patch is another cleanup which unifies the status variables naming > > > > in the driver. > > > > > > Acked-by: Andy Shevchenko <andy@kernel.org> > > > > Awesome! Thanks. > > Not really :-) > This series broke iDMA32 + SPI PXA2xx on Intel Merrifield. Damn. Sorry to hear that.( > I haven't > had time to investigate further, but rolling back all patches helps. > > +Cc: Ferry who might also test and maybe investigate as he reported the > issue to me initially. Ferry, could you please roll back the series patch-by-patch to find out the particular commit to blame? -Serge(y) > > -- > With Best Regards, > Andy Shevchenko > > >
On Sat, Sep 14, 2024 at 10:06:16PM +0300, Serge Semin wrote: > Hi Andy > > On Sat, Sep 14, 2024 at 09:50:48PM +0300, Andy Shevchenko wrote: > > On Mon, Aug 05, 2024 at 03:25:35PM +0300, Serge Semin wrote: > > > On Sat, Aug 03, 2024 at 09:29:54PM +0200, Andy Shevchenko wrote: > > > > On Fri, Aug 2, 2024 at 9:51 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > > > > > The main goal of this series is to fix the data disappearance in case of > > > > > the DW UART handled by the DW AHB DMA engine. The problem happens on a > > > > > portion of the data received when the pre-initialized DEV_TO_MEM > > > > > DMA-transfer is paused and then disabled. The data just hangs up in the > > > > > DMA-engine FIFO and isn't flushed out to the memory on the DMA-channel > > > > > suspension (see the second commit log for details). On a way to find the > > > > > denoted problem fix it was discovered that the driver doesn't verify the > > > > > peripheral device address width specified by a client driver, which in its > > > > > turn if unsupported or undefined value passed may cause DMA-transfer being > > > > > misconfigured. It's fixed in the first patch of the series. > > > > > > > > > > In addition to that three cleanup patches follow the fixes described above > > > > > in order to make the DWC-engine configuration procedure more coherent. > > > > > First one simplifies the CTL_LO register setup methods. Second and third > > > > > patches simplify the max-burst calculation procedure and unify it with the > > > > > rest of the verification methods. Please see the patches log for more > > > > > details. > > > > > > > > > > Final patch is another cleanup which unifies the status variables naming > > > > > in the driver. > > > > > > > > Acked-by: Andy Shevchenko <andy@kernel.org> > > > > > > Awesome! Thanks. > > > > Not really :-) > > This series broke iDMA32 + SPI PXA2xx on Intel Merrifield. > > Damn. Sorry to hear that.( > > > I haven't > > had time to investigate further, but rolling back all patches helps. > > > > +Cc: Ferry who might also test and maybe investigate as he reported the > > issue to me initially. > > Ferry, could you please roll back the series patch-by-patch to find > out the particular commit to blame? Plus to that it would be nice to have some log/info/details/etc about what exactly is happening. -Serge(y) > > -Serge(y) > > > > > -- > > With Best Regards, > > Andy Shevchenko > > > > > >
On Sat, Sep 14, 2024 at 10:08 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Sat, Sep 14, 2024 at 10:06:16PM +0300, Serge Semin wrote: > > Hi Andy > > > > On Sat, Sep 14, 2024 at 09:50:48PM +0300, Andy Shevchenko wrote: > > > On Mon, Aug 05, 2024 at 03:25:35PM +0300, Serge Semin wrote: > > > > On Sat, Aug 03, 2024 at 09:29:54PM +0200, Andy Shevchenko wrote: > > > > > On Fri, Aug 2, 2024 at 9:51 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > > > > > > > The main goal of this series is to fix the data disappearance in case of > > > > > > the DW UART handled by the DW AHB DMA engine. The problem happens on a > > > > > > portion of the data received when the pre-initialized DEV_TO_MEM > > > > > > DMA-transfer is paused and then disabled. The data just hangs up in the > > > > > > DMA-engine FIFO and isn't flushed out to the memory on the DMA-channel > > > > > > suspension (see the second commit log for details). On a way to find the > > > > > > denoted problem fix it was discovered that the driver doesn't verify the > > > > > > peripheral device address width specified by a client driver, which in its > > > > > > turn if unsupported or undefined value passed may cause DMA-transfer being > > > > > > misconfigured. It's fixed in the first patch of the series. > > > > > > > > > > > > In addition to that three cleanup patches follow the fixes described above > > > > > > in order to make the DWC-engine configuration procedure more coherent. > > > > > > First one simplifies the CTL_LO register setup methods. Second and third > > > > > > patches simplify the max-burst calculation procedure and unify it with the > > > > > > rest of the verification methods. Please see the patches log for more > > > > > > details. > > > > > > > > > > > > Final patch is another cleanup which unifies the status variables naming > > > > > > in the driver. > > > > > > > > > > Acked-by: Andy Shevchenko <andy@kernel.org> > > > > > > > > Awesome! Thanks. > > > > > > Not really :-) > > > This series broke iDMA32 + SPI PXA2xx on Intel Merrifield. > > > > Damn. Sorry to hear that.( > > > > > I haven't > > > had time to investigate further, but rolling back all patches helps. > > > > > > +Cc: Ferry who might also test and maybe investigate as he reported the > > > issue to me initially. > > > > Ferry, could you please roll back the series patch-by-patch to find > > out the particular commit to blame? > > Plus to that it would be nice to have some log/info/details/etc about > what exactly is happening. For me with patch spitest -l -s1000000 -b128 /dev/spidev5.1 SPI: [mode 0x20, bits_per_word 8, speed 1000000 Hz] [ 164.525604] pxa2xx_spi_pci 0000:00:07.1: DMA slave config failed [ 164.536105] pxa2xx_spi_pci 0000:00:07.1: failed to get DMA TX descriptor [ 164.543213] spidev spi-SPT0001:00: SPI transfer failed: -16 [ 164.550140] spi_master spi5: failed to transfer one message from queue [ 164.557126] spi_master spi5: noqueue transfer failed spitest: SPI transfer failed in iteration #0: Device or resource busy Without spitest -s 1000000 -b 128 -l /dev/spidev5.1 SPI: [mode 0x20, bits_per_word 8, speed 1000000 Hz] SEND: [00000000] ff 97 d0 54 d5 69 85 6e ca e7 b3 e1 a1 e5 1a 9d ... RECV: [00000000] ff 97 d0 54 d5 69 85 6e ca e7 b3 e1 a1 e5 1a 9d ... `spitest` is our internal tool, so what it does there is: 1) opens SPI device for speed 1MHz in loopback mode 2) generates 128 byte of random data 3) tries to send and receive them 4) compares I believe the similar behaviour can be achieved with the one that is in the kernel tree.
On Sun, Sep 15, 2024 at 02:43:19PM +0300, Andy Shevchenko wrote: > On Sat, Sep 14, 2024 at 10:08 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > On Sat, Sep 14, 2024 at 10:06:16PM +0300, Serge Semin wrote: > > > Hi Andy > > > > > > On Sat, Sep 14, 2024 at 09:50:48PM +0300, Andy Shevchenko wrote: > > > > On Mon, Aug 05, 2024 at 03:25:35PM +0300, Serge Semin wrote: > > > > > On Sat, Aug 03, 2024 at 09:29:54PM +0200, Andy Shevchenko wrote: > > > > > > On Fri, Aug 2, 2024 at 9:51 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > > > > > > > > > The main goal of this series is to fix the data disappearance in case of > > > > > > > the DW UART handled by the DW AHB DMA engine. The problem happens on a > > > > > > > portion of the data received when the pre-initialized DEV_TO_MEM > > > > > > > DMA-transfer is paused and then disabled. The data just hangs up in the > > > > > > > DMA-engine FIFO and isn't flushed out to the memory on the DMA-channel > > > > > > > suspension (see the second commit log for details). On a way to find the > > > > > > > denoted problem fix it was discovered that the driver doesn't verify the > > > > > > > peripheral device address width specified by a client driver, which in its > > > > > > > turn if unsupported or undefined value passed may cause DMA-transfer being > > > > > > > misconfigured. It's fixed in the first patch of the series. > > > > > > > > > > > > > > In addition to that three cleanup patches follow the fixes described above > > > > > > > in order to make the DWC-engine configuration procedure more coherent. > > > > > > > First one simplifies the CTL_LO register setup methods. Second and third > > > > > > > patches simplify the max-burst calculation procedure and unify it with the > > > > > > > rest of the verification methods. Please see the patches log for more > > > > > > > details. > > > > > > > > > > > > > > Final patch is another cleanup which unifies the status variables naming > > > > > > > in the driver. > > > > > > > > > > > > Acked-by: Andy Shevchenko <andy@kernel.org> > > > > > > > > > > Awesome! Thanks. > > > > > > > > Not really :-) > > > > This series broke iDMA32 + SPI PXA2xx on Intel Merrifield. > > > > > > Damn. Sorry to hear that.( > > > > > > > I haven't > > > > had time to investigate further, but rolling back all patches helps. > > > > > > > > +Cc: Ferry who might also test and maybe investigate as he reported the > > > > issue to me initially. > > > > > > Ferry, could you please roll back the series patch-by-patch to find > > > out the particular commit to blame? > > > > Plus to that it would be nice to have some log/info/details/etc about > > what exactly is happening. > > For me with patch > > spitest -l -s1000000 -b128 /dev/spidev5.1 > SPI: [mode 0x20, bits_per_word 8, speed 1000000 Hz] > [ 164.525604] pxa2xx_spi_pci 0000:00:07.1: DMA slave config failed > [ 164.536105] pxa2xx_spi_pci 0000:00:07.1: failed to get DMA TX descriptor > [ 164.543213] spidev spi-SPT0001:00: SPI transfer failed: -16 > [ 164.550140] spi_master spi5: failed to transfer one message from queue > [ 164.557126] spi_master spi5: noqueue transfer failed > spitest: SPI transfer failed in iteration #0: Device or resource busy Thanks for the log. As I suspected there is a safety-check failure, which prevents the client driver from using the DMA-controller with the specified transfer parameters. It would be helpful if you find out which conditional statement in the dwc_verify_p_buswidth(), dwc_verify_m_buswidth() methods cause the failure. Also it would be useful to get the max_width, mem_width, reg_width and reg_burst variables values. -Serge(y) > > Without > > spitest -s 1000000 -b 128 -l /dev/spidev5.1 > SPI: [mode 0x20, bits_per_word 8, speed 1000000 Hz] > SEND: [00000000] ff 97 d0 54 d5 69 85 6e ca e7 b3 e1 a1 e5 1a 9d > ... > RECV: [00000000] ff 97 d0 54 d5 69 85 6e ca e7 b3 e1 a1 e5 1a 9d > ... > > `spitest` is our internal tool, so what it does there is: > 1) opens SPI device for speed 1MHz in loopback mode > 2) generates 128 byte of random data > 3) tries to send and receive them > 4) compares > > I believe the similar behaviour can be achieved with the one that is > in the kernel tree. > > -- > With Best Regards, > Andy Shevchenko
The main goal of this series is to fix the data disappearance in case of the DW UART handled by the DW AHB DMA engine. The problem happens on a portion of the data received when the pre-initialized DEV_TO_MEM DMA-transfer is paused and then disabled. The data just hangs up in the DMA-engine FIFO and isn't flushed out to the memory on the DMA-channel suspension (see the second commit log for details). On a way to find the denoted problem fix it was discovered that the driver doesn't verify the peripheral device address width specified by a client driver, which in its turn if unsupported or undefined value passed may cause DMA-transfer being misconfigured. It's fixed in the first patch of the series. In addition to that three cleanup patches follow the fixes described above in order to make the DWC-engine configuration procedure more coherent. First one simplifies the CTL_LO register setup methods. Second and third patches simplify the max-burst calculation procedure and unify it with the rest of the verification methods. Please see the patches log for more details. Final patch is another cleanup which unifies the status variables naming in the driver. Link: https://lore.kernel.org/dmaengine/20240416162908.24180-1-fancer.lancer@gmail.com/ Changelog v2: - Add a note to the Patch #1 commit message about having the verification method called in the dwc_config() function. (Andy) - Add hyphen to "1byte" in the in-situ comment. (Andy) - Convert "err" to "ret" variables and add a new patch which unifies the status variables naming. (Andy) - Add a in-situ comment regarding why the memory-side bus width verification was required. (Andy) - Group sms+dms and smsize+dmsize local variables initializations up. (Andy) - Move the zero initializations out to the variables init block in the prepare_ctllo() callbacks. (Andy) - Directly refer to dwc_config() in the commit messages. (Andy) - Convert dwc_verify_maxburst() to returning zero. (Andy) - Add a comment regarding the values utilized in dwc_verify_p_buswidth() being pre-verified before the method is called. (Andy) - Add new patches: [PATCH v2 4/6] dmaengine: dw: Define encode_maxburst() above prepare_ctllo() callbacks [PATCH v2 6/6] dmaengine: dw: Unify ret-val local variable naming (Andy) Link: https://lore.kernel.org/dmaengine/20240419175655.25547-1-fancer.lancer@gmail.com/ Changelog v3: - Rebase onto the kernel 6.10-rc4. - Just resend. Link: https://lore.kernel.org/dmaengine/20240627172231.24856-1-fancer.lancer@gmail.com/ Changelog v4: - Rebase onto the kernel 6.11-rc1. - Just resend. base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b Signed-off-by: Serge Semin <fancer.lancer@gmail.com> Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jiri Slaby <jirislaby@kernel.org> Cc: dmaengine@vger.kernel.org Cc: linux-serial@vger.kernel.org Cc: linux-kernel@vger.kernel.org Serge Semin (6): dmaengine: dw: Add peripheral bus width verification dmaengine: dw: Add memory bus width verification dmaengine: dw: Simplify prepare CTL_LO methods dmaengine: dw: Define encode_maxburst() above prepare_ctllo() callbacks dmaengine: dw: Simplify max-burst calculation procedure dmaengine: dw: Unify ret-val local variables naming drivers/dma/dw/core.c | 131 +++++++++++++++++++++++++++++++------- drivers/dma/dw/dw.c | 40 +++++++----- drivers/dma/dw/idma32.c | 19 +++--- drivers/dma/dw/platform.c | 20 +++--- drivers/dma/dw/regs.h | 1 - 5 files changed, 154 insertions(+), 57 deletions(-)