All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] Bluetooth: btusb: Fix race when waiting for BTUSB_DOWNLOADING
@ 2015-01-30  8:58 Johan Hedberg
  2015-01-30  8:58 ` [PATCH 2/2] Bluetooth: btusb: Use wait_on_bit_timeout() for BTUSB_BOOTING Johan Hedberg
  2015-01-30 10:07 ` [PATCH 1/2] Bluetooth: btusb: Fix race when waiting for BTUSB_DOWNLOADING Marcel Holtmann
  0 siblings, 2 replies; 4+ messages in thread
From: Johan Hedberg @ 2015-01-30  8:58 UTC (permalink / raw)
  To: linux-bluetooth

From: Johan Hedberg <johan.hedberg@intel.com>

The test for BTUSB_DOWNLOADING must be after adding to the wait queue
and setting the TASK_INTERRUPTIBLE state. Otherwise the flag may get
cleared after we test for it and we end up getting a timeout since
schedule_timeout() waits for the full duration. This patch uses a
wait_on_bit_timeout() + wake_up_bit(). To perform the task both
race-free as well as in a much simpler way.

Since there's no global wait_on_bit_timeout() helper yet (even though
all the building blocks for it are in place) this patch creates a
temporary local btusb copy of it until the global one has made it to
upstream trees.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 drivers/bluetooth/btusb.c | 67 ++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 3aa4b3c776b2..808568d2722f 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -334,6 +334,16 @@ struct btusb_data {
 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
 };
 
+static int btusb_wait_on_bit_timeout(void *word, int bit, unsigned long timeout,
+				     unsigned mode)
+{
+	might_sleep();
+	if (!test_bit(bit, word))
+		return 0;
+	return out_of_line_wait_on_bit_timeout(word, bit, bit_wait_timeout,
+					       mode, timeout);
+}
+
 static inline void btusb_free_frags(struct btusb_data *data)
 {
 	unsigned long flags;
@@ -1800,8 +1810,10 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
 
 			if (test_and_clear_bit(BTUSB_DOWNLOADING,
 					       &data->flags) &&
-			    test_bit(BTUSB_FIRMWARE_LOADED, &data->flags))
-				wake_up_interruptible(&hdev->req_wait_q);
+			    test_bit(BTUSB_FIRMWARE_LOADED, &data->flags)) {
+				smp_mb__after_atomic();
+				wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
+			}
 		}
 
 		/* When switching to the operational firmware the device
@@ -2165,43 +2177,32 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 
 	set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);
 
+	BT_INFO("%s: Waiting for firmware download to complete", hdev->name);
+
 	/* Before switching the device into operational mode and with that
 	 * booting the loaded firmware, wait for the bootloader notification
 	 * that all fragments have been successfully received.
 	 *
-	 * When the event processing receives the notification, then this
-	 * flag will be cleared. So just in case that happens really quickly,
-	 * check it first before adding the wait queue.
+	 * When the event processing receives the notification, then the
+	 * BTUSB_DOWNLOADING flag will be cleared.
+	 *
+	 * The firmware loading should not take longer than 5 seconds
+	 * and thus just timeout if that happens and fail the setup
+	 * of this device.
 	 */
