diff mbox series

[BlueZ,v2] shared/gatt-client:Ignore orphaned characteristics

Message ID 20200501192218.73993-1-alainm@chromium.org (mailing list archive)
State Accepted
Headers show
Series [BlueZ,v2] shared/gatt-client:Ignore orphaned characteristics | expand

Commit Message

Alain Michaud May 1, 2020, 7:22 p.m. UTC
The gatt discovery proceedure simplification to discover all
characteristics at once has exposed a device side issue where some
device implementation reports orphaned characteristics.  While this
technically shouldn't be allowed, some instances of this were observed
(namely on some Android phones).

The fix is to simply skip over orphaned characteristics and continue
with everything else that is valid.

This has been tested as part of the Android CTS tests against an
affected platform and was confirmed to have worked around the issue.

Signed-off-by: Alain Michaud <alainm@chromium.org>
---

 src/shared/gatt-client.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Luiz Augusto von Dentz May 1, 2020, 8:45 p.m. UTC | #1
Hi Alain,

On Fri, May 1, 2020 at 12:27 PM Alain Michaud <alainm@chromium.org> wrote:
>
> The gatt discovery proceedure simplification to discover all
> characteristics at once has exposed a device side issue where some
> device implementation reports orphaned characteristics.  While this
> technically shouldn't be allowed, some instances of this were observed
> (namely on some Android phones).
>
> The fix is to simply skip over orphaned characteristics and continue
> with everything else that is valid.
>
> This has been tested as part of the Android CTS tests against an
> affected platform and was confirmed to have worked around the issue.
>
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
>
>  src/shared/gatt-client.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 963ad619f..507b4d304 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -632,7 +632,13 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
>                         util_debug(client->debug_callback, client->debug_data,
>                                 "Failed to insert characteristic at 0x%04x",
>                                 chrc_data->value_handle);
> -                       goto failed;
> +
> +                       /* Some devices have been seen reporting orphaned
> +                        * characteristics.  In order to favor interoperability
> +                        * we skip over characteristics in error
> +                        */
> +                       free(chrc_data);
> +                       continue;
>                 }
>
>                 if (gatt_db_attribute_get_handle(attr) !=
> --
> 2.26.2.526.g744177e7f7-goog
>

Applied, thanks. Note that I drop the Signed-off-by line since we
don't use that on userspace.
Alain Michaud May 4, 2020, 12:54 a.m. UTC | #2
Hi Luiz,

On Fri, May 1, 2020 at 4:46 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alain,
>
> On Fri, May 1, 2020 at 12:27 PM Alain Michaud <alainm@chromium.org> wrote:
> >
> > The gatt discovery proceedure simplification to discover all
> > characteristics at once has exposed a device side issue where some
> > device implementation reports orphaned characteristics.  While this
> > technically shouldn't be allowed, some instances of this were observed
> > (namely on some Android phones).
> >
> > The fix is to simply skip over orphaned characteristics and continue
> > with everything else that is valid.
> >
> > This has been tested as part of the Android CTS tests against an
> > affected platform and was confirmed to have worked around the issue.
> >
> > Signed-off-by: Alain Michaud <alainm@chromium.org>
> > ---
> >
> >  src/shared/gatt-client.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> > index 963ad619f..507b4d304 100644
> > --- a/src/shared/gatt-client.c
> > +++ b/src/shared/gatt-client.c
> > @@ -632,7 +632,13 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
> >                         util_debug(client->debug_callback, client->debug_data,
> >                                 "Failed to insert characteristic at 0x%04x",
> >                                 chrc_data->value_handle);
> > -                       goto failed;
> > +
> > +                       /* Some devices have been seen reporting orphaned
> > +                        * characteristics.  In order to favor interoperability
> > +                        * we skip over characteristics in error
> > +                        */
> > +                       free(chrc_data);
> > +                       continue;
> >                 }
> >
> >                 if (gatt_db_attribute_get_handle(attr) !=
> > --
> > 2.26.2.526.g744177e7f7-goog
> >
>
> Applied, thanks. Note that I drop the Signed-off-by line since we
> don't use that on userspace.
Thanks, sorry, this is automatically added by the tools I use to
automate the patch upload and I need to remember to remove it.  Sadly,
it's a manual process and is vulnerable to my poor memory :P
>
> --
> Luiz Augusto von Dentz
diff mbox series

Patch

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 963ad619f..507b4d304 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -632,7 +632,13 @@  static bool discover_descs(struct discovery_op *op, bool *discovering)
 			util_debug(client->debug_callback, client->debug_data,
 				"Failed to insert characteristic at 0x%04x",
 				chrc_data->value_handle);
-			goto failed;
+
+			/* Some devices have been seen reporting orphaned
+			 * characteristics.  In order to favor interoperability
+			 * we skip over characteristics in error
+			 */
+			free(chrc_data);
+			continue;
 		}
 
 		if (gatt_db_attribute_get_handle(attr) !=