Skip to content

drivers: ethernet: use net_if_carrier correct #100809

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.

Sign up for GitHub

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

Open
maass-hamburg wants to merge 2 commits into
base: main
Choose a base branch
from
+6 −43
Open

drivers: ethernet: use net_if_carrier correct #100809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7fb1fa2
drivers: ethernet: nxp: correct net_if_carrier use
maass-hamburg Dec 10, 2025
f2aac10
usb: device_next: usbd_cdc_ncm: correct net_if_carrier use
maass-hamburg Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 2 additions & 42 deletions drivers/ethernet/eth_nxp_s32_gmac.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ struct eth_nxp_s32_config {
struct eth_nxp_s32_data {
struct net_if *iface;
uint8_t mac_addr[ETH_NXP_S32_MAC_ADDR_LEN];
uint8_t if_suspended;
struct k_mutex tx_mutex;
struct k_sem rx_sem;
struct k_sem tx_sem;
Expand Down Expand Up @@ -119,13 +118,6 @@ static void phy_link_state_changed(const struct device *pdev,

cfg->base->MAC_CONFIGURATION |= GMAC_MAC_CONFIGURATION_DM(gmac_cfg.Duplex);

/* net iface should be down even if PHY link state is up
* till the upper network layers have suspended the iface.
*/
if (ctx->if_suspended) {
return;
}

LOG_DBG("Link up");
net_eth_carrier_on(ctx->iface);
} else {
Expand Down Expand Up @@ -230,33 +222,12 @@ static int eth_nxp_s32_init(const struct device *dev)
static int eth_nxp_s32_start(const struct device *dev)
{
const struct eth_nxp_s32_config *cfg = dev->config;
struct eth_nxp_s32_data *ctx = dev->data;
struct phy_link_state state;

Gmac_Ip_EnableController(cfg->instance);

irq_enable(cfg->rx_irq);
irq_enable(cfg->tx_irq);

/* If upper layers enable the net iface then mark it as
* not suspended so that PHY Link changes can have the impact
*/
ctx->if_suspended = false;

if (cfg->phy_dev) {
phy_get_link_state(cfg->phy_dev, &state);

/* Enable net_iface only when Ethernet PHY link is up or else
* if net_iface is enabled when link is down and tx happens
* in this state then the used tx buffers will never be recovered back.
*/
if (state.is_up == true) {
net_eth_carrier_on(ctx->iface);
}
} else {
net_eth_carrier_on(ctx->iface);
}

LOG_DBG("GMAC%d started", cfg->instance);

return 0;
Expand All @@ -265,20 +236,12 @@ static int eth_nxp_s32_start(const struct device *dev)
static int eth_nxp_s32_stop(const struct device *dev)
{
const struct eth_nxp_s32_config *cfg = dev->config;
struct eth_nxp_s32_data *ctx = dev->data;
Gmac_Ip_StatusType status;
int err = 0;

irq_disable(cfg->rx_irq);
irq_disable(cfg->tx_irq);

/* If upper layers disable the net iface then mark it as suspended
* in order to save it from the PHY link state changes
*/
ctx->if_suspended = true;

net_eth_carrier_off(ctx->iface);

status = Gmac_Ip_DisableController(cfg->instance);
if (status != GMAC_STATUS_SUCCESS) {
LOG_ERR("Failed to disable controller GMAC%d (%d)", cfg->instance, status);
Expand Down Expand Up @@ -308,17 +271,14 @@ static void eth_nxp_s32_iface_init(struct net_if *iface)
ctx->mac_addr[0], ctx->mac_addr[1], ctx->mac_addr[2],
ctx->mac_addr[3], ctx->mac_addr[4], ctx->mac_addr[5]);

/* Make sure that the net iface state is not suspended unless
* upper layers explicitly stop the iface
*/
ctx->if_suspended = false;

/* No PHY available, link is always up and MAC speed/duplex settings are fixed */
if (cfg->phy_dev == NULL) {
net_if_carrier_on(iface);
return;
}

net_eth_carrier_off(iface);

/*
* GMAC controls the PHY. If PHY is configured either as fixed
* link or autoneg, the callback is executed at least once
Expand Down
5 changes: 4 additions & 1 deletion subsys/usb/device_next/class/usbd_cdc_ncm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,7 @@ static void ncm_handle_notifications(const struct device *dev, const int err)
if (data->if_state == IF_STATE_CONNECTION_STATUS_SUBMITTED) {
data->if_state = IF_STATE_CONNECTION_STATUS_SENT;
LOG_INF("Connection status sent");
net_if_carrier_on(data->iface);
}
}

Expand Down Expand Up @@ -840,6 +841,7 @@ static void usbd_cdc_ncm_update(struct usbd_class_data *const c_data,

if (data_iface == iface && alternate == 0) {
atomic_clear_bit(&data->state, CDC_NCM_DATA_IFACE_ENABLED);
net_if_carrier_off(data->iface);
data->tx_seq = 0;
data->rx_seq = 0;
}
Expand Down Expand Up @@ -868,6 +870,8 @@ static void usbd_cdc_ncm_disable(struct usbd_class_data *const c_data)
atomic_clear_bit(&data->state, CDC_NCM_DATA_IFACE_ENABLED);
atomic_clear_bit(&data->state, CDC_NCM_CLASS_SUSPENDED);

net_if_carrier_off(data->iface);

LOG_INF("Disabled %s", c_data->name);
}

Expand Down Expand Up @@ -1123,7 +1127,6 @@ static int cdc_ncm_iface_start(const struct device *dev)
LOG_DBG("Start interface %d", net_if_get_by_iface(data->iface));

atomic_set_bit(&data->state, CDC_NCM_IFACE_UP);
net_if_carrier_on(data->iface);
Copy link
Contributor

@tmon-nordic tmon-nordic Dec 10, 2025

Choose a reason for hiding this comment

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

This way it is no longer wrong. To really make use of it, net_if_carrier should be changed at the points where a connection is established or disconnected. But that is for later.

Aren't these points simply usbd_cdc_ncm_enable() and usbd_cdc_ncm_disable()? If yes, why not just put the calls there instead of commenting "that is for later".

Copy link
Contributor

@tmon-nordic tmon-nordic Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Or is it more like usbd_cdc_ncm_shutdown and .update callback in struct usbd_class_api which are bound to functional state (device configured)?

Copy link
Member Author

@maass-hamburg maass-hamburg Dec 11, 2025

Choose a reason for hiding this comment

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

I now changed it, so that it works with plugging the usb cable out and in. (tested on a renesas ek_ra8d1 with the zperf sample and the provided confs and overlays)


if (atomic_test_bit(&data->state, CDC_NCM_DATA_IFACE_ENABLED)) {
(void)k_work_reschedule(&data->notif_work, K_MSEC(1));
Expand Down