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 0B23FC54EAA for ; Mon, 30 Jan 2023 12:16:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235958AbjA3MQR (ORCPT ); Mon, 30 Jan 2023 07:16:17 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48088 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229919AbjA3MQQ (ORCPT ); Mon, 30 Jan 2023 07:16:16 -0500 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8945E93D1; Mon, 30 Jan 2023 04:16:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1675080966; x=1706616966; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=OoZvW5mYAmBAqzGauPrm+It1FwPUT3sQTZsO9ABVQOg=; b=ebfWs4e6SKPb1ImT3zEl30068j5KQ5KLkN4Hge3p499dhVfZqfTBsbvD rV7TuHGjFBwJiB63HFIL16H10cQ5blOePHIGHgpOp0H0NkuzfQX1Xk+ud EPDZYQBi/7Ssue3f71wZ5HRW1nlYQfCiox9lCXhB1PZyTQ3HmW/UIujew 5u0dTttJ/S3dECz++qK5RjjTeAVjc+LgznIA87wo8gNGy3mjmRMGKYf/z aQyDx56LOSFURj3xpA2dXv/8jXB6lCgXfa2yPycUJh/GEjh6kA+VM3TA9 Ap849Q69NpcbZvPMdLZcRCLnxkM+8F9jlAEfZ78k4n4+RW23DNs+DejpP w==; X-IronPort-AV: E=McAfee;i="6500,9779,10605"; a="315501226" X-IronPort-AV: E=Sophos;i="5.97,257,1669104000"; d="scan'208";a="315501226" 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:16:05 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10605"; a="732677175" X-IronPort-AV: E=Sophos;i="5.97,257,1669104000"; d="scan'208";a="732677175" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga004.fm.intel.com with ESMTP; 30 Jan 2023 04:15:55 -0800 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1pMT4h-00HL8S-0F; Mon, 30 Jan 2023 14:15:51 +0200 Date: Mon, 30 Jan 2023 14:15:50 +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 08/11] driver core: fw_devlink: Make cycle detection more robust Message-ID: References: <20230127001141.407071-1-saravanak@google.com> <20230127001141.407071-9-saravanak@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On Fri, Jan 27, 2023 at 11:34:28PM -0800, Saravana Kannan wrote: > On Fri, Jan 27, 2023 at 1:43 AM Andy Shevchenko > wrote: > > On Thu, Jan 26, 2023 at 04:11:35PM -0800, Saravana Kannan wrote: ... > Lol, you are the king of nit picks :) Not always, only when it comes with something else. ... > > > + * Return true if one or more cycles were found. Otherwise, return false. > > > > Return: > > I'm following the rest of the function docs in this file. Okay, it makes sense. We will need to address them all. > > (you may run `kernel-doc -v ...` to see all warnings) > > Hmmm I ran it on the patch file and it didn't give me anything useful. > Running it on the whole file is just a lot of lines to dig through. The function description missing Return section. Something like this I can get from the kernel doc without it. ... > > > +static bool __fw_devlink_relax_cycles(struct device *con, > > > + struct fwnode_handle *sup_handle) > > > +{ > > > + struct fwnode_link *link; > > > + struct device_link *dev_link; > > > > > + struct device *sup_dev = NULL, *par_dev = NULL; > > > > You can put it the first line since it's long enough. > > Wait, is that a style guideline to have the longer lines first? No, but it's easier to read. > > But why do you need sup_dev assignment? > > Defensive programming I suppose. I can see this function being > refactored in the future where a goto out; is inserted before sup_dev > is assigned. And then the put_device(sup_dev) at "out" will end up > operating on some junk value and causing memory corruption. We don't need to be defensive here. Moreover, it's harder and easy to make a mistake with predefined values (it's actually better NOT to define anything qr at least define as closest to its user as possible, so you want miss the compiler warnings as I believe you run your compiler with `make W=1 ...`, and if not, I highly recommend to get this habit). ... > > > + sup_dev = get_dev_from_fwnode(sup_handle); > > > + > > > > I would put it closer to the condition: > > > > > + /* Termination condition. */ > > > + if (sup_dev == con) { > > > > /* Get supplier device and check for termination condition */ > > sup_dev = get_dev_from_fwnode(sup_handle); > > if (sup_dev == con) { > > I put it the way it is because sup_dev is used for more than just > checking for termination condition. Yes, but still it's better to see what you are actually checking. > > > + ret = true; > > > + goto out; > > > + } ... > > > + if (__fw_devlink_relax_cycles(con, > > > + dev_link->supplier->fwnode)) { > > > > Keep it on one line. > > It'll make it > 80. Is this some recent change about allowing > 80 > cols? I'm leaving it as is for now. Is it a problem? Do you have any tool that complains about it? -- With Best Regards, Andy Shevchenko 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 21F15C54EAA for ; Mon, 30 Jan 2023 17:32:58 +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=mj/24m7u3yGtTT/HCPT7ojC/qCC+xEZQKDnMOj/nffo=; b=Sp1DXgscFrz1dY zH1MeJMTCIn4UTZ/Jyb3rS8UfAp3IfVFRt1mHMnAmZgL99E/8XNwjCX9fR8ccqBlmM644pW+lLOH9 j+XnXb0Ei2mi8PkwB2njZ/JFyVhPd1bwFIs5iY1AXj31UIKM1Iycls7HyNjn+5LGrKJH/LA8vAW// Wy4IoL5Yf53GpgqZHf6dV4F8AF6a3Ukpx2Rkuwk/UP2NB21qSzUzgwwQX+E7+K4cDHkRjMh5f7Xhq 49QPLXXBb+3fNl57XJ3n1holk9jW+oRcs+Fb/GdhOhXPVBAADSQFHpIGvhBbIijkXY5/VyT6rlO2y 1u4xI5dQR2nsi3sywvYQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pMY0R-004Zsi-1R; Mon, 30 Jan 2023 17:31:47 +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 1pMXuo-004Wz9-IB for linux-arm-kernel@lists.infradead.org; Mon, 30 Jan 2023 17:26:02 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1675099559; x=1706635559; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=OoZvW5mYAmBAqzGauPrm+It1FwPUT3sQTZsO9ABVQOg=; b=Z2MuISveuVbOP+URmK+90YYNP3ic915OuwmD3ZlVs/aqyGhYoLsuDUSk pPz5L9U711ldNRcKS2V1pJrLq61KRWeOttawrEQZJFyrZFhwQbRFF2NBu wPCMoNrCWwwklehfncpNXN61Xxrl0QiBaVBJ/MLsrQhDOKjZNCxlZaSb+ /+9pNcSVXwdiuI0s9z/+Elcj378L3apckFV8tgm+JhKe2GOamOHZAG3Qe ipMBSEk7lKiH89/DNldSxpJDvK9cwpH3ICc+EtKPRfeEAM7QJjnFQICwW ONG/SphVt/NY2VZctjTosa7vx3DHJJNQEkxL73+GjEpB8OYTymqSDfrg0 A==; X-IronPort-AV: E=McAfee;i="6500,9779,10605"; a="315501231" X-IronPort-AV: E=Sophos;i="5.97,257,1669104000"; d="scan'208";a="315501231" 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:16:05 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10605"; a="732677175" X-IronPort-AV: E=Sophos;i="5.97,257,1669104000"; d="scan'208";a="732677175" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga004.fm.intel.com with ESMTP; 30 Jan 2023 04:15:55 -0800 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1pMT4h-00HL8S-0F; Mon, 30 Jan 2023 14:15:51 +0200 Date: Mon, 30 Jan 2023 14:15:50 +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 08/11] driver core: fw_devlink: Make cycle detection more robust Message-ID: References: <20230127001141.407071-1-saravanak@google.com> <20230127001141.407071-9-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_092559_516232_F549DE6E X-CRM114-Status: GOOD ( 38.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, Jan 27, 2023 at 11:34:28PM -0800, Saravana Kannan wrote: > On Fri, Jan 27, 2023 at 1:43 AM Andy Shevchenko > wrote: > > On Thu, Jan 26, 2023 at 04:11:35PM -0800, Saravana Kannan wrote: ... > Lol, you are the king of nit picks :) Not always, only when it comes with something else. ... > > > + * Return true if one or more cycles were found. Otherwise, return false. > > > > Return: > > I'm following the rest of the function docs in this file. Okay, it makes sense. We will need to address them all. > > (you may run `kernel-doc -v ...` to see all warnings) > > Hmmm I ran it on the patch file and it didn't give me anything useful. > Running it on the whole file is just a lot of lines to dig through. The function description missing Return section. Something like this I can get from the kernel doc without it. ... > > > +static bool __fw_devlink_relax_cycles(struct device *con, > > > + struct fwnode_handle *sup_handle) > > > +{ > > > + struct fwnode_link *link; > > > + struct device_link *dev_link; > > > > > + struct device *sup_dev = NULL, *par_dev = NULL; > > > > You can put it the first line since it's long enough. > > Wait, is that a style guideline to have the longer lines first? No, but it's easier to read. > > But why do you need sup_dev assignment? > > Defensive programming I suppose. I can see this function being > refactored in the future where a goto out; is inserted before sup_dev > is assigned. And then the put_device(sup_dev) at "out" will end up > operating on some junk value and causing memory corruption. We don't need to be defensive here. Moreover, it's harder and easy to make a mistake with predefined values (it's actually better NOT to define anything qr at least define as closest to its user as possible, so you want miss the compiler warnings as I believe you run your compiler with `make W=1 ...`, and if not, I highly recommend to get this habit). ... > > > + sup_dev = get_dev_from_fwnode(sup_handle); > > > + > > > > I would put it closer to the condition: > > > > > + /* Termination condition. */ > > > + if (sup_dev == con) { > > > > /* Get supplier device and check for termination condition */ > > sup_dev = get_dev_from_fwnode(sup_handle); > > if (sup_dev == con) { > > I put it the way it is because sup_dev is used for more than just > checking for termination condition. Yes, but still it's better to see what you are actually checking. > > > + ret = true; > > > + goto out; > > > + } ... > > > + if (__fw_devlink_relax_cycles(con, > > > + dev_link->supplier->fwnode)) { > > > > Keep it on one line. > > It'll make it > 80. Is this some recent change about allowing > 80 > cols? I'm leaving it as is for now. Is it a problem? Do you have any tool that complains about it? -- 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