| Message ID | 20240826090044.560142-1-quic_chejiang@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] adapter: Cancel the service authorization when remote is disconnected | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/BuildEll | success | Build ELL PASS |
| tedd_an/BluezMake | success | Bluez Make PASS |
| tedd_an/MakeCheck | success | Bluez Make Check PASS |
| tedd_an/MakeDistcheck | success | Make Distcheck PASS |
| tedd_an/CheckValgrind | success | Check Valgrind PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/bluezmakeextell | success | Make External ELL PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
| tedd_an/ScanBuild | success | Scan Build PASS |
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=883234 ---Test result--- Test Summary: CheckPatch PASS 0.41 seconds GitLint PASS 0.27 seconds BuildEll PASS 24.55 seconds BluezMake PASS 1728.48 seconds MakeCheck PASS 13.32 seconds MakeDistcheck PASS 182.69 seconds CheckValgrind PASS 254.20 seconds CheckSmatch PASS 355.43 seconds bluezmakeextell PASS 119.41 seconds IncrementalBuild PASS 1507.74 seconds ScanBuild PASS 1003.07 seconds --- Regards, Linux Bluetooth
Hi Cheng, On Mon, Aug 26, 2024 at 5:01 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote: > > If the remote device drops the connection before DUT confirm the > service authorization, the DUT still must wait for service > authorization timeout before processing future request. > > Cancel the service authorization request when connection is dropped. > --- > src/adapter.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/src/adapter.c b/src/adapter.c > index 245de4456..3ad000425 100644 > --- a/src/adapter.c > +++ b/src/adapter.c > @@ -8502,6 +8502,25 @@ static void disconnect_notify(struct btd_device *dev, uint8_t reason) > } > } > > +static void adapter_cancel_service_auth(struct btd_adapter *adapter, > + struct btd_device *device) > +{ > + struct service_auth *auth; > + GList *l; > + > + auth = NULL; > + for (l = adapter->auths->head; l != NULL; l = l->next) { > + auth = l->data; > + if (auth->device == device) > + break; > + } > + if (auth != NULL) { > + DBG("Cancel pending auth: %s", auth->uuid); > + g_queue_remove(auth->adapter->auths, auth); > + service_auth_cancel(auth); > + } > +} > + > static void dev_disconnected(struct btd_adapter *adapter, > const struct mgmt_addr_info *addr, > uint8_t reason) > @@ -8516,6 +8535,7 @@ static void dev_disconnected(struct btd_adapter *adapter, > device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type); > if (device) { > adapter_remove_connection(adapter, device, addr->type); > + adapter_cancel_service_auth(adapter, device); This shall probably be placed together with device_cancel_authentication in adapter_remove_connection but we need to track in what bearer the authorization is for and remove all authorization requests like in btd_adapter_remove_device: l = adapter->auths->head; while (l != NULL) { struct service_auth *auth = l->data; GList *next = g_list_next(l); if (auth->device != dev) { l = next; continue; } g_queue_delete_link(adapter->auths, l); l = next; service_auth_cancel(auth); } I'd probably move the above code to a helper function so it can be called from different places, as for doing this on a per bearer basis would probably need to add bearer information to btd_service but I guess that can be treated separately. > disconnect_notify(device, reason); > } > > -- > 2.25.1 > >
Hi Luiz, Thank you for your feedback. Please check the comment inline. On 8/26/2024 9:43 PM, Luiz Augusto von Dentz wrote: > Hi Cheng, > > On Mon, Aug 26, 2024 at 5:01 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote: >> >> If the remote device drops the connection before DUT confirm the >> service authorization, the DUT still must wait for service >> authorization timeout before processing future request. >> >> Cancel the service authorization request when connection is dropped. >> --- >> src/adapter.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/src/adapter.c b/src/adapter.c >> index 245de4456..3ad000425 100644 >> --- a/src/adapter.c >> +++ b/src/adapter.c >> @@ -8502,6 +8502,25 @@ static void disconnect_notify(struct btd_device *dev, uint8_t reason) >> } >> } >> >> +static void adapter_cancel_service_auth(struct btd_adapter *adapter, >> + struct btd_device *device) >> +{ >> + struct service_auth *auth; >> + GList *l; >> + >> + auth = NULL; >> + for (l = adapter->auths->head; l != NULL; l = l->next) { >> + auth = l->data; >> + if (auth->device == device) >> + break; >> + } >> + if (auth != NULL) { >> + DBG("Cancel pending auth: %s", auth->uuid); >> + g_queue_remove(auth->adapter->auths, auth); >> + service_auth_cancel(auth); >> + } >> +} >> + >> static void dev_disconnected(struct btd_adapter *adapter, >> const struct mgmt_addr_info *addr, >> uint8_t reason) >> @@ -8516,6 +8535,7 @@ static void dev_disconnected(struct btd_adapter *adapter, >> device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type); >> if (device) { >> adapter_remove_connection(adapter, device, addr->type); >> + adapter_cancel_service_auth(adapter, device); > > This shall probably be placed together with > device_cancel_authentication in adapter_remove_connection but we need > to track in what bearer the authorization is for and remove all > authorization requests like in btd_adapter_remove_device: Yes. It can be put in device_cancel_authentication, but the condition to call this function in adapter_remove_connection need change. device_is_authenticating is only true when request, notify or confirm pincode/passkey. As for bearer, the service authorize is only happened on BDADDR_BREDR. So it can be called when the address type is BDADDR_BREDR in dev_disconnected. > > l = adapter->auths->head; > while (l != NULL) { > struct service_auth *auth = l->data; > GList *next = g_list_next(l); > > if (auth->device != dev) { > l = next; > continue; > } > > g_queue_delete_link(adapter->auths, l); > l = next; > > service_auth_cancel(auth); > } > > I'd probably move the above code to a helper function so it can be > called from different places, as for doing this on a per bearer basis > would probably need to add bearer information to btd_service but I > guess that can be treated separately. Yes, it's great. Need use the above code to cancel all pending authorize. > > >> disconnect_notify(device, reason); >> } >> >> -- >> 2.25.1 >> >> > >
Hi Cheng, On Mon, Aug 26, 2024 at 10:30 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote: > > Hi Luiz, > > Thank you for your feedback. Please check the comment inline. > > On 8/26/2024 9:43 PM, Luiz Augusto von Dentz wrote: > > Hi Cheng, > > > > On Mon, Aug 26, 2024 at 5:01 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote: > >> > >> If the remote device drops the connection before DUT confirm the > >> service authorization, the DUT still must wait for service > >> authorization timeout before processing future request. > >> > >> Cancel the service authorization request when connection is dropped. > >> --- > >> src/adapter.c | 20 ++++++++++++++++++++ > >> 1 file changed, 20 insertions(+) > >> > >> diff --git a/src/adapter.c b/src/adapter.c > >> index 245de4456..3ad000425 100644 > >> --- a/src/adapter.c > >> +++ b/src/adapter.c > >> @@ -8502,6 +8502,25 @@ static void disconnect_notify(struct btd_device *dev, uint8_t reason) > >> } > >> } > >> > >> +static void adapter_cancel_service_auth(struct btd_adapter *adapter, > >> + struct btd_device *device) > >> +{ > >> + struct service_auth *auth; > >> + GList *l; > >> + > >> + auth = NULL; > >> + for (l = adapter->auths->head; l != NULL; l = l->next) { > >> + auth = l->data; > >> + if (auth->device == device) > >> + break; > >> + } > >> + if (auth != NULL) { > >> + DBG("Cancel pending auth: %s", auth->uuid); > >> + g_queue_remove(auth->adapter->auths, auth); > >> + service_auth_cancel(auth); > >> + } > >> +} > >> + > >> static void dev_disconnected(struct btd_adapter *adapter, > >> const struct mgmt_addr_info *addr, > >> uint8_t reason) > >> @@ -8516,6 +8535,7 @@ static void dev_disconnected(struct btd_adapter *adapter, > >> device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type); > >> if (device) { > >> adapter_remove_connection(adapter, device, addr->type); > >> + adapter_cancel_service_auth(adapter, device); > > > > This shall probably be placed together with > > device_cancel_authentication in adapter_remove_connection but we need > > to track in what bearer the authorization is for and remove all > > authorization requests like in btd_adapter_remove_device: > Yes. It can be put in device_cancel_authentication, but the condition to call > this function in adapter_remove_connection need change. device_is_authenticating > is only true when request, notify or confirm pincode/passkey. > As for bearer, the service authorize is only happened on BDADDR_BREDR. So it > can be called when the address type is BDADDR_BREDR in dev_disconnected. > > > > l = adapter->auths->head; > > while (l != NULL) { > > struct service_auth *auth = l->data; > > GList *next = g_list_next(l); > > > > if (auth->device != dev) { > > l = next; > > continue; > > } > > > > g_queue_delete_link(adapter->auths, l); > > l = next; > > > > service_auth_cancel(auth); > > } > > > > I'd probably move the above code to a helper function so it can be > > called from different places, as for doing this on a per bearer basis > > would probably need to add bearer information to btd_service but I > > guess that can be treated separately. > Yes, it's great. Need use the above code to cancel all pending authorize. Looks like you never send another version of this one, are you still planning on having this fixed? > > > > > >> disconnect_notify(device, reason); > >> } > >> > >> -- > >> 2.25.1 > >> > >> > > > > >
Hi Luiz, Sorry for the delay, will send the new patch soon. On 9/20/2024 3:53 AM, Luiz Augusto von Dentz wrote: > Hi Cheng, > > On Mon, Aug 26, 2024 at 10:30 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote: >> >> Hi Luiz, >> >> Thank you for your feedback. Please check the comment inline. >> >> On 8/26/2024 9:43 PM, Luiz Augusto von Dentz wrote: >>> Hi Cheng, >>> >>> On Mon, Aug 26, 2024 at 5:01 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote: >>>> >>>> If the remote device drops the connection before DUT confirm the >>>> service authorization, the DUT still must wait for service >>>> authorization timeout before processing future request. >>>> >>>> Cancel the service authorization request when connection is dropped. >>>> --- >>>> src/adapter.c | 20 ++++++++++++++++++++ >>>> 1 file changed, 20 insertions(+) >>>> >>>> diff --git a/src/adapter.c b/src/adapter.c >>>> index 245de4456..3ad000425 100644 >>>> --- a/src/adapter.c >>>> +++ b/src/adapter.c >>>> @@ -8502,6 +8502,25 @@ static void disconnect_notify(struct btd_device *dev, uint8_t reason) >>>> } >>>> } >>>> >>>> +static void adapter_cancel_service_auth(struct btd_adapter *adapter, >>>> + struct btd_device *device) >>>> +{ >>>> + struct service_auth *auth; >>>> + GList *l; >>>> + >>>> + auth = NULL; >>>> + for (l = adapter->auths->head; l != NULL; l = l->next) { >>>> + auth = l->data; >>>> + if (auth->device == device) >>>> + break; >>>> + } >>>> + if (auth != NULL) { >>>> + DBG("Cancel pending auth: %s", auth->uuid); >>>> + g_queue_remove(auth->adapter->auths, auth); >>>> + service_auth_cancel(auth); >>>> + } >>>> +} >>>> + >>>> static void dev_disconnected(struct btd_adapter *adapter, >>>> const struct mgmt_addr_info *addr, >>>> uint8_t reason) >>>> @@ -8516,6 +8535,7 @@ static void dev_disconnected(struct btd_adapter *adapter, >>>> device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type); >>>> if (device) { >>>> adapter_remove_connection(adapter, device, addr->type); >>>> + adapter_cancel_service_auth(adapter, device); >>> >>> This shall probably be placed together with >>> device_cancel_authentication in adapter_remove_connection but we need >>> to track in what bearer the authorization is for and remove all >>> authorization requests like in btd_adapter_remove_device: >> Yes. It can be put in device_cancel_authentication, but the condition to call >> this function in adapter_remove_connection need change. device_is_authenticating >> is only true when request, notify or confirm pincode/passkey. >> As for bearer, the service authorize is only happened on BDADDR_BREDR. So it >> can be called when the address type is BDADDR_BREDR in dev_disconnected. >>> >>> l = adapter->auths->head; >>> while (l != NULL) { >>> struct service_auth *auth = l->data; >>> GList *next = g_list_next(l); >>> >>> if (auth->device != dev) { >>> l = next; >>> continue; >>> } >>> >>> g_queue_delete_link(adapter->auths, l); >>> l = next; >>> >>> service_auth_cancel(auth); >>> } >>> >>> I'd probably move the above code to a helper function so it can be >>> called from different places, as for doing this on a per bearer basis >>> would probably need to add bearer information to btd_service but I >>> guess that can be treated separately. >> Yes, it's great. Need use the above code to cancel all pending authorize. > > Looks like you never send another version of this one, are you still > planning on having this fixed? > >>> >>> >>>> disconnect_notify(device, reason); >>>> } >>>> >>>> -- >>>> 2.25.1 >>>> >>>> >>> >>> >> > >
diff --git a/src/adapter.c b/src/adapter.c index 245de4456..3ad000425 100644 --- a/src/adapter.c +++ b/src/adapter.c @@ -8502,6 +8502,25 @@ static void disconnect_notify(struct btd_device *dev, uint8_t reason) } } +static void adapter_cancel_service_auth(struct btd_adapter *adapter, + struct btd_device *device) +{ + struct service_auth *auth; + GList *l; + + auth = NULL; + for (l = adapter->auths->head; l != NULL; l = l->next) { + auth = l->data; + if (auth->device == device) + break; + } + if (auth != NULL) { + DBG("Cancel pending auth: %s", auth->uuid); + g_queue_remove(auth->adapter->auths, auth); + service_auth_cancel(auth); + } +} + static void dev_disconnected(struct btd_adapter *adapter, const struct mgmt_addr_info *addr, uint8_t reason) @@ -8516,6 +8535,7 @@ static void dev_disconnected(struct btd_adapter *adapter, device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type); if (device) { adapter_remove_connection(adapter, device, addr->type); + adapter_cancel_service_auth(adapter, device); disconnect_notify(device, reason); }