dmaengine: dw: Fix src/dst addr width misconfig

Currently the src_addr_width and dst_addr_width fields of the dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the properly aligned data passed to the target device. It's done just by converting the passed peripheral bus width to the encoded value using the __ffs() function. This implementation has several problematic sides:

1. __ffs() is undefined if no bit exist in the passed value. Thus if the specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return unexpected value depending on the platform-specific implementation.

2. DW AHB DMA-engine permits having the power-of-2 transfer width limited by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying bus-width out of that constraints scope will definitely cause unexpected result since the destination reg will be only partly touched than the client driver implied.

Let's fix all of that by adding the peripheral bus width verification method and calling it in dwc_config() which is supposed to be executed before preparing any transfer. The new method will make sure that the passed source or destination address width is valid and if undefined then the driver will just fallback to the 1-byte width transfer.

Quoted 17 lines · Serge Semin

Currently the src_addr_width and dst_addr_width fields of the dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the properly aligned data passed to the target device. It's done just by converting the passed peripheral bus width to the encoded value using the __ffs() function. This implementation has several problematic sides:

1. __ffs() is undefined if no bit exist in the passed value. Thus if the specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return unexpected value depending on the platform-specific implementation.

2. DW AHB DMA-engine permits having the power-of-2 transfer width limited by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying bus-width out of that constraints scope will definitely cause unexpected result since the destination reg will be only partly touched than the client driver implied.

Quoted 5 lines

Let's fix all of that by adding the peripheral bus width verification method and calling it in dwc_config() which is supposed to be executed before preparing any transfer. The new method will make sure that the passed source or destination address width is valid and if undefined then the driver will just fallback to the 1-byte width transfer.

This patch broke Intel Merrifield iDMA32 + SPI PXA2xx configuration to me. Since it's first in the series and most likely the rest is dependent and we are almost at the release date I propose to roll back and start again after v6.12-rc1 will be out. Vinod, can we revert the entire series, please?

Signature · 3 lines

-- 
With Best Regards,
Andy Shevchenko

Quoted 23 lines · Serge Semin

Quoted 22 lines

Currently the src_addr_width and dst_addr_width fields of the dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the properly aligned data passed to the target device. It's done just by converting the passed peripheral bus width to the encoded value using the __ffs() function. This implementation has several problematic sides:

1. __ffs() is undefined if no bit exist in the passed value. Thus if the specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return unexpected value depending on the platform-specific implementation.

2. DW AHB DMA-engine permits having the power-of-2 transfer width limited by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying bus-width out of that constraints scope will definitely cause unexpected result since the destination reg will be only partly touched than the client driver implied.

Let's fix all of that by adding the peripheral bus width verification method and calling it in dwc_config() which is supposed to be executed before preparing any transfer. The new method will make sure that the passed source or destination address width is valid and if undefined then the driver will just fallback to the 1-byte width transfer.

Quoted 5 lines

This patch broke Intel Merrifield iDMA32 + SPI PXA2xx configuration to me. Since it's first in the series and most likely the rest is dependent and we are almost at the release date I propose to roll back and start again after v6.12-rc1 will be out. Vinod, can we revert the entire series, please?

Quoted 6 lines

I guess it's not the best option, since the patch has already been backported to the stable kernels anyway. Rolling back it from all of them seems tiresome. Let's at least try to fix the just discovered problem?

Could you please provide more details about what exactly happening?

Quoted 7 lines

-Serge(y)

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 24 lines · Andy Shevchenko

Quoted 17 lines · Serge Semin

Currently the src_addr_width and dst_addr_width fields of the dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the properly aligned data passed to the target device. It's done just by converting the passed peripheral bus width to the encoded value using the __ffs() function. This implementation has several problematic sides:

1. __ffs() is undefined if no bit exist in the passed value. Thus if the specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return unexpected value depending on the platform-specific implementation.

2. DW AHB DMA-engine permits having the power-of-2 transfer width limited by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying bus-width out of that constraints scope will definitely cause unexpected result since the destination reg will be only partly touched than the client driver implied.

Quoted 5 lines

Let's fix all of that by adding the peripheral bus width verification method and calling it in dwc_config() which is supposed to be executed before preparing any transfer. The new method will make sure that the passed source or destination address width is valid and if undefined then the driver will just fallback to the 1-byte width transfer.

Quoted 5 lines

