From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0C7B1CD1292 for ; Thu, 4 Apr 2024 08:38:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ynEo7rqsTearn/DuMRLxk4jK8FogOwnhv/fBWVrBu0c=; b=IJYra/0xlm27Ur 4hGEpxQ94r4ZS74eL3ecuIQK3NgndtcDOS6L9/eVJlUhMqVuyIsOCRmn9DfYlfEZdtvU7ByTnr72h 0GAypTo93goxDBOhxWR2QXpnn4v+e3PlyLQosMQkJX6gZyWZTFl3/UxWxnPQtHaJi6AZ3iaitusNt RyIse0CqwMRWGiAFDjKXb05bBk/LBwI/E+Rk/Y5qnHDwL6QwCK6ZeUeGk2Wd0t8xlh1Wk2klmvrx+ 5iar1kjwMsFAVCCCFqLIOOf+t/g4KqgAl7kq1+EA9n8hYnLD5cgTMefjCW8XUmTBTEfj3ELEFK3MB Xfqe//r9j/PmF1+RyLIA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rsIcL-00000001sNr-1c6X; Thu, 04 Apr 2024 08:38:41 +0000 Received: from relay8-d.mail.gandi.net ([217.70.183.201]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rsIcH-00000001sLR-1oLC for linux-arm-kernel@lists.infradead.org; Thu, 04 Apr 2024 08:38:39 +0000 Received: by mail.gandi.net (Postfix) with ESMTPSA id 432ED1BF20E; Thu, 4 Apr 2024 08:38:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1712219910; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=PgvOBYUv+YR+r+p5EFl1tLWF9neUclhq/ma3+kacwA4=; b=cOULh+T8sdtlXD7SmlUA0IOvIPd34zcwacPNaE0zSa5VGX4OnJOEhkwrLa9QOK7x7OVbbv eHe7yddb0K7kAcwEDUfSA/bS2zjHbxT/NbjaMEbfrptATARz8XdpV3yRyjwJ9kj6WbM00v 1v+C3V2x6Vop+WixAMMXz87zUSFd1wYUJx7bhZ9kXKM7BGZIo1jdUdAUTOhEv8Unr4o8SF mAZpWcb78nEWuV2bEtW/m0zprJ+3RHw5aA4gqjQodRMATI9v8TbAoEm+O9sXj5yJFY0e9L sBQaG/t1J2tcoLS6Bqi7Q3bYlkNtL3uq6EkYDQtz+WUD+IIoisd9Opo1Z1qobQ== Date: Thu, 4 Apr 2024 10:38:25 +0200 From: Maxime Chevallier To: Jakub Kicinski Cc: davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com, Andrew Lunn , Eric Dumazet , Paolo Abeni , Russell King , linux-arm-kernel@lists.infradead.org, Christophe Leroy , Herve Codina , Florian Fainelli , Heiner Kallweit , Vladimir Oltean , =?UTF-8?B?S8O2cnk=?= Maincent , Jesse Brandeburg , Jonathan Corbet , Marek =?UTF-8?B?QmVow7pu?= , Piergiorgio Beruto , Oleksij Rempel , =?UTF-8?B?Tmljb2zDsg==?= Veronese , Simon Horman , mwojtas@chromium.org Subject: Re: [PATCH net-next v10 07/13] net: ethtool: Introduce a command to list PHYs on an interface Message-ID: <20240404103825.6a596bec@device-28.home> In-Reply-To: <20240308211842.6d64b5bc@kernel.org> References: <20240304151011.1610175-1-maxime.chevallier@bootlin.com> <20240304151011.1610175-8-maxime.chevallier@bootlin.com> <20240308211842.6d64b5bc@kernel.org> Organization: Bootlin X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-GND-Sasl: maxime.chevallier@bootlin.com X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240404_013837_778331_97157EC8 X-CRM114-Status: GOOD ( 29.62 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hello Jakub, Sorry for the late reply... On Fri, 8 Mar 2024 21:18:42 -0800 Jakub Kicinski 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel