Re: [3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host()

2018-03-13 Thread SF Markus Elfring 
>> @@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct 
>> btmrvl_private *priv)
>>  break;
>>  }
>>
>> -exit:
>> -if (ret) {
>> -hdev->stat.err_rx++;
>> -kfree_skb(skb);
>> -}
>> +return 0;
>> +
>> +free_skb:
>> +kfree_skb(skb);
>> +e_io:
>> +ret = -EIO;
>> +goto increment_counter;
>>
>> +e_inval:
>> +ret = -EINVAL;
>> +increment_counter:
>> +hdev->stat.err_rx++;
>>  return ret;
> 
> Nope!
> 
> This is not easier to read for me. This goto exit jumping and I hate that.

Can the software design direction become feasible to omit the repeated check
for the variable “ret” (and further initialisations)?

Regards,
Markus

Re: [3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host()

2018-03-13 Thread SF Markus Elfring 
>> @@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct 
>> btmrvl_private *priv)
>>  break;
>>  }
>>
>> -exit:
>> -if (ret) {
>> -hdev->stat.err_rx++;
>> -kfree_skb(skb);
>> -}
>> +return 0;
>> +
>> +free_skb:
>> +kfree_skb(skb);
>> +e_io:
>> +ret = -EIO;
>> +goto increment_counter;
>>
>> +e_inval:
>> +ret = -EINVAL;
>> +increment_counter:
>> +hdev->stat.err_rx++;
>>  return ret;
> 
> Nope!
> 
> This is not easier to read for me. This goto exit jumping and I hate that.

Can the software design direction become feasible to omit the repeated check
for the variable “ret” (and further initialisations)?

Regards,
Markus

Re: [PATCH 3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host() after error detection

2018-03-12 Thread Marcel Holtmann 
Hi Markus,

> One check could be repeated by the btmrvl_sdio_card_to_host() function
> during error handling even if the relevant properties can be determined
> for the involved variables before by source code analysis.
> 
> * Adjust jump targets so that an extra check can be omitted at the end.
> 
> * Reuse a bit of exception handling better.
> 
> * Delete an initialisation for the local variable "skb"
>  which became unnecessary with this refactoring.
> 
> Signed-off-by: Markus Elfring 
> ---
> drivers/bluetooth/btmrvl_sdio.c | 35 ++-
> 1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
> index 84c222abf0f7..9854addc8e96 100644
> --- a/drivers/bluetooth/btmrvl_sdio.c
> +++ b/drivers/bluetooth/btmrvl_sdio.c
> @@ -687,5 +687,5 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
> *priv)
> {
>   u16 buf_len = 0;
>   int ret, num_blocks, blksz;
> - struct sk_buff *skb = NULL;
> + struct sk_buff *skb;
>   u32 type;
> @@ -695,16 +695,14 @@ static int btmrvl_sdio_card_to_host(struct 
> btmrvl_private *priv)
> 
>   if (!card || !card->func) {
>   BT_ERR("card or function is NULL!");
> - ret = -EINVAL;
> - goto exit;
> + goto e_inval;
>   }
> 
>   /* Read the length of data to be transferred */
>   ret = btmrvl_sdio_read_rx_len(card, _len);
>   if (ret < 0) {
>   BT_ERR("read rx_len failed");
> - ret = -EIO;
> - goto exit;
> + goto e_io;
>   }
> 
>   blksz = SDIO_BLOCK_SIZE;
> @@ -713,8 +711,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
> *priv)
>   if (buf_len <= SDIO_HEADER_LEN
>   || (num_blocks * blksz) > ALLOC_BUF_SIZE) {
>   BT_ERR("invalid packet length: %d", buf_len);
> - ret = -EINVAL;
> - goto exit;
> + goto e_inval;
>   }
> 
>   /* Allocate buffer */
> @@ -722,7 +719,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
> *priv)
>   if (!skb) {
>   BT_ERR("No free skb");
>   ret = -ENOMEM;
> - goto exit;
> + goto increment_counter;
>   }
> 
>   if ((unsigned long) skb->data & (BTSDIO_DMA_ALIGN - 1)) {
> @@ -738,8 +735,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
> *priv)
> num_blocks * blksz);
>   if (ret < 0) {
>   BT_ERR("readsb failed: %d", ret);
> - ret = -EIO;
> - goto exit;
> + goto free_skb;
>   }
> 
>   /* This is SDIO specific header length: byte[2][1][0], type: byte[3]
> @@ -753,8 +749,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
> *priv)
>   if (buf_len > blksz * num_blocks) {
>   BT_ERR("Skip incorrect packet: hdrlen %d buffer %d",
>  buf_len, blksz * num_blocks);
> - ret = -EIO;
> - goto exit;
> + goto free_skb;
>   }
> 
>   type = payload[3];
> @@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct 
> btmrvl_private *priv)
>   break;
>   }
> 
> -exit:
> - if (ret) {
> - hdev->stat.err_rx++;
> - kfree_skb(skb);
> - }
> + return 0;
> +
> +free_skb:
> + kfree_skb(skb);
> +e_io:
> + ret = -EIO;
> + goto increment_counter;
> 
> +e_inval:
> + ret = -EINVAL;
> +increment_counter:
> + hdev->stat.err_rx++;
>   return ret;

Nope!

This is not easier to read for me. This goto exit jumping and I hate that. 
Actually to be honest this kind of goto jumping makes my brain hurt and I want 
to avoid it.

Regards

Marcel

Re: [PATCH 3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host() after error detection

2018-03-12 Thread Marcel Holtmann 
Hi Markus,

> One check could be repeated by the btmrvl_sdio_card_to_host() function
> during error handling even if the relevant properties can be determined
> for the involved variables before by source code analysis.
> 
> * Adjust jump targets so that an extra check can be omitted at the end.
> 
> * Reuse a bit of exception handling better.
> 
> * Delete an initialisation for the local variable "skb"
>  which became unnecessary with this refactoring.
> 
> Signed-off-by: Markus Elfring 
> ---
> drivers/bluetooth/btmrvl_sdio.c | 35 ++-
> 1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
> index 84c222abf0f7..9854addc8e96 100644
> --- a/drivers/bluetooth/btmrvl_sdio.c
> +++ b/drivers/bluetooth/btmrvl_sdio.c
> @@ -687,5 +687,5 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
> *priv)
> {
>   u16 buf_len = 0;
>   int ret, num_blocks, blksz;
> - struct sk_buff *skb = NULL;
> + struct sk_buff *skb;
>   u32 type;
> @@ -695,16 +695,14 @@ static int btmrvl_sdio_card_to_host(struct 
> btmrvl_private *priv)
> 
>   if (!card || !card->func) {
>   BT_ERR("card or function is NULL!");
> - ret = -EINVAL;
> - goto exit;
> + goto e_inval;
>   }
> 
>   /* Read the length of data to be transferred */
>   ret = btmrvl_sdio_read_rx_len(card, _len);
>   if (ret < 0) {
>   BT_ERR("read rx_len failed");
> - ret = -EIO;
> - goto exit;
> + goto e_io;
>   }
> 
>   blksz = SDIO_BLOCK_SIZE;
> @@ -713,8 +711,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
> *priv)
>   if (buf_len <= SDIO_HEADER_LEN
>   || (num_blocks * blksz) > ALLOC_BUF_SIZE) {
>   BT_ERR("invalid packet length: %d", buf_len);
> - ret = -EINVAL;
> - goto exit;
> + goto e_inval;
>   }
> 
>   /* Allocate buffer */
> @@ -722,7 +719,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
> *priv)
>   if (!skb) {
>   BT_ERR("No free skb");
>   ret = -ENOMEM;
> - goto exit;
> + goto increment_counter;
>   }
> 
>   if ((unsigned long) skb->data & (BTSDIO_DMA_ALIGN - 1)) {
> @@ -738,8 +735,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
> *priv)
> num_blocks * blksz);
>   if (ret < 0) {
>   BT_ERR("readsb failed: %d", ret);
> - ret = -EIO;
> - goto exit;
> + goto free_skb;
>   }
> 
>   /* This is SDIO specific header length: byte[2][1][0], type: byte[3]
> @@ -753,8 +749,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
> *priv)
>   if (buf_len > blksz * num_blocks) {
>   BT_ERR("Skip incorrect packet: hdrlen %d buffer %d",
>  buf_len, blksz * num_blocks);
> - ret = -EIO;
> - goto exit;
> + goto free_skb;
>   }
> 
>   type = payload[3];
> @@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct 
> btmrvl_private *priv)
>   break;
>   }
> 
> -exit:
> - if (ret) {
> - hdev->stat.err_rx++;
> - kfree_skb(skb);
> - }
> + return 0;
> +
> +free_skb:
> + kfree_skb(skb);
> +e_io:
> + ret = -EIO;
> + goto increment_counter;
> 
> +e_inval:
> + ret = -EINVAL;
> +increment_counter:
> + hdev->stat.err_rx++;
>   return ret;

Nope!

This is not easier to read for me. This goto exit jumping and I hate that. 
Actually to be honest this kind of goto jumping makes my brain hurt and I want 
to avoid it.

Regards

Marcel

[PATCH 3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host() after error detection

2018-03-12 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Mon, 12 Mar 2018 11:13:00 +0100

One check could be repeated by the btmrvl_sdio_card_to_host() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Adjust jump targets so that an extra check can be omitted at the end.

* Reuse a bit of exception handling better.

* Delete an initialisation for the local variable "skb"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 35 ++-
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 84c222abf0f7..9854addc8e96 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -687,5 +687,5 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
 {
u16 buf_len = 0;
int ret, num_blocks, blksz;
-   struct sk_buff *skb = NULL;
+   struct sk_buff *skb;
u32 type;
@@ -695,16 +695,14 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
 
if (!card || !card->func) {
BT_ERR("card or function is NULL!");
-   ret = -EINVAL;
-   goto exit;
+   goto e_inval;
}
 
/* Read the length of data to be transferred */
ret = btmrvl_sdio_read_rx_len(card, _len);
if (ret < 0) {
BT_ERR("read rx_len failed");
-   ret = -EIO;
-   goto exit;
+   goto e_io;
}
 
