diff mbox series

[BlueZ] policy: Fix reset Control/Target/HS retry counters

Message ID 20250526194328.15875-1-repk@triplefau.lt
State New
Headers show
Series [BlueZ] policy: Fix reset Control/Target/HS retry counters | expand

Commit Message

Remi Pommarel May 26, 2025, 7:43 p.m. UTC
Control and Target retries counter were reset when service state
goes from CONNECTED to DISCONNECTED, but usually an extra DISCONNECTING
state is reach before going to DISCONNECTED. This causes retry counter
to not being reset in this case, leading to service not being able to
initialize on next connection. HS retry counter was only reset when
limit is reached.

Reset the counter as soon as CONNECTED state is reached to avoid that.
---
 plugins/policy.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

bluez.test.bot@gmail.com May 26, 2025, 9:13 p.m. UTC | #1
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=966495

---Test result---

Test Summary:
CheckPatch                    PENDING   0.30 seconds
GitLint                       PENDING   0.25 seconds
BuildEll                      PASS      20.17 seconds
BluezMake                     PASS      2674.35 seconds
MakeCheck                     PASS      20.15 seconds
MakeDistcheck                 PASS      199.75 seconds
CheckValgrind                 PASS      275.98 seconds
CheckSmatch                   PASS      301.13 seconds
bluezmakeextell               PASS      128.24 seconds
IncrementalBuild              PENDING   0.24 seconds
ScanBuild                     PASS      907.68 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz May 27, 2025, 1 p.m. UTC | #2
Hi Remi,

On Mon, May 26, 2025 at 3:52 PM Remi Pommarel <repk@triplefau.lt> wrote:
>
> Control and Target retries counter were reset when service state
> goes from CONNECTED to DISCONNECTED, but usually an extra DISCONNECTING
> state is reach before going to DISCONNECTED. This causes retry counter
> to not being reset in this case, leading to service not being able to
> initialize on next connection. HS retry counter was only reset when
> limit is reached.
>
> Reset the counter as soon as CONNECTED state is reached to avoid that.

Looks correct, but shouldn't we update for other services as well? Not
just HS and CT and TG.

> ---
>  plugins/policy.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/policy.c b/plugins/policy.c
> index 0e533ba1f..02ce16f67 100644
> --- a/plugins/policy.c
> +++ b/plugins/policy.c
> @@ -383,6 +383,7 @@ static void hs_cb(struct btd_service *service, btd_service_state_t old_state,
>                 else if (btd_service_get_state(sink) !=
>                                                 BTD_SERVICE_STATE_CONNECTED)
>                         policy_set_sink_timer(data);
> +               data->hs_retries = 0;
>                 break;
>         case BTD_SERVICE_STATE_DISCONNECTING:
>                 break;
> @@ -537,8 +538,6 @@ static void controller_cb(struct btd_service *service,
>                                 timeout_remove(data->ct_timer);
>                                 data->ct_timer = 0;
>                         }
> -               } else if (old_state == BTD_SERVICE_STATE_CONNECTED) {
> -                       data->ct_retries = 0;
>                 }
>                 break;
>         case BTD_SERVICE_STATE_CONNECTING:
> @@ -548,6 +547,7 @@ static void controller_cb(struct btd_service *service,
>                         timeout_remove(data->ct_timer);
>                         data->ct_timer = 0;
>                 }
> +               data->ct_retries = 0;
>                 break;
>         case BTD_SERVICE_STATE_DISCONNECTING:
>                 break;
> @@ -587,8 +587,6 @@ static void target_cb(struct btd_service *service,
>                                 timeout_remove(data->tg_timer);
>                                 data->tg_timer = 0;
>                         }
> -               } else if (old_state == BTD_SERVICE_STATE_CONNECTED) {
> -                       data->tg_retries = 0;
>                 }
>                 break;
>         case BTD_SERVICE_STATE_CONNECTING:
> @@ -598,6 +596,7 @@ static void target_cb(struct btd_service *service,
>                         timeout_remove(data->tg_timer);
>                         data->tg_timer = 0;
>                 }
> +               data->tg_retries = 0;
>                 break;
>         case BTD_SERVICE_STATE_DISCONNECTING:
>                 break;
> --
> 2.40.0
>
>
Remi Pommarel May 27, 2025, 1:31 p.m. UTC | #3
On Tue, May 27, 2025 at 09:00:51AM -0400, Luiz Augusto von Dentz wrote:
> Hi Remi,
> 
> On Mon, May 26, 2025 at 3:52 PM Remi Pommarel <repk@triplefau.lt> wrote:
> >
> > Control and Target retries counter were reset when service state
> > goes from CONNECTED to DISCONNECTED, but usually an extra DISCONNECTING
> > state is reach before going to DISCONNECTED. This causes retry counter
> > to not being reset in this case, leading to service not being able to
> > initialize on next connection. HS retry counter was only reset when
> > limit is reached.
> >
> > Reset the counter as soon as CONNECTED state is reached to avoid that.
> 
> Looks correct, but shouldn't we update for other services as well? Not
> just HS and CT and TG.

Thats right, looks like I missed sink and source retry counters.

Thanks
diff mbox series

Patch

diff --git a/plugins/policy.c b/plugins/policy.c
index 0e533ba1f..02ce16f67 100644
--- a/plugins/policy.c
+++ b/plugins/policy.c
@@ -383,6 +383,7 @@  static void hs_cb(struct btd_service *service, btd_service_state_t old_state,
 		else if (btd_service_get_state(sink) !=
 						BTD_SERVICE_STATE_CONNECTED)
 			policy_set_sink_timer(data);
+		data->hs_retries = 0;
 		break;
 	case BTD_SERVICE_STATE_DISCONNECTING:
 		break;
@@ -537,8 +538,6 @@  static void controller_cb(struct btd_service *service,
 				timeout_remove(data->ct_timer);
 				data->ct_timer = 0;
 			}
-		} else if (old_state == BTD_SERVICE_STATE_CONNECTED) {
-			data->ct_retries = 0;
 		}
 		break;
 	case BTD_SERVICE_STATE_CONNECTING:
@@ -548,6 +547,7 @@  static void controller_cb(struct btd_service *service,
 			timeout_remove(data->ct_timer);
 			data->ct_timer = 0;
 		}
+		data->ct_retries = 0;
 		break;
 	case BTD_SERVICE_STATE_DISCONNECTING:
 		break;
@@ -587,8 +587,6 @@  static void target_cb(struct btd_service *service,
 				timeout_remove(data->tg_timer);
 				data->tg_timer = 0;
 			}
-		} else if (old_state == BTD_SERVICE_STATE_CONNECTED) {
-			data->tg_retries = 0;
 		}
 		break;
 	case BTD_SERVICE_STATE_CONNECTING:
@@ -598,6 +596,7 @@  static void target_cb(struct btd_service *service,
 			timeout_remove(data->tg_timer);
 			data->tg_timer = 0;
 		}
+		data->tg_retries = 0;
 		break;
 	case BTD_SERVICE_STATE_DISCONNECTING:
 		break;