From: "H. Nikolaus Schaller" <hns@goldelico.com>
To: Ladislav Michl <ladis@linux-mips.org>
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>,
Chanwoo Choi <cw00.choi@samsung.com>,
linux-kernel@vger.kernel.org, letux-kernel@openphoenux.org,
kernel@pyra-handheld.com, linux-omap@vger.kernel.org
Subject: Re: [PATCH v3] extcon: palmas: hide error messages if gpio returns -EPROBE_DEFER
Date: Mon, 17 Feb 2020 20:33:52 +0100 [thread overview]
Message-ID: <017C406F-0DD9-478F-8AD5-D950A4000305@goldelico.com> (raw)
In-Reply-To: <20200217190745.GA147152@lenoch>
Hi Ladis,
> Am 17.02.2020 um 20:07 schrieb Ladislav Michl <ladis@linux-mips.org>:
>
> On Mon, Feb 17, 2020 at 07:38:16PM +0100, H. Nikolaus Schaller wrote:
>> Hi,
>>
>>> Am 17.02.2020 um 19:29 schrieb Ladislav Michl <ladis@linux-mips.org>:
>>>
>>> On Mon, Feb 17, 2020 at 02:58:14PM +0100, H. Nikolaus Schaller wrote:
>>>>
>>>>> Am 17.02.2020 um 14:38 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>>>>
>>>>> If the gpios are probed after this driver (e.g. if they
>>>>> come from an i2c expander) there is no need to print an
>>>>> error message.
>>>>>
>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>> ---
>>>>> drivers/extcon/extcon-palmas.c | 8 ++++++--
>>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>>>>> index edc5016f46f1..cea58d0cb457 100644
>>>>> --- a/drivers/extcon/extcon-palmas.c
>>>>> +++ b/drivers/extcon/extcon-palmas.c
>>>>> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
>>>>>
>>>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>>>>> GPIOD_IN);
>>>>> - if (IS_ERR(palmas_usb->id_gpiod)) {
>>>>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
>>>>> + return -EPROBE_DEFER;
>>>>> + } else if (IS_ERR(palmas_usb->id_gpiod)) {
>>>>
>>>> Hm.
>>>>
>>>> While looking again at that: why do we need the "{" and "} else "?
>>>>
>>>> It should be sufficient to have
>>>>
>>>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>>>>> GPIOD_IN);
>>>>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER)
>>>>> + return -EPROBE_DEFER;
>>>>> if (IS_ERR(palmas_usb->id_gpiod)) {
>>>>
>>>> What do you think is better coding style here?
>>>
>>> How about something like this? (just an idea with some work left for you ;-))
>>>
>>> --- a/drivers/extcon/extcon-palmas.c
>>> +++ b/drivers/extcon/extcon-palmas.c
>>> @@ -206,8 +206,10 @@ static int palmas_usb_probe(struct platform_device *pdev)
>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>>> GPIOD_IN);
>>> if (IS_ERR(palmas_usb->id_gpiod)) {
>>> - dev_err(&pdev->dev, "failed to get id gpio\n");
>>> - return PTR_ERR(palmas_usb->id_gpiod);
>>> + status = PTR_ERR(palmas_usb->id_gpiod);
>>> + if (status != -EPROBE_DEFER)
>>> + dev_err(&pdev->dev, "failed to get id gpio: %d\n", status);
>>> + return status;
>>> }
>>
>> Well, what would be the improvement?
>
> Linux kernel prints so many lines on bootup and only few of them are
> valuable. Lets improve it by printing error value to give a clue
> why it failed.
Hm. The upstream code does already print the error. This feature is not removed.
But it is also printing an error in the EPROBE_DEFER case as well, where it is
not needed or not an error.
And that is the whole purpose of this patch to get rid of it in the EPROBE_DEFER
case.
My question meant: what your proposal does improve over previous versions of
this patch. My first one was:
https://lkml.org/lkml/2020/2/17/220
which is very similar except not using an explicit temporary variable (which I
think is something every modern compiler will introduce). So the assembler
code is likely the same.
>
>> It needs an additional variable and makes the change more complex.
>
> That additional variable is already there...
Or a register assigned by the compiler.
>
>> The main suggestion by Chanwoo Choi was to move the check for EPROBE_DEFER
>> outside of the IS_ERR() because checking this first and then for EPROBE_DEFER
>> is not necessary.
>
> True, but there are two checks either way and this is slow path.
Well, it depends on likelihood of each code path... That is quite
difficult to assess.
>
>> If acceptable I'd prefer my last proposal. It just adds 2 LOC before
>> and without touching the existing if (IS_ERR(...)).
>
> I have no strong opinion. I was just waiting for project to compile
> so, consider my reply as product of boredom :)
Yes, compile times have increased significantly over the years :)
> (However, I do not like "let's touch only minimal number of lines"
> argument. End result should still matter more)
I would have been happy with my first proposal, but review suggested
to change it.
There is also some discussion for using IS_ERR() and PTR_ERR() == -Esomething
first or second: https://lore.kernel.org/patchwork/patch/999602/
Well, about the end-result: this code path is run only once during
probe time. And that makes a difference in the sub-µs range. So I don't
mind about the likelyhood. More concern is to have the code correct
and not introduce regressions.
And there the lines of code rule comes in: the less I change the less
I can break.
(Yes my compiler is also busy making me wait for result... so that
I can formulate such fundamental statements :).
>
>> If the compiler is clever it can cache palmas_usb->id_gpiod in a register
>> which serves the same purpose as the status variable.
>
> I'm not trying to outsmart compiler, but note status variable is needed
> three times.
BR and thanks,
Nikolaus
next prev parent reply other threads:[~2020-02-17 19:34 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20200217133832epcas1p329af393e88fa76189ca141d2534f9ad2@epcas1p3.samsung.com>
2020-02-17 13:38 ` [PATCH v3] extcon: palmas: hide error messages if gpio returns -EPROBE_DEFER H. Nikolaus Schaller
2020-02-17 13:58 ` H. Nikolaus Schaller
2020-02-17 18:29 ` Ladislav Michl
2020-02-17 18:38 ` H. Nikolaus Schaller
2020-02-17 19:07 ` Ladislav Michl
2020-02-17 19:33 ` H. Nikolaus Schaller [this message]
2020-02-17 20:19 ` Ladislav Michl
2020-02-17 20:25 ` H. Nikolaus Schaller
2020-02-18 11:49 ` Ladislav Michl
2020-02-18 11:13 ` Chanwoo Choi
2020-02-18 3:28 ` Chanwoo Choi
2020-02-18 10:21 ` Ladislav Michl
2020-02-18 10:35 ` Chanwoo Choi
2020-02-18 10:48 ` Ladislav Michl
2020-02-18 11:09 ` Chanwoo Choi
2020-02-21 7:47 ` Ladislav Michl
2020-02-24 2:12 ` Chanwoo Choi
2020-02-24 7:31 ` Ladislav Michl
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=017C406F-0DD9-478F-8AD5-D950A4000305@goldelico.com \
--to=hns@goldelico.com \
--cc=cw00.choi@samsung.com \
--cc=kernel@pyra-handheld.com \
--cc=ladis@linux-mips.org \
--cc=letux-kernel@openphoenux.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=myungjoo.ham@samsung.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: 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).