blksz = SDIO_BLOCK_SIZE;
@@ -713,8 +711,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (buf_len <= SDIO_HEADER_LEN
|| (num_blocks * blksz) > ALLOC_BUF_SIZE) {
BT_ERR("invalid packet length: %d", buf_len);
-   ret = -EINVAL;
-   goto exit;
+   goto e_inval;
}
 
/* Allocate buffer */
@@ -722,7 +719,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (!skb) {
BT_ERR("No free skb");
ret = -ENOMEM;
-   goto exit;
+   goto increment_counter;
}
 
if ((unsigned long) skb->data & (BTSDIO_DMA_ALIGN - 1)) {
@@ -738,8 +735,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
  num_blocks * blksz);
if (ret < 0) {
BT_ERR("readsb failed: %d", ret);
-   ret = -EIO;
-   goto exit;
+   goto free_skb;
}
 
/* This is SDIO specific header length: byte[2][1][0], type: byte[3]
@@ -753,8 +749,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (buf_len > blksz * num_blocks) {
BT_ERR("Skip incorrect packet: hdrlen %d buffer %d",
   buf_len, blksz * num_blocks);
-   ret = -EIO;
-   goto exit;
+   goto free_skb;
}
 
type = payload[3];
@@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
break;
}
 
-exit:
-   if (ret) {
-   hdev->stat.err_rx++;
-   kfree_skb(skb);
-   }
+   return 0;
+
+free_skb:
+   kfree_skb(skb);
+e_io:
+   ret = -EIO;
+   goto increment_counter;
 
+e_inval:
+   ret = -EINVAL;
+increment_counter:
+   hdev->stat.err_rx++;
return ret;
 }
 
-- 
2.16.2

[PATCH 3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host() after error detection

2018-03-12 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Mon, 12 Mar 2018 11:13:00 +0100

One check could be repeated by the btmrvl_sdio_card_to_host() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Adjust jump targets so that an extra check can be omitted at the end.

* Reuse a bit of exception handling better.

* Delete an initialisation for the local variable "skb"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 35 ++-
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 84c222abf0f7..9854addc8e96 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -687,5 +687,5 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
 {
u16 buf_len = 0;
int ret, num_blocks, blksz;
-   struct sk_buff *skb = NULL;
+   struct sk_buff *skb;
u32 type;
@@ -695,16 +695,14 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
 
if (!card || !card->func) {
BT_ERR("card or function is NULL!");
-   ret = -EINVAL;
-   goto exit;
+   goto e_inval;
}
 
/* Read the length of data to be transferred */
ret = btmrvl_sdio_read_rx_len(card, _len);
if (ret < 0) {
BT_ERR("read rx_len failed");
-   ret = -EIO;
-   goto exit;
+   goto e_io;
}
 
blksz = SDIO_BLOCK_SIZE;
@@ -713,8 +711,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (buf_len <= SDIO_HEADER_LEN
|| (num_blocks * blksz) > ALLOC_BUF_SIZE) {
BT_ERR("invalid packet length: %d", buf_len);
-   ret = -EINVAL;
-   goto exit;
+   goto e_inval;
}
 
/* Allocate buffer */
@@ -722,7 +719,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (!skb) {
BT_ERR("No free skb");
ret = -ENOMEM;
-   goto exit;
+   goto increment_counter;
}
 
if ((unsigned long) skb->data & (BTSDIO_DMA_ALIGN - 1)) {
@@ -738,8 +735,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
  num_blocks * blksz);
if (ret < 0) {
BT_ERR("readsb failed: %d", ret);
-   ret = -EIO;
-   goto exit;
+   goto free_skb;
}
 
/* This is SDIO specific header length: byte[2][1][0], type: byte[3]
@@ -753,8 +749,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (buf_len > blksz * num_blocks) {
BT_ERR("Skip incorrect packet: hdrlen %d buffer %d",
   buf_len, blksz * num_blocks);
-   ret = -EIO;
-   goto exit;
+   goto free_skb;
}
 
type = payload[3];
@@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
break;
}
 
-exit:
-   if (ret) {
-   hdev->stat.err_rx++;
-   kfree_skb(skb);
-   }
+   return 0;
+
+free_skb:
+   kfree_skb(skb);
+e_io:
+   ret = -EIO;
+   goto increment_counter;
 
+e_inval:
+   ret = -EINVAL;
+increment_counter:
+   hdev->stat.err_rx++;
return ret;
 }
 
-- 
2.16.2