Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/enc28j60: fix ISR routine and bth #19461

Merged
merged 2 commits into from
Apr 12, 2023
Merged

Conversation

peteut
Copy link
Contributor

@peteut peteut commented Apr 10, 2023

Contribution description

When the /drivers/enc28j60 driver was in use, many resets have been observed. A closer look revealed there is an SPI access in ISR context, which can lead to deadly embraces:

cmd_bfc((enc28j60_t *)arg, REG_EIE, -1, EIE_INTIE);

To fix this issue, the SPI access has been moved to thread context.

Another potential issue is the Receive Packet Pending Interrupt Flag (PKTIF), which is not reliable. Refer to
ENC28J60 Silicon Errata and Data Sheet Clarification, section 6 Module: Interrupts.

To address this issue, the proposed workaround from the errata has been implemented.

Testing procedure

./RIOT/dist/tools/benchmark_udp run on the host and USEMODULE += benchmark_udp on the target has been used for testing. No more freezes have been observed. The network stack in use was lwip.

A patch has been applied to test on IP4:

diff --git a/sys/test_utils/benchmark_udp/benchmark_udp.c b/sys/test_utils/benchmark_udp/benchmark_udp.c
index dd41972508..d8043c4556 100644
--- a/sys/test_utils/benchmark_udp/benchmark_udp.c
+++ b/sys/test_utils/benchmark_udp/benchmark_udp.c
@@ -144,11 +144,10 @@ static void *_send_thread(void *ctx)
 
 int benchmark_udp_start(const char *server, uint16_t port)
 {
-    netif_t *netif;
-    sock_udp_ep_t local = { .family = AF_INET6,
+    sock_udp_ep_t local = { .family = AF_INET,
                             .netif = SOCK_ADDR_ANY_NETIF,
                             .port = port };
-    sock_udp_ep_t remote = { .family = AF_INET6,
+    sock_udp_ep_t remote = { .family = AF_INET,
                              .port = port };
 
     /* stop threads first */
@@ -159,15 +158,11 @@ int benchmark_udp_start(const char *server, uint16_t port)
         return 1;
     }
 
-    if (netutils_get_ipv6((ipv6_addr_t *)&remote.addr.ipv6, &netif, server) < 0) {
+    if (netutils_get_ipv4((ipv4_addr_t *)&remote.addr.ipv4, server) < 0) {
         puts("can't resolve remote address");
         return 1;
     }
-    if (netif) {
-        remote.netif = netif_get_id(netif);
-    } else {
-        remote.netif = SOCK_ADDR_ANY_NETIF;
-    }
+    remote.netif = SOCK_ADDR_ANY_NETIF;
 
     running = true;
     thread_create(listen_thread_stack, sizeof(listen_thread_stack),
$ make -C $RIOTBASE/dist/tools/benchmark_udp/ run
...
host                	bandwidth	num TX	num RX		num RT		RTT	pkg size
::ffff:192.168.1.118	 477 b/s	25885	25885 (100%)	25879 (99%)	1918 µs	48
$ make info-stm32 
CPU: stm32wb55vg
	Line: STM32WB55xx
	Pin count:	
	ROM size:	824K (843776 Bytes)
	RAM size:	256KiB
$ make info-packages 
lwip
tinyusb

Issues/PRs references

Fixes #17593.

@github-actions github-actions bot added the Area: drivers Area: Device drivers label Apr 10, 2023
@peteut peteut marked this pull request as draft April 10, 2023 15:48
@peteut peteut changed the title Enc28j60 fixes /drivers/enc28j60: fix ISR routine and bth Apr 10, 2023
@peteut peteut marked this pull request as ready for review April 10, 2023 16:35
@benpicco benpicco requested a review from maribu April 10, 2023 22:46
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 10, 2023
@benpicco benpicco changed the title /drivers/enc28j60: fix ISR routine and bth drivers/enc28j60: fix ISR routine and bth Apr 10, 2023
@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 10, 2023
@benpicco benpicco requested a review from jia200x April 10, 2023 22:51
@riot-ci
Copy link

riot-ci commented Apr 10, 2023

Murdock results

✔️ PASSED

226b8cf drivers/enc28j60: fix PKTIF issue.

Success Failures Total Runtime
6882 0 6882 09m:36s

Artifacts

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and you provided testing results.
Just a whitespace nitpick:

drivers/enc28j60/enc28j60.c Outdated Show resolved Hide resolved
The PKTIF does not reliably report the status of pending packags.
Apply the proposed workaround [1].

[1] https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/Errata/80349c.pdf
@benpicco
Copy link
Contributor

bors merge

bors bot added a commit that referenced this pull request Apr 12, 2023
19411: cpu/gd32v: add riotboot support r=benpicco a=gschorcht

### Contribution description

This PR provides `riotboot` support for GD32V.

### Testing procedure

Use any GD32V board with a JTAG adapter and flash the bootloader:
```python
PROGRAMMER=openocd BOARD=sipeed-longan-nano make -C bootloaders/riotboot flash
```
Flash slot 0 and set `RIOT_VERSION` to 1
```python
USEMODULE=stdio_uart FEATURES_REQUIRED=riotboot RIOT_VERSION=1 \
PROGRAMMER=openocd BOARD=sipeed-longan-nano make -C tests/shell riotboot/flash-slot0
...
### Flashing Target ###
Binfile detected, adding ROM base address: 0x08000000
Flashing with IMAGE_OFFSET: 0x08001000
```
```python
> main(): This is RIOT! (Version: 1)
test_shell.
```
Flash slot 1 and set `RIOT_VERSION` to 2
```python
USEMODULE=stdio_uart FEATURES_REQUIRED=riotboot RIOT_VERSION=2 \
PROGRAMMER=openocd BOARD=sipeed-longan-nano make -C tests/shell riotboot/flash-slot1
...
### Flashing Target ###
Binfile detected, adding ROM base address: 0x08000000
Flashing with IMAGE_OFFSET: 0x08010800
```
```python
> main(): This is RIOT! (Version: 2)
test_shell.
```

### Issues/PRs references


19432: boards/esp32: deduplication in common ESP32x board definitions r=benpicco a=gschorcht

### Contribution description

The PR reduced code duplication in `boards/common/esp32*`.

The PR moves the header files from `boards/common/esp32s3/include` that can be used for all types of ESP32x SoCs to a new common ESP32x board definition which is then included by all common ESP32x board definitions.

### Testing procedure

Green CI.

### Issues/PRs references


19461: drivers/enc28j60: fix ISR routine and bth r=benpicco a=peteut



Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Co-authored-by: Alain Péteut <alain.peteut@yahoo.com>
@bors
Copy link
Contributor

bors bot commented Apr 12, 2023

Build failed (retrying...):

@bors
Copy link
Contributor

bors bot commented Apr 12, 2023

Build succeeded:

@bors bors bot merged commit 99b13cb into RIOT-OS:master Apr 12, 2023
@peteut peteut deleted the enc28j60-fixes branch April 12, 2023 17:31
@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enc28j60: irq handling
3 participants