-	if (test_bit(BTUSB_DOWNLOADING, &data->flags)) {
-		DECLARE_WAITQUEUE(wait, current);
-		signed long timeout;
-
-		BT_INFO("%s: Waiting for firmware download to complete",
-			hdev->name);
-
-		add_wait_queue(&hdev->req_wait_q, &wait);
-		set_current_state(TASK_INTERRUPTIBLE);
-
-		/* The firmware loading should not take longer than 5 seconds
-		 * and thus just timeout if that happens and fail the setup
-		 * of this device.
-		 */
-		timeout = schedule_timeout(msecs_to_jiffies(5000));
-
-		remove_wait_queue(&hdev->req_wait_q, &wait);
-
-		if (signal_pending(current)) {
-			BT_ERR("%s: Firmware loading interrupted", hdev->name);
-			err = -EINTR;
-			goto done;
-		}
+	err = btusb_wait_on_bit_timeout(&data->flags, BTUSB_DOWNLOADING,
+					msecs_to_jiffies(5000),
+					TASK_INTERRUPTIBLE);
+	if (err == 1) {
+		BT_ERR("%s: Firmware loading interrupted", hdev->name);
+		err = -EINTR;
+		goto done;
+	}
 
-		if (!timeout) {
-			BT_ERR("%s: Firmware loading timeout", hdev->name);
-			err = -ETIMEDOUT;
-			goto done;
-		}
+	if (err) {
+		BT_ERR("%s: Firmware loading timeout", hdev->name);
+		err = -ETIMEDOUT;
+		goto done;
 	}
 
 	if (test_bit(BTUSB_FIRMWARE_FAILED, &data->flags)) {
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/2] Bluetooth: btusb: Use wait_on_bit_timeout() for BTUSB_BOOTING
  2015-01-30  8:58 [PATCH 1/2] Bluetooth: btusb: Fix race when waiting for BTUSB_DOWNLOADING Johan Hedberg
@ 2015-01-30  8:58 ` Johan Hedberg
  2015-01-30 10:07   ` Marcel Holtmann
  2015-01-30 10:07 ` [PATCH 1/2] Bluetooth: btusb: Fix race when waiting for BTUSB_DOWNLOADING Marcel Holtmann
  1 sibling, 1 reply; 4+ messages in thread
From: Johan Hedberg @ 2015-01-30  8:58 UTC (permalink / raw)
  To: linux-bluetooth

From: Johan Hedberg <johan.hedberg@intel.com>

The wait_on_bit_timeout() is a simpler and race-free way of waiting for
a bit to be cleared than the current code in btusb.c. This patch updates
the code to use the helper function (its btusb copy - to be later
updated to use a global one).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 drivers/bluetooth/btusb.c | 44 +++++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 808568d2722f..f67bf761144e 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1822,8 +1822,10 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
 		 */
 		if (skb->len == 9 && hdr->evt == 0xff && hdr->plen == 0x07 &&
 		    skb->data[2] == 0x02) {
-			if (test_and_clear_bit(BTUSB_BOOTING, &data->flags))
-				wake_up_interruptible(&hdev->req_wait_q);
+			if (test_and_clear_bit(BTUSB_BOOTING, &data->flags)) {
+				smp_mb__after_atomic();
+				wake_up_bit(&data->flags, BTUSB_BOOTING);
+			}
 		}
 	}
 
@@ -2236,33 +2238,25 @@ done:
 
 	/* The bootloader will not indicate when the device is ready. This
 	 * is done by the operational firmware sending bootup notification.
+	 *
+	 * Booting into operational firmware should not take longer than
+	 * 1 second. However if that happens, then just fail the setup
+	 * since something went wrong.
 	 */
-	if (test_bit(BTUSB_BOOTING, &data->flags)) {
-		DECLARE_WAITQUEUE(wait, current);
-		signed long timeout;
-
-		BT_INFO("%s: Waiting for device to boot", hdev->name);
-
-		add_wait_queue(&hdev->req_wait_q, &wait);
-		set_current_state(TASK_INTERRUPTIBLE);
-
-		/* Booting into operational firmware should not take
-		 * longer than 1 second. However if that happens, then
-		 * just fail the setup since something went wrong.
-		 */
-		timeout = schedule_timeout(msecs_to_jiffies(1000));
+	BT_INFO("%s: Waiting for device to boot", hdev->name);
 
-		remove_wait_queue(&hdev->req_wait_q, &wait);
+	err = btusb_wait_on_bit_timeout(&data->flags, BTUSB_BOOTING,
+					msecs_to_jiffies(1000),
+					TASK_INTERRUPTIBLE);
 
-		if (signal_pending(current)) {
-			BT_ERR("%s: Device boot interrupted", hdev->name);
-			return -EINTR;
-		}
+	if (err == 1) {
+		BT_ERR("%s: Device boot interrupted", hdev->name);
+		return -EINTR;
+	}
 
-		if (!timeout) {
-			BT_ERR("%s: Device boot timeout", hdev->name);
-			return -ETIMEDOUT;
-		}
+	if (err) {
+		BT_ERR("%s: Device boot timeout", hdev->name);
+		return -ETIMEDOUT;
 	}
 
 	rettime = ktime_get();
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] Bluetooth: btusb: Use wait_on_bit_timeout() for BTUSB_BOOTING
  2015-01-30  8:58 ` [PATCH 2/2] Bluetooth: btusb: Use wait_on_bit_timeout() for BTUSB_BOOTING Johan Hedberg
@ 2015-01-30 10:07   ` Marcel Holtmann
  0 siblings, 0 replies; 4+ messages in thread
From: Marcel Holtmann @ 2015-01-30 10:07 UTC (permalink / raw)
  To: Johan Hedberg; +Cc: linux-bluetooth

Hi Johan,

> The wait_on_bit_timeout() is a simpler and race-free way of waiting for
> a bit to be cleared than the current code in btusb.c. This patch updates
> the code to use the helper function (its btusb copy - to be later
> updated to use a global one).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> drivers/bluetooth/btusb.c | 44 +++++++++++++++++++-------------------------
> 1 file changed, 19 insertions(+), 25 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] Bluetooth: btusb: Fix race when waiting for BTUSB_DOWNLOADING
  2015-01-30  8:58 [PATCH 1/2] Bluetooth: btusb: Fix race when waiting for BTUSB_DOWNLOADING Johan Hedberg
  2015-01-30  8:58 ` [PATCH 2/2] Bluetooth: btusb: Use wait_on_bit_timeout() for BTUSB_BOOTING Johan Hedberg
@ 2015-01-30 10:07 ` Marcel Holtmann
  1 sibling, 0 replies; 4+ messages in thread
From: Marcel Holtmann @ 2015-01-30 10:07 UTC (permalink / raw)
  To: Johan Hedberg; +Cc: linux-bluetooth

Hi Johan,

> The test for BTUSB_DOWNLOADING must be after adding to the wait queue
> and setting the TASK_INTERRUPTIBLE state. Otherwise the flag may get
> cleared after we test for it and we end up getting a timeout since
> schedule_timeout() waits for the full duration. This patch uses a
> wait_on_bit_timeout() + wake_up_bit(). To perform the task both
> race-free as well as in a much simpler way.
> 
> Since there's no global wait_on_bit_timeout() helper yet (even though
> all the building blocks for it are in place) this patch creates a
> temporary local btusb copy of it until the global one has made it to
> upstream trees.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> drivers/bluetooth/btusb.c | 67 ++++++++++++++++++++++++-----------------------
> 1 file changed, 34 insertions(+), 33 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-01-30 10:07 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-30  8:58 [PATCH 1/2] Bluetooth: btusb: Fix race when waiting for BTUSB_DOWNLOADING Johan Hedberg
2015-01-30  8:58 ` [PATCH 2/2] Bluetooth: btusb: Use wait_on_bit_timeout() for BTUSB_BOOTING Johan Hedberg
2015-01-30 10:07   ` Marcel Holtmann
2015-01-30 10:07 ` [PATCH 1/2] Bluetooth: btusb: Fix race when waiting for BTUSB_DOWNLOADING Marcel Holtmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.