cpu/stm32/periph_eth: fix typo in initialization code

[maribu]

Contribution description

A single character type resulted in way fewer TX descriptors being available than allocated. Not only resulted this in wasting memory, but also when more iolist chunks than descriptors are send, the

    assert(iolist_count(iolist) <= ETH_TX_DESCRIPTOR_COUNT);

does not trigger. As a result, old TX descriptors are being overwritten in this case.

Testing procedure

@fabian18 has a test that reliably triggers the bug. He should be able to confirm it is gone now. But actually, it should be pretty obvious from the code that this fixes a typo.

Issues/PRs references

None

[benpicco]

Good catch!

[maribu]

Good catch!

took many hours of debugging over the course of a couple of days 😢

@chrysn: Since I need to ping you anyway due to this being backport material: Would this be a memory corruption that rust could catch? The corruption takes place in the DMA, not in the CPU, so this may be tricky to check.

[maribu]

The PR and commit description makes zero sense.

[maribu]

@chrysn: Since I need to ping you anyway due to this being backport material: Would this be a memory corruption that rust could catch? The corruption takes place in the DMA, not in the CPU, so this may be tricky to check.

No, it doesn't. The typo is in the index, not in the variable name. I was jumping to incorrect conclusions here.

@fabian18 gnrc_ndp_opt_sl2a_build adds new pktsnip_t *s (or from the point of the netdev new io_list_t * elements). Hence, it results in overflowing the 8 possible chunks the STM32 can send per frame without triggering the assert() that is indented to for users to increase ETH_TX_DESCRIPTOR_COUNT so that more chunks can be send per frame.

[chrysn]

On the Rust question: Depends on how it'd be implemented. Bounds checks are generally done in Rust (with provisions to optimize them out often), so if it were to write out of bounds, that would have resulted in a runtime error, or in this case likely even a build error. In the "wasting memory" case, if I understand it correctly, it wouldn't have caught anything, but there's also nothing worse happening than not using some memory.

Took me a while to see what's happening, but yes I agree this is backport material. As we need to have an RC3 anyway due to #18413, this is not creating much overhead. I'm taking the liberty to prioritize this in the build queue to enable a speedy backport. (edit: only moved #18403 as the others should be quick enough; #18396 turned out to be 200 and not 20 tests but was already some way in when I saw that)

[chrysn]

No, it doesn't. The typo is in the index

Oh, I missed that part. If the error were in the name ... well, I think it's hard to compare, for in Rust that list might be initialized statically, so there'd be no danger of confusion. Such linked lists are generally not very idiomatic Rust, though, and if DMA requires them, it might be that one'd use and initialize them either through some local unsafe code (in which things like that could easily happen) or an abstraction for this kind of DMA lists.

[fabian18]

Hurray, thank you for spotting and fixing this!

[fabian18]

restarted Murdock, due to binary hashes mismatching with and without Kconfig

[maribu]

Thx :)

[maribu]

Backport provided in #18420