…
> +++ b/net/bluetooth/hci_request.c
> @@ -106,8 +106,7 @@ void hci_req_sync_complete(struct hci_dev *hdev, u8 result, u16 opcode,
> hdev->req_result = result;
> hdev->req_status = HCI_REQ_DONE;
> if (skb) {
> - kfree_skb(hdev->req_skb);
> - hdev->req_skb = skb_get(skb);
> + hci_req_skb_release_and_set(hdev, skb_get(skb));
> }
> wake_up_interruptible(&hdev->req_wait_q);
…
How do you think about to omit any curly brackets here?
…
> +++ b/net/bluetooth/hci_request.h
> @@ -28,6 +28,15 @@
>
> #define hci_req_sync_lock(hdev) mutex_lock(&hdev->req_lock)
> #define hci_req_sync_unlock(hdev) mutex_unlock(&hdev->req_lock)
> +#define hci_req_skb_release_and_set(hdev, val) \
> +({ \
> + if (hdev->req_skb) { \
> + spin_lock(&hdev->req_skb_lock); \
> + kfree_skb(hdev->req_skb); \
> + hdev->req_skb = val; \
> + spin_unlock(&hdev->req_skb_lock); \
> + } \
> +})
…
* Do you expect that any data synchronisation should be performed
for the shown pointer check?
* Can it eventually matter to implement such a macro with a statement
like “guard(spinlock)(&hdev->req_skb_lock);”?
https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/spinlock.h#L561
Regards,
Markus
