Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1299988 > unrolled thread

[PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] net-libertas: Better exception handling in  if_spi_host_to_card_worker() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-01 22:40 +0100
    Re: [PATCH] net-libertas: Better exception handling in  if_spi_host_to_card_worker() Julia Lawall <julia.lawall@lip6.fr> - 2016-01-01 22:50 +0100
    Re: [PATCH] net-libertas: Better exception handling in  if_spi_host_to_card_worker() Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-01-02 00:20 +0100
      Re: [PATCH] net-libertas: Better exception handling in  if_spi_host_to_card_worker() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-02 09:20 +0100
        Re: [PATCH] net-libertas: Better exception handling in  if_spi_host_to_card_worker() Julia Lawall <julia.lawall@lip6.fr> - 2016-01-02 09:30 +0100
          Re: net-libertas: Better exception handling in  if_spi_host_to_card_worker() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-02 10:20 +0100
            Re: net-libertas: Better exception handling in  if_spi_host_to_card_worker() Arend van Spriel <aspriel@gmail.com> - 2016-01-02 11:20 +0100
              Re: net-libertas: Better exception handling in  if_spi_host_to_card_worker() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-02 12:30 +0100
                Re: net-libertas: Better exception handling in  if_spi_host_to_card_worker() Arend van Spriel <aspriel@gmail.com> - 2016-01-03 10:40 +0100
                  Re: net-libertas: Better exception handling in  if_spi_host_to_card_worker() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-03 13:20 +0100
                  Re: net-libertas: Better exception handling in if_spi_host_to_card_worker() Rafał Miłecki <zajec5@gmail.com> - 2016-01-03 16:20 +0100
                    Re: net-libertas: Better exception handling in  if_spi_host_to_card_worker() Arend van Spriel <aspriel@gmail.com> - 2016-01-04 11:10 +0100
                      Re: net-libertas: Better exception handling in if_spi_host_to_card_worker() Rafał Miłecki <zajec5@gmail.com> - 2016-01-04 12:20 +0100

#1299988 — [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 22:27:20 +0100

This issue was detected by using the Coccinelle software.

Move the jump label directly before the desired log statement
so that the variable "err" will not be checked once more
after it was determined that a function call failed.
Use the identifier "report_failure" instead of the label "err".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/marvell/libertas/if_spi.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/if_spi.c b/drivers/net/wireless/marvell/libertas/if_spi.c
index 82c0796..c9ae27e 100644
--- a/drivers/net/wireless/marvell/libertas/if_spi.c
+++ b/drivers/net/wireless/marvell/libertas/if_spi.c
@@ -880,18 +880,18 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
 				&hiStatus);
 	if (err) {
 		netdev_err(priv->dev, "I/O error\n");
-		goto err;
+		goto report_failure;
 	}
 
 	if (hiStatus & IF_SPI_HIST_CMD_UPLOAD_RDY) {
 		err = if_spi_c2h_cmd(card);
 		if (err)
-			goto err;
+			goto report_failure;
 	}
 	if (hiStatus & IF_SPI_HIST_RX_UPLOAD_RDY) {
 		err = if_spi_c2h_data(card);
 		if (err)
-			goto err;
+			goto report_failure;
 	}
 
 	/*
@@ -940,9 +940,10 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
 	if (hiStatus & IF_SPI_HIST_CARD_EVENT)
 		if_spi_e2h(card);
 
-err:
-	if (err)
+	if (err) {
+report_failure:
 		netdev_err(priv->dev, "%s: got error %d\n", __func__, err);
+	}
 
 	lbs_deb_leave(LBS_DEB_SPI);
 }
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1299989

On Fri, 1 Jan 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 22:27:20 +0100
> 
> This issue was detected by using the Coccinelle software.
> 
> Move the jump label directly before the desired log statement
> so that the variable "err" will not be checked once more
> after it was determined that a function call failed.

Putting a label inside an if is ugly.

julia

> Use the identifier "report_failure" instead of the label "err".
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/marvell/libertas/if_spi.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/libertas/if_spi.c b/drivers/net/wireless/marvell/libertas/if_spi.c
> index 82c0796..c9ae27e 100644
> --- a/drivers/net/wireless/marvell/libertas/if_spi.c
> +++ b/drivers/net/wireless/marvell/libertas/if_spi.c
> @@ -880,18 +880,18 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
>  				&hiStatus);
>  	if (err) {
>  		netdev_err(priv->dev, "I/O error\n");
> -		goto err;
> +		goto report_failure;
>  	}
>  
>  	if (hiStatus & IF_SPI_HIST_CMD_UPLOAD_RDY) {
>  		err = if_spi_c2h_cmd(card);
>  		if (err)
> -			goto err;
> +			goto report_failure;
>  	}
>  	if (hiStatus & IF_SPI_HIST_RX_UPLOAD_RDY) {
>  		err = if_spi_c2h_data(card);
>  		if (err)
> -			goto err;
> +			goto report_failure;
>  	}
>  
>  	/*
> @@ -940,9 +940,10 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
>  	if (hiStatus & IF_SPI_HIST_CARD_EVENT)
>  		if_spi_e2h(card);
>  
> -err:
> -	if (err)
> +	if (err) {
> +report_failure:
>  		netdev_err(priv->dev, "%s: got error %d\n", __func__, err);
> +	}
>  
>  	lbs_deb_leave(LBS_DEB_SPI);
>  }
> -- 
> 2.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300010

Hello

On 1/2/2016 12:33 AM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 22:27:20 +0100
>
> This issue was detected by using the Coccinelle software.
>
> Move the jump label directly before the desired log statement
> so that the variable "err" will not be checked once more
> after it was determined that a function call failed.
> Use the identifier "report_failure" instead of the label "err".

    Why? The code was smart enough and you're making it uglier that it needs 
to be.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300039

>> Move the jump label directly before the desired log statement
>> so that the variable "err" will not be checked once more
>> after it was determined that a function call failed.
>> Use the identifier "report_failure" instead of the label "err".
> 
>    Why?

I suggest to reconsider the places with which such a jump label
is connected.


> The code was smart enough

Which action should really be performed after a failure was detected
and handled a bit already?

* Another condition check

* Just additional error logging


> and you're making it uglier that it needs to be.

I assume that a software development taste can evolve, can't it?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300040

On Sat, 2 Jan 2016, SF Markus Elfring wrote:

> >> Move the jump label directly before the desired log statement
> >> so that the variable "err" will not be checked once more
> >> after it was determined that a function call failed.
> >> Use the identifier "report_failure" instead of the label "err".
> > 
> >    Why?
> 
> I suggest to reconsider the places with which such a jump label
> is connected.
> 
> 
> > The code was smart enough
> 
> Which action should really be performed after a failure was detected
> and handled a bit already?
> 
> * Another condition check
> 
> * Just additional error logging
> 
> 
> > and you're making it uglier that it needs to be.
> 
> I assume that a software development taste can evolve, can't it?

So far, you have gotten several down votes for this kind of change, and no 
enthusiasm.

Admittedly, this is a trivial case, because there are no local variables, 
but do you actually know the semantics in C of a jump into a block?  And 
if you do know, do you think that this semantics is common knowledge?  And 
do you really think that introducing poorly understandable code is really 
worth saving an if test of a single variable on a non-critical path?

Most of the kernel code is not performance critical at the level of a 
single if test.  So the goal should be for the code to be easy to 
understand and robust to change.  The code that is performance critical, 
you should probably not touch, ever.  The people who wrote it knew what 
was important and what was not.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300043 — Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

>> I assume that a software development taste can evolve, can't it?
> 
> So far, you have gotten several down votes for this kind of change,

I am curious when more contributors will share corresponding opinions.


> and no enthusiasm.

How many software designers and developers can become enthusiastic
about better exception handling to some degree?


> The code that is performance critical, you should probably not touch, ever.

I imagine that technical evolution will result in further considerations
so that "unchangeable" components can be adjusted once more.


> The people who wrote it knew what was important and what was not.

I might come along at some places where the affected knowledge will also evolve.

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300048 — Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

On 02-01-16 10:08, SF Markus Elfring wrote:
>>> I assume that a software development taste can evolve, can't it?
>>
>> So far, you have gotten several down votes for this kind of change,
> 
> I am curious when more contributors will share corresponding opinions.

Let's burn some cycles on this while the holidays give me time to do so.
"software development taste" is another term for "coding style". In
every project battles are fought over this between friends and foes. I
have never seen much evolution going on in this area.

>> and no enthusiasm.
> 
> How many software designers and developers can become enthusiastic
> about better exception handling to some degree?

I had to  take a look at this particular patch and I have to say that I
don't see, using your favorite term, evolution at work. It looks more
like the result of inbred. What the patch tries to do is avoid the extra
'if (err)'. Setting coding style aside, the question is whether there is
a good metric for the patch. So does it really safe processing time? Did
you look at the resulting assembly code for different target architectures?

You got pushed back on the change so you have to come up with solid
arguments for your change instead of spewing ideas about evolution in
software development. Running Coccinelle is one thing, but understanding
the results and what you are ultimately proposing to be changed is more
important.

Regards,
Arend

>> The code that is performance critical, you should probably not touch, ever.
> 
> I imagine that technical evolution will result in further considerations
> so that "unchangeable" components can be adjusted once more.
> 
> 
>> The people who wrote it knew what was important and what was not.
> 
> I might come along at some places where the affected knowledge will also evolve.
> 
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300059 — Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

> I have never seen much evolution going on in this area.

I can get an other impression from a specific document for example.
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/log/Documentation/CodingStyle


> What the patch tries to do is avoid the extra 'if (err)'.

Yes. - I propose to look at related consequences together with the usage
of a popular short jump label once more.


> Setting coding style aside, the question is whether there is
> a good metric for the patch.

A software development challenge is to accept changes also around a topic
like "error handling". My update suggestion for the source file
"drivers/net/wireless/marvell/libertas/if_spi.c" should only improve
exception handling. (I came along other source files where more improvements
will eventually be possible.)

When will the run-time behaviour matter also for exceptional situations?


> Did you look at the resulting assembly code for different target architectures?

Not yet. - Which execution system variants would you recommend for
further comparisons?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300281 — Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

On 02-01-16 12:21, SF Markus Elfring wrote:
>> I have never seen much evolution going on in this area.
> 
> I can get an other impression from a specific document for example.
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/log/Documentation/CodingStyle
> 
> 
>> What the patch tries to do is avoid the extra 'if (err)'.
> 
> Yes. - I propose to look at related consequences together with the usage
> of a popular short jump label once more.

When I read a subject saying "Better exception handling" it sounds like
a functional improvement. Your change does not change anything
functionally and may or may not save a bit of execution time depending
on how smart the compiler is. What you change does is confuse people
reading the code. So please explain why your update improves exception
handling here. I don't see it. The code is not making the driver more
robust against failures in this function, which is what I think of
reading "better exception handling".

>> Setting coding style aside, the question is whether there is
>> a good metric for the patch.
> 
> A software development challenge is to accept changes also around a topic
> like "error handling". My update suggestion for the source file
> "drivers/net/wireless/marvell/libertas/if_spi.c" should only improve
> exception handling. (I came along other source files where more improvements
> will eventually be possible.)
> 
> When will the run-time behaviour matter also for exceptional situations?
> 
> 
>> Did you look at the resulting assembly code for different target architectures?
> 
> Not yet. - Which execution system variants would you recommend for
> further comparisons?

Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300290 — Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

>>> What the patch tries to do is avoid the extra 'if (err)'.
>>
>> Yes. - I propose to look at related consequences together with the usage
>> of a popular short jump label once more.
> 
> When I read a subject saying "Better exception handling" it sounds like
> a functional improvement. Your change does not change anything
> functionally and may or may not save a bit of execution time depending
> on how smart the compiler is.

Can it eventually matter to skip another condition check in three cases?


> What you change does is confuse people reading the code.

A few software developers might find this proposal unusual.


> So please explain why your update improves exception handling here.
> I don't see it.

How does this feedback fit to the mentioned check avoidance?


> The code is not making the driver more robust against failures

That's true for this update suggestion.


> in this function, which is what I think of reading "better exception handling".

Other implementation details are affected by the shown fine-tuning.

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300319 — Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

On 3 January 2016 at 10:36, Arend van Spriel <aspriel@gmail.com> wrote:
> On 02-01-16 12:21, SF Markus Elfring wrote:
>>> Did you look at the resulting assembly code for different target architectures?
>>
>> Not yet. - Which execution system variants would you recommend for
>> further comparisons?
>
> Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.

Oh, don't forget about MIPS with its fancy branches handling. You know
about it, don't you?

I'm against this patch as well.

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300599 — Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

On 03-01-16 16:18, Rafał Miłecki wrote:
> On 3 January 2016 at 10:36, Arend van Spriel <aspriel@gmail.com> wrote:
>> On 02-01-16 12:21, SF Markus Elfring wrote:
>>>> Did you look at the resulting assembly code for different target architectures?
>>>
>>> Not yet. - Which execution system variants would you recommend for
>>> further comparisons?
>>
>> Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.
> 
> Oh, don't forget about MIPS with its fancy branches handling. You know
> about it, don't you?

You are asking me, right ;-) ? I have come across my share of MIPS
platforms here at Broadcom, but I still try to avoid them as much as
possible.

> I'm against this patch as well.

and counting... :-p

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300637 — Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

On 4 January 2016 at 11:05, Arend van Spriel <aspriel@gmail.com> wrote:
> On 03-01-16 16:18, Rafał Miłecki wrote:
>> On 3 January 2016 at 10:36, Arend van Spriel <aspriel@gmail.com> wrote:
>>> On 02-01-16 12:21, SF Markus Elfring wrote:
>>>>> Did you look at the resulting assembly code for different target architectures?
>>>>
>>>> Not yet. - Which execution system variants would you recommend for
>>>> further comparisons?
>>>
>>> Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.
>>
>> Oh, don't forget about MIPS with its fancy branches handling. You know
>> about it, don't you?
>
> You are asking me, right ;-) ? I have come across my share of MIPS
> platforms here at Broadcom, but I still try to avoid them as much as
> possible.

I was more thinking about author on this patch. But it's indeed an
interesting thing, just to know, how MIPS CPU handles branches ;)

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web