Skip to content

net: shell: iface: Add link speed settings command #90374

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
wants to merge 1 commit into
base: main
Choose a base branch
from
+84 −0
Open

net: shell: iface: Add link speed settings command #90374

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
84 changes: 84 additions & 0 deletions subsys/net/lib/shell/iface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <zephyr/logging/log.h>
#include <zephyr/random/random.h>
#include <strings.h>
#include <stdlib.h>
LOG_MODULE_DECLARE(net_shell);

#include <zephyr/sys/base64.h>
Expand Down Expand Up @@ -817,6 +818,82 @@ static int cmd_net_default_iface(const struct shell *sh, size_t argc, char *argv
return 0;
}

#if defined(CONFIG_ETH_PHY_DRIVER)
static int cmd_net_link_speed(const struct shell *sh, size_t argc, char *argv[])
{
int idx = get_iface_idx(sh, argv[1]);
const struct device *phy_dev;
bool half_duplex = false;
uint16_t user_input_spd;
struct net_if *iface;
uint16_t speed = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uint16_t speed = 0;
uint16_t speed = 0U;


if (argc < 3) {
PR_WARNING("Usage: net iface set_link <index>"
Copy link
Member

Choose a reason for hiding this comment

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

I think there should be a space between index and speed examples. So

Suggested change
PR_WARNING("Usage: net iface set_link <index>"
PR_WARNING("Usage: net iface set_link <index> "

"<Speed:10/100/1000/2500/5000> <Duplex[optional]:h/f>\n");
Copy link
Member

Choose a reason for hiding this comment

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

Optional parameters are usually marked by [] so here you could say

Suggested change
"<Speed:10/100/1000/2500/5000> <Duplex[optional]:h/f>\n");
"<Speed:10/100/1000/2500/5000> [Duplex:h/f]\n");

return -ENOEXEC;
}

iface = net_if_get_by_index(idx);
if (net_if_l2(iface) != &NET_L2_GET_NAME(ETHERNET)) {
PR_WARNING("Interface %d is not Ethernet type\n", idx);
return -EINVAL;
}

phy_dev = net_eth_get_phy(iface);
if (!phy_dev) {
PR_WARNING("No PHY device found for interface %d\n", idx);
return -ENOEXEC;
}

for (int k = 2; k < argc; k++) {
if ((k + 1 < argc) && (argv[k+1][0] == 'h')) {
half_duplex = true;
} else {
half_duplex = false;
}

user_input_spd = atoi(argv[k]);
Copy link
Member

Choose a reason for hiding this comment

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

We should not use atoi() in the shell modules and have been trying to get rid of them. Please use shell_strtol() instead and check the return values so that we can catch invalid strings.

switch (user_input_spd) {
case 0:
break;
case 10:
speed |= half_duplex ? LINK_HALF_10BASE : LINK_FULL_10BASE;
break;
case 100:
speed |= half_duplex ? LINK_HALF_100BASE : LINK_FULL_100BASE;
break;
case 1000:
speed |= half_duplex ? LINK_HALF_1000BASE : LINK_FULL_1000BASE;
break;
case 2500:
if (half_duplex) {
PR_WARNING("2500BASE half-duplex not supported\n");
return -ENOTSUP;
}
speed |= LINK_FULL_2500BASE;
break;
case 5000:
if (half_duplex) {
PR_WARNING("5000BASE half-duplex not supported\n");
return -ENOTSUP;
}
speed |= LINK_FULL_5000BASE;
break;
default:
PR_WARNING("Unsupported speed %d\n", user_input_spd);
return -ENOTSUP;
}
}

if (speed) {
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid checking non boolean values like this. It certainly works but there is a MISRA rule that says we should not try to do that, atm there is no automated checker for these but if found in the review this will be complained. So better is to say

Suggested change
if (speed) {
if (speed != 0U) {

return phy_configure_link(phy_dev, speed);
}
PR_WARNING("No speed specified\n");
return -ENOEXEC;
}
#endif /* CONFIG_ETH_PHY_DRIVER */

#if defined(CONFIG_NET_SHELL_DYN_CMD_COMPLETION)

#include "iface_dynamic.h"
Expand All @@ -841,6 +918,13 @@ SHELL_STATIC_SUBCMD_SET_CREATE(net_cmd_iface,
SHELL_CMD(default, IFACE_DYN_CMD,
"'net iface default [<index>]' displays or sets the default network interface.",
cmd_net_default_iface),
#if defined(CONFIG_ETH_PHY_DRIVER)
SHELL_CMD(set_link, IFACE_DYN_CMD,
"'net iface set_link <index> <Speed 10/100/1000/2500/5000> "
"<Duplex[optional]:h/f>'"
" sets link speed for the network interface.",
cmd_net_link_speed),
#endif /* CONFIG_ETH_PHY_DRIVER */
SHELL_SUBCMD_SET_END
);

Expand Down