This patch broke Intel Merrifield iDMA32 + SPI PXA2xx configuration to me. Since it's first in the series and most likely the rest is dependent and we are almost at the release date I propose to roll back and start again after v6.12-rc1 will be out. Vinod, can we revert the entire series, please?

I guess it's not the best option, since the patch has already been backported to the stable kernels anyway. Rolling back it from all of them seems tiresome. Let's at least try to fix the just discovered problem?

Could you please provide more details about what exactly happening?

Quoted 31 lines · Serge Semin

Quoted 24 lines · Andy Shevchenko

Quoted 17 lines · Serge Semin

Currently the src_addr_width and dst_addr_width fields of the dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the properly aligned data passed to the target device. It's done just by converting the passed peripheral bus width to the encoded value using the __ffs() function. This implementation has several problematic sides:

1. __ffs() is undefined if no bit exist in the passed value. Thus if the specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return unexpected value depending on the platform-specific implementation.

2. DW AHB DMA-engine permits having the power-of-2 transfer width limited by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying bus-width out of that constraints scope will definitely cause unexpected result since the destination reg will be only partly touched than the client driver implied.

Quoted 5 lines

Let's fix all of that by adding the peripheral bus width verification method and calling it in dwc_config() which is supposed to be executed before preparing any transfer. The new method will make sure that the passed source or destination address width is valid and if undefined then the driver will just fallback to the 1-byte width transfer.

Quoted 5 lines

This patch broke Intel Merrifield iDMA32 + SPI PXA2xx configuration to me. Since it's first in the series and most likely the rest is dependent and we are almost at the release date I propose to roll back and start again after v6.12-rc1 will be out. Vinod, can we revert the entire series, please?

Quoted 4 lines

I guess it's not the best option, since the patch has already been backported to the stable kernels anyway. Rolling back it from all of them seems tiresome. Let's at least try to fix the just discovered problem?

Please, provide one we can test!

Quoted 1 line

Could you please provide more details about what exactly happening?

Sure. AFAICT the only problematic line is this:

        else if (!is_power_of_2(reg_width) || reg_width > max_width)

in your patch, and it may trigger, for example, when max_width == 0. This, in accordance with my brief investigation, happens due to the following.

The DMA slave configuration is being copied twice in DW DMA code:
1) when respective filter function triggers (see acpi/of glue code);
2) when the channel is about to be allocated.

The iDMA32 has only a single master, and hence m_master == p_master, BUT the filter function in the acpi code is universal and it copies the wrong (from the iDMA32 perspective) value to p_master. As the result, when you retrieve the max_width, it takes the value from p_master, which is defined to 1 (sic!), and hence assigns it to 0.

I don't know how to quickfix this as the proper fix seems to provide the correct data in the first place.

Any ideas, patches we may test?

Quoted 52 lines · Andy Shevchenko

Quoted 31 lines · Serge Semin

Quoted 24 lines · Andy Shevchenko

Quoted 17 lines · Serge Semin

Currently the src_addr_width and dst_addr_width fields of the dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the properly aligned data passed to the target device. It's done just by converting the passed peripheral bus width to the encoded value using the __ffs() function. This implementation has several problematic sides:

1. __ffs() is undefined if no bit exist in the passed value. Thus if the specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return unexpected value depending on the platform-specific implementation.

2. DW AHB DMA-engine permits having the power-of-2 transfer width limited by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying bus-width out of that constraints scope will definitely cause unexpected result since the destination reg will be only partly touched than the client driver implied.

Quoted 5 lines

Let's fix all of that by adding the peripheral bus width verification method and calling it in dwc_config() which is supposed to be executed before preparing any transfer. The new method will make sure that the passed source or destination address width is valid and if undefined then the driver will just fallback to the 1-byte width transfer.

Quoted 5 lines

This patch broke Intel Merrifield iDMA32 + SPI PXA2xx configuration to me. Since it's first in the series and most likely the rest is dependent and we are almost at the release date I propose to roll back and start again after v6.12-rc1 will be out. Vinod, can we revert the entire series, please?

Quoted 4 lines

I guess it's not the best option, since the patch has already been backported to the stable kernels anyway. Rolling back it from all of them seems tiresome. Let's at least try to fix the just discovered problem?

Please, provide one we can test!

Quoted 1 line

Could you please provide more details about what exactly happening?

