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 892DCC43334 for ; Mon, 18 Jul 2022 20:08:54 +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=NaikKzy/KWsJV2zPVedG10cw6PRO9q69rhs6b/ZSWhk=; b=KQhajjU8dA0iWD QTnD1gjchGEZ/z3Od09dVKBdcr0O24LZfU3zEA5hK+YrfHCBvT2r21GqBhsKVFBo/zv7rzjJ4X8W9 DomuBiZy7MTquc2bn8XI4vtUHWef2+8AvANteNjcgrW/LdSJWdDJAuRgPBMMLM+A99xmfXdKr5Wek 3hihRaolf1IG2U9Z51n2Oieqa2goHz3dqkOWFpPl2P+vn59YNC4bxxn8WEpy91rHh4+CiQEIQnAAR rszHHaU6M9dt+zXD+2bKQWZ7Vb74J9o6J3BpSuZanKUrM4/Qbf3vfZJodOyrqlT4SByqCsd7JqoY4 9Y0FqYtxQep53qjZs2eg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oDX1w-000etV-Bn; Mon, 18 Jul 2022 20:07:48 +0000 Received: from mga07.intel.com ([134.134.136.100]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oDX1s-000eqt-BD; Mon, 18 Jul 2022 20:07:46 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1658174864; x=1689710864; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=6c8zIjxHQ8nkeBT76+tE1tfZwAKSyLtonXBo9InpdJw=; b=Gtjr9ptDQz1LBuG7KhLdrPGyLso6RAvxba/T+YCjUYLBOpGxF5EIr9uh XcjU64VheIioP9wuFw9chqSn7Z6TsMVo7jycvby8SL9gDare1mFccpK+6 hmJ4kYzV3tGe+FDlLDsFz40rqbyeTrPpUxWWc9hhuyly2jz6Ez17hTnye LywOGLOubQz2THtpV76buAyp5p2UAD+klHj8mwF0ge3dWzgtzFI2mxNuc nzuiOZGmAaIjAm4Om4hBg9rD5D39TxeZxKTN6ggopWIV1bparXS9KaMUA nvHUk+NbxeQxyxfJs72D6NmVFMSar2Y6zsTLIPOHqtIMJblP+wtEHqXIy w==; X-IronPort-AV: E=McAfee;i="6400,9594,10412"; a="350273575" X-IronPort-AV: E=Sophos;i="5.92,282,1650956400"; d="scan'208";a="350273575" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jul 2022 13:07:42 -0700 X-IronPort-AV: E=Sophos;i="5.92,282,1650956400"; d="scan'208";a="547636013" Received: from smile.fi.intel.com ([10.237.72.54]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jul 2022 13:07:35 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1oDX1e-001OTQ-1x; Mon, 18 Jul 2022 23:07:30 +0300 Date: Mon, 18 Jul 2022 23:07:30 +0300 From: Andy Shevchenko To: "Russell King (Oracle)" Cc: Vladimir Oltean , Andrew Lunn , Heiner Kallweit , Alexandre Belloni , Alvin __ipraga , 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 =?iso-8859-1?Q?Beh=FAn?= Subject: Re: [PATCH net-next 2/6] software node: allow named software node to be created Message-ID: References: <20220715201715.foea4rifegmnti46@skbuf> <20220715204841.pwhvnue2atrkc2fx@skbuf> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220718_130744_459867_884775D4 X-CRM114-Status: GOOD ( 63.51 ) 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="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Jul 18, 2022 at 08:11:40PM +0100, Russell King (Oracle) wrote: > On Mon, Jul 18, 2022 at 09:43:41PM +0300, Andy Shevchenko wrote: > > On Mon, Jul 18, 2022 at 02:27:02PM +0100, Russell King (Oracle) wrote: > > > On Mon, Jul 18, 2022 at 03:29:52PM +0300, Andy Shevchenko wrote: > > > > On Fri, Jul 15, 2022 at 11:48:41PM +0300, Vladimir Oltean wrote: > > > > > So won't kobject_init_and_add() fail on namespace collision? Is i= t the > > > > > problem that it's going to fail, or that it's not trivial to stat= ically > > > > > determine whether it'll fail? > > > > > = > > > > > Sorry, but I don't see something actionable about this. > > > > = > > > > I'm talking about validation before a runtime. But if you think tha= t is fine, > > > > let's fail it at runtime, okay, and consume more backtraces in the = future. > > > = > > > Is there any sane way to do validation of this namespace before > > > runtime? > > = > > For statically compiled, I think we can do it (to some extent). > > Currently only three drivers, if I'm not mistaken, define software node= s with > > names. It's easy to check that their node names are unique. > > = > > When you allow such an API then we might have tracebacks (from sysfs) b= out name > > collisions. Not that is something new to kernel (we have seen many of a= kind), > > but I prefer, if possible, to validate this before sysfs issues a trace= back. > > = > > > The problem in this instance is we need a node named "fixed-link" that > > > is attached to the parent node as that is defined in the binding doc, > > > and we're creating swnodes to provide software generated nodes for > > > this binding. > > = > > And how you guarantee that it will be only a single one with unique pat= hname? > > = > > For example, you have two DSA cards (or whatever it's called) in the SM= P system, > > it mean that there is non-zero probability of coexisting swnodes for th= em. > = > Good point - I guess we at least need to attach the swnode parent to the > device so its path is unique, because right now that isn't the case. I'm > guessing that: > = > new_port_fwnode =3D fwnode_create_software_node(port_props, NULL); > = > will create something at the root of the swnode tree, and then: > = > fixed_link_fwnode =3D fwnode_create_named_software_node(fixed_lin= k_props, > new_port_fw= node, > "fixed-link= "); > = > will create a node with a fixed name. I guess it in part depends what > pathname the first node gets (which we don't specify.) I'm not familiar > with the swnode code to know what happens with the naming for the first > node. First node's name will be unique which is guaranteed by IDA framework. If we have already 2B nodes, then yes, it would be problematic (but 2^31 ought to= be enough :-). > However, it seems sensible to me to attach the first node to the device > node, thus giving it a unique fwnode path. Does that solve the problem > in swnode land? Yes, but in the driver you will have that as child of the device, analogue = in DT my_root_node { // equal the level of device node you attach it to fixed-link { } } (Sorry, I don't know the DT syntax by heart, but I hope you got the idea.) To access it will be something like child =3D fwnode_get_named_child_node(fwnode, "fixed-link"); And reading properties, if needed, ret =3D fnode_property_read_...(child, ...); But this might require to adopt drivers, no? Or I misunderstand the hierarc= hy. > > > There could be several such nodes scattered around, but in this > > > instance they are very short-lived before they are destroyed, they > > > don't even need to be published to userspace (and its probably a waste > > > of CPU cycles for them to be published there.) > > > = > > > So, for this specific case, is this the best approach, or is there > > > some better way to achieve what we need here? > > = > > Honestly, I don't know. > > = > > The "workaround" (but it looks to me rather a hack) is to create unique= swnode > > and make fixed-link as a child of it. > > = > > Or entire concept of the root swnodes (when name is provided) should be > > reconsidered, so somehow we will have a uniqueness so that the entire > > path(s) behind it will be caller-dependent. But this I also don't like. > > = > > Maybe Heikki, Sakari, Rafael can share their thoughts... > > = > > Just for my learning, why PHY uses "fixed-link" instead of relying on a > > (firmware) graph? It might be the actual solution to your problem. > = > That's a question for Andrew, but I've tried to solicit his comments on > several occasions concerning this "feature" of DSA but I keep getting > no reply. Honestly, I don't know the answer to your question. > = > The only thing that I know is that Andrew has been promoting this > feature where a switch port, whether it be connected to the CPU or > to another switch, which doesn't specify any link parameters will > automatically use the fastest "phy interface mode" and the fastest > link speed that can be supported by the DSA device. > = > This has caused issues over the last few years which we've bodged > around in various ways, and with updates to one of the DSA drivers > this bodging is becoming more of a wart that's spreading. So, I'm > trying to find a way to solve this. > = > My initial approach was to avoid fiddling with the firmware tree, > but Vladimir proposed this approach as being cleaner - and it means > the "bodge" becomes completely localised in the DSA (distributed > switch architecture) code rather than being spread into phylink. > = > I wish we could get rid of this "feature" but since it's been > established for many years, and we have at least one known driver > that uses it, getting rid of it breaks existing firmware trees. > I think we also have one other driver that makes use of it as > well, but I can't say for certain (because it's not really possible > to discern which drivers use this feature from reading the driver > code.) I've tried asking Andrew if he knows and got no response. > = > So I'm in a complete information vacuum here - all that I know is > that trying to convert the mv88e6xxx DSA driver to use phylink_pcs > will break it (as reported by Marek Beh=FAn), because phylink doesn't > get used if firmware is using this "defaulting" feature. > = > It's part of the DT binding, and remains so today - the properties > specifying the "phy-mode", "fixed-link" etc all remain optional. Okay, grepping the kernel I see this: dn =3D fwnode_get_named_child_node(fwnode, "fixed-link"); This seems the same what you need. I dunno why swnode should be created with a name for this? Eliminating an empty root node sounds plausible effect, but the consequences are not 1:1 mapping of swnodes as it's designed for firmware device node +=3D unique root swnode property "X" +=3D property "Y" child "A" +=3D child "B" Resulting firmware node as driver sees it: device node property "X" property "Y" child "A" child "B" That's all said, I guess the way with a two swnodes (hierarhy) is the corre= ct one from the beginning. To the API, now I can tell you how to validate! Just be sure if there is no name provided, we are just fine. Otherwise parent _swnode_ should be non-NULL. In such case parent can be only set either dynamically _or_ statically assigned with a name. -- = With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel