linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	Guenter Roeck <linux@roeck-us.net>
Cc: Alim Akhtar <alim.akhtar@samsung.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org, kernel@pengutronix.de,
	linux-kernel@vger.kernel.org,
	Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	linux-watchdog@vger.kernel.org
Subject: [PATCH 0/3] watchdog: s3c2410_wdt: Simplify using dev_err_probe()
Date: Mon,  6 Mar 2023 10:09:16 +0100	[thread overview]
Message-ID: <20230306090919.2206871-1-u.kleine-koenig@pengutronix.de> (raw)
In-Reply-To: <891023d7-9510-445e-9053-ad5c0398d350@roeck-us.net>

Hello,

On Sun, Mar 05, 2023 at 06:31:08AM -0800, Guenter Roeck wrote:
> On Sun, Mar 05, 2023 at 12:15:00PM +0100, Uwe Kleine-König wrote:
> > On Sat, Mar 04, 2023 at 02:10:47PM -0800, Guenter Roeck wrote:
> > > The primary reason to call dev_err_probe() is that the error may be
> > > -EPROBE_DEFER, in which case the error message is suppressed.
> > > That is not the case for those two functions; they never return
> > > -EPROBE_DEFER. Calling dev_err_probe() would give the false impression
> > > that the functions _might_ return -EPROBE_DEFER.
> > 
> > That is subjective. In my book dev_err_probe() handling -EPROBE_DEFER is
> > only one aspect. Another is that using it allows to have return and error
> > message in a single line and also that if already other exit paths use
> > it to get a consistent style for the emitted messages. Having said that
> > *I* wouldn't assume that the previous call might return -EPROBE_DEFER
> > just because dev_err_probe() is used.
> > 
> > Having said that, I also don't think there is much harm if someone
> > thinks that a given function (here devm_request_irq()) might return
> > -EPROBE_DEFER.
> > 
> 
> I guess we'll have to agree to disagree.

I don't agree to disagree. In my eyes you weight the corresponding facts
in an irrational way. To have some code changes to talk about, I created
this series (on top of Guenter's patches to this driver from Saturday
[1]).

Patch 1 is necessary to effectively make use of dev_err_probe(). I admit
this annoys me a bit as it shows that dev_err_probe() isn't a plug-in
replacement, but given that it reduces the binary size a bit, it has at
least positive usefulness. (But maybe this is subjective? *shrug*)

Patch 2 unifies the format of the error messages between the error paths
that make use of dev_err_probe() and those that (only) use dev_err().
IMHO this has mixed value, but still positive in sum. While having a
consistent error log style is nice, having to manually use
dev_err_probe()'s style is a bit ugly. Together with a) the messages get
more useful mentioning the error code and b) the downside is removed by
patch 3, I still like it. (And if you don't because of b), please
consider squashing patches 2 and 3 together. In fact I only created
patch 2 to make the upsides for patch 3 more obvious and I don't mind a
squash.)

Patch 3 does the actual conversion to dev_err_probe() for all error
paths in .probe(). The (unarguable?) upsides are:

 - Reduced line count
 - Reduced binary size (and the reduction mentioned in the commit log
   doesn't even account for the shorter format strings!)

The fact where Guenter doesn't agree to me (and Krzysztof) is:

 - You cannot any more defer from the usage of dev_err_probe() vs.
   dev_err() if the function that failed before might return
   -EPROBE_DEFER.

In my eyes this is irrelevant because there is no objective reason to
have to know that. If the function might return -EPROBE_DEFER or not
shouldn't (and doesn't!) have an influence on how the driver should
behave in the error case. Also the ability to defer that property is
very unreliable. It's not even reliable in drivers/watchdog, look at how
imx2_wdt handles devm_clk_get() or keembay_wdt handles clk_get_rate()
returning zero.

So using dev_err_probe() has two big benefits in contrast to a dubious
and unreliable connection between -EPROBE_DEFER and dev_err_probe().

Best regards
Uwe

[1] https://lore.kernel.org/linux-watchdog/20230304165653.2179835-1-linux@roeck-us.net

Uwe Kleine-König (3):
  watchdog: s3c2410_wdt: Fold s3c2410_get_wdt_drv_data() into only
    caller
  watchdog: s3c2410_wdt: Unify error logging format in probe function
  watchdog: s3c2410_wdt: Simplify using dev_err_probe()

 drivers/watchdog/s3c2410_wdt.c | 93 ++++++++++++++--------------------
 1 file changed, 37 insertions(+), 56 deletions(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
prerequisite-patch-id: 775bdd863307268e1ef16250bf2f40862637b453
prerequisite-patch-id: 924ddfbe583e97e7c9a46c2460ecbc88c29ee319
-- 
2.39.1


  reply	other threads:[~2023-03-06  9:09 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-04 16:56 [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers Guenter Roeck
2023-03-04 16:56 ` [PATCH 2/2] watchdog: s3c2410_wdt: Use devm_add_action_or_reset() to disable watchdog Guenter Roeck
2023-03-06  9:15   ` Uwe Kleine-König
2023-03-06 15:23     ` Guenter Roeck
2023-03-04 21:46 ` [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers Christophe JAILLET
2023-03-04 22:10   ` Guenter Roeck
2023-03-05 11:15     ` Uwe Kleine-König
2023-03-05 13:27       ` Krzysztof Kozlowski
2023-03-05 14:31       ` Guenter Roeck
2023-03-06  9:09         ` Uwe Kleine-König [this message]
2023-03-06  9:09           ` [PATCH 1/3] watchdog: s3c2410_wdt: Fold s3c2410_get_wdt_drv_data() into only caller Uwe Kleine-König
2023-03-06 17:17             ` Guenter Roeck
2023-03-06 17:47               ` Uwe Kleine-König
2023-03-06 18:12                 ` Guenter Roeck
2023-03-06  9:09           ` [PATCH 2/3] watchdog: s3c2410_wdt: Unify error logging format in probe function Uwe Kleine-König
2023-03-06  9:09           ` [PATCH 3/3] watchdog: s3c2410_wdt: Simplify using dev_err_probe() Uwe Kleine-König
2023-03-06  9:10 ` [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers Uwe Kleine-König
2023-04-18  6:56   ` Uwe Kleine-König
2023-04-22 11:22     ` Wim Van Sebroeck
2023-04-23  8:08       ` Uwe Kleine-König

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=20230306090919.2206871-1-u.kleine-koenig@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=alim.akhtar@samsung.com \
    --cc=christophe.jaillet@wanadoo.fr \
    --cc=kernel@pengutronix.de \
    --cc=krzysztof.kozlowski@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=wim@linux-watchdog.org \
    /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: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).