Skip to content

drivers/kw41zrf: don't re-get UIDs during reset#14363

Closed
benemorius wants to merge 1 commit intoRIOT-OS:masterfrom
benemorius:pr/kw41zrf-uid-reset
Closed

drivers/kw41zrf: don't re-get UIDs during reset#14363
benemorius wants to merge 1 commit intoRIOT-OS:masterfrom
benemorius:pr/kw41zrf-uid-reset

Conversation

@benemorius
Copy link
Member

Contribution description

This prevents kw41zrf from changing the long and short addresses during ifconfig 7 set state reset.

Testing procedure

  • Using gnrc_networking example, run ifconfig and observe addresses.
  • Run ifconfig 7 set state reset
  • Run ifconfig again and observe whether the long and short addresses have changed.
  • Without this PR, the addresses will have changed.

@benpicco benpicco requested review from jia200x and maribu June 26, 2020 10:44
@benpicco
Copy link
Contributor

This problem affects all drivers, I'd rather have a more general solution.

@miri64 miri64 added Area: drivers Area: Device drivers Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jul 2, 2020
@miri64
Copy link
Member

miri64 commented Jul 2, 2020

Partly fixes #9656, but I agree with @benpicco, a more holistic solution would be better.

@maribu
Copy link
Member

maribu commented Jul 2, 2020

This problem affects all drivers, I'd rather have a more general solution.

👍

I think it is an issue with the architecture of the drivers. IMO, the address should be generated once during <FOO>_init() or <FOO>_setup(). In <FOO>_reset() only the already generated address should be applied to the hardware. In case of IEEE 802.15.4 transceivers, one might be tempted to introduce an netdev_ieee802154_init() to do just this and have the individual 802.15.4 drivers call this during their <FOO>_init() functions. (Ideally consistently e.g. as first line in the function body.)

@benpicco
Copy link
Contributor

benpicco commented Jul 9, 2020

#13746 will assign IDs to netdevs, this way we can always request the same ID from luid_custom without having to store the result.

@maribu
Copy link
Member

maribu commented Oct 8, 2020

#13746 will assign IDs to netdevs, this way we can always request the same ID from luid_custom without having to store the result.

IMO this not the solution to the problem here. If the user changes the L2 address by hand and triggers a reset (e.g. both could be done via the ifconfig shell command), one would expect the device to still have the assigned address. And the network stack store the L2 addresses in RAM anyway - so there is no cost.

But we shouldn't do something like

void foo_init(foo_dev_t *dev)
{
    /* ... */
    foo_reset(dev);
    /* ... */
}

void foo_reset(foo_dev_t *dev)
{
    static bool is_first_reset = 1;
    /* ... */
    if (is_first_reset) {
        is_first_reset = 0;
        generate_new_addr();
    }
    apply_addr();
}

but instead:

void foo_init(foo_dev_t *dev)
{
    /* ... */
    generate_new_addr();
    foo_reset(dev);
    /* ... */
}

void foo_reset(foo_dev_t *dev)
{
    /* ... */
    apply_addr();
}

(Or have the L2 addr generate in the foo_setup() rather than the foo_init().) The latter would have no cost in terms of RAM or ROM and fixes the issue.

Still, #13746 provides stable L2 addresses (regardless of number and order of network devices). So #13746 solves a problem, but not this.

@maribu
Copy link
Member

maribu commented Oct 8, 2020

So #13746 solves a problem, but not this.

Now it does solve this as well :-)

@benpicco
Copy link
Contributor

Should be fixed now.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@fjmolinas
Copy link
Contributor

Tested on master this is no longer an issuse:

2021-11-18 10:40:18,454 # Iface  6  HWaddr: 04:42  Channel: 26  Page: 0  NID: 0x23 
2021-11-18 10:40:18,458 #           Long HWaddr: 42:84:62:A2:5A:ED:6D:A6 
2021-11-18 10:40:18,466 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2021-11-18 10:40:18,471 #           ACK_REQ  CSMA  L2-PDU:102  MTU:1280  HL:64  RTR  
2021-11-18 10:40:18,472 #           6LO  IPHC  
2021-11-18 10:40:18,476 #           Source address length: 8
2021-11-18 10:40:18,478 #           Link type: wireless
2021-11-18 10:40:18,485 #           inet6 addr: fe80::4084:62a2:5aed:6da6  scope: link  VAL
2021-11-18 10:40:18,487 #           inet6 group: ff02::2
2021-11-18 10:40:18,491 #           inet6 group: ff02::1
2021-11-18 10:40:18,493 #           inet6 group: ff02::1:ffed:6da6
2021-11-18 10:40:18,495 #           
2021-11-18 10:40:18,498 #           Statistics for Layer 2
2021-11-18 10:40:18,501 #             RX packets 0  bytes 0
2021-11-18 10:40:18,506 #             TX packets 2 (Multicast: 2)  bytes 86
2021-11-18 10:40:18,509 #             TX succeeded 2 errors 0
2021-11-18 10:40:18,511 #           Statistics for IPv6
2021-11-18 10:40:18,515 #             RX packets 0  bytes 0
2021-11-18 10:40:18,519 #             TX packets 2 (Multicast: 2)  bytes 128
2021-11-18 10:40:18,521 #             TX succeeded 2 errors 0
2021-11-18 10:40:18,522 # 
> ifconfig 6 set state reset
2021-11-18 10:40:24,170 # ifconfig 6 set state reset
2021-11-18 10:40:24,176 # success: set state of interface 6 to RESET
> ifconfig
2021-11-18 10:40:25,681 # ifconfig
2021-11-18 10:40:25,686 # Iface  6  HWaddr: 04:42  Channel: 26  Page: 0  NID: 0x23 
2021-11-18 10:40:25,691 #           Long HWaddr: 42:84:62:A2:5A:ED:6D:A6 
2021-11-18 10:40:25,698 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2021-11-18 10:40:25,703 #           ACK_REQ  CSMA  L2-PDU:102  MTU:1280  HL:64  RTR  
2021-11-18 10:40:25,705 #           6LO  IPHC  
2021-11-18 10:40:25,708 #           Source address length: 2
2021-11-18 10:40:25,711 #           Link type: wireless
2021-11-18 10:40:25,717 #           inet6 addr: fe80::4084:62a2:5aed:6da6  scope: link  VAL
2021-11-18 10:40:25,719 #           inet6 group: ff02::2
2021-11-18 10:40:25,722 #           inet6 group: ff02::1
2021-11-18 10:40:25,726 #           inet6 group: ff02::1:ffed:6da6
2021-11-18 10:40:25,727 #           
2021-11-18 10:40:25,730 #           Statistics for Layer 2
2021-11-18 10:40:25,733 #             RX packets 0  bytes 0
2021-11-18 10:40:25,737 #             TX packets 3 (Multicast: 3)  bytes 129
2021-11-18 10:40:25,740 #             TX succeeded 3 errors 0
2021-11-18 10:40:25,743 #           Statistics for IPv6
2021-11-18 10:40:25,746 #             RX packets 0  bytes 0
2021-11-18 10:40:25,751 #             TX packets 3 (Multicast: 3)  bytes 192
2021-11-18 10:40:25,754 #             TX succeeded 3 errors 0
2021-11-18 10:40:25,754 # 

@fjmolinas fjmolinas closed this Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: drivers Area: Device drivers 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.

6 participants