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 54DD8C433EF for ; Thu, 7 Jul 2022 20:25:50 +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:In-Reply-To:MIME-Version:References: 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=NYvFYkvmpQLwh+Ad+azRYovGz9L1l6JUIvyyh95FNIU=; b=SZcByvAkIZ6DO2 m6EqEgghdTC+YcLcBFDlA3P+UfxudqxtWTyqmW900+W8dVMixSM1+Yj0uvX/2lPABNbwq+YRT+E5t KgB9k7hxokinO8HkUzTfXg7TZHhApb9ajKOAImmPimr6r+Zc1Z+8f9su0uzWfKjjhLuqvBKrkgZrq 9rQNS/Pnbrz/Fbc33YPD/enG1hBC9R3jSKJeE7tJCoc9ZvusYmH3XO9KmuIMb+E7Zpvo83s8v3WO4 PnWj4Ke//88gEkjxozFOPtwOIUeSpOSIuzzFNPFJD2CcIRo2g6vO1PD1mKGkjlYc2IkIttSNZ/EsE FQYEcbAO0shgXDvk0JCg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1o9Y2z-00061a-H5; Thu, 07 Jul 2022 20:24:25 +0000 Received: from pandora.armlinux.org.uk ([2001:4d48:ad52:32c8:5054:ff:fe00:142]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1o9Y2u-00060E-Ab; Thu, 07 Jul 2022 20:24:22 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=PD22TGCCsABJm2qKs+DYKyvw+YQ8I3vKEf5j/ZatNso=; b=Iy4IvCjI7hmla+Qm6hPIe6Cmzn OKYb/4mzecFrJoK8/sDKfmBlakRkUrVgkxHbXYMXUzeXUHAOmZFZCY6oGkOly3ljSkCIu/obEYdqP qUz6HzkpCRarIoQMdF5wUqsrQjZmyVRNHCJtKttSAedS3OK9AYlRvVDlKJyca44egc+G5NGHewUnf 2yRiCwwlFpiXAcU8vtiblvwC5r6mmoM2fNHEACaciRY4wnod0140cjFS1u4teUDhwgHzrzxFnxxXk zdHsQl7C7oCNqc4FYI/rS7QZDrIsNFFy3Inul5h+58Wfx8JYxe1jK8PPPE0/7uPvkk1qkBG9fGGZE NUI76uKA==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:33244) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1o9Y2T-0004KL-G2; Thu, 07 Jul 2022 21:23:53 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.94.2) (envelope-from ) id 1o9Y2M-0005ZU-Pr; Thu, 07 Jul 2022 21:23:46 +0100 Date: Thu, 7 Jul 2022 21:23:46 +0100 From: "Russell King (Oracle)" To: Vladimir Oltean , Andrew Lunn Cc: Heiner Kallweit , Alexandre Belloni , Alvin __ipraga , Claudiu Manoil , "David S. Miller" , DENG Qingfang , Eric Dumazet , Florian Fainelli , George McCollister , Hauke Mehrtens , Jakub Kicinski , Kurt Kanzenbach , Landen Chao , Linus Walleij , linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, Matthias Brugger , netdev@vger.kernel.org, Paolo Abeni , Sean Wang , UNGLinuxDriver@microchip.com, Vivien Didelot , Woojung Huh , Marek =?iso-8859-1?Q?Beh=FAn?= Subject: Re: [PATCH RFC net-next 5/5] net: dsa: always use phylink for CPU and DSA ports Message-ID: References: <20220706102621.hfubvn3wa6wlw735@skbuf> <20220707152727.foxrd4gvqg3zb6il@skbuf> <20220707163831.cjj54a6ys5bceb22@skbuf> <20220707193753.2j67ni3or3bfkt6k@skbuf> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220707193753.2j67ni3or3bfkt6k@skbuf> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220707_132420_558076_940584A5 X-CRM114-Status: GOOD ( 54.24 ) 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 On Thu, Jul 07, 2022 at 10:37:53PM +0300, Vladimir Oltean wrote: > On Thu, Jul 07, 2022 at 06:15:46PM +0100, Russell King (Oracle) wrote: > > > This is why dsa_port_phylink_register() calls phylink_of_phy_connect() > > > without checking whether it has a fixed-link or a PHY, because it > > > doesn't fail even if it doesn't do anything. > > > > > > In fact I've wanted to make a correction to my previous phrasing that > > > "this function shouldn't be called if phylink_{,fwnode_}connect_phy() is > > > going to be called later". The correction is "... with a phy-handle". > > > > I'm not sure that clarification makes sense when talking about > > phylink_connect_phy(), so I think if you're clarifying it with a > > firmware property, you're only talking about > > phylink_fwnode_connect_phy() now? > > Yes, it's super hard to verbalize, and this is the reason why I didn't > add "... with a phy-handle" in the first place. > > I wanted to say: phylink_connect_phy(), OR phylink_fwnode_connect_phy() > WITH a phy-handle. I shouldn't have conflated them in the first place. Ah, right, because I interpreted it quite differently! > > > > > Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() > > > > > based on the following? > > > > > > > > > > (1) struct phylink_config gets extended with a bool fallback_max_fixed_link. > > > > > (2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register(). > > > > > (3) phylink_set_max_fixed_link() is hooked into this -ENODEV error > > > > > condition from phylink_fwnode_phy_connect(): > > > > > > > > > > phy_fwnode = fwnode_get_phy_node(fwnode); > > > > > if (IS_ERR(phy_fwnode)) { > > > > > if (pl->cfg_link_an_mode == MLO_AN_PHY) > > > > > return -ENODEV; <- here > > > > > return 0; > > > > > } > > > > > > > > My question in response would be - why should this DSA specific behaviour > > > > be handled completely internally within phylink, when it's a DSA > > > > specific behaviour? Why do we need boolean flags for this? > > > > > > Because the end result will be simpler if we respect the separation of > > > concerns that continues to exist, and it's still phylink's business to > > > say what is and isn't valid. DSA still isn't aware of the bindings > > > required by phylink, it just passes its fwnode to it. Practically > > > speaking, I wouldn't be scratching my head as to why we're checking for > > > half the prerequisites of phylink_set_max_fixed_link() in one place and > > > for the other half in another. > > > > > > True, through this patch set DSA is creating its own context specific > > > extension of phylink bindings, but arguably those existed before DSA was > > > even integrated with phylink, and we're just fixing something now we > > > didn't realize at the time we'd need to do. > > > > > > I can reverse the question, why would phylink even want to be involved > > > in how the max fixed link parameters are deduced, and it doesn't just > > > require that a fixed-link software node is constructed somehow > > > (irrelevant to phylink how), and phylink is just modified to find and > > > work with that if it exists? Isn't it for the exact same reason, > > > separation of concerns, that it's easiest for phylink to figure out what > > > is the most appropriate maximum fixed-link configuration? > > > > If that could be done, I'd love it, because then we don't have this in > > phylink at all, and it can all be a DSA problem to solve. It also means > > that others won't be tempted to use the interface incorrectly. > > > > I'm not sure how practical that is when we have both DT and ACPI to deal > > with, and ACPI is certainly out of my knowledge area to be able to > > construct a software node to specify a fixed-link. Maybe it can be done > > at the fwnode layer? I don't know. > > I don't want to be misunderstood. I brought up software nodes because > I'm sure you must have thought about this too, before proposing what you > did here. And unless there's a technical reason against software nodes > (which there doesn't appear to be, but I don't want to get ahead of > myself), I figured you must be OK with phylink absorbing the logic, case > in which I just don't understand why you are pushing back on a proposal > how to make phylink absorb the logic completely. The reason I hadn't is because switching DSA to fwnode brings with it issues for ACPI, and Andrew wants to be very careful about ACPI in networking - and I think quite rightly. As soon as one switches from DT APIs to fwnode APIs, you basically permit people an easy path to re-use DT properties in ACPI-land without the ACPI issues being first considered. So, I think if we did go this route, we need Andrew's input. > > Do you have a handy example of what you're suggesting? > > No, I didn't, but I thought, how hard can it be, and here's a hacked up > attempt on one of my boards: Thanks - that looks like something that should be possible to do, and way better than trying to shoe-horn this into phylink. My only comment would be that Andrew would disagree with you about this being "fixing up broken DT" - he has actively encouraged some drivers to adopt this "default" mode, which means it's anything but "broken" but it really is part of the DSA firmware description. > [ 4.315754] sja1105 spi0.1: configuring for fixed/rgmii link mode > [ 4.322653] sja1105 spi0.1 swp5 (uninitialized): PHY [mdio@2d24000:06] driver [Broadcom BCM5464] (irq=POLL) > [ 4.334796] sja1105 spi0.1 swp2 (uninitialized): PHY [mdio@2d24000:03] driver [Broadcom BCM5464] (irq=POLL) > [ 4.345853] sja1105 spi0.1 swp3 (uninitialized): PHY [mdio@2d24000:04] driver [Broadcom BCM5464] (irq=POLL) > [ 4.356859] sja1105 spi0.1 swp4 (uninitialized): PHY [mdio@2d24000:05] driver [Broadcom BCM5464] (irq=POLL) > [ 4.367245] device eth2 entered promiscuous mode > [ 4.371864] DSA: tree 0 setup > [ 4.376971] sja1105 spi0.1: Link is Up - 1Gbps/Full - flow control off > (...) > root@black:~# ip link set swp2 up && dhclient -i swp2 && ip addr show swp2 > [ 64.762756] fsl-gianfar soc:ethernet@2d90000 eth2: Link is Up - 1Gbps/Full - flow control off > [ 64.771530] sja1105 spi0.1 swp2: configuring for phy/rgmii-id link mode > [ 68.955048] sja1105 spi0.1 swp2: Link is Up - 1Gbps/Full - flow control off > 12: swp2@eth2: mtu 1500 qdisc noqueue state UP group default qlen 1000 > link/ether 00:1f:7b:63:02:48 brd ff:ff:ff:ff:ff:ff > inet 10.0.0.68/24 brd 10.0.0.255 scope global dynamic swp2 > valid_lft 600sec preferred_lft 600sec > > It's by far the messiest patch I've posted to the list (in the interest > of responding quickly), but if you study the code you can obviously see > what's missing, basically I've hardcoded the speed to 1000 and I'm > copying the phy-mode from the real DT node. Yep - there's at least one other property we need to carry over from the DT node, which is the "ethernet" property. > Unfortunately I don't have the time (and most importantly the interest) > in pushing this any further than that. If you want to take this from > here and integrate it with phylink_get_caps() I'd be glad to review > the result. Otherwise, feel free to continue with phylink_set_max_fixed_link(). I think this could be a much better solution to this problem, quite simply because we then don't end up with phylink_set_max_fixed_link() which could be abused - and this keeps the complexity where it should be, in the DSA code. As I say, though, I think we need Andrew's input on this. Andrew? I'll look at turning this into a proper solution tomorrow if Andrew is okay with the fwnode change. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel