[PATCH] bluetooth: btusb: Fix issue with suspend

[PATCH] bluetooth: btusb: Fix issue with suspend

[Larry Finger]

From: Champion Chen <champion_chen@realsil.com.cn>

Suspend could fail for some platforms because
btusb_suspend==> btusb_stop_traffic ==> usb_kill_anchored_urbs,

When btusb_bulk_complete returns before system suspend and resubmits an urb,
the system cannot enter suspend state.

Signed-off-by: Champion Chen <champion_chen@realsil.com.cn>
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Stable <stable@vger.kernel.org>
---
Johan,

To help Champion with the process, I have formatted the patch in
the correct manner. I hope I understand the issue correctly and
stated it in a coherent manner in the commit message.

Larry
---
 drivers/bluetooth/btusb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 292c38e..45a7211 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -330,6 +330,9 @@ static void btusb_intr_complete(struct urb *urb)
 			BT_ERR("%s corrupted event packet", hdev->name);
 			hdev->stat.err_rx++;
 		}
+	} else if (urb->status == -ENOENT) {
+		/* Avoid suspend failed when usb_kill_urb */
+		return;
 	}
 
 	if (!test_bit(BTUSB_INTR_RUNNING, &data->flags))
@@ -417,6 +420,9 @@ static void btusb_bulk_complete(struct urb *urb)
 						urb->actual_length) < 0) {
 			BT_ERR("%s corrupted ACL packet", hdev->name);
 			hdev->stat.err_rx++;
+		} else if (urb->status == -ENOENT) {
+			/* Avoid suspend failed when usb_kill_urb */
+			return;
 		}
 	}
 
@@ -512,6 +518,9 @@ static void btusb_isoc_complete(struct urb *urb)
 				hdev->stat.err_rx++;
 			}
 		}
+	} else if (urb->status == -ENOENT) {
+		/* Avoid suspend failed when usb_kill_urb */
+		return;
 	}
 
 	if (!test_bit(BTUSB_ISOC_RUNNING, &data->flags))
-- 
1.8.4.5

Re: [PATCH] bluetooth: btusb: Fix issue with suspend

[Marcel Holtmann]

Hi Larry,

> Suspend could fail for some platforms because
> btusb_suspend==> btusb_stop_traffic ==> usb_kill_anchored_urbs,
> 
> When btusb_bulk_complete returns before system suspend and resubmits an urb,
> the system cannot enter suspend state.
> 
> Signed-off-by: Champion Chen <champion_chen@realsil.com.cn>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Stable <stable@vger.kernel.org>
> ---
> Johan,
> 
> To help Champion with the process, I have formatted the patch in
> the correct manner. I hope I understand the issue correctly and
> stated it in a coherent manner in the commit message.
> 
> Larry
> ---
> drivers/bluetooth/btusb.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 292c38e..45a7211 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -330,6 +330,9 @@ static void btusb_intr_complete(struct urb *urb)
> 			BT_ERR("%s corrupted event packet", hdev->name);
> 			hdev->stat.err_rx++;
> 		}
> +	} else if (urb->status == -ENOENT) {
> +		/* Avoid suspend failed when usb_kill_urb */
> +		return;
> 	}
> 
> 	if (!test_bit(BTUSB_INTR_RUNNING, &data->flags))
> @@ -417,6 +420,9 @@ static void btusb_bulk_complete(struct urb *urb)
> 						urb->actual_length) < 0) {
> 			BT_ERR("%s corrupted ACL packet", hdev->name);
> 			hdev->stat.err_rx++;
> +		} else if (urb->status == -ENOENT) {
> +			/* Avoid suspend failed when usb_kill_urb */
> +			return;
> 		}

this one is utterly bogus. Either urb->status is 0 or it is -ENOENT, but it will not be both. I think you put it to the wrong if clause.

> 	}
> 
> @@ -512,6 +518,9 @@ static void btusb_isoc_complete(struct urb *urb)
> 				hdev->stat.err_rx++;
> 			}
> 		}
> +	} else if (urb->status == -ENOENT) {
> +		/* Avoid suspend failed when usb_kill_urb */
> +		return;
> 	}
> 
> 	if (!test_bit(BTUSB_ISOC_RUNNING, &data->flags))

Regards

Marcel

Re: [PATCH] bluetooth: btusb: Fix issue with suspend

[Larry Finger]

On 09/06/2014 12:02 PM, Marcel Holtmann wrote:
> Hi Larry,
>
>> Suspend could fail for some platforms because
>> btusb_suspend==> btusb_stop_traffic ==> usb_kill_anchored_urbs,
>>
>> When btusb_bulk_complete returns before system suspend and resubmits an urb,
>> the system cannot enter suspend state.
>>
>> Signed-off-by: Champion Chen <champion_chen@realsil.com.cn>
>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
>> Cc: Stable <stable@vger.kernel.org>
>> ---
>> Johan,
>>
>> To help Champion with the process, I have formatted the patch in
>> the correct manner. I hope I understand the issue correctly and
>> stated it in a coherent manner in the commit message.
>>
>> Larry
>> ---
>> drivers/bluetooth/btusb.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 292c38e..45a7211 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -330,6 +330,9 @@ static void btusb_intr_complete(struct urb *urb)
>> 			BT_ERR("%s corrupted event packet", hdev->name);
>> 			hdev->stat.err_rx++;
>> 		}
>> +	} else if (urb->status == -ENOENT) {
>> +		/* Avoid suspend failed when usb_kill_urb */
>> +		return;
>> 	}
>>
>> 	if (!test_bit(BTUSB_INTR_RUNNING, &data->flags))
>> @@ -417,6 +420,9 @@ static void btusb_bulk_complete(struct urb *urb)
>> 						urb->actual_length) < 0) {
>> 			BT_ERR("%s corrupted ACL packet", hdev->name);
>> 			hdev->stat.err_rx++;
>> +		} else if (urb->status == -ENOENT) {
>> +			/* Avoid suspend failed when usb_kill_urb */
>> +			return;
>> 		}
>
> this one is utterly bogus. Either urb->status is 0 or it is -ENOENT, but it will not be both. I think you put it to the wrong if clause.

Thanks for the review. Obviously, you are correct. A revised patch will be sent 
soon.

The code sent by Champion is now posted at 
http://github.com/lwfinger/rtl8723au_bt/new. It works for everyone that has 
tested it.

Larry

Re: [PATCH] bluetooth: btusb: Fix issue with suspend

[Marcel Holtmann]

Hi Larry,

>>> Suspend could fail for some platforms because
>>> btusb_suspend==> btusb_stop_traffic ==> usb_kill_anchored_urbs,
>>> 
>>> When btusb_bulk_complete returns before system suspend and resubmits an urb,
>>> the system cannot enter suspend state.
>>> 
>>> Signed-off-by: Champion Chen <champion_chen@realsil.com.cn>
>>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
>>> Cc: Stable <stable@vger.kernel.org>
>>> ---
>>> Johan,
>>> 
>>> To help Champion with the process, I have formatted the patch in
>>> the correct manner. I hope I understand the issue correctly and
>>> stated it in a coherent manner in the commit message.
>>> 
>>> Larry
>>> ---
>>> drivers/bluetooth/btusb.c | 9 +++++++++
>>> 1 file changed, 9 insertions(+)
>>> 
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 292c38e..45a7211 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -330,6 +330,9 @@ static void btusb_intr_complete(struct urb *urb)
>>> 			BT_ERR("%s corrupted event packet", hdev->name);
>>> 			hdev->stat.err_rx++;
>>> 		}
>>> +	} else if (urb->status == -ENOENT) {
>>> +		/* Avoid suspend failed when usb_kill_urb */
>>> +		return;
>>> 	}
>>> 
>>> 	if (!test_bit(BTUSB_INTR_RUNNING, &data->flags))
>>> @@ -417,6 +420,9 @@ static void btusb_bulk_complete(struct urb *urb)
>>> 						urb->actual_length) < 0) {
>>> 			BT_ERR("%s corrupted ACL packet", hdev->name);
>>> 			hdev->stat.err_rx++;
>>> +		} else if (urb->status == -ENOENT) {
>>> +			/* Avoid suspend failed when usb_kill_urb */
>>> +			return;
>>> 		}
>> 
>> this one is utterly bogus. Either urb->status is 0 or it is -ENOENT, but it will not be both. I think you put it to the wrong if clause.
> 
> Thanks for the review. Obviously, you are correct. A revised patch will be sent soon.

one other thing. Please let maintainers decide when a patch is useful for stable and when not. Patches need to go upstream first anyway. People spamming stable@vger.kernel.org with patches that are not reviewed yet is pretty much a waste of time.

Regards

Marcel

Re: [PATCH] bluetooth: btusb: Fix issue with suspend

[Larry Finger]

On 09/06/2014 12:35 PM, Marcel Holtmann wrote:
> Hi Larry,
>
>
> one other thing. Please let maintainers decide when a patch is useful for stable and when not. Patches need to go upstream first anyway. People spamming stable@vger.kernel.org with patches that are not reviewed yet is pretty much a waste of time.

Sorry. In the wireless tree, the patch author suggests that the patch is 
suitable for stable. If the maintainer disagrees, he strips the Cc to stable. It 
seems that you do it the other way. In either case, I do not think the stable 
patches are picked up by the maintainers of stable versions until the patch hits 
mainline.

Larry

Re: [PATCH] bluetooth: btusb: Fix issue with suspend

[Marcel Holtmann]

Hi Larry,

>> one other thing. Please let maintainers decide when a patch is useful for stable and when not. Patches need to go upstream first anyway. People spamming stable@vger.kernel.org with patches that are not reviewed yet is pretty much a waste of time.
> 
> Sorry. In the wireless tree, the patch author suggests that the patch is suitable for stable. If the maintainer disagrees, he strips the Cc to stable. It seems that you do it the other way. In either case, I do not think the stable patches are picked up by the maintainers of stable versions until the patch hits mainline.

I have no idea what the stable@vger.kernel.org guys do with patches that are not yet upstream. However it is a waste of their time sending stuff to their mailing list that has not even seen a maintainer review. You can just mention it in the comment section that this might qualify for stable.

Regards

Marcel

Re: [PATCH] bluetooth: btusb: Fix issue with suspend

[Greg KH]

On Sat, Sep 06, 2014 at 12:12:44PM -0700, Marcel Holtmann wrote:
> Hi Larry,
> 
> >> one other thing. Please let maintainers decide when a patch is
> >> useful for stable and when not. Patches need to go upstream first
> >> anyway. People spamming stable@vger.kernel.org with patches that
> >> are not reviewed yet is pretty much a waste of time.
> > 
> > Sorry. In the wireless tree, the patch author suggests that the
> > patch is suitable for stable. If the maintainer disagrees, he strips
> > the Cc to stable. It seems that you do it the other way. In either
> > case, I do not think the stable patches are picked up by the
> > maintainers of stable versions until the patch hits mainline.
> 
> I have no idea what the stable@vger.kernel.org guys do with patches
> that are not yet upstream.

I ignore them :)

> However it is a waste of their time sending stuff to their mailing
> list that has not even seen a maintainer review. You can just mention
> it in the comment section that this might qualify for stable.

It doesn't bother me at all for the cc:, they are easy to filter away,
and it keeps me informed as to where patches will be coming from in the
future, and helps to track down things that get dropped.

thanks,

greg k-h