Bug in ETH_DMARxDescListInit - reported in the forum

[pavel-a]

Please see description and fix here:
https://community.st.com/s/question/0D50X0000Bea7dNSQQ/hellomay-be-it-is-wrong-ethernet-dma-tail-calculation

In STM32H7xx_HAL_Driver/Src/stm32h7xx_hal_eth.c, line 2704

[ALABSTM]

Hi Pavel,

Thank you for having reported this point. It will be transmitted to our development teams. We will be back to you as soon as they provide us with a feedback. Thank you once more.

With regards,

[pavel-a]

Thank you Ali. Kudos for finding this issue go to ashchepkov581 (on the forum).

[ALABSTM]

Hi Pavel,

Our development teams confirmed this issue. A fix will be implemented and published in the frame of a future release. Thank you again for your contribution.

With regards,

[ALABSTM]

ST Internal Reference: 76913

[pavel-a]

Still not fixed in v. 1.6.0

STM32CubeH7/Drivers/STM32H7xx_HAL_Driver/Src/stm32h7xx_hal_eth.c

Line 2706 in 3d3ffbe

WRITE_REG(heth->Instance->DMACRDTPR, ((uint32_t)(heth->Init.RxDesc + (((uint32_t)(ETH_RX_DESC_CNT - 1))*sizeof(ETH_DMADescTypeDef)))));

[ALABSTM]

Hi Pavel,

This bug is intended to be fixed in the frame of package v1.7.0 release (or a later one).

With regards,

[ALABSTM]

Hi Pavel,

The fix of this issue will not be part of the v1.7.0 pacakge which will be published soon but part of a later one.

In the meanwhile, here is how the erroneous line will be updated:

-WRITE_REG(heth->Instance->DMACRDTPR, ((uint32_t)(heth->Init.RxDesc + (((uint32_t)(ETH_RX_DESC_CNT - 1))*sizeof(ETH_DMADescTypeDef)))));
+WRITE_REG(heth->Instance->DMACRDTPR, ((uint32_t)(heth->Init.RxDesc) + ((uint32_t)(ETH_RX_DESC_CNT - 1))));

We hope this helps and we thank you once more for your contribution.

With regards,

[pavel-a]

Ali, could they review it again, please? Per the docum (RM0433 page 2889) ETH_DMACRXDTPR indicated location of the last RX descriptor. I understand this as it contains address of the last descriptor, that is (base addr) + sizeof (one descr) * (number of desc. -1)
But in their variant: (base addr) + (number of desc. -1).
-- pa

[ALABSTM]

Hi Pavel,

I had a look to the ETH_DMADescTypeDef structure type and I think I understand better your point. Would you confirm please before I loop back with our development teams? Thanks.

Below is the definition of the ETH_DMADescTypeDef structre type:

STM32CubeH7/Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_hal_eth.h

Lines 57 to 65 in 79196b0

	typedef struct
	{
	__IO uint32_t DESC0;
	__IO uint32_t DESC1;
	__IO uint32_t DESC2;
	__IO uint32_t DESC3;
	__IO uint32_t BackupAddr0; /* used to store rx buffer 1 address */
	__IO uint32_t BackupAddr1; /* used to store rx buffer 2 address */
	}ETH_DMADescTypeDef;

Now, sizeof(ETH_DMADescTypeDef) would return 24 (6 members x 4 bytes per member).

And here is the point:

• On the one hand, removing the sizeof(ETH_DMADescTypeDef) from the expression would not yield the correct result, as you are saying (resulting address less than the targeted one).
• On the other hand, leaving the sizeof(ETH_DMADescTypeDef) as it is would not yield the correct result neither (resulting address greater than the targeted one).

Rules of pointer arithmetic require rather the following:

• sizeof(ETH_DMADescTypeDef) / sizeof(uint32_t)

Correct implementation would then be:

-WRITE_REG(heth->Instance->DMACRDTPR, ((uint32_t)(heth->Init.RxDesc + (((uint32_t)(ETH_RX_DESC_CNT - 1))*sizeof(ETH_DMADescTypeDef)))));
+WRITE_REG(heth->Instance->DMACRDTPR, ((uint32_t)(heth->Init.RxDesc) + (((uint32_t)(ETH_RX_DESC_CNT - 1)) * (uint32_t)(sizeof(ETH_DMADescTypeDef) / sizeof(uint32_t)))));

With regards,

[pavel-a]

Ali, sorry for delay. So now there are two "correct" variants. (I assume that red line with - is wrong and green line with + is proposed correction).

