Message ID | 20231013100537.3867-2-claudia.rosu@nxp.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | bap: Fix source+sink endpoint registration | expand |
Context | Check | Description |
---|---|---|
tedd_an/pre-ci_am | success | Success |
tedd_an/CheckPatch | warning | WARNING:LONG_LINE: line length of 86 exceeds 80 columns #158: FILE: src/shared/bap.c:2720: + queue_foreach(sessions, notify_session_pac_added, pac_broadcast_sink); /github/workspace/src/src/13420604.patch total: 0 errors, 1 warnings, 115 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13420604.patch has style problems, please review. NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. |
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=792936 ---Test result--- Test Summary: CheckPatch FAIL 1.32 seconds GitLint PASS 0.74 seconds BuildEll PASS 27.69 seconds BluezMake PASS 884.34 seconds MakeCheck PASS 11.65 seconds MakeDistcheck PASS 172.79 seconds CheckValgrind PASS 267.85 seconds CheckSmatch PASS 361.35 seconds bluezmakeextell PASS 115.77 seconds IncrementalBuild PASS 1379.67 seconds ScanBuild PASS 1048.46 seconds Details ############################## Test: CheckPatch - FAIL Desc: Run checkpatch.pl script Output: [BlueZ,1/2] bap: Fix source+sink endpoint registration WARNING:LONG_LINE: line length of 86 exceeds 80 columns #158: FILE: src/shared/bap.c:2720: + queue_foreach(sessions, notify_session_pac_added, pac_broadcast_sink); /github/workspace/src/src/13420604.patch total: 0 errors, 1 warnings, 115 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13420604.patch has style problems, please review. NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. --- Regards, Linux Bluetooth
Hi Claudia, On Fri, Oct 13, 2023 at 3:06 AM Claudia Draghicescu <claudia.rosu@nxp.com> wrote: > > Create new endpoint name for the simulated broadcast sink that is > created when registering a broadcast source endpoint. > This removes the ambiguity when having registered a local > broadcast sink and fixes the situation when multiple remote > endpoints are created when discovering a broadcast source. > > --- > src/shared/bap.c | 54 +++++++++++++++++++++++++++--------------------- > src/shared/bap.h | 1 + > 2 files changed, 32 insertions(+), 23 deletions(-) > > diff --git a/src/shared/bap.c b/src/shared/bap.c > index 925501c48..266116235 100644 > --- a/src/shared/bap.c > +++ b/src/shared/bap.c > @@ -644,7 +644,7 @@ static struct bt_bap_endpoint *bap_endpoint_new_broadcast(struct bt_bap_db *bdb, > if (type == BT_BAP_BCAST_SINK) > ep->dir = BT_BAP_BCAST_SOURCE; > else > - ep->dir = BT_BAP_BCAST_SINK; > + ep->dir = BT_BAP_SIMULATED_BCAST_SINK; > > return ep; > } > @@ -1500,12 +1500,12 @@ static void ep_config_cb(struct bt_bap_stream *stream, int err) > return; > > if (bt_bap_stream_get_type(stream) == BT_BAP_STREAM_TYPE_BCAST) { > - if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SINK) > + if (bt_bap_stream_io_dir(stream) == BT_BAP_SIMULATED_BCAST_SINK) > stream_set_state_broadcast(stream, > - BT_BAP_STREAM_STATE_QOS); > + BT_BAP_STREAM_STATE_QOS); > else if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SOURCE) > stream_set_state_broadcast(stream, > - BT_BAP_STREAM_STATE_CONFIG); > + BT_BAP_STREAM_STATE_CONFIG); > return; > } > > @@ -2710,15 +2710,15 @@ struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db, > break; > case BT_BAP_BCAST_SOURCE: > bap_add_broadcast_source(pac); > - if (queue_isempty(bdb->broadcast_sinks)) { > - /* When adding a local broadcast source, add also a > - * local broadcast sink > - */ > - pac_broadcast_sink = bap_pac_new(bdb, name, > - BT_BAP_BCAST_SINK, &codec, qos, > - data, metadata); > - bap_add_broadcast_sink(pac_broadcast_sink); > - } > + /* When adding a local broadcast source, add also a > + * local broadcast sink > + */ > + pac_broadcast_sink = bap_pac_new(bdb, name, > + BT_BAP_SIMULATED_BCAST_SINK, &codec, qos, > + data, metadata); I'm not really sure why this is needed to begin with, if that is to have a remote endpoint I'd say we need to change the logic so broadcast src role is allowed to create streams without a remote endpoint, anyway we should probably have better documentation around this logic. > + bap_add_broadcast_sink(pac_broadcast_sink); > + queue_foreach(sessions, notify_session_pac_added, pac_broadcast_sink); > + return pac; > break; > case BT_BAP_BCAST_SINK: > bap_add_broadcast_sink(pac); > @@ -4457,13 +4457,23 @@ static void bap_foreach_pac(struct queue *l, struct queue *r, > for (er = queue_get_entries(r); er; er = er->next) { > struct bt_bap_pac *rpac = er->data; > > + if ((lpac->type == BT_BAP_BCAST_SOURCE) > + && (rpac->type != BT_BAP_SIMULATED_BCAST_SINK)) > + continue; > + if ((rpac->type == BT_BAP_SIMULATED_BCAST_SINK) > + && (lpac->type == BT_BAP_BCAST_SOURCE)) { > + func(lpac, rpac, user_data); > + return; > + } > + > /* Skip checking codec for bcast source, > * it will be checked when BASE info are received > */ > if ((rpac->type != BT_BAP_BCAST_SOURCE) && > (!bap_codec_equal(&lpac->codec, &rpac->codec))) > continue; > - > + if (rpac->type == BT_BAP_SIMULATED_BCAST_SINK) > + continue; > if (!func(lpac, rpac, user_data)) > return; > } > @@ -4484,12 +4494,6 @@ void bt_bap_foreach_pac(struct bt_bap *bap, uint8_t type, > return bap_foreach_pac(bap->ldb->sinks, bap->rdb->sources, > func, user_data); > case BT_BAP_BCAST_SOURCE: > - if (queue_isempty(bap->rdb->broadcast_sources) > - && queue_isempty(bap->rdb->broadcast_sinks)) > - return bap_foreach_pac(bap->ldb->broadcast_sources, > - bap->ldb->broadcast_sinks, > - func, user_data); > - > return bap_foreach_pac(bap->ldb->broadcast_sinks, > bap->rdb->broadcast_sources, > func, user_data); > @@ -4497,6 +4501,10 @@ void bt_bap_foreach_pac(struct bt_bap *bap, uint8_t type, > return bap_foreach_pac(bap->ldb->broadcast_sinks, > bap->rdb->broadcast_sources, > func, user_data); > + case BT_BAP_SIMULATED_BCAST_SINK: > + return bap_foreach_pac(bap->ldb->broadcast_sources, > + bap->ldb->broadcast_sinks, > + func, user_data); > } > } > > @@ -4927,12 +4935,12 @@ unsigned int bt_bap_stream_enable(struct bt_bap_stream *stream, > queue_foreach(stream->links, bap_stream_enable_link, metadata); > break; > case BT_BAP_STREAM_TYPE_BCAST: > - if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SINK) > + if (bt_bap_stream_io_dir(stream) == BT_BAP_SIMULATED_BCAST_SINK) > stream_set_state_broadcast(stream, > - BT_BAP_STREAM_STATE_CONFIG); > + BT_BAP_STREAM_STATE_CONFIG); > else if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SOURCE) > stream_set_state_broadcast(stream, > - BT_BAP_STREAM_STATE_STREAMING); > + BT_BAP_STREAM_STATE_STREAMING); > > return 1; > } > diff --git a/src/shared/bap.h b/src/shared/bap.h > index ebe4dbf7d..af3b6be71 100644 > --- a/src/shared/bap.h > +++ b/src/shared/bap.h > @@ -19,6 +19,7 @@ > #define BT_BAP_SOURCE 0x02 > #define BT_BAP_BCAST_SOURCE 0x03 > #define BT_BAP_BCAST_SINK 0x04 > +#define BT_BAP_SIMULATED_BCAST_SINK 0x05 > > #define BT_BAP_STREAM_TYPE_UCAST 0x01 > #define BT_BAP_STREAM_TYPE_BCAST 0x02 > -- > 2.39.2 >
Hi Luiz, > -----Original Message----- > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> > Sent: Friday, October 13, 2023 8:30 PM > To: Claudia Cristina Draghicescu <claudia.rosu@nxp.com> > Cc: linux-bluetooth@vger.kernel.org; Iulia Tanasescu > <iulia.tanasescu@nxp.com>; Mihai-Octavian Urzica <mihai- > octavian.urzica@nxp.com>; Silviu Florian Barbulescu > <silviu.barbulescu@nxp.com>; Vlad Pruteanu <vlad.pruteanu@nxp.com>; > Andrei Istodorescu <andrei.istodorescu@nxp.com> > Subject: [EXT] Re: [PATCH BlueZ 1/2] bap: Fix source+sink endpoint > registration > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report > this email' button > > > Hi Claudia, > > On Fri, Oct 13, 2023 at 3:06 AM Claudia Draghicescu <claudia.rosu@nxp.com> > wrote: > > > > Create new endpoint name for the simulated broadcast sink that is > > created when registering a broadcast source endpoint. > > This removes the ambiguity when having registered a local broadcast > > sink and fixes the situation when multiple remote endpoints are > > created when discovering a broadcast source. > > > > --- > > src/shared/bap.c | 54 > > +++++++++++++++++++++++++++--------------------- > > src/shared/bap.h | 1 + > > 2 files changed, 32 insertions(+), 23 deletions(-) > > > > diff --git a/src/shared/bap.c b/src/shared/bap.c index > > 925501c48..266116235 100644 > > --- a/src/shared/bap.c > > +++ b/src/shared/bap.c > > @@ -644,7 +644,7 @@ static struct bt_bap_endpoint > *bap_endpoint_new_broadcast(struct bt_bap_db *bdb, > > if (type == BT_BAP_BCAST_SINK) > > ep->dir = BT_BAP_BCAST_SOURCE; > > else > > - ep->dir = BT_BAP_BCAST_SINK; > > + ep->dir = BT_BAP_SIMULATED_BCAST_SINK; > > > > return ep; > > } > > @@ -1500,12 +1500,12 @@ static void ep_config_cb(struct bt_bap_stream > *stream, int err) > > return; > > > > if (bt_bap_stream_get_type(stream) == > BT_BAP_STREAM_TYPE_BCAST) { > > - if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SINK) > > + if (bt_bap_stream_io_dir(stream) == > > + BT_BAP_SIMULATED_BCAST_SINK) > > stream_set_state_broadcast(stream, > > - BT_BAP_STREAM_STATE_QOS); > > + BT_BAP_STREAM_STATE_QOS); > > else if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SOURCE) > > stream_set_state_broadcast(stream, > > - BT_BAP_STREAM_STATE_CONFIG); > > + BT_BAP_STREAM_STATE_CONFIG); > > return; > > } > > > > @@ -2710,15 +2710,15 @@ struct bt_bap_pac > *bt_bap_add_vendor_pac(struct gatt_db *db, > > break; > > case BT_BAP_BCAST_SOURCE: > > bap_add_broadcast_source(pac); > > - if (queue_isempty(bdb->broadcast_sinks)) { > > - /* When adding a local broadcast source, add also a > > - * local broadcast sink > > - */ > > - pac_broadcast_sink = bap_pac_new(bdb, name, > > - BT_BAP_BCAST_SINK, &codec, qos, > > - data, metadata); > > - bap_add_broadcast_sink(pac_broadcast_sink); > > - } > > + /* When adding a local broadcast source, add also a > > + * local broadcast sink > > + */ > > + pac_broadcast_sink = bap_pac_new(bdb, name, > > + BT_BAP_SIMULATED_BCAST_SINK, &codec, qos, > > + data, metadata); > > I'm not really sure why this is needed to begin with, if that is to have a > remote endpoint I'd say we need to change the logic so broadcast src role is > allowed to create streams without a remote endpoint, anyway we should > probably have better documentation around this logic. I agree that a broadcast source should be allowed to create an endpoint without a remote pac. We can try this approach and submit a new patch. Just to be sure, when you say "remote endpoint" do you mean the remote pac or other bap ep structure? > > > + bap_add_broadcast_sink(pac_broadcast_sink); > > + queue_foreach(sessions, notify_session_pac_added, > pac_broadcast_sink); > > + return pac; > > break; > > case BT_BAP_BCAST_SINK: > > bap_add_broadcast_sink(pac); @@ -4457,13 +4457,23 @@ > > static void bap_foreach_pac(struct queue *l, struct queue *r, > > for (er = queue_get_entries(r); er; er = er->next) { > > struct bt_bap_pac *rpac = er->data; > > > > + if ((lpac->type == BT_BAP_BCAST_SOURCE) > > + && (rpac->type != BT_BAP_SIMULATED_BCAST_SINK)) > > + continue; > > + if ((rpac->type == BT_BAP_SIMULATED_BCAST_SINK) > > + && (lpac->type == BT_BAP_BCAST_SOURCE)) { > > + func(lpac, rpac, user_data); > > + return; > > + } > > + > > /* Skip checking codec for bcast source, > > * it will be checked when BASE info are received > > */ > > if ((rpac->type != BT_BAP_BCAST_SOURCE) && > > (!bap_codec_equal(&lpac->codec, &rpac->codec))) > > continue; > > - > > + if (rpac->type == BT_BAP_SIMULATED_BCAST_SINK) > > + continue; > > if (!func(lpac, rpac, user_data)) > > return; > > } > > @@ -4484,12 +4494,6 @@ void bt_bap_foreach_pac(struct bt_bap *bap, > uint8_t type, > > return bap_foreach_pac(bap->ldb->sinks, bap->rdb->sources, > > func, user_data); > > case BT_BAP_BCAST_SOURCE: > > - if (queue_isempty(bap->rdb->broadcast_sources) > > - && queue_isempty(bap->rdb->broadcast_sinks)) > > - return bap_foreach_pac(bap->ldb->broadcast_sources, > > - bap->ldb->broadcast_sinks, > > - func, user_data); > > - > > return bap_foreach_pac(bap->ldb->broadcast_sinks, > > bap->rdb->broadcast_sources, > > func, user_data); @@ -4497,6 > > +4501,10 @@ void bt_bap_foreach_pac(struct bt_bap *bap, uint8_t type, > > return bap_foreach_pac(bap->ldb->broadcast_sinks, > > bap->rdb->broadcast_sources, > > func, user_data); > > + case BT_BAP_SIMULATED_BCAST_SINK: > > + return bap_foreach_pac(bap->ldb->broadcast_sources, > > + bap->ldb->broadcast_sinks, > > + func, user_data); > > } > > } > > > > @@ -4927,12 +4935,12 @@ unsigned int bt_bap_stream_enable(struct > bt_bap_stream *stream, > > queue_foreach(stream->links, bap_stream_enable_link, > metadata); > > break; > > case BT_BAP_STREAM_TYPE_BCAST: > > - if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SINK) > > + if (bt_bap_stream_io_dir(stream) == > > + BT_BAP_SIMULATED_BCAST_SINK) > > stream_set_state_broadcast(stream, > > - BT_BAP_STREAM_STATE_CONFIG); > > + BT_BAP_STREAM_STATE_CONFIG); > > else if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SOURCE) > > stream_set_state_broadcast(stream, > > - BT_BAP_STREAM_STATE_STREAMING); > > + BT_BAP_STREAM_STATE_STREAMING); > > > > return 1; > > } > > diff --git a/src/shared/bap.h b/src/shared/bap.h index > > ebe4dbf7d..af3b6be71 100644 > > --- a/src/shared/bap.h > > +++ b/src/shared/bap.h > > @@ -19,6 +19,7 @@ > > #define BT_BAP_SOURCE 0x02 > > #define BT_BAP_BCAST_SOURCE 0x03 > > #define BT_BAP_BCAST_SINK 0x04 > > +#define BT_BAP_SIMULATED_BCAST_SINK 0x05 > > > > #define BT_BAP_STREAM_TYPE_UCAST 0x01 > > #define BT_BAP_STREAM_TYPE_BCAST 0x02 > > -- > > 2.39.2 > > > > > -- > Luiz Augusto von Dentz Regards, Claudia
diff --git a/src/shared/bap.c b/src/shared/bap.c index 925501c48..266116235 100644 --- a/src/shared/bap.c +++ b/src/shared/bap.c @@ -644,7 +644,7 @@ static struct bt_bap_endpoint *bap_endpoint_new_broadcast(struct bt_bap_db *bdb, if (type == BT_BAP_BCAST_SINK) ep->dir = BT_BAP_BCAST_SOURCE; else - ep->dir = BT_BAP_BCAST_SINK; + ep->dir = BT_BAP_SIMULATED_BCAST_SINK; return ep; } @@ -1500,12 +1500,12 @@ static void ep_config_cb(struct bt_bap_stream *stream, int err) return; if (bt_bap_stream_get_type(stream) == BT_BAP_STREAM_TYPE_BCAST) { - if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SINK) + if (bt_bap_stream_io_dir(stream) == BT_BAP_SIMULATED_BCAST_SINK) stream_set_state_broadcast(stream, - BT_BAP_STREAM_STATE_QOS); + BT_BAP_STREAM_STATE_QOS); else if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SOURCE) stream_set_state_broadcast(stream, - BT_BAP_STREAM_STATE_CONFIG); + BT_BAP_STREAM_STATE_CONFIG); return; } @@ -2710,15 +2710,15 @@ struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db, break; case BT_BAP_BCAST_SOURCE: bap_add_broadcast_source(pac); - if (queue_isempty(bdb->broadcast_sinks)) { - /* When adding a local broadcast source, add also a - * local broadcast sink - */ - pac_broadcast_sink = bap_pac_new(bdb, name, - BT_BAP_BCAST_SINK, &codec, qos, - data, metadata); - bap_add_broadcast_sink(pac_broadcast_sink); - } + /* When adding a local broadcast source, add also a + * local broadcast sink + */ + pac_broadcast_sink = bap_pac_new(bdb, name, + BT_BAP_SIMULATED_BCAST_SINK, &codec, qos, + data, metadata); + bap_add_broadcast_sink(pac_broadcast_sink); + queue_foreach(sessions, notify_session_pac_added, pac_broadcast_sink); + return pac; break; case BT_BAP_BCAST_SINK: bap_add_broadcast_sink(pac); @@ -4457,13 +4457,23 @@ static void bap_foreach_pac(struct queue *l, struct queue *r, for (er = queue_get_entries(r); er; er = er->next) { struct bt_bap_pac *rpac = er->data; + if ((lpac->type == BT_BAP_BCAST_SOURCE) + && (rpac->type != BT_BAP_SIMULATED_BCAST_SINK)) + continue; + if ((rpac->type == BT_BAP_SIMULATED_BCAST_SINK) + && (lpac->type == BT_BAP_BCAST_SOURCE)) { + func(lpac, rpac, user_data); + return; + } + /* Skip checking codec for bcast source, * it will be checked when BASE info are received */ if ((rpac->type != BT_BAP_BCAST_SOURCE) && (!bap_codec_equal(&lpac->codec, &rpac->codec))) continue; - + if (rpac->type == BT_BAP_SIMULATED_BCAST_SINK) + continue; if (!func(lpac, rpac, user_data)) return; } @@ -4484,12 +4494,6 @@ void bt_bap_foreach_pac(struct bt_bap *bap, uint8_t type, return bap_foreach_pac(bap->ldb->sinks, bap->rdb->sources, func, user_data); case BT_BAP_BCAST_SOURCE: - if (queue_isempty(bap->rdb->broadcast_sources) - && queue_isempty(bap->rdb->broadcast_sinks)) - return bap_foreach_pac(bap->ldb->broadcast_sources, - bap->ldb->broadcast_sinks, - func, user_data); - return bap_foreach_pac(bap->ldb->broadcast_sinks, bap->rdb->broadcast_sources, func, user_data); @@ -4497,6 +4501,10 @@ void bt_bap_foreach_pac(struct bt_bap *bap, uint8_t type, return bap_foreach_pac(bap->ldb->broadcast_sinks, bap->rdb->broadcast_sources, func, user_data); + case BT_BAP_SIMULATED_BCAST_SINK: + return bap_foreach_pac(bap->ldb->broadcast_sources, + bap->ldb->broadcast_sinks, + func, user_data); } } @@ -4927,12 +4935,12 @@ unsigned int bt_bap_stream_enable(struct bt_bap_stream *stream, queue_foreach(stream->links, bap_stream_enable_link, metadata); break; case BT_BAP_STREAM_TYPE_BCAST: - if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SINK) + if (bt_bap_stream_io_dir(stream) == BT_BAP_SIMULATED_BCAST_SINK) stream_set_state_broadcast(stream, - BT_BAP_STREAM_STATE_CONFIG); + BT_BAP_STREAM_STATE_CONFIG); else if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SOURCE) stream_set_state_broadcast(stream, - BT_BAP_STREAM_STATE_STREAMING); + BT_BAP_STREAM_STATE_STREAMING); return 1; } diff --git a/src/shared/bap.h b/src/shared/bap.h index ebe4dbf7d..af3b6be71 100644 --- a/src/shared/bap.h +++ b/src/shared/bap.h @@ -19,6 +19,7 @@ #define BT_BAP_SOURCE 0x02 #define BT_BAP_BCAST_SOURCE 0x03 #define BT_BAP_BCAST_SINK 0x04 +#define BT_BAP_SIMULATED_BCAST_SINK 0x05 #define BT_BAP_STREAM_TYPE_UCAST 0x01 #define BT_BAP_STREAM_TYPE_BCAST 0x02