by Jakub Kicinski

[permalink] [raw]

Subject: Re: [PATCH net-next v10 13/13] Documentation: networking: document phy_link_topology

We should :ref: to this doc from the PHY_GET in the ethtool one as well?

On Mon, 4 Mar 2024 16:10:09 +0100 Maxime Chevallier wrote:
> +An Ethernet Interface from userspace's point of view is nothing but a

interface

> +:c:type:`struct net_device <net_device>`, which exposes configuration options
> +through the legacy ioctls and the ethool netlink commands. The base assumption

ethtool

> +when designing these configuration channels were that the link looked

nit: s/channels/APIs/ channels sometimes means IRQs/queues in netdev :S
s/looked/looks/

> +something like this ::

s/this//

> + +-----------------------+ +----------+ +--------------+
> + | Ethernet Controller / | | Ethernet | | Connector / |
> + | MAC | ------ | PHY | ---- | Port | ---... to LP
> + +-----------------------+ +----------+ +--------------+
> + struct net_device struct phy_device
> +
> +Commands that needs to configure the PHY will go through the net_device.phydev
> +field to reach the PHY and perform the relevant configuration.
> +
> +This assumption falls apart in more complex topologies that can arise when,
> +for example, using SFP transceivers (although that's not the only specific case).

s/specific/such/

> +Here, we have 2 basic scenarios. Either the MAC is able to output a serialized
> +interface, that can directly be fed to an SFP cage, such as SGMII, 1000BaseX,
> +10GBaseR, etc.

The "Either" makes me expect and "or" at some state in this paragraph..

> +The link topology then looks like this (when an SFP module is inserted) ::
> +
> + +-----+ SGMII +------------+
> + | MAC | ------- | SFP Module |
> + +-----+ +------------+
> +
> +Knowing that some modules embed a PHY, the actual link is more like ::
> +
> + +-----+ SGMII +--------------+
> + | MAC | -------- | PHY (on SFP) |
> + +-----+ +--------------+
> +
> +In this case, the SFP PHY is handled by phylib, and registered by phylink through
> +its SFP upstream ops.
> +
> +Now some Ethernet controllers aren't able to output a serialized interface, so
> +we can't directly connect them to an SFP cage. However, some PHYs can be used

s/However, some/In such cases, certain/

> +as media-converters, to translate the non-serialized MAC MII interface to a
> +serialized MII interface fed to the SFP ::
> +
> + +-----+ RGMII +-----------------------+ SGMII +--------------+
> + | MAC | ------- | PHY (media converter) | ------- | PHY (on SFP) |
> + +-----+ +-----------------------+ +--------------+

2024-04-04 08:38:51

by Maxime Chevallier

[permalink] [raw]

Subject: Re: [PATCH net-next v10 07/13] net: ethtool: Introduce a command to list PHYs on an interface

Hello Jakub,

Sorry for the late reply...

On Fri, 8 Mar 2024 21:18:42 -0800
Jakub Kicinski <[email protected]> wrote:

> On Mon, 4 Mar 2024 16:10:03 +0100 Maxime Chevallier wrote:
> > +PHY_GET
> > +=======
> > +
> > +Retrieve information about a given Ethernet PHY sitting on the link. As there
> > +can be more than one PHY, the DUMP operation can be used to list the PHYs
> > +present on a given interface, by passing an interface index or name in
> > +the dump request
>
> Could be worth re-stating the default behavior of the DO request?
> That the DO will return the dev->phydev by default and if PHY_INDEX
> is specified in the header, the particular PHY with that index?
>
> > + if (phydev->drv && nla_put_string(skb, ETHTOOL_A_PHY_DRVNAME, phydev->drv->name))
>
> could you break the lines at 80 where it doesn't hurt readability?
> This way:
>
> if (phydev->drv &&
> nla_put_string(skb, ETHTOOL_A_PHY_DRVNAME, phydev->drv->name))

Ack, will do

>
> > + return -EMSGSIZE;
> > +
> > + if (ptype == PHY_UPSTREAM_PHY) {
> > + struct phy_device *upstream = pdn->upstream.phydev;
> > + const char *sfp_upstream_name;
> > +
> > + /* Parent index */
> > + if (nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_INDEX, upstream->phyindex))
> > + return -EMSGSIZE;
> > +
> > + if (pdn->parent_sfp_bus) {
> > + sfp_upstream_name = sfp_get_name(pdn->parent_sfp_bus);
> > + if (sfp_upstream_name && nla_put_string(skb,
> > + ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,
> > + sfp_upstream_name))
>
> ditto

Indeed :)

>
> > + return -EMSGSIZE;
> > + }
> > + }
>
> > +int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > + struct phy_req_info req_info = {};
> > + struct nlattr **tb = info->attrs;
> > + struct sk_buff *rskb;
> > + void *reply_payload;
> > + int reply_len;
> > + int ret;
> > +
> > + ret = ethnl_parse_header_dev_get(&req_info.base,
> > + tb[ETHTOOL_A_PHY_HEADER],
> > + genl_info_net(info), info->extack,
> > + true);
> > + if (ret < 0)
> > + return ret;
> > +
> > + rtnl_lock();
> > +
> > + ret = ethnl_phy_parse_request(&req_info.base, tb);
> > + if (ret < 0)
> > + goto err_unlock_rtnl;
> > +
> > + /* No PHY, return early */
> > + if (!req_info.pdn.phy)
> > + goto err_unlock_rtnl;
> > +
> > + ret = ethnl_phy_reply_size(&req_info.base, info->extack);
> > + if (ret < 0)
> > + goto err_unlock_rtnl;
> > + reply_len = ret + ethnl_reply_header_size();
> > +
> > + rskb = ethnl_reply_init(reply_len, req_info.base.dev,
> > + ETHTOOL_MSG_PHY_GET_REPLY,
> > + ETHTOOL_A_PHY_HEADER,
> > + info, &reply_payload);
> > + if (!rskb) {
> > + ret = -ENOMEM;
> > + goto err_unlock_rtnl;
> > + }
> > +
> > + ret = ethnl_phy_fill_reply(&req_info.base, rskb);
> > + if (ret)
> > + goto err_free_msg;
> > +
> > + rtnl_unlock();
> > + ethnl_parse_header_dev_put(&req_info.base);
> > + genlmsg_end(rskb, reply_payload);
> > +
> > + return genlmsg_reply(rskb, info);
> > +
> > +err_free_msg:
> > + nlmsg_free(rskb);
> > +err_unlock_rtnl:
> > + rtnl_unlock();
> > + ethnl_parse_header_dev_put(&req_info.base);
> > + return ret;
> > +}
> > +
> > +struct ethnl_phy_dump_ctx {
> > + struct phy_req_info *phy_req_info;
> > + unsigned long ifindex;
> > + unsigned long phy_index;
> > +};
> > +
> > +int ethnl_phy_start(struct netlink_callback *cb)
> > +{
> > + const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>
> You can save some chars by using genl_info_dump() here
>
> > + struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
> > + struct nlattr **tb = info->info.attrs;
>
> then you can ditch this
>
> > + int ret;
> > +
> > + BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
> > +
> > + ctx->phy_req_info = kzalloc(sizeof(*ctx->phy_req_info), GFP_KERNEL);
> > + if (!ctx->phy_req_info)
> > + return -ENOMEM;
> > +
> > + ret = ethnl_parse_header_dev_get(&ctx->phy_req_info->base,
> > + tb[ETHTOOL_A_PHY_HEADER],
>
> and:
>
> info->attrs[ETHTOOL_A_PHY_HEADER],

Nice improvement, thanks !

>
> > + sock_net(cb->skb->sk), cb->extack,
> > + false);
>
> leaking ctx->phy_req_info on error?

Ah true, I relied on .done() but if ret is an error, it won't be
called... thanks !

>
> > + ctx->ifindex = 0;
> > + ctx->phy_index = 0;
> > + return ret;
> > +}
>
> > +static int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
> > + struct netlink_callback *cb)
> > +{
> > + struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
> > + struct phy_req_info *pri = ctx->phy_req_info;
> > + struct phy_device_node *pdn;
> > + int ret = 0;
> > + void *ehdr;
> > +
> > + pri->base.dev = dev;
> > +
> > + xa_for_each_start(&dev->link_topo->phys, ctx->phy_index, pdn, ctx->phy_index) {
> > + ehdr = ethnl_dump_put(skb, cb,
> > + ETHTOOL_MSG_PHY_GET_REPLY);
>
> this one OTOH fits on a line :)
>
> > + if (!ehdr) {
> > + ret = -EMSGSIZE;
> > + break;
> > + }
> > +
> > + ret = ethnl_fill_reply_header(skb, dev,
> > + ETHTOOL_A_PHY_HEADER);
>
> ditto

Ack for both :)