I'm confused.... where is pointer arithmetic in (uint32_t)(heth->Init.RxDesc) ? If it were (uint32_t*)(heth->Init.RxDesc) then maybe... Lets' ask help from the ethernet experts on the forum.

Regards,
Pavel

[kele14x]

I think @ALABSTM is right. The correct calculation should be (uint32_t)(heth->Init.RxDesc) + offset_bytes

I meet this issue and debug for half of day. I can't believe this bug is not fixed for this long time.

[ALABSTM]

Hi Pavel,

Pointer arithmetic arrives on the scene because structure member RxDesc is actually a pointer to the first DMA Rx descriptor.

STM32CubeH7/Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_hal_eth.h

Line 378 in 79196b0

ETH_DMADescTypeDef *RxDesc; /*!< Provides the address of the first DMA Rx descriptor in the list */

Hence, as RxDesc points on a ETH_DMADescTypeDef structure, expression (uint32_t)(heth->Init.RxDesc + (uint32_t)1), for instance, will be automatically reevaluated @firstRxDesc + sizeof(ETH_DMADescTypeDef).

To conclude, first fix suggestion is the correct one as confirmed by @kele14x.

-WRITE_REG(heth->Instance->DMACRDTPR, ((uint32_t)(heth->Init.RxDesc + (((uint32_t)(ETH_RX_DESC_CNT - 1))*sizeof(ETH_DMADescTypeDef)))));
+WRITE_REG(heth->Instance->DMACRDTPR, ((uint32_t)(heth->Init.RxDesc) + ((uint32_t)(ETH_RX_DESC_CNT - 1))));

The above expression (in green) will be automatically reevaluated @firstRxDesc + (ETH_RX_DESC_CNT - 1) * sizeof(ETH_DMADescTypeDef).

I hope this makes things clearer. Please do not hesitate if you still have questions about the suggested fix.

With regards,

[pavel-a]

Ali, @kele14x, with all respect it seems there's some misunderstanding.
I put a little example here to explain: rextester.com/LRKA74918
As soon as you cast a pointer to a number (uint32_t)(heth->Init.RxDesc), it becomes a number and pointer stuff no longer applies to it.

However, as @alister commented on the forum, it looks like there are multiple variants of setting this register, depending on the overall design of the driver. If the person who proposed the correction will provide also a thorough fix of the ETH driver, he/she likely knows better. If such fix won't be provided, this issue alone is not important.

Regards,
Pavel A.

[ALABSTM]

Hi Pavel,

I confirm there is a misunderstanding and a typo (from my side) too. In the expression I reported of the suggested fix, parenthesis are not correctly placed. With such an expression, pointer arithmetic applies no more. You are absolutely right about it.

The fix would rather be:

-WRITE_REG(heth->Instance->DMACRDTPR, ((uint32_t)(heth->Init.RxDesc + (((uint32_t)(ETH_RX_DESC_CNT - 1))*sizeof(ETH_DMADescTypeDef)))));
+WRITE_REG(heth->Instance->DMACRDTPR, ((uint32_t)(heth->Init.RxDesc + (uint32_t)(ETH_RX_DESC_CNT - 1))));

I hope you agree on what is written here. I do apologize for the confusion caused.

With regards,

[pavel-a]

Ali, no problem, thanks for checking. The bigger question now is, how about a deeper revision of the eth driver? When this could be expected?

• P.

[tushartp]

Hello Guys
I have question if we are changing the RX descriptor tail pointer address.
Do you think the TX descriptors should also be change from

/* Set Transmit Descriptor Tail pointer /
(Line 2663) WRITE_REG(heth->Instance->DMACTDTPR, (uint32_t) heth->Init.TxDesc);
to
/ Set Transmit Descriptor Tail pointer */
(Line 2663) WRITE_REG(heth->Instance->DMACTDTPR, ((uint32_t)(heth->Init.TxDesc + (uint32_t)(ETH_TX_DESC_CNT - 1))));

Please let me know if there is any difference in TX and RX descriptors. I am not expert on this Ethernet HAL handling on stm32h7.

Thanks
Tushar

[pavel-a]

@tushartp You're welcome to ask this in the forum.

[thomask77]

To make the code's intention clearer, I would suggest

WRITE_REG(heth->Instance->DMACRDTPR, (uint32_t)(&heth->Init.RxDesc[ETH_RX_DESC_CNT - 1]));

Much clearer, and doesn't require knowledge of pointer arithmetic rules.