diff mbox series

[v1,3/3] Bluetooth: btintel_pcie: Reduce driver buffer posting to prevent race condition

Message ID 20250525053940.520283-3-kiran.k@intel.com
State New
Headers show
Series [v1,1/3] Bluetooth: btintel_pcie: Fix driver not posting maximum rx buffers | expand

Commit Message

K, Kiran May 25, 2025, 5:39 a.m. UTC 
From: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>

Modify the driver to post 3 fewer buffers than the maximum rx buffers
(64) allowed for the firmware. This change mitigates a hardware issue
causing a race condition in the firmware, improving stability and data
handling.

Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>
Signed-off-by: Kiran K <kiran.k@intel.com>
---
 drivers/bluetooth/btintel_pcie.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

K, Kiran May 26, 2025, 3:56 p.m. UTC | #1 
Hi Paul,

Thanks for your comments.

>Subject: Re: [PATCH v1 3/3] Bluetooth: btintel_pcie: Reduce driver buffer
>posting to prevent race condition
>
>Dear Chandrashekar, dear Kiran,
>
>
>Thank you for the patch.
>
>Am 25.05.25 um 07:39 schrieb Kiran K:
>> From: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>
>>
>> Modify the driver to post 3 fewer buffers than the maximum rx buffers
>> (64) allowed for the firmware. This change mitigates a hardware issue
>> causing a race condition in the firmware, improving stability and data
>> handling.
>

>Interesting. Please elaborate, which firmware versions are affected, and if a fix
>is going to be expected, and how to reproduce the issue, so it can be tested
>without and with your patch.
>
The firmware is for the upcoming product and is not available in public yet. As I said in 1/3,  the issue is seen on android kernel and  it's very hard to reproduce the issue on vanilla kernel. 

>Is the errata published? 
>
Our internal documents are updated.  I have also entered a comment in code.

> Why three buffers less and not two or four?
The recommendation from firmware / HW is to use at least 3 buffers as guard buffers. Anything less than three causes RFH (receive flow handler - RX path) blockage. 

>Out of curiosity: Does the Microsoft Windows driver do the same?
Yes.

>
>> Signed-off-by: Chandrashekar Devegowda
>> <chandrashekar.devegowda@intel.com>
>> Signed-off-by: Kiran K <kiran.k@intel.com>
>> ---
>>   drivers/bluetooth/btintel_pcie.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/btintel_pcie.c
>> b/drivers/bluetooth/btintel_pcie.c
>> index 03f13de4a723..14f000e08e1e 100644
>> --- a/drivers/bluetooth/btintel_pcie.c
>> +++ b/drivers/bluetooth/btintel_pcie.c
>> @@ -398,7 +398,10 @@ static int btintel_pcie_start_rx(struct
>btintel_pcie_data *data)
>>   	int i, ret;
>>   	struct rxq *rxq = &data->rxq;
>>
>> -	for (i = 0; i < rxq->count; i++) {
>> +	/* Post (BTINTEL_PCIE_RX_DESCS_COUNT - 3) buffers to overcome the
>> +	 * hardware issues leading to race condition at the firmware.
>
>If you had an errata, it’d be great to document it here to.
>
>I’d remove *the*.
Ack
>
>> +	 */
>> +	for (i = 0; i < rxq->count - 3; i++) {
>>   		ret = btintel_pcie_submit_rx(data);
>>   		if (ret)
>>   			return ret;
>
>
>Kind regards,
>
>Paul

Regards,
Kiran
Luiz Augusto von Dentz May 27, 2025, 1:49 p.m. UTC | #2 
Hi Kiran,

On Mon, May 26, 2025 at 11:58 AM K, Kiran <kiran.k@intel.com> wrote:
>
>
> Hi Paul,
>
> Thanks for your comments.
>
> >Subject: Re: [PATCH v1 3/3] Bluetooth: btintel_pcie: Reduce driver buffer
> >posting to prevent race condition
> >
> >Dear Chandrashekar, dear Kiran,
> >
> >
> >Thank you for the patch.
> >
> >Am 25.05.25 um 07:39 schrieb Kiran K:
> >> From: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>
> >>
> >> Modify the driver to post 3 fewer buffers than the maximum rx buffers
> >> (64) allowed for the firmware. This change mitigates a hardware issue
> >> causing a race condition in the firmware, improving stability and data
> >> handling.
> >
>
> >Interesting. Please elaborate, which firmware versions are affected, and if a fix
> >is going to be expected, and how to reproduce the issue, so it can be tested
> >without and with your patch.
> >
> The firmware is for the upcoming product and is not available in public yet. As I said in 1/3,  the issue is seen on android kernel and  it's very hard to reproduce the issue on vanilla kernel.

If it affects Android specific versions only then it should be posted
for upstream, anyway this sounds like it is more of a workaround then
a proper fix for the problem.

> >Is the errata published?
> >
> Our internal documents are updated.  I have also entered a comment in code.
>
> > Why three buffers less and not two or four?
> The recommendation from firmware / HW is to use at least 3 buffers as guard buffers. Anything less than three causes RFH (receive flow handler - RX path) blockage.

Are these buffers discovered at runtime or they are hardcoded, for the
former then the firmware shall be adjusted to give a proper number and
in case of the later then the driver shall be updated, either way
having to do -3 sounds like a bad idea in the long term.


> >Out of curiosity: Does the Microsoft Windows driver do the same?
> Yes.
>
> >
> >> Signed-off-by: Chandrashekar Devegowda
> >> <chandrashekar.devegowda@intel.com>
> >> Signed-off-by: Kiran K <kiran.k@intel.com>
> >> ---
> >>   drivers/bluetooth/btintel_pcie.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/bluetooth/btintel_pcie.c
> >> b/drivers/bluetooth/btintel_pcie.c
> >> index 03f13de4a723..14f000e08e1e 100644
> >> --- a/drivers/bluetooth/btintel_pcie.c
> >> +++ b/drivers/bluetooth/btintel_pcie.c
> >> @@ -398,7 +398,10 @@ static int btintel_pcie_start_rx(struct
> >btintel_pcie_data *data)
> >>      int i, ret;
> >>      struct rxq *rxq = &data->rxq;
> >>
> >> -    for (i = 0; i < rxq->count; i++) {
> >> +    /* Post (BTINTEL_PCIE_RX_DESCS_COUNT - 3) buffers to overcome the
> >> +     * hardware issues leading to race condition at the firmware.
> >
> >If you had an errata, it’d be great to document it here to.
> >
> >I’d remove *the*.
> Ack
> >
> >> +     */
> >> +    for (i = 0; i < rxq->count - 3; i++) {
> >>              ret = btintel_pcie_submit_rx(data);
> >>              if (ret)
> >>                      return ret;
> >
> >
> >Kind regards,
> >
> >Paul
>
> Regards,
> Kiran
>


--
Luiz Augusto von Dentz
K, Kiran May 28, 2025, 2:04 a.m. UTC | #3 
Hi Luiz,

>Subject: Re: [PATCH v1 3/3] Bluetooth: btintel_pcie: Reduce driver buffer
>posting to prevent race condition
>
>Hi Kiran,
>
>On Mon, May 26, 2025 at 11:58 AM K, Kiran <kiran.k@intel.com> wrote:
>>
>>
>> Hi Paul,
>>
>> Thanks for your comments.
>>
>> >Subject: Re: [PATCH v1 3/3] Bluetooth: btintel_pcie: Reduce driver
>> >buffer posting to prevent race condition
>> >
>> >Dear Chandrashekar, dear Kiran,
>> >
>> >
>> >Thank you for the patch.
>> >
>> >Am 25.05.25 um 07:39 schrieb Kiran K:
>> >> From: Chandrashekar Devegowda
><chandrashekar.devegowda@intel.com>
>> >>
>> >> Modify the driver to post 3 fewer buffers than the maximum rx
>> >> buffers
>> >> (64) allowed for the firmware. This change mitigates a hardware
>> >> issue causing a race condition in the firmware, improving stability
>> >> and data handling.
>> >
>>
>> >Interesting. Please elaborate, which firmware versions are affected,
>> >and if a fix is going to be expected, and how to reproduce the issue,
>> >so it can be tested without and with your patch.
>> >
>> The firmware is for the upcoming product and is not available in public yet.
>As I said in 1/3,  the issue is seen on android kernel and  it's very hard to
>reproduce the issue on vanilla kernel.
>
>If it affects Android specific versions only then it should be posted for
>upstream, anyway this sounds like it is more of a workaround then a proper fix
>for the problem.

The issue is seen on Linux also but the reproduction rate - (1/25) is less compared to android (1/5). As the firmware content is same for both, I feel this work around  is applicable for Linux as well.
>
>> >Is the errata published?
>> >
>> Our internal documents are updated.  I have also entered a comment in
>code.
>>
>> > Why three buffers less and not two or four?
>> The recommendation from firmware / HW is to use at least 3 buffers as guard
>buffers. Anything less than three causes RFH (receive flow handler - RX path)
>blockage.
>
>Are these buffers discovered at runtime or they are hardcoded, for the former
>then the firmware shall be adjusted to give a proper number and in case of the
>later then the driver shall be updated, either way having to do -3 sounds like a
>bad idea in the long term.
>
The maximum number of buffers for RX (N) and the buffers granted for firmware(N-3)  are hardcoded. As the issue is related to the hardware, it would take sometime to get it fixed. 
>
>> >Out of curiosity: Does the Microsoft Windows driver do the same?
>> Yes.
>>
>> >
>> >> Signed-off-by: Chandrashekar Devegowda
>> >> <chandrashekar.devegowda@intel.com>
>> >> Signed-off-by: Kiran K <kiran.k@intel.com>
>> >> ---
>> >>   drivers/bluetooth/btintel_pcie.c | 5 ++++-
>> >>   1 file changed, 4 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/bluetooth/btintel_pcie.c
>> >> b/drivers/bluetooth/btintel_pcie.c
>> >> index 03f13de4a723..14f000e08e1e 100644
>> >> --- a/drivers/bluetooth/btintel_pcie.c
>> >> +++ b/drivers/bluetooth/btintel_pcie.c
>> >> @@ -398,7 +398,10 @@ static int btintel_pcie_start_rx(struct
>> >btintel_pcie_data *data)
>> >>      int i, ret;
>> >>      struct rxq *rxq = &data->rxq;
>> >>
>> >> -    for (i = 0; i < rxq->count; i++) {
>> >> +    /* Post (BTINTEL_PCIE_RX_DESCS_COUNT - 3) buffers to overcome the
>> >> +     * hardware issues leading to race condition at the firmware.
>> >
>> >If you had an errata, it’d be great to document it here to.
>> >
>> >I’d remove *the*.
>> Ack
>> >
>> >> +     */
>> >> +    for (i = 0; i < rxq->count - 3; i++) {
>> >>              ret = btintel_pcie_submit_rx(data);
>> >>              if (ret)
>> >>                      return ret;
>> >
>> >
>> >Kind regards,
>> >
>> >Paul
>>
>> Regards,
>> Kiran
>>
>
>
>--
>Luiz Augusto von Dentz

Thanks,
Kiran
diff mbox series

Patch

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index 03f13de4a723..14f000e08e1e 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -398,7 +398,10 @@  static int btintel_pcie_start_rx(struct btintel_pcie_data *data)
 	int i, ret;
 	struct rxq *rxq = &data->rxq;
 
-	for (i = 0; i < rxq->count; i++) {
+	/* Post (BTINTEL_PCIE_RX_DESCS_COUNT - 3) buffers to overcome the
+	 * hardware issues leading to race condition at the firmware.
+	 */
+	for (i = 0; i < rxq->count - 3; i++) {
 		ret = btintel_pcie_submit_rx(data);
 		if (ret)
 			return ret;