From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Saravana Kannan <saravanak@google.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael@kernel.org>, Sudeep Holla <sudeep.holla@arm.com>, Cristian Marussi <cristian.marussi@arm.com>, Linus Walleij <linus.walleij@linaro.org>, Bartosz Golaszewski <brgl@bgdev.pl>, Thomas Gleixner <tglx@linutronix.de>, Marc Zyngier <maz@kernel.org>, Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix Kernel Team <kernel@pengutronix.de>, Fabio Estevam <festevam@gmail.com>, NXP Linux Team <linux-imx@nxp.com>, Rob Herring <robh+dt@kernel.org>, Frank Rowand <frowand.list@gmail.com>, Geert Uytterhoeven <geert+renesas@glider.be>, Magnus Damm <magnus.damm@gmail.com>, Len Brown <lenb@kernel.org>, Daniel Scally <djrscally@gmail.com>, Heikki Krogerus <heikki.krogerus@linux.intel.com>, Sakari Ailus <sakari.ailus@linux.intel.com>, Tony Lindgren <tony@atomide.com>, Linux Kernel Functional Testing <lkft@linaro.org>, Naresh Kamboju <naresh.kamboju@linaro.org>, Abel Vesa <abel.vesa@linaro.org>, Alexander Stein <alexander.stein@ew.tq-group.com>, Geert Uytterhoeven <geert@linux-m68k.org>, John Stultz <jstultz@google.com>, Doug Anderson <dianders@chromium.org>, Guenter Roeck <linux@roeck-us.net>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Maxim Kiselev <bigunclemax@gmail.com>, Maxim Kochetkov <fido_max@inbox.ru>, Miquel Raynal <miquel.raynal@bootlin.com>, Luca Weiss <luca.weiss@fairphone.com>, Colin Foster <colin.foster@in-advantage.com>, Martin Kepplinger <martin.kepplinger@puri.sm>, Jean-Philippe Brucker <jpb@kernel.org>, 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 Date: Mon, 30 Jan 2023 14:15:50 +0200 [thread overview] Message-ID: <Y9e09qUa9CDxHFcb@smile.fi.intel.com> (raw) In-Reply-To: <CAGETcx-PiV12pKnVuKyvNcjYbHA=YFJG1QUa-o-G1cL3iMHgcA@mail.gmail.com> On Fri, Jan 27, 2023 at 11:34:28PM -0800, Saravana Kannan wrote: > On Fri, Jan 27, 2023 at 1:43 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Saravana Kannan <saravanak@google.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael@kernel.org>, Sudeep Holla <sudeep.holla@arm.com>, Cristian Marussi <cristian.marussi@arm.com>, Linus Walleij <linus.walleij@linaro.org>, Bartosz Golaszewski <brgl@bgdev.pl>, Thomas Gleixner <tglx@linutronix.de>, Marc Zyngier <maz@kernel.org>, Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix Kernel Team <kernel@pengutronix.de>, Fabio Estevam <festevam@gmail.com>, NXP Linux Team <linux-imx@nxp.com>, Rob Herring <robh+dt@kernel.org>, Frank Rowand <frowand.list@gmail.com>, Geert Uytterhoeven <geert+renesas@glider.be>, Magnus Damm <magnus.damm@gmail.com>, Len Brown <lenb@kernel.org>, Daniel Scally <djrscally@gmail.com>, Heikki Krogerus <heikki.krogerus@linux.intel.com>, Sakari Ailus <sakari.ailus@linux.intel.com>, Tony Lindgren <tony@atomide.com>, Linux Kernel Functional Testing <lkft@linaro.org>, Naresh Kamboju <naresh.kamboju@linaro.org>, Abel Vesa <abel.vesa@linaro.org>, Alexander Stein <alexander.stein@ew.tq-group.com>, Geert Uytterhoeven <geert@linux-m68k.org>, John Stultz <jstultz@google.com>, Doug Anderson <dianders@chromium.org>, Guenter Roeck <linux@roeck-us.net>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Maxim Kiselev <bigunclemax@gmail.com>, Maxim Kochetkov <fido_max@inbox.ru>, Miquel Raynal <miquel.raynal@bootlin.com>, Luca Weiss <luca.weiss@fairphone.com>, Colin Foster <colin.foster@in-advantage.com>, Martin Kepplinger <martin.kepplinger@puri.sm>, Jean-Philippe Brucker <jpb@kernel.org>, 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 Date: Mon, 30 Jan 2023 14:15:50 +0200 [thread overview] Message-ID: <Y9e09qUa9CDxHFcb@smile.fi.intel.com> (raw) In-Reply-To: <CAGETcx-PiV12pKnVuKyvNcjYbHA=YFJG1QUa-o-G1cL3iMHgcA@mail.gmail.com> On Fri, Jan 27, 2023 at 11:34:28PM -0800, Saravana Kannan wrote: > On Fri, Jan 27, 2023 at 1:43 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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
next prev parent reply other threads:[~2023-01-30 12:16 UTC|newest] Thread overview: 146+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-01-27 0:11 [PATCH v2 00/11] fw_devlink improvements Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-27 0:11 ` [PATCH v2 01/11] driver core: fw_devlink: Don't purge child fwnode's consumer links Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-27 9:21 ` Andy Shevchenko 2023-01-27 9:21 ` Andy Shevchenko 2023-01-28 7:33 ` Saravana Kannan 2023-01-28 7:33 ` Saravana Kannan 2023-01-30 12:04 ` Andy Shevchenko 2023-01-30 12:04 ` Andy Shevchenko 2023-01-27 0:11 ` [PATCH v2 02/11] driver core: fw_devlink: Improve check for fwnode with no device/driver Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-27 0:11 ` [PATCH v2 03/11] soc: renesas: Move away from using OF_POPULATED for fw_devlink Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-27 8:11 ` Geert Uytterhoeven 2023-01-27 8:11 ` Geert Uytterhoeven 2023-01-28 7:18 ` Saravana Kannan 2023-01-28 7:18 ` Saravana Kannan 2023-01-30 8:42 ` Geert Uytterhoeven 2023-01-30 8:42 ` Geert Uytterhoeven 2023-01-30 20:00 ` Saravana Kannan 2023-01-30 20:00 ` Saravana Kannan 2023-01-31 8:14 ` Geert Uytterhoeven 2023-01-31 8:14 ` Geert Uytterhoeven 2023-02-04 22:30 ` Saravana Kannan 2023-02-04 22:30 ` Saravana Kannan 2023-01-27 9:25 ` Andy Shevchenko 2023-01-27 9:25 ` Andy Shevchenko 2023-01-27 9:30 ` Geert Uytterhoeven 2023-01-27 9:30 ` Geert Uytterhoeven 2023-01-27 9:44 ` Andy Shevchenko 2023-01-27 9:44 ` Andy Shevchenko 2023-01-27 0:11 ` [PATCH v2 04/11] gpiolib: Clear the gpio_device's fwnode initialized flag before adding Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-27 9:27 ` Andy Shevchenko 2023-01-27 9:27 ` Andy Shevchenko 2023-01-28 7:33 ` Saravana Kannan 2023-01-28 7:33 ` Saravana Kannan 2023-01-30 12:05 ` Andy Shevchenko 2023-01-30 12:05 ` Andy Shevchenko 2023-01-30 14:31 ` Sudeep Holla 2023-01-30 14:31 ` Sudeep Holla 2023-01-30 15:14 ` Andy Shevchenko 2023-01-30 15:14 ` Andy Shevchenko 2023-01-31 4:01 ` Saravana Kannan 2023-01-31 4:01 ` Saravana Kannan 2023-01-31 10:13 ` Sudeep Holla 2023-01-31 10:13 ` Sudeep Holla 2023-02-04 22:32 ` Saravana Kannan 2023-02-04 22:32 ` Saravana Kannan 2023-01-27 0:11 ` [PATCH v2 05/11] driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-27 9:29 ` Andy Shevchenko 2023-01-27 9:29 ` Andy Shevchenko 2023-01-27 9:30 ` Andy Shevchenko 2023-01-27 9:30 ` Andy Shevchenko 2023-01-27 9:55 ` Geert Uytterhoeven 2023-01-27 9:55 ` Geert Uytterhoeven 2023-01-28 7:34 ` Saravana Kannan 2023-01-28 7:34 ` Saravana Kannan 2023-01-30 12:08 ` Andy Shevchenko 2023-01-30 12:08 ` Andy Shevchenko 2023-01-27 0:11 ` [PATCH v2 06/11] driver core: fw_devlink: Allow marking a fwnode link as being part of a cycle Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-27 9:33 ` Andy Shevchenko 2023-01-27 9:33 ` Andy Shevchenko 2023-01-28 7:34 ` Saravana Kannan 2023-01-28 7:34 ` Saravana Kannan 2023-01-30 12:09 ` Andy Shevchenko 2023-01-30 12:09 ` Andy Shevchenko 2023-01-27 0:11 ` [PATCH v2 07/11] driver core: fw_devlink: Consolidate device link flag computation Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-27 0:11 ` [PATCH v2 08/11] driver core: fw_devlink: Make cycle detection more robust Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-27 9:43 ` Andy Shevchenko 2023-01-27 9:43 ` Andy Shevchenko 2023-01-27 9:52 ` Geert Uytterhoeven 2023-01-27 9:52 ` Geert Uytterhoeven 2023-01-27 10:10 ` Andy Shevchenko 2023-01-27 10:10 ` Andy Shevchenko 2023-01-27 10:29 ` Geert Uytterhoeven 2023-01-27 10:29 ` Geert Uytterhoeven 2023-01-28 7:34 ` Saravana Kannan 2023-01-28 7:34 ` Saravana Kannan 2023-01-30 12:15 ` Andy Shevchenko [this message] 2023-01-30 12:15 ` Andy Shevchenko 2023-01-30 14:36 ` Geert Uytterhoeven 2023-01-30 14:36 ` Geert Uytterhoeven 2023-01-30 15:16 ` Andy Shevchenko 2023-01-30 15:16 ` Andy Shevchenko 2023-01-27 0:11 ` [PATCH v2 09/11] of: property: Simplify of_link_to_phandle() Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-30 14:39 ` Sakari Ailus 2023-01-30 14:39 ` Sakari Ailus 2023-01-31 3:51 ` Saravana Kannan 2023-01-31 3:51 ` Saravana Kannan 2023-01-27 0:11 ` [PATCH v2 10/11] irqchip/irq-imx-gpcv2: Mark fwnode device as not initialized Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-27 9:51 ` Andy Shevchenko 2023-01-27 9:51 ` Andy Shevchenko 2023-01-28 7:34 ` Saravana Kannan 2023-01-28 7:34 ` Saravana Kannan 2023-01-27 0:11 ` [PATCH v2 11/11] firmware: arm_scmi: Set fwnode for the scmi_device Saravana Kannan 2023-01-27 0:11 ` Saravana Kannan 2023-01-27 9:52 ` Andy Shevchenko 2023-01-27 9:52 ` Andy Shevchenko 2023-01-27 10:48 ` Sudeep Holla 2023-01-27 10:48 ` Sudeep Holla 2023-01-27 20:30 ` [PATCH v2 00/11] fw_devlink improvements Colin Foster 2023-01-27 20:30 ` Colin Foster 2023-01-27 21:35 ` Saravana Kannan 2023-01-27 21:35 ` Saravana Kannan 2023-01-30 8:55 ` Naresh Kamboju 2023-01-30 8:55 ` Naresh Kamboju 2023-01-30 10:49 ` Marc Zyngier 2023-01-30 10:49 ` Marc Zyngier 2023-01-30 23:03 ` Saravana Kannan 2023-01-30 23:03 ` Saravana Kannan 2023-01-31 10:18 ` Sudeep Holla 2023-01-31 10:18 ` Sudeep Holla 2023-02-02 17:36 ` Maxim Kiselev 2023-02-02 17:36 ` Maxim Kiselev 2023-02-03 6:07 ` Saravana Kannan 2023-02-03 6:07 ` Saravana Kannan 2023-02-03 9:39 ` Maxim Kiselev 2023-02-03 9:39 ` Maxim Kiselev 2023-02-06 1:32 ` Saravana Kannan 2023-02-06 1:32 ` Saravana Kannan 2023-02-06 2:17 ` Saravana Kannan 2023-02-06 2:17 ` Saravana Kannan 2023-02-06 9:39 ` Miquel Raynal 2023-02-06 9:39 ` Miquel Raynal 2023-02-06 20:08 ` Saravana Kannan 2023-02-06 20:08 ` Saravana Kannan 2023-02-24 14:46 ` Miquel Raynal 2023-02-24 14:46 ` Miquel Raynal 2023-02-06 15:18 ` Rob Herring 2023-02-06 15:18 ` Rob Herring 2023-02-06 19:59 ` Saravana Kannan 2023-02-06 19:59 ` Saravana Kannan 2023-01-30 10:48 ` Miquel Raynal 2023-01-30 10:48 ` Miquel Raynal 2023-01-30 12:08 ` Maxim Kiselev 2023-01-30 12:08 ` Maxim Kiselev 2023-01-31 1:20 ` Saravana Kannan 2023-01-31 1:20 ` Saravana Kannan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Y9e09qUa9CDxHFcb@smile.fi.intel.com \ --to=andriy.shevchenko@linux.intel.com \ --cc=abel.vesa@linaro.org \ --cc=alexander.stein@ew.tq-group.com \ --cc=bigunclemax@gmail.com \ --cc=brgl@bgdev.pl \ --cc=colin.foster@in-advantage.com \ --cc=cristian.marussi@arm.com \ --cc=devicetree@vger.kernel.org \ --cc=dianders@chromium.org \ --cc=djrscally@gmail.com \ --cc=dmitry.baryshkov@linaro.org \ --cc=festevam@gmail.com \ --cc=fido_max@inbox.ru \ --cc=frowand.list@gmail.com \ --cc=geert+renesas@glider.be \ --cc=geert@linux-m68k.org \ --cc=gregkh@linuxfoundation.org \ --cc=heikki.krogerus@linux.intel.com \ --cc=jpb@kernel.org \ --cc=jstultz@google.com \ --cc=kernel-team@android.com \ --cc=kernel@pengutronix.de \ --cc=lenb@kernel.org \ --cc=linus.walleij@linaro.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-renesas-soc@vger.kernel.org \ --cc=linux@roeck-us.net \ --cc=lkft@linaro.org \ --cc=luca.weiss@fairphone.com \ --cc=magnus.damm@gmail.com \ --cc=martin.kepplinger@puri.sm \ --cc=maz@kernel.org \ --cc=miquel.raynal@bootlin.com \ --cc=naresh.kamboju@linaro.org \ --cc=rafael@kernel.org \ --cc=robh+dt@kernel.org \ --cc=s.hauer@pengutronix.de \ --cc=sakari.ailus@linux.intel.com \ --cc=saravanak@google.com \ --cc=shawnguo@kernel.org \ --cc=sudeep.holla@arm.com \ --cc=tglx@linutronix.de \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.