Re: [PATCH v3] extcon: palmas: hide error messages if gpio returns -EPROBE_DEFER

[H. Nikolaus Schaller]

Hi Ladis,

> Am 17.02.2020 um 20:07 schrieb Ladislav Michl <ladis@xxxxxxxxxxxxxx>:
> 
> 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@xxxxxxxxxxxxxx>:
>>> 
>>> 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@xxxxxxxxxxxxx>:
>>>>> 
>>>>> 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@xxxxxxxxxxxxx>
>>>>> ---
>>>>> 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