From: Marcel Holtmann <marcel@holtmann.org>
To: SF Markus Elfring <elfring@users.sourceforge.net>
Cc: linux-bluetooth@vger.kernel.org,
Johan Hedberg <johan.hedberg@gmail.com>,
LKML <linux-kernel@vger.kernel.org>,
kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host() after error detection
Date: Mon, 12 Mar 2018 17:12:51 +0100 [thread overview]
Message-ID: <609D1B28-2B75-4AB5-B707-FDB5D0500F1A@holtmann.org> (raw)
In-Reply-To: <a988a80d-12c0-a414-ab81-ff4ab075855c@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
> ---
> 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, &buf_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
next prev parent reply other threads:[~2018-03-12 16:12 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-12 11:30 [PATCH 0/5] Bluetooth/btmrvl_sdio: Adjustments for three function implementations SF Markus Elfring
2018-03-12 11:31 ` [PATCH 1/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_register_dev() SF Markus Elfring
2018-03-12 16:18 ` Marcel Holtmann
2018-03-12 11:32 ` [PATCH 2/5] Bluetooth: btmrvl: Delete an unnecessary variable initialisation " SF Markus Elfring
2018-03-12 16:11 ` Marcel Holtmann
2018-03-12 11:33 ` [PATCH 3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host() after error detection SF Markus Elfring
2018-03-12 16:12 ` Marcel Holtmann [this message]
2018-03-13 7:46 ` [3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host() SF Markus Elfring
2018-03-12 11:34 ` [PATCH 4/5] Bluetooth: btmrvl: Delete an unnecessary variable initialisation " SF Markus Elfring
2018-03-12 16:11 ` Marcel Holtmann
2018-03-12 11:35 ` [PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper() SF Markus Elfring
2018-03-12 16:06 ` Marcel Holtmann
2018-03-13 7:55 ` SF Markus Elfring
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=609D1B28-2B75-4AB5-B707-FDB5D0500F1A@holtmann.org \
--to=marcel@holtmann.org \
--cc=elfring@users.sourceforge.net \
--cc=johan.hedberg@gmail.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/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).