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 A8993C433EF for ; Sat, 16 Jul 2022 10:58:25 +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=+5+i3uNUlDKu/pwvwWxBxF27G0iJWpQUxTorZN3qsGc=; b=rxWpnl58PZfXhh twxa7b7Lbqzz5c2V68e692vcjO5LpEWYDxIZ3ns7EIgP+ji3wEMDrNTt0QhadXeBNgsHLvEyxc/oY IFMypJHaBKXeufkjl3lGzUSo48Mu/aiKec/OxJoLJ6XFiQyJ609e00L9+VlCq3RZCgrfV96DGYUje 6X079gg7jYWg6XbJu5qfPBhL24zk5nfFp/peeL9HfW77JeYX3RRMtEsXsq2M5MkearYYQN3o13eom q25kySjMvtGP8f5cNIsSm3HHoUUcHEuZtP+/6ujdrimBBL7AoV6K/idkTrnz7bHmfQCwJfWLEByvV 4B2Fjmzp0nvXUMsxe49g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oCfU9-00FYfJ-Gk; Sat, 16 Jul 2022 10:57:21 +0000 Received: from mail-ed1-x52c.google.com ([2a00:1450:4864:20::52c]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oCfU5-00FYcs-Ds; Sat, 16 Jul 2022 10:57:19 +0000 Received: by mail-ed1-x52c.google.com with SMTP id fd6so9209104edb.5; Sat, 16 Jul 2022 03:57:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=Y2w+/D0qVC/MxuYc8kSX8+b66qSmQVaXiMac4uHSn80=; b=CYfq/a44gFubyWQKFIXHlM7reCKU/S/gz8mxBEBgeJxbgYLT0jDP07ae47dFNpwTyb cCLvh2L+CbnG3bLUr+cqP2HhTIh/1O7/Tpu6RwFD0FZsfuvBIDYkONQLpT6tf45HExbe nUle/gN8RRGAFT9ulovCzcYnBTLCyoGmkqjiUKhVbiU0wLzBtXFd8Wjb2VS8D8k1GxGL 0+T9Sffv0kP9cE3g+4+7VZcyEf5U+n5a4W3hETqNNvjRHx4Xo+Lbn4fxkABb6vQUi6sL SSWUIyaPoK4uGFkc4jB9jYAVT5XMNojlZZD9FfwA/o+YP6tLmRrjAEYDB2oQXr2m2cyR YLEw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=Y2w+/D0qVC/MxuYc8kSX8+b66qSmQVaXiMac4uHSn80=; b=04q/4Lxat+c06XYFdFQ7vGGCF21F5b5gxsTr974ruoT7LOk3tKiqaU4+UjAbrwMC+e H1+n4ajujz9udOn5HYWu8SKyclV+AXAj9bQjg7RKdQwUcFREXzTDyZnzI9mlibKkBhTP VYIb2UR3JvTTpg9YOOMDQLoIVWd5K4WPbkx7vlOwXwqVOjkzgtOiAI0LYDNCsuWdMk4A Tl0e7CLHplfTiInD/dUmQGOxZ8zJxsC0C/pKd8fjPu5eCDoM0/ytIo/EsXsyw4cmvl3U bqzfKB3yrZ1A60P153PyWxbX0LsoSGak2uYXZLNMoaph32khBnYRC6He+szdYrXePbn+ 2/2Q== X-Gm-Message-State: AJIora/P1o8vSjeZPppwuhWOOd0Ikjdk1t2vyz68Eu/gT5Dj5oycLGiV uxXNebTlIJD7hTjquTHyagI= X-Google-Smtp-Source: AGRyM1tTQPUNZdfW5CjlZaMUuGmjZY0BtVvaB4lz4j+y8Rw9J+waSuNBuaZcCkhDKqXIhjZCyyB9hA== X-Received: by 2002:a05:6402:5001:b0:437:8918:8dbe with SMTP id p1-20020a056402500100b0043789188dbemr24547489eda.70.1657969035042; Sat, 16 Jul 2022 03:57:15 -0700 (PDT) Received: from skbuf ([188.25.231.115]) by smtp.gmail.com with ESMTPSA id e2-20020a50fb82000000b0042bdb6a3602sm4382499edq.69.2022.07.16.03.57.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 16 Jul 2022 03:57:14 -0700 (PDT) Date: Sat, 16 Jul 2022 13:57:11 +0300 From: Vladimir Oltean To: "Russell King (Oracle)" Cc: Andrew Lunn , Heiner Kallweit , Alexandre Belloni , Alvin __ipraga , Andy Shevchenko , Claudiu Manoil , Daniel Scally , "David S. Miller" , DENG Qingfang , Eric Dumazet , Florian Fainelli , George McCollister , Greg Kroah-Hartman , Hauke Mehrtens , Heikki Krogerus , Jakub Kicinski , Kurt Kanzenbach , Landen Chao , Linus Walleij , linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, Matthias Brugger , netdev@vger.kernel.org, Paolo Abeni , "Rafael J. Wysocki" , Sakari Ailus , Sean Wang , UNGLinuxDriver@microchip.com, Vivien Didelot , Woojung Huh , Marek =?utf-8?B?QmVow7pu?= Subject: Re: [PATCH net-next 3/6] net: dsa: add support for retrieving the interface mode Message-ID: <20220716105711.bjsh763smf6bfjy2@skbuf> References: <20220715172444.yins4kb2b6b35aql@skbuf> <20220715222348.okmeyd55o5u3gkyi@skbuf> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220716_035717_498770_16E867C6 X-CRM114-Status: GOOD ( 55.00 ) 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 Fri, Jul 15, 2022 at 11:57:20PM +0100, Russell King (Oracle) wrote: > > > The problem is this - we call get_caps(), and we have to read registers > > > to work out what the port supports. If we have a separate callback, then > > > we need to re-read those registers to get the same information to report > > > what the default interface should be. > > > > > > Since almost all of the Marvell implementations the values for both the > > > list of supported interfaces and the default interface both require > > > reading a register and translating it to a phy_interface_t, and then > > > setting the support mask, it seems logical to combine these two > > > functioalities into one function. > > > > In essence that doesn't mean much; DSA isn't Marvell only, but I'll give > > it to you: if only the Marvell driver (and Broadcom later, I expect) is > > going to add support for the context-specific interpretation of CPU port > > OF nodes, then we may consider tailoring the implementation to their > > hardware register layout details. In any case, my concern can be > > addressed even if you insist on keeping the default interface as an > > argument of phylink_get_caps. There just needs to be a lot more > > documentation explaining who needs to populate that argument and why. > > I don't get the point you're making here. The point I'm making is that I dislike where this is going. The addition of "default_interface" to phylink_get_caps is confusing because it lacks proper qualifiers. The concrete reasons why it's confusing are: (a) there is no comment which specifies on which kinds of ports (DSA and CPU) the default_interface will be used. This might result in useless effort from driver authors to report a default_interface for a port where it will never be asked for. (b) there is no comment which specifies that only the drivers which have DT blobs with missing phylink bindings on CPU and DSA ports should fill this out. I wouldn't want to see a new driver use this facility, I just don't see a reason for it. I'd rather see a comment that the recommendation for new drivers is to validate their bindings and not rely on context-specific interpretations of empty DT nodes. (c) especially with the dsa_port_find_max_caps() heuristic in place, I can't say I'm clear at all on who should populate "default_interface" and who could safely rely on the heuristic if they populate supported_interfaces. It's simply put unclear what is the expectation from driver authors. For (b) I was thinking that making it a separate function would make it clearer that it isn't for everyone. Doing just that wouldn't solve everything, so I've also said that adding more documentation to this function prototype would go a long way. Some dsa_switch_ops already have inline comments in include/net/dsa.h, see get_tag_protocol, change_tag_protocol, port_change_mtu. Also, there is the the "PHY devices and link management" chapter in Documentation/networking/dsa/dsa.rst. We have places to document what the DSA framework expects drivers to do. I was expecting that wherever default_interface gets reported, we could see some answers and explanations to the questions above. > > Also, perhaps more importantly, a real effort needs to be put to prevent > > breakage for drivers that work without a phylink instance registered for > > the CPU port, and also don't report the default interface. Practically > > that just means not deleting the current logic, but making it one of 3 > > options. > > > > fwnode is valid from phylink's perspective? > > / \ > > yes / \ no > > / \ > > register with phylink can we determine the link parameters to create > > a fixed-link software node? > > / \ \ > > yes / \ no | > > / \ | this is missing > > / \ | > > create the software node and don't put the port down, | > > register with phylink don't register with phylink / > > This is exactly what we have today, Wait a minute, how come this is exactly what we have "today"? In tree we have this: fwnode is valid from phylink's perspective? / \ yes / \ no / \ register with phylink \ don't put the port down, don't register with phylink In your patch set we have this: fwnode is valid from phylink's perspective? / \ yes / \ no / \ register with phylink can we determine the link parameters to create a fixed-link software node? / \ yes / \ no / \ / \ create the software node and fail to create the port register with phylink > and is exactly what I'm trying to get rid of, so we have _consistency_ > in the implementation, to prevent fuckups like I've created by > converting many DSA drivers to use phylink_pcs. Any DSA driver that > used a PCS for the DSA or CPu port and has been converted to > phylink_pcs support has been broken in the last few kernel cycles. I'm > trying to address that breakage before converting the Marvell DSA > driver - which is the driver that highlighted the problem. You are essentially saying that it's of no use to keep in DSA the fallback logic of not registering with phylink, because the phylink_pcs conversions have broken the defaulting functionality already in all other drivers. I may have missed something, but this is new information to me. Specifically, before you've said that it is *this* patch set which would risk introducing breakage (by forcing a link down + a phylink creation). https://lore.kernel.org/netdev/YsCqFM8qM1h1MKu%2F@shell.armlinux.org.uk/ What you're saying now directly contradicts that. Do you have concrete evidence that there is actually any regression of this kind introduced by prior phylink_pcs conversions? Because if there is, I retract the proposal to keep the fallback logic. > We need to move away from the current model in DSA where we only use > stuff in random situations. > > Well, at this point, I'm just going to give up with this kernel cycle. > It seems impossible to get this sorted. It seems impossible to move > forward with the conversion of Marvell DSA to phylink_pcs. In fact, > I might just give up with the idea of further phylink development > because it's just too fucking difficult, and getting feedback is just > impossible. > > -- > 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