Sure. AFAICT the only problematic line is this:

        else if (!is_power_of_2(reg_width) || reg_width > max_width)

in your patch, and it may trigger, for example, when max_width == 0. This, in accordance with my brief investigation, happens due to the following.

The DMA slave configuration is being copied twice in DW DMA code:
1) when respective filter function triggers (see acpi/of glue code);
2) when the channel is about to be allocated.

Quoted 5 lines

The iDMA32 has only a single master, and hence m_master == p_master, BUT the filter function in the acpi code is universal and it copies the wrong (from the iDMA32 perspective) value to p_master. As the result, when you retrieve the max_width, it takes the value from p_master, which is defined to 1 (sic!), and hence assigns it to 0.

Quoted 4 lines

I don't know how to quickfix this as the proper fix seems to provide the correct data in the first place.

Any ideas, patches we may test?

P.S. for your advocacy it seems that your change actually revealed an inconsistency in the existing code. But still, it made a regression.

On Mon, Sep 16, 2024 at 02:45:41PM +0300, Andy Shevchenko wrote:

Okay that was the theory, now I made a hack patch, i.e. supply 0 in acpi.c in the filter function and everything starts to work. So, my theory is correct.

Quoted 1 line · Andy Shevchenko

On Mon, Sep 16, 2024 at 02:45:41PM +0300, Andy Shevchenko wrote:

...

Quoted 2 lines

Okay that was the theory, now I made a hack patch, i.e. supply 0 in acpi.c in the filter function and everything starts to work. So, my theory is correct.

FWIW, I have reapplied the whole series and it seems fine, so the only regression is that I described in the previous replies.

Signature · 3 lines

-- 
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.

Quoted 19 lines · Serge Semin

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.

Quoted 2 lines

Final patch is another cleanup which unifies the status variables naming in the driver.

Acked-by Andy Shevchenko <[email protected]>

Signature · 3 lines

-- 
With Best Regards,
Andy Shevchenko

Quoted 20 lines · Andy Shevchenko

Quoted 19 lines · Serge Semin

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.

Quoted 4 lines

Quoted 2 lines

Final patch is another cleanup which unifies the status variables naming in the driver.

Acked-by Andy Shevchenko <[email protected]>

Awesome! Thanks.

Quoted 24 lines · Serge Semin

Quoted 23 lines · Andy Shevchenko

Quoted 21 lines · Serge Semin

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.

Quoted 3 lines

Quoted 1 line

Acked-by Andy Shevchenko <[email protected]>

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.

Signature · 3 lines

-- 
With Best Regards,
Andy Shevchenko

Hi Andy

Quoted 25 lines · Andy Shevchenko

Quoted 24 lines · Serge Semin

Quoted 23 lines · Andy Shevchenko

Quoted 21 lines · Serge Semin

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Acked-by Andy Shevchenko <[email protected]>

Awesome! Thanks.

Not really :-)
This series broke iDMA32 + SPI PXA2xx on Intel Merrifield.

Damn. Sorry to hear that.(

Quoted 5 lines

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)

Quoted 7 lines

Signature · 3 lines

-- 
With Best Regards,
Andy Shevchenko

Quoted 40 lines · Serge Semin

Hi Andy

Quoted 25 lines · Andy Shevchenko

Quoted 24 lines · Serge Semin

Quoted 23 lines · Andy Shevchenko

Quoted 21 lines · Serge Semin

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Acked-by Andy Shevchenko <[email protected]>

Awesome! Thanks.

Not really :-)
This series broke iDMA32 + SPI PXA2xx on Intel Merrifield.

Damn. Sorry to hear that.(

Quoted 3 lines

I haven't
had time to investigate further, but rolling back all patches helps.

Quoted 5 lines

Quoted 2 lines

+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.

Quoted 42 lines · Serge Semin

Quoted 40 lines · Serge Semin

Hi Andy

Quoted 25 lines · Andy Shevchenko

Quoted 24 lines · Serge Semin

Quoted 23 lines · Andy Shevchenko

Quoted 21 lines · Serge Semin

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Acked-by Andy Shevchenko <[email protected]>

Awesome! Thanks.

Not really :-)
This series broke iDMA32 + SPI PXA2xx on Intel Merrifield.

Damn. Sorry to hear that.(

Quoted 3 lines

I haven't
had time to investigate further, but rolling back all patches helps.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

+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.

Signature · 2 lines

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.

[...]

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.

[...]