>
> > + if (ret < 0) {
> > + genlmsg_cancel(skb, ehdr);
> > + break;
> > + }
> > +
> > + memcpy(&pri->pdn, pdn, sizeof(*pdn));
> > + ret = ethnl_phy_fill_reply(&pri->base, skb);
>
> On a DO fill() shouldn't fail, because we size the skb, but if we pack
> many entries into the skb on a DUMP it can. So:
>
> if (ret < 0) {
> genlmsg_cancel(skb, ehdr);
> break;
> }
>
> no?

True, I'll add that

>
> > + genlmsg_end(skb, ehdr);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> > +{
> > + struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
> > + struct net *net = sock_net(skb->sk);
> > + struct net_device *dev;
> > + int ret = 0;
> > +
> > + rtnl_lock();
> > +
> > + if (ctx->phy_req_info->base.dev) {
> > + ret = ethnl_phy_dump_one_dev(skb, ctx->phy_req_info->base.dev, cb);
> > + } else {
> > + for_each_netdev_dump(net, dev, ctx->ifindex) {
> > + ret = ethnl_phy_dump_one_dev(skb, dev, cb);
> > + if (ret)
> > + break;
> > +
> > + ctx->phy_index = 0;
> > + }
> > + }
> > + rtnl_unlock();
> > +
> > + if (ret == -EMSGSIZE && skb->len)
> > + return skb->len;
>
> you can remove this if thanks to (very recent) commit b5a899154aa9

Ah nice ! thanks :)

>
> > + return ret;
> > +}
>
> Very sorry for the late review, BTW :(

No worries. I'll send a new version very shortly, taking all you
comments into account, thanks for taking a look at this !

Maxime