Return-Path: MIME-Version: 1.0 In-Reply-To: References: <20180419140312.22707-1-grzegorz.kolodziejczyk@codecoup.pl> <20180419140312.22707-4-grzegorz.kolodziejczyk@codecoup.pl> From: =?UTF-8?Q?Grzegorz_Ko=C5=82odziejczyk?= Date: Fri, 20 Apr 2018 10:11:41 +0200 Message-ID: Subject: Re: [PATCH BlueZ v2 4/4] shared/gatt-server: Fix prepare write queuing To: Luiz Augusto von Dentz Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Luiz, I did this fix while testing it agains PTS test case "4.6.39 GATT/SR/GAW/BV-10-C [Nested Long Characteristic Value Reliable Writes - to Server]" Without this fix bluez behaves as described: 1) PTS do prepare write to CHARACTERISTIC1 with offset =3D 0 and value with size =3D ATT_MTU-5. 2) Bluez do new prepare write for CHARACTERISTIC1 3) PTS do prepare write to CHARACTERISTIC2 with offset =3D 0 and value with size =3D ATT_MTU-5. 4) Bluez do new prepare write for CHARACTERISTIC2 5) PTS do prepare write to CHARACTERISTIC1 with offset =3D ATT_MTU-5 and value with size < ATT_MTU-5. 6) Bluez do new prepare write for CHARACTERISTIC1 7) PTS do prepare write to CHARACTERISTIC2 with offset =3D ATT_MTU-5 and value with size < ATT_MTU-5. 8) Bluez do new prepare write for CHARACTERISTIC2 9) Execut write is issued with flag "Immediately write all pend-ing prepared values". 10) PTS do read of those two CHARACTERISTICS and got response with value from last prepare write to CHARACTERISTIC (this from second prepare write form offset ATT_MTU-5) cause thos are 4 seperate prepare writes queues (bluez appends only if last prepare write was to the same handle). 11) Test result fails cause expected value isn't the same as read. What's more, spec says" "If a Characteristic Value is prepared two or more times during this sub- procedure, then all prepared values are written to the same Characteristic Value in the order that they were prepared." BLUETOOTH SPECIFICATION Version 5.0 | Vol 3, Part G (4.9.5) "Each client=E2=80=99s queued values are separate; the execution of one que= ue shall not affect the preparation or execution of any other client=E2=80=99s queued values." BLUETOOTH SPECIFICATION Version 5.0 | Vol 3, Part F (3.4.6.1) Regards, Grzegorz 2018-04-19 17:11 GMT+02:00 Luiz Augusto von Dentz : > Hi Grzegorz, > > On Thu, Apr 19, 2018 at 5:03 PM, Grzegorz Kolodziejczyk > wrote: >> Multiple prepare writes may be done for multiple attributes. Queue >> mechanism must be aware of handle under which preparation is done. >> --- >> src/shared/gatt-server.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c >> index 4b554f665..d1efa83a1 100644 >> --- a/src/shared/gatt-server.c >> +++ b/src/shared/gatt-server.c >> @@ -1190,6 +1190,14 @@ static bool prep_data_new(struct bt_gatt_server *= server, >> return true; >> } >> >> +static bool match_prep_attr_handle(const void *data, const void *match_= data) >> +{ >> + const struct prep_write_data *prep_data =3D data; >> + const uint16_t *match_handle =3D match_data; >> + >> + return prep_data->handle =3D=3D *match_handle; >> +} >> + >> static bool store_prep_data(struct bt_gatt_server *server, >> uint16_t handle, uint16_t offset= , >> uint16_t length, uint8_t *value) >> @@ -1200,9 +1208,10 @@ static bool store_prep_data(struct bt_gatt_server= *server, >> * Now lets check if prep write is a continuation of long write >> * If so do aggregation of data >> */ >> - prep_data =3D queue_peek_tail(server->prep_queue); >> - if (prep_data && (prep_data->handle =3D=3D handle) && >> - (offset =3D=3D (prep_data->length + prep_data->o= ffset))) >> + prep_data =3D queue_find(server->prep_queue, match_prep_attr_han= dle, >> + &handle)= ; > > If I got this right it is not that the code is broken, it just don't > group together the writes if there are other handles in the prepare > queue. Afaik that was done because the order we write the handles may > matter, so it is up to the remote stack to make the order proper if it > wants long writes to all be executed in sequence, or interleaved which > means we would have to store new chunks in the order they are > prepared. > >> + if (prep_data && (offset =3D=3D (prep_data->length + prep_data->= offset))) >> return append_prep_data(prep_data, handle, length, value= ); >> >> return prep_data_new(server, handle, offset, length, value); >> -- >> 2.13.6 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-bluetoot= h" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Luiz Augusto von Dentz