Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191211103436.15369-1-marcel@holtmann.org>
In-Reply-To: <20191211103436.15369-1-marcel@holtmann.org>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Wed, 11 Dec 2019 09:32:12 -0500
Message-ID: <CALWDO_Xw_rniqTCwF3u1UYh2EgQy8T7aa=p_w-XFz3if4ip7gA@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: Remove usage of BT_ERR_RATELIMITED macro
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     BlueZ <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

looks good to me.  Thanks for picking it up, I was about to this morning :)

On Wed, Dec 11, 2019 at 5:34 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> The macro is really not needed and can be replaced with either usage of
> bt_err_ratelimited or bt_dev_err_ratelimited.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
>  include/net/bluetooth/bluetooth.h |  4 +---
>  net/bluetooth/hci_event.c         | 14 ++++++--------
>  2 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index bd2675266859..e42bb8e03c09 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -138,8 +138,6 @@ void bt_err_ratelimited(const char *fmt, ...);
>  #define BT_ERR(fmt, ...)       bt_err(fmt "\n", ##__VA_ARGS__)
>  #define BT_DBG(fmt, ...)       pr_debug(fmt "\n", ##__VA_ARGS__)
>
> -#define BT_ERR_RATELIMITED(fmt, ...) bt_err_ratelimited(fmt "\n", ##__VA_ARGS__)
> -
>  #define bt_dev_info(hdev, fmt, ...)                            \
>         BT_INFO("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
>  #define bt_dev_warn(hdev, fmt, ...)                            \
> @@ -152,7 +150,7 @@ void bt_err_ratelimited(const char *fmt, ...);
>  #define bt_dev_warn_ratelimited(hdev, fmt, ...)                        \
>         bt_warn_ratelimited("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
>  #define bt_dev_err_ratelimited(hdev, fmt, ...)                 \
> -       BT_ERR_RATELIMITED("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
> +       bt_err_ratelimited("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
>
>  /* Connection and socket states */
>  enum {
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index c1d3a303d97f..1941f120a376 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5451,7 +5451,7 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
>         hci_dev_unlock(hdev);
>  }
>
> -static u8 ext_evt_type_to_legacy(u16 evt_type)
> +static u8 ext_evt_type_to_legacy(struct hci_dev *hdev, u16 evt_type)
>  {
>         if (evt_type & LE_EXT_ADV_LEGACY_PDU) {
>                 switch (evt_type) {
> @@ -5468,10 +5468,7 @@ static u8 ext_evt_type_to_legacy(u16 evt_type)
>                         return LE_ADV_SCAN_RSP;
>                 }
>
> -               BT_ERR_RATELIMITED("Unknown advertising packet type: 0x%02x",
> -                                  evt_type);
> -
> -               return LE_ADV_INVALID;
> +               goto invalid;
>         }
>
>         if (evt_type & LE_EXT_ADV_CONN_IND) {
> @@ -5491,8 +5488,9 @@ static u8 ext_evt_type_to_legacy(u16 evt_type)
>             evt_type & LE_EXT_ADV_DIRECT_IND)
>                 return LE_ADV_NONCONN_IND;
>
> -       BT_ERR_RATELIMITED("Unknown advertising packet type: 0x%02x",
> -                                  evt_type);
> +invalid:
> +       bt_dev_err_ratelimited(hdev, "Unknown advertising packet type: 0x%02x",
> +                              evt_type);
>
>         return LE_ADV_INVALID;
>  }
> @@ -5510,7 +5508,7 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
>                 u16 evt_type;
>
>                 evt_type = __le16_to_cpu(ev->evt_type);
> -               legacy_evt_type = ext_evt_type_to_legacy(evt_type);
> +               legacy_evt_type = ext_evt_type_to_legacy(hdev, evt_type);
>                 if (legacy_evt_type != LE_ADV_INVALID) {
>                         process_adv_report(hdev, legacy_evt_type, &ev->bdaddr,
>                                            ev->bdaddr_type, NULL, 0, ev->rssi,
> --
> 2.23.0
>