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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 91482C38142 for ; Sat, 28 Jan 2023 07:34:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231890AbjA1HeJ (ORCPT ); Sat, 28 Jan 2023 02:34:09 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35782 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230104AbjA1HeG (ORCPT ); Sat, 28 Jan 2023 02:34:06 -0500 Received: from mail-pj1-x102e.google.com (mail-pj1-x102e.google.com [IPv6:2607:f8b0:4864:20::102e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 31AAA3B3F8 for ; Fri, 27 Jan 2023 23:34:05 -0800 (PST) Received: by mail-pj1-x102e.google.com with SMTP id z1-20020a17090a66c100b00226f05b9595so6771800pjl.0 for ; Fri, 27 Jan 2023 23:34:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=48cPMwKfIB+JYXBPIFLFeL3uIHX0dr8TubcBcaVKSWg=; b=eVNLr6tMgu58ayq3Ao3L3DJzKMEFfj2tbmldarzOO8OyCpMG/ZZ2rlLfICfMDBCo5+ VPYoXRhj8EtItKKFrhehPMIQ7V1tPMG99UpZlCDM+Mf1oyRyrBCvCMbMtihyB6F18Nao 9o2PNPcr9v3ZFB/7Z1DSqGc7acdqB66aVOInvkxBZPLNSmIILNTUYiDsDD+44ujq4lEd cLTSvGeHYw69BwlArYctelJXJx5wZKHpPd5Qyzrkwm48CtOFvqXRpCvtFZ0A4kHJNfdv wjR9G1GhyqmB7Hs2pnSNtyQ6eABh67bfwViZQ35v1D/NA5ofowoPu+IqSZmLzYz9vXhP J71w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=48cPMwKfIB+JYXBPIFLFeL3uIHX0dr8TubcBcaVKSWg=; b=oylvXH3NDOL0FAXKddNg6j+hEWtd/y+sotElhHnIikqlB6y3/NXaAVtXHwrDFuT4w5 rTgR2Zxpwa1nr2fMxKQjpOvW1Nos2F7JBoFUn9peeHHkLRC2KwGO3vjmMhFDFnWx7NJK TR6mtDRgAB2RRGWYc0IH6pEGE+4GKMQgaju8i0airgp7qmwfCE6RV4XV6HWUztHthM2D YUwxk82ETGxVw2T5buNALQ+75cugp7dBJ0qKsrNPq/vT9/zFSgkxLt+ZfhsQQ4PEk7Zm /4f9NvtaPS8BCqCgEVB9bD3xBwBa0QZUCBnjpkfmUpegrUDzgGly9zGG4zsojk4IIeQp lwIg== X-Gm-Message-State: AO0yUKXrsa0/+R+gHFzgs//GnR02EwwdGMgYMx1YrR7xnTZ3wDrRbwf1 E9Zd94OaHy+Eu9ECl4y42CDPTO6pQA85Fu4y9CDMEg== X-Google-Smtp-Source: AK7set9myMbUwY2oQoYCC6s/hiNjKYlL3tr83m91kF4ZOSzpcq9WlGrvx111fb4zpvHUxMc/iM2X6KckP/ZL7OtRtIk= X-Received: by 2002:a17:90a:64c5:b0:22b:ef05:ea5b with SMTP id i5-20020a17090a64c500b0022bef05ea5bmr2696800pjm.50.1674891244385; Fri, 27 Jan 2023 23:34:04 -0800 (PST) MIME-Version: 1.0 References: <20230127001141.407071-1-saravanak@google.com> <20230127001141.407071-2-saravanak@google.com> In-Reply-To: From: Saravana Kannan Date: Fri, 27 Jan 2023 23:33:28 -0800 Message-ID: Subject: Re: [PATCH v2 01/11] driver core: fw_devlink: Don't purge child fwnode's consumer links To: Andy Shevchenko 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 Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org 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: > > When a device X is bound successfully to a driver, if it has a child > > firmware node Y that doesn't have a struct device created by then, we > > delete fwnode links where the child firmware node Y is the supplier. We > > did this to avoid blocking the consumers of the child firmware node Y > > from deferring probe indefinitely. > > > > While that a step in the right direction, it's better to make the > > consumers of the child firmware node Y to be consumers of the device X > > because device X is probably implementing whatever functionality is > > represented by child firmware node Y. By doing this, we capture the > > device dependencies more accurately and ensure better > > probe/suspend/resume ordering. > > ... > > > 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. > > ... > > > -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. > > ... > > > +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) > > +{ > > > + int ret = 0; > > Redundant assignment. Thanks. Will fix in v3. > > > + mutex_lock(&fwnode_link_lock); > > + ret = __fwnode_link_add(con, sup); > > + mutex_unlock(&fwnode_link_lock); > > return ret; > > } > > ... > > > 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. > 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. Thanks, Saravana 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 31CBCC27C76 for ; Sat, 28 Jan 2023 07:35:16 +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:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=jJgDZvWz9rTZVlVNJg6S6+mDJO/Y7DCGuNr0zsVJhxY=; b=0UNELJW+Q2pl0k bhTRiGAXNwJ0YF9ICp9uZba1MV8LprLgdHsufSmseqkLXcR+Ua/b1rWkJTdDLTWJlsKDpc79W6Bgv 2Xat6ZetjPGM+IGzp/iuWWOC+bCqBvLvzoGCdor68u6VDoCh3H+vcygXhGVAj2MZzhsWop12EPCYL M38XsmQrKgTj0BPpzHlFglds3AkvDTcLaWQ71d5JYIeCsmHtW/Tj+NoOTSBhGPptuXJoEhHnzE3TP p39LaftTHaAV55u3V0HyBVOJS0OZM9BsDWRFfqK1MTvvtQqTmbeBkBVTfeWBIC5FAOftigImjI5Ux IymfBar8iDyykU0YrOZA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pLfj3-00HMkJ-Sw; Sat, 28 Jan 2023 07:34:14 +0000 Received: from mail-pj1-x1033.google.com ([2607:f8b0:4864:20::1033]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pLfiz-00HMja-Qs for linux-arm-kernel@lists.infradead.org; Sat, 28 Jan 2023 07:34:11 +0000 Received: by mail-pj1-x1033.google.com with SMTP id rm7-20020a17090b3ec700b0022c05558d22so6710609pjb.5 for ; Fri, 27 Jan 2023 23:34:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=48cPMwKfIB+JYXBPIFLFeL3uIHX0dr8TubcBcaVKSWg=; b=eVNLr6tMgu58ayq3Ao3L3DJzKMEFfj2tbmldarzOO8OyCpMG/ZZ2rlLfICfMDBCo5+ VPYoXRhj8EtItKKFrhehPMIQ7V1tPMG99UpZlCDM+Mf1oyRyrBCvCMbMtihyB6F18Nao 9o2PNPcr9v3ZFB/7Z1DSqGc7acdqB66aVOInvkxBZPLNSmIILNTUYiDsDD+44ujq4lEd cLTSvGeHYw69BwlArYctelJXJx5wZKHpPd5Qyzrkwm48CtOFvqXRpCvtFZ0A4kHJNfdv wjR9G1GhyqmB7Hs2pnSNtyQ6eABh67bfwViZQ35v1D/NA5ofowoPu+IqSZmLzYz9vXhP J71w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=48cPMwKfIB+JYXBPIFLFeL3uIHX0dr8TubcBcaVKSWg=; b=BjKxbD/qw2OMTxHr/LCiMECDeBb4lVAQhYmU2BGOqm+NmxnpcgfiplhonquDVfSNA/ uFSfqGtLOmecyjIm7OmNmsPNyZ7AP9rhAuQIEupc6Kd83XAeLCzHUgwXp2ZFLoyRoqg2 ib4RRTF6jG1cm9NFPmu2pqMV0wKeVSC5cbFUKwpJxEDr7b4l7kTizuDpbE/LXtAASweo FSP07XJ9fp54d94wvtTLQLdYqNeWNTzBUdaYuOIaKzBotNNvfOMDxel4oPq1gi8JuXCJ WGfWUIXC41lUuZKvCYF+aTHoUpbirUehZHKfnCuQiEAuduRjE4P1HcG3kle+X83TrJma Qc6A== X-Gm-Message-State: AO0yUKWHj5qyI/IclZ6wspprE128zbmXO207yIwa79kudn7baJkWgSBX i1AjwWY+tQ1G9UQfKH3ECpnLd2zgQGBPaBJZBLyApQ== X-Google-Smtp-Source: AK7set9myMbUwY2oQoYCC6s/hiNjKYlL3tr83m91kF4ZOSzpcq9WlGrvx111fb4zpvHUxMc/iM2X6KckP/ZL7OtRtIk= X-Received: by 2002:a17:90a:64c5:b0:22b:ef05:ea5b with SMTP id i5-20020a17090a64c500b0022bef05ea5bmr2696800pjm.50.1674891244385; Fri, 27 Jan 2023 23:34:04 -0800 (PST) MIME-Version: 1.0 References: <20230127001141.407071-1-saravanak@google.com> <20230127001141.407071-2-saravanak@google.com> In-Reply-To: From: Saravana Kannan Date: Fri, 27 Jan 2023 23:33:28 -0800 Message-ID: Subject: Re: [PATCH v2 01/11] driver core: fw_devlink: Don't purge child fwnode's consumer links To: Andy Shevchenko 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 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230127_233409_895568_36FC8094 X-CRM114-Status: GOOD ( 38.69 ) 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 1:22 AM Andy Shevchenko wrote: > > On Thu, Jan 26, 2023 at 04:11:28PM -0800, Saravana Kannan wrote: > > When a device X is bound successfully to a driver, if it has a child > > firmware node Y that doesn't have a struct device created by then, we > > delete fwnode links where the child firmware node Y is the supplier. We > > did this to avoid blocking the consumers of the child firmware node Y > > from deferring probe indefinitely. > > > > While that a step in the right direction, it's better to make the > > consumers of the child firmware node Y to be consumers of the device X > > because device X is probably implementing whatever functionality is > > represented by child firmware node Y. By doing this, we capture the > > device dependencies more accurately and ensure better > > probe/suspend/resume ordering. > > ... > > > 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. > > ... > > > -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. > > ... > > > +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) > > +{ > > > + int ret = 0; > > Redundant assignment. Thanks. Will fix in v3. > > > + mutex_lock(&fwnode_link_lock); > > + ret = __fwnode_link_add(con, sup); > > + mutex_unlock(&fwnode_link_lock); > > return ret; > > } > > ... > > > 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. > 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. Thanks, Saravana _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel