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

Groups > linux.kernel > #1445067 > unrolled thread

[PATCH 0/9] staging: ks7010: Fine-tuning for a SDIO card driver

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 0/9] staging: ks7010: Fine-tuning for a SDIO card driver SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:10 +0200
    [PATCH 2/9] staging: ks7010: Delete unnecessary assignments for  buffer variables SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:20 +0200
      Re: [PATCH 2/9] staging: ks7010: Delete unnecessary assignments for  buffer variables Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 17:50 +0200
    [PATCH 1/9] staging: ks7010: Delete unnecessary checks before the  function call "kfree" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:20 +0200
      Re: [PATCH 1/9] staging: ks7010: Delete unnecessary checks before  the function call "kfree" Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 17:50 +0200
    [PATCH 3/9] staging: ks7010: Return directly after a failed kmalloc() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:20 +0200
      Re: [PATCH 3/9] staging: ks7010: Return directly after a failed  kmalloc() Julia Lawall <julia.lawall@lip6.fr> - 2016-07-17 21:00 +0200
        Re: [PATCH 3/9] staging: ks7010: Return directly after a failed  kmalloc() Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 17:50 +0200
          Re: [PATCH 3/9] staging: ks7010: Return directly after a failed  kmalloc() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-20 20:50 +0200
            Re: [PATCH 3/9] staging: ks7010: Return directly after a failed  kmalloc() Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 08:30 +0200
              Re: staging: ks7010: Return directly after a failed kmalloc() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-22 09:40 +0200
                Re: staging: ks7010: Return directly after a failed kmalloc() Wolfram Sang <wsa@the-dreams.de> - 2016-07-22 09:50 +0200
    [PATCH 7/9] staging: ks7010: Replace three printk() calls by pr_err() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:30 +0200
      Re: [PATCH 7/9] staging: ks7010: Replace three printk() calls by  pr_err() Joe Perches <joe@perches.com> - 2016-07-17 22:30 +0200
        Re: [PATCH 7/9] staging: ks7010: Replace three printk() calls by  pr_err() Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
    [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the variable  "retval" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:30 +0200
      Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" Julia Lawall <julia.lawall@lip6.fr> - 2016-07-17 21:00 +0200
        Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
          Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-20 20:50 +0200
            Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 08:30 +0200
              Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 09:30 +0200
              Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" Julia Lawall <julia.lawall@lip6.fr> - 2016-07-21 09:30 +0200
                Re: staging: ks7010: Delete unnecessary uses of the variable "retval" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 14:50 +0200
                  Re: staging: ks7010: Delete unnecessary uses of the variable  "retval" Julia Lawall <julia.lawall@lip6.fr> - 2016-07-21 15:00 +0200
                    Re: staging: ks7010: Delete unnecessary uses of the variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 18:40 +0200
              Re: staging: ks7010: Delete unnecessary uses of the variable "retval" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 15:40 +0200
                Re: staging: ks7010: Delete unnecessary uses of the variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 18:40 +0200
                  Re: staging: ks7010: Delete unnecessary uses of the variable "retval" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 23:30 +0200
                    Re: staging: ks7010: Delete unnecessary uses of the variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-22 08:00 +0200
    [PATCH 8/9] staging: ks7010: Delete a variable in write_to_device() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:30 +0200
      Re: [PATCH 8/9] staging: ks7010: Delete a variable in  write_to_device() Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
    [PATCH 4/9] staging: ks7010: Rename jump labels SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:30 +0200
      Re: [PATCH 4/9] staging: ks7010: Rename jump labels Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
        Re: staging: ks7010: Rename jump labels SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-20 20:30 +0200
          Re: staging: ks7010: Rename jump labels Jean Delvare <jdelvare@suse.de> - 2016-07-20 23:20 +0200
            Re: staging: ks7010: Rename jump labels Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 08:30 +0200
            Re: staging: ks7010: Rename jump labels "SF Markus Elfring" <elfring@users.sourceforge.net> - 2016-07-21 10:00 +0200
              Re: staging: ks7010: Rename jump labels Jean Delvare <jdelvare@suse.de> - 2016-07-25 14:40 +0200
            Re: staging: ks7010: Rename jump labels SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 17:40 +0200
              Re: staging: ks7010: Rename jump labels Jean Delvare <jdelvare@suse.de> - 2016-07-21 21:20 +0200
                Re: staging: ks7010: Rename jump labels SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 22:30 +0200
                  Re: staging: ks7010: Rename jump labels Jean Delvare <jdelvare@suse.de> - 2016-07-25 14:40 +0200
                    Re: staging: ks7010: Rename jump labels SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-25 18:20 +0200
                      Re: staging: ks7010: Rename jump labels Jean Delvare <jdelvare@suse.de> - 2016-07-25 23:10 +0200
    [PATCH 6/9] staging: ks7010: Delete unnecessary braces SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:30 +0200
      Re: [PATCH 6/9] staging: ks7010: Delete unnecessary braces Julia Lawall <julia.lawall@lip6.fr> - 2016-07-17 21:00 +0200
        Re: [PATCH 6/9] staging: ks7010: Delete unnecessary braces Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
    [PATCH 9/9] staging: ks7010: Delete three unnecessary variable  initialisations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:40 +0200
      Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable  initialisations Julia Lawall <julia.lawall@lip6.fr> - 2016-07-17 21:00 +0200
        Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable  initialisations Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
        Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable  initialisations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 15:50 +0200
          Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable  initialisations Julia Lawall <julia.lawall@lip6.fr> - 2016-07-21 16:00 +0200
            Re: staging: ks7010: Delete three unnecessary variable  initialisations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 16:20 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1447661 — Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the variable "retval"

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 21, 2016 at 09:26:33AM +0200, Julia Lawall wrote:
> 
> 
> On Thu, 21 Jul 2016, Wolfram Sang wrote:
> 
> >
> > > >>>  	if (atomic_read(&priv->sleepstatus.status) == 0) {
> > > >>>  		rw_data = GCR_B_DOZE;
> > > >>> -		retval =
> > > >>> -		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
> > > >>> -		if (retval) {
> > > >>> +		if (ks7010_sdio_write(priv,
> > > >>> +				      GCR_B,
> > > >>> +				      &rw_data,
> > > >>> +				      sizeof(rw_data))) {
> > > >>
> > > >> A multi-line function call in an if test does not look nice at all.  The
> > > >> original code was an easy-to-read expectable pattern.
> > > >
> > > > I agree. I am not strict on the 80 char limit, especially in cases like
> > > > the above.
> > >
> > > Would you try an other source code formatting for the suggested change pattern?
> >
> > I don't understand the question?
> 
> I think the original code was fine.  x = blah(); if (x) ... is a perfectly
> familiar kernel coding pattern.  There is no benefit in terms of
> performance or understandability in dropping the variable.

I certainly agree to that.

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

#1447666 — Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the variable "retval"

On Thu, 21 Jul 2016, Wolfram Sang wrote:

>
> > >>>  	if (atomic_read(&priv->sleepstatus.status) == 0) {
> > >>>  		rw_data = GCR_B_DOZE;
> > >>> -		retval =
> > >>> -		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
> > >>> -		if (retval) {
> > >>> +		if (ks7010_sdio_write(priv,
> > >>> +				      GCR_B,
> > >>> +				      &rw_data,
> > >>> +				      sizeof(rw_data))) {
> > >>
> > >> A multi-line function call in an if test does not look nice at all.  The
> > >> original code was an easy-to-read expectable pattern.
> > >
> > > I agree. I am not strict on the 80 char limit, especially in cases like
> > > the above.
> >
> > Would you try an other source code formatting for the suggested change pattern?
>
> I don't understand the question?

I think the original code was fine.  x = blah(); if (x) ... is a perfectly
familiar kernel coding pattern.  There is no benefit in terms of
performance or understandability in dropping the variable.

julia

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

#1447851 — Re: staging: ks7010: Delete unnecessary uses of the variable "retval"

> I think the original code was fine.

I suggest to reconsider involved implementation details once more.


> x = blah(); if (x) ... is a perfectly familiar kernel coding pattern.

I can agree to such a general information.


> There is no benefit in terms of performance

It might be possible that a good compiler can also optimise
some unnecessary variable accesses away.

Examples for further background information:
* "Minimize local variables"
   https://eventhelix.com/realtimemantra/basics/optimizingcandcppcode.htm#Minimize%20Local%20Variables

* "Temporary Objects" by Danny Kalev
   http://www.informit.com/guides/content.aspx?g=cplusplus&seqNum=198


> or understandability in dropping the variable.

I guess that we have got different opinions on such an aspect.

* Do you really want to assign every return value from a function call
  to an extra variable before it is used again?

* How many reading and understanding capacity do you need for each
  extra variable?

Regards,
Markus

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

#1447861 — Re: staging: ks7010: Delete unnecessary uses of the variable "retval"

On Thu, 21 Jul 2016, SF Markus Elfring wrote:

> > I think the original code was fine.
>
> I suggest to reconsider involved implementation details once more.
>
>
> > x = blah(); if (x) ... is a perfectly familiar kernel coding pattern.
>
> I can agree to such a general information.
>
>
> > There is no benefit in terms of performance
>
> It might be possible that a good compiler can also optimise
> some unnecessary variable accesses away.

A compiler written by an undergrad can optimize the variable away in this
case.  That's what compilers do.

> Examples for further background information:
> * "Minimize local variables"
>    https://eventhelix.com/realtimemantra/basics/optimizingcandcppcode.htm#Minimize%20Local%20Variables
>
> * "Temporary Objects" by Danny Kalev
>    http://www.informit.com/guides/content.aspx?g=cplusplus&seqNum=198
>
>
> > or understandability in dropping the variable.
>
> I guess that we have got different opinions on such an aspect.
>
> * Do you really want to assign every return value from a function call
>   to an extra variable before it is used again?

If it's a choice between 1) function call on many lines, 2) function call
over 80 characters, and 3) extra variable, I definitely prefer the extra
variable.

julia

>
> * How many reading and understanding capacity do you need for each
>   extra variable?
>
> Regards,
> Markus
> --
> 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
>

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

#1447998 — Re: staging: ks7010: Delete unnecessary uses of the variable "retval"

[Multipart message — attachments visible in raw view] — view raw

> > * Do you really want to assign every return value from a function call
> >   to an extra variable before it is used again?
> 
> If it's a choice between 1) function call on many lines, 2) function call
> over 80 characters, and 3) extra variable, I definitely prefer the extra
> variable.

+1

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

#1447885 — Re: staging: ks7010: Delete unnecessary uses of the variable "retval"

>>>>>  	if (atomic_read(&priv->sleepstatus.status) == 0) {
>>>>>  		rw_data = GCR_B_DOZE;
>>>>> -		retval =
>>>>> -		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
>>>>> -		if (retval) {
>>>>> +		if (ks7010_sdio_write(priv,
>>>>> +				      GCR_B,
>>>>> +				      &rw_data,
>>>>> +				      sizeof(rw_data))) {
>>>>
>>>> A multi-line function call in an if test does not look nice at all.  The
>>>> original code was an easy-to-read expectable pattern.
>>>
>>> I agree. I am not strict on the 80 char limit, especially in cases like
>>> the above.

Will this line length limitation trigger any more collateral evolution
in the discussed software module?


>> Would you try an other source code formatting for the suggested change pattern?
> 
> I don't understand the question?

Can you follow expectations around the proposed refactoring of any
function implementations?

Regards,
Markus

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

#1447999 — Re: staging: ks7010: Delete unnecessary uses of the variable "retval"

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 21, 2016 at 03:35:03PM +0200, SF Markus Elfring wrote:
> >>>>>  	if (atomic_read(&priv->sleepstatus.status) == 0) {
> >>>>>  		rw_data = GCR_B_DOZE;
> >>>>> -		retval =
> >>>>> -		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
> >>>>> -		if (retval) {
> >>>>> +		if (ks7010_sdio_write(priv,
> >>>>> +				      GCR_B,
> >>>>> +				      &rw_data,
> >>>>> +				      sizeof(rw_data))) {
> >>>>
> >>>> A multi-line function call in an if test does not look nice at all.  The
> >>>> original code was an easy-to-read expectable pattern.
> >>>
> >>> I agree. I am not strict on the 80 char limit, especially in cases like
> >>> the above.
> 
> Will this line length limitation trigger any more collateral evolution
> in the discussed software module?
> 
> 
> >> Would you try an other source code formatting for the suggested change pattern?
> > 
> > I don't understand the question?
> 
> Can you follow expectations around the proposed refactoring of any
> function implementations?

I don't understand both questions. Maybe you need to give examples?

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

#1448172 — Re: staging: ks7010: Delete unnecessary uses of the variable "retval"

>> Can you follow expectations around the proposed refactoring of any
>> function implementations?
> 
> I don't understand both questions. Maybe you need to give examples?

I suggest to try the following script (semantic patch for working with
the Coccinelle software) out on the discussed source files.


@checking_function_calls_directly@
identifier checker, retval, work;
expression list el;
statement is, es;
type rt;
@@
 rt checker(...)
 {
  <+...
-retval = work(el);
 if (
-    retval
+    work(el)
    )
    is
 else
    es
  ...+>
 }


Do you find such a source code transformation useful?

Regards,
Markus

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

#1448408 — Re: staging: ks7010: Delete unnecessary uses of the variable "retval"

[Multipart message — attachments visible in raw view] — view raw

> Do you find such a source code transformation useful?

No. I already gave that answer by agreeing to Julia's viewpoint.

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

#1445076 — [PATCH 8/9] staging: ks7010: Delete a variable in write_to_device()

From 01291121668ccb54f1a784765a8d2b5811afa75a Mon Sep 17 00:00:00 2001
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 17 Jul 2016 19:26:15 +0200
Subject: [PATCH 8/9] staging: ks7010: Delete a variable in write_to_device()

The local variable "rc" was assigned a zero at one place.
But it was not read within this function. Thus delete it.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/ks7010/ks7010_sdio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index 4b15aec..e06bc3c 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -291,11 +291,10 @@ static int enqueue_txdev(struct ks_wlan_private *priv, unsigned char *p,
 static int write_to_device(struct ks_wlan_private *priv, unsigned char *buffer,
 			   unsigned long size)
 {
-	int rc, retval;
+	int retval;
 	unsigned char rw_data;
 	struct hostif_hdr *hdr;
 	hdr = (struct hostif_hdr *)buffer;
-	rc = 0;
 
 	DPRINTK(4, "size=%d\n", hdr->size);
 	if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) {
-- 
2.9.1

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

#1447320 — Re: [PATCH 8/9] staging: ks7010: Delete a variable in write_to_device()

[Multipart message — attachments visible in raw view] — view raw

On Sun, Jul 17, 2016 at 08:28:44PM +0200, SF Markus Elfring wrote:
> From 01291121668ccb54f1a784765a8d2b5811afa75a Mon Sep 17 00:00:00 2001
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 17 Jul 2016 19:26:15 +0200
> Subject: [PATCH 8/9] staging: ks7010: Delete a variable in write_to_device()
> 
> The local variable "rc" was assigned a zero at one place.
> But it was not read within this function. Thus delete it.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Reviewed-by: Wolfram Sang <wsa@the-dreams.de>

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

#1445077 — [PATCH 4/9] staging: ks7010: Rename jump labels

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 17 Jul 2016 16:26:18 +0200

Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/ks7010/ks7010_sdio.c | 81 +++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index 9b954cb..eea18fb 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -113,7 +113,7 @@ void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv)
 		DPRINTK(1, "sleep_mode=%d\n", priv->sleep_mode);
 	}
 
- out:
+out:
 	priv->sleep_mode = atomic_read(&priv->sleepstatus.status);
 	return;
 }
@@ -144,7 +144,7 @@ void ks_wlan_hw_sleep_wakeup_request(struct ks_wlan_private *priv)
 		DPRINTK(1, "sleep_mode=%d\n", priv->sleep_mode);
 	}
 
- out:
+out:
 	priv->sleep_mode = atomic_read(&priv->sleepstatus.status);
 	return;
 }
@@ -480,7 +480,7 @@ static void ks_wlan_hw_rx(void *dev, uint16_t size)
 	/* rx_event_task((void *)priv); */
 	tasklet_schedule(&priv->ks_wlan_hw.rx_bh_task);
 
- error_out:
+error_out:
 	return;
 }
 
@@ -523,18 +523,18 @@ static void ks7010_rw_function(struct work_struct *work)
 			queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
 					   &priv->ks_wlan_hw.rw_wq, 1);
 		}
-		goto err_out;
+		goto release_host;
 	}
 
 	/* sleep mode doze */
 	if (atomic_read(&priv->sleepstatus.doze_request) == 1) {
 		ks_wlan_hw_sleep_doze_request(priv);
-		goto err_out;
+		goto release_host;
 	}
 	/* sleep mode wakeup */
 	if (atomic_read(&priv->sleepstatus.wakeup_request) == 1) {
 		ks_wlan_hw_sleep_wakeup_request(priv);
-		goto err_out;
+		goto release_host;
 	}
 
 	/* read (WriteStatus/ReadDataSize FN1:00_0014) */
@@ -543,7 +543,7 @@ static void ks7010_rw_function(struct work_struct *work)
 	if (retval) {
 		DPRINTK(1, " error : WSTATUS_RSIZE=%02X psstatus=%d\n", rw_data,
 			atomic_read(&priv->psstatus.status));
-		goto err_out;
+		goto release_host;
 	}
 	DPRINTK(4, "WSTATUS_RSIZE=%02X\n", rw_data);
 
@@ -555,8 +555,7 @@ static void ks7010_rw_function(struct work_struct *work)
 		tx_device_task((void *)priv);
 	}
 	_ks_wlan_hw_power_save(priv);
-
- err_out:
+release_host:
 	sdio_release_host(priv->ks_wlan_hw.sdio_card->func);
 
 	return;
@@ -664,7 +663,7 @@ static void ks_sdio_interrupt(struct sdio_func *func)
 		} while (rsize);
 	}
 
- intr_out:
+intr_out:
 	queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
 			   &priv->ks_wlan_hw.rw_wq, 0);
 	return;
@@ -720,15 +719,13 @@ static int ks7010_sdio_update_index(struct ks_wlan_private *priv, u32 index)
 	retval = ks7010_sdio_write(priv, WRITE_INDEX, data_buf, sizeof(index));
 	if (retval) {
 		rc = 2;
-		goto error_out;
+		goto free_buf;
 	}
 
 	retval = ks7010_sdio_write(priv, READ_INDEX, data_buf, sizeof(index));
-	if (retval) {
+	if (retval)
 		rc = 3;
-		goto error_out;
-	}
- error_out:
+free_buf:
 	kfree(data_buf);
 	return rc;
 }
@@ -748,16 +745,15 @@ static int ks7010_sdio_data_compare(struct ks_wlan_private *priv, u32 address,
 	retval = ks7010_sdio_read(priv, address, read_buf, size);
 	if (retval) {
 		rc = 2;
-		goto error_out;
+		goto free_buf;
 	}
 	retval = memcmp(data, read_buf, size);
 
 	if (retval) {
 		DPRINTK(0, "data compare error (%d) \n", retval);
 		rc = 3;
-		goto error_out;
 	}
- error_out:
+free_buf:
 	kfree(read_buf);
 	return rc;
 }
@@ -784,7 +780,7 @@ static int ks7010_upload_firmware(struct ks_wlan_private *priv,
 	if (rw_data == GCR_A_RUN) {
 		DPRINTK(0, "MAC firmware running ...\n");
 		rc = 0;
-		goto error_out0;
+		goto release_host;
 	}
 
 	retval = request_firmware(&fw_entry, ROM_FILE, &priv->ks_wlan_hw.sdio_card->func->dev);
@@ -814,14 +810,14 @@ static int ks7010_upload_firmware(struct ks_wlan_private *priv,
 					     KS7010_IRAM_ADDRESS + offset);
 		if (retval) {
 			rc = 6;
-			goto error_out1;
+			goto release_fw;
 		}
 
 		/* Write data */
 		retval = ks7010_sdio_write(priv, DATA_WINDOW, rom_buf, size);
 		if (retval) {
 			rc = 8;
-			goto error_out1;
+			goto release_fw;
 		}
 
 		/* compare */
@@ -829,7 +825,7 @@ static int ks7010_upload_firmware(struct ks_wlan_private *priv,
 		    ks7010_sdio_data_compare(priv, DATA_WINDOW, rom_buf, size);
 		if (retval) {
 			rc = 9;
-			goto error_out1;
+			goto release_fw;
 		}
 		n += size;
 
@@ -840,7 +836,7 @@ static int ks7010_upload_firmware(struct ks_wlan_private *priv,
 	retval = ks7010_sdio_write(priv, GCR_A, &rw_data, sizeof(rw_data));
 	if (retval) {
 		rc = 11;
-		goto error_out1;
+		goto release_fw;
 	}
 	DPRINTK(4, " REMAP Request : GCR_A=%02X\n", rw_data);
 
@@ -851,7 +847,7 @@ static int ks7010_upload_firmware(struct ks_wlan_private *priv,
 		    ks7010_sdio_read(priv, GCR_A, &rw_data, sizeof(rw_data));
 		if (retval) {
 			rc = 11;
-			goto error_out1;
+			goto release_fw;
 		}
 		if (rw_data == GCR_A_RUN)
 			break;
@@ -860,14 +856,13 @@ static int ks7010_upload_firmware(struct ks_wlan_private *priv,
 	if ((50) <= n) {
 		DPRINTK(1, "firmware can't start\n");
 		rc = 12;
-		goto error_out1;
+		goto release_fw;
 	}
 
 	rc = 0;
-
- error_out1:
+release_fw:
 	release_firmware(fw_entry);
- error_out0:
+release_host:
 	sdio_release_host(card->func);
 	kfree(rom_buf);
 	return rc;
@@ -994,20 +989,20 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 	ret = sdio_enable_func(func);
 	DPRINTK(5, "sdio_enable_func() %d\n", ret);
 	if (ret)
-		goto error_free_card;
+		goto free_card;
 
 	/* interrupt disable */
 	sdio_writeb(func, 0, INT_ENABLE, &ret);
 	if (ret)
-		goto error_free_card;
+		goto free_card;
 	sdio_writeb(func, 0xff, INT_PENDING, &ret);
 	if (ret)
-		goto error_disable_func;
+		goto disable_func;
 
 	/* setup interrupt handler */
 	ret = sdio_claim_irq(func, ks_sdio_interrupt);
 	if (ret)
-		goto error_disable_func;
+		goto disable_func;
 
 	sdio_release_host(func);
 
@@ -1020,11 +1015,11 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 	netdev = alloc_etherdev(sizeof(*priv));
 	if (netdev == NULL) {
 		printk(KERN_ERR "ks7010 : Unable to alloc new net device\n");
-		goto error_release_irq;
+		goto release_irq;
 	}
 	if (dev_alloc_name(netdev, "wlan%d") < 0) {
 		printk(KERN_ERR "ks7010 :  Couldn't get name!\n");
-		goto error_free_netdev;
+		goto free_dev;
 	}
 
 	priv = netdev_priv(netdev);
@@ -1038,7 +1033,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 	priv->ks_wlan_hw.read_buf = NULL;
 	priv->ks_wlan_hw.read_buf = kmalloc(RX_DATA_SIZE, GFP_KERNEL);
 	if (!priv->ks_wlan_hw.read_buf) {
-		goto error_free_netdev;
+		goto free_dev;
 	}
 	priv->dev_state = DEVICE_STATE_PREBOOT;
 	priv->net_dev = netdev;
@@ -1066,7 +1061,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 		printk(KERN_ERR
 		       "ks7010: firmware load failed !! retern code = %d\n",
 		       ret);
-		goto error_free_read_buf;
+		goto free_buf;
 	}
 
 	/* interrupt setting */
@@ -1094,7 +1089,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 	priv->ks_wlan_hw.ks7010sdio_wq = create_workqueue("ks7010sdio_wq");
 	if (!priv->ks_wlan_hw.ks7010sdio_wq) {
 		DPRINTK(1, "create_workqueue failed !!\n");
-		goto error_free_read_buf;
+		goto free_buf;
 	}
 
 	INIT_DELAYED_WORK(&priv->ks_wlan_hw.rw_wq, ks7010_rw_function);
@@ -1102,22 +1097,22 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 
 	ret = register_netdev(priv->net_dev);
 	if (ret)
-		goto error_free_read_buf;
+		goto free_buf;
 
 	return 0;
 
- error_free_read_buf:
+free_buf:
 	kfree(priv->ks_wlan_hw.read_buf);
 	priv->ks_wlan_hw.read_buf = NULL;
- error_free_netdev:
+free_dev:
 	free_netdev(priv->net_dev);
 	card->priv = NULL;
- error_release_irq:
+release_irq:
 	sdio_claim_host(func);
 	sdio_release_irq(func);
- error_disable_func:
+disable_func:
 	sdio_disable_func(func);
- error_free_card:
+free_card:
 	sdio_release_host(func);
 	sdio_set_drvdata(func, NULL);
 	kfree(card);
-- 
2.9.1

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

#1447323 — Re: [PATCH 4/9] staging: ks7010: Rename jump labels

[Multipart message — attachments visible in raw view] — view raw

On Sun, Jul 17, 2016 at 08:20:12PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 17 Jul 2016 16:26:18 +0200
> 
> Adjust jump targets according to the Linux coding style convention.

Really? Is that documented somewhere?

Quoting Jean Delvare:

"> It is generally accepted to indent labels with a single space. This
 > avoids breaking the -p option of diff."

So, NACK for now unless we know 'diff' has been fixed.

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

#1447390 — Re: staging: ks7010: Rename jump labels

>> Adjust jump targets according to the Linux coding style convention.
> 
> Really? Is that documented somewhere?

How do you think about information from the chapter "7: Centralized exiting of functions"?
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle?id=47ef4ad2684d380dd6d596140fb79395115c3950#n389


> Quoting Jean Delvare:
> 
> "> It is generally accepted to indent labels with a single space. This
>  > avoids breaking the -p option of diff."

Would you like to take another look at the warning "LEADING_SPACE"?
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=47ef4ad2684d380dd6d596140fb79395115c3950#n3004

Does such a check need further considerations?


> So, NACK for now unless we know 'diff' has been fixed.

I am also curious on corresponding software evolution.

Regards,
Markus

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

#1447481 — Re: staging: ks7010: Rename jump labels

Hello Markus, Wolfram,

On Wed, 20 Jul 2016 20:21:25 +0200, SF Markus Elfring wrote:
> >> Adjust jump targets according to the Linux coding style convention.
> > 
> > Really? Is that documented somewhere?
> 
> How do you think about information from the chapter "7: Centralized exiting of functions"?
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle?id=47ef4ad2684d380dd6d596140fb79395115c3950#n389

I'm not impressed by this piece of documentation. For example, <<also
don't name them after the goto location like "err_kmalloc_failed:">> is
as unclear as you can get. It would be much better to tell what to name
them, if the author thinks it really matters. (Personally I think it is
out of scope of coding style rules.)

Back to the lack of space before labels, it's at best a personal
preference. If you insist on standardizing, I'd call it a bug in the
documentation, which should be fixed. One space before label is the way
to go.

> > Quoting Jean Delvare:

I'm honored :)

> > "> It is generally accepted to indent labels with a single space. This
> >  > avoids breaking the -p option of diff."
> 
> Would you like to take another look at the warning "LEADING_SPACE"?
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=47ef4ad2684d380dd6d596140fb79395115c3950#n3004
> 
> Does such a check need further considerations?

checkpatch has become so complex, it's full of false positives, trying
to enforce rules just because one developer wants to force his view on
all others. I stopped following everything the script says long ago.
I'm cherry-picking now.

That being said... checkpatch does not complain about leading space
before labels. Not even with --strict. So why are you mentioning it
here?

> > So, NACK for now unless we know 'diff' has been fixed.

Full nack from myself as well. Just looking at the thread on lkml makes
me feel dizzy. When you are about to send that amount of messages, you
should pause and think again. Is it really worth it? I think I'd be
less annoyed by regular spam.

> I am also curious on corresponding software evolution.

I wouldn't consider it a bug. Despite the description of option -p in
the diff man page calling it C specific, it looks very generic to me.
As I understand it, it looks for any line not starting with a blank and
containing at least one letter (or maybe just starting with a letter -
I didn't look at the code.) Thankfully it doesn't try to parse C. So it
depends on how you indent your code. Live with it.

It's been that way for so long that changing it now is hardly an
option. That change would be seen as a gratuitous regression by many
(including myself.) And for the record, "git diff" behaves exactly the
same. So it's a de facto standard.

-- 
Jean Delvare
SUSE L3 Support

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

#1447638 — Re: staging: ks7010: Rename jump labels

[Multipart message — attachments visible in raw view] — view raw

Thank you very much for the heads up, Jean!

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

#1447687 — Re: staging: ks7010: Rename jump labels

> > How do you think about information from the chapter "7: Centralized exiting of functions"?
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle?id=47ef4ad2684d380dd6d596140fb79395115c3950#n389
> 
> I'm not impressed by this piece of documentation.

I would also appreciate further improvements there.


> Back to the lack of space before labels, it's at best a personal preference.
> If you insist on standardizing, I'd call it a bug in the documentation,
> which should be fixed. One space before label is the way to go.

Would you like to contribute another patch for such a coding style issue?


> That being said... checkpatch does not complain about leading space
> before labels. Not even with --strict. So why are you mentioning it here?

How do you think about a similar software update?

staging: lustre: Fix a jump label position in osc_get_info()
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c71d264543f759fea147734cb63de36397817534
https://lkml.org/lkml/2015/12/21/401


> Just looking at the thread on lkml makes me feel dizzy.

I understand your concern to some degree.


> When you are about to send that amount of messages,
> you should pause and think again.

This happened already.


> Is it really worth it?

I came to such a conclusion for the shown source code clean-up.
Some software developers can also support it.


> I think I'd be less annoyed by regular spam.

It can be harder occasionally to become familiar and comfortable with
the suggested change pattern.
Could you accept other details from my update suggestion?

Regards,
Markus

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

#1449450 — Re: staging: ks7010: Rename jump labels

Hi Markus,

On Thu, 21 Jul 2016 09:55:55 +0200, SF Markus Elfring wrote:
> > > How do you think about information from the chapter "7: Centralized exiting of functions"?
> > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle?id=47ef4ad2684d380dd6d596140fb79395115c3950#n389
> > 
> > I'm not impressed by this piece of documentation.
> 
> I would also appreciate further improvements there.
> 
> 
> > Back to the lack of space before labels, it's at best a personal preference.
> > If you insist on standardizing, I'd call it a bug in the documentation,
> > which should be fixed. One space before label is the way to go.
> 
> Would you like to contribute another patch for such a coding style issue?

I just sent a patch out, let's see what people think about it.

> > That being said... checkpatch does not complain about leading space
> > before labels. Not even with --strict. So why are you mentioning it here?
> 
> How do you think about a similar software update?
> 
> staging: lustre: Fix a jump label position in osc_get_info()
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c71d264543f759fea147734cb63de36397817534
> https://lkml.org/lkml/2015/12/21/401

Intending labels with a tab is wrong, so fixing it is welcome. I'd turn
the tab into a space instead, for reasons explained before, but no
indentation at all is still better than a tab.

-- 
Jean Delvare
SUSE L3 Support

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

#1447973 — Re: staging: ks7010: Rename jump labels

> That being said... checkpatch does not complain about leading space
> before labels. Not even with --strict. So why are you mentioning it here?

I remembered a warning like "INDENTED_LABEL" instead.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=92d21ac74a9e3c09b0b01c764e530657e4c85c49#n4326


How do you generally think about jump label renaming?

Regards,
Markus

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

#1448086 — Re: staging: ks7010: Rename jump labels

Hi Markus,

On Thu, 21 Jul 2016 17:37:52 +0200, SF Markus Elfring wrote:
> > That being said... checkpatch does not complain about leading space
> > before labels. Not even with --strict. So why are you mentioning it here?
> 
> I remembered a warning like "INDENTED_LABEL" instead.
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=92d21ac74a9e3c09b0b01c764e530657e4c85c49#n4326

"#goto labels aren't indented, allow a single space however"

Can't be clearer :-)

> How do you generally think about jump label renaming?

Renaming from "out0:", "out1:" etc to something meaningful, yes. Did
you have anything else in mind?

-- 
Jean Delvare
SUSE L3 Support

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

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web