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 C13D3C54EAA for ; Mon, 30 Jan 2023 17:32:13 +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=q18UcMqW8YGA4B/lkqPa/n3AUqRbeWVIXn3UPmjdUvQ=; b=Z4ROAcg3M//c+i t08qiqzzz7Jk8PSPtzQgT4sG9Kp11az69HUrtH23GzBp9QDrb7K7QhD3/NkfS54jHni2cjc8s8coB lthm1Dv4YX4Vq2PVU9E8oUiE+7p6lUgxCHvAlLD39S0VfcMtUXu2KjGDVb3FsUovifHNKg7WXq/bE HQ7Nxw6F7P78yn8bMKskT9TwWmtU/4jn2QVGmR8dX2dZmUrm5WVpu08vUeL+1zAkdG2IIgnZR1AkY LcPfQdTnoILsxWh5ir8PZO+GpIAGzK0AwwYgb5Giayoh3BjAodJt66Fuxp6JiQDF6G/AgdlRM1O9t kZsSMDL+CzvG6rs3rHVg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pMXzR-004ZNb-7i; Mon, 30 Jan 2023 17:30:46 +0000 Received: from mga02.intel.com ([134.134.136.20]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pMXum-004Wz9-U1 for linux-arm-kernel@lists.infradead.org; Mon, 30 Jan 2023 17:25:58 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1675099556; x=1706635556; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=Rj5FdHRZxfvSxZ+bvMThiL/pa+WbSCwCmjvsd0eDOSg=; b=Klt6F7EnlSO7q3TADww6Mn8FrQvCySmcUHgbfJUkwW20WZF5RYcNmCFj N4GS6ooP1zwahDpgdUq435bUlEgt+k3b/hFoBoUnvAaE0Wzg8KwmhMEx3 kgScy60m5AGAdv6blR6mwG5mCfGOsU6wwjtJgE/6jCZcUOf81wT3tYLjQ wVfno53Wb8X8EuSB5UFCoYayRj4QKMZ7hIvtlVZT4lffT9Pc6o9GZVJzc mD+ESC7g2KPsx/L8/hVbTBnxZR8YjBjY/FMgAJqzLT2ezYjgC0LEvsZ8y 92ZirvIuEzEmaKudGmcQ041JcQIV46rUtlD+qxDpz5mVFeQZnzQ8B+8uV Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10605"; a="315499572" X-IronPort-AV: E=Sophos;i="5.97,257,1669104000"; d="scan'208";a="315499572" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Jan 2023 04:04:16 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10605"; a="732674303" X-IronPort-AV: E=Sophos;i="5.97,257,1669104000"; d="scan'208";a="732674303" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga004.fm.intel.com with ESMTP; 30 Jan 2023 04:04:05 -0800 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1pMStF-00HKsM-2X; Mon, 30 Jan 2023 14:04:01 +0200 Date: Mon, 30 Jan 2023 14:04:01 +0200 From: Andy Shevchenko To: Saravana Kannan Cc: Greg Kroah-Hartman , "Rafael J. Wysocki" , Sudeep Holla , Cristian Marussi , Linus Walleij , Bartosz Golaszewski , Thomas Gleixner , Marc Zyngier , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Rob Herring , Frank Rowand , Geert Uytterhoeven , Magnus Damm , Len Brown , Daniel Scally , Heikki Krogerus , Sakari Ailus , Tony Lindgren , Linux Kernel Functional Testing , Naresh Kamboju , Abel Vesa , Alexander Stein , Geert Uytterhoeven , John Stultz , Doug Anderson , Guenter Roeck , Dmitry Baryshkov , Maxim Kiselev , Maxim Kochetkov , Miquel Raynal , Luca Weiss , Colin Foster , Martin Kepplinger , Jean-Philippe Brucker , kernel-team@android.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-acpi@vger.kernel.org Subject: Re: [PATCH v2 01/11] driver core: fw_devlink: Don't purge child fwnode's consumer links Message-ID: References: <20230127001141.407071-1-saravanak@google.com> <20230127001141.407071-2-saravanak@google.com> 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-20230130_092557_051922_547A9B69 X-CRM114-Status: GOOD ( 41.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 Fri, Jan 27, 2023 at 11:33:28PM -0800, Saravana Kannan wrote: > On Fri, Jan 27, 2023 at 1:22 AM Andy Shevchenko > wrote: > > On Thu, Jan 26, 2023 at 04:11:28PM -0800, Saravana Kannan wrote: ... > > > static unsigned int defer_sync_state_count = 1; > > > static DEFINE_MUTEX(fwnode_link_lock); > > > static bool fw_devlink_is_permissive(void); > > > +static void __fw_devlink_link_to_consumers(struct device *dev); > > > static bool fw_devlink_drv_reg_done; > > > static bool fw_devlink_best_effort; > > > > I'm wondering if may avoid adding more forward declarations... > > > > Perhaps it's a sign that devlink code should be split to its own > > module? > > I've thought about that before, but I'm not there yet. Maybe once my > remaining refactors and TODOs are done, it'd be a good time to revisit > this question. > > But I don't think it should be done for the reason of forward > declaration as we'd just end up moving these into base.h and we can do > that even today. What I meant is that the stacking up forward declarations is a good sign that something has to be done sooner than later. ... > > > -int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) > > > +static int __fwnode_link_add(struct fwnode_handle *con, > > > + struct fwnode_handle *sup) > > > > I believe we tolerate a bit longer lines, so you may still have it on a single > > line. > > That'd make it >80 cols. I'm going to leave it as is. Is it a problem? ... > > > if (dev->fwnode && dev->fwnode->dev == dev) { > > > > You may have above something like > > > > fwnode = dev_fwnode(dev); > > I'll leave it as-is for now. I see dev->fwnode vs dev_fwnode() don't > always give the same results. I need to re-examine other places I use > dev->fwnode in fw_devlink code before I start using that function. But > in general it seems like a good idea. I'll add this to my TODOs. Please do, the rationale is to actually move the fwnode to the proper layer, now we have the single linked list defined in struct fwnode_handle and dereferencing fwnode from struct device without helper adds a lot of headache in the future. So, I really would like to see that we stopped doing that. > > if (fwnode && fwnode->dev == dev) { > > > > > struct fwnode_handle *child; > > > fwnode_links_purge_suppliers(dev->fwnode); > > > + mutex_lock(&fwnode_link_lock); > > > fwnode_for_each_available_child_node(dev->fwnode, child) > > > - fw_devlink_purge_absent_suppliers(child); > > > + __fw_devlink_pickup_dangling_consumers(child, > > > + dev->fwnode); > > > > __fw_devlink_pickup_dangling_consumers(child, fwnode); > > I like the dev->fwnode->dev == dev check. It makes it super clear that > I'm checking "The device's fwnode points back to the device". If I > just use fwnode->dev == dev, then one will have to go back and read > what fwnode is set to, etc. Also, when reading all these function > calls it's easier to see that I'm working on the dev's fwnode (where > dev is the device that was just bound to a driver) instead of some > other fwnode. > > So I find it more readable as is and the compiler would optimize it > anyway. If you feel strongly about this, I can change to use fwnode > instead of dev->fwnode. Please, read above. -- 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