Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
From:   Claudia Draghicescu <claudia.rosu@nxp.com>
To:     luiz.dentz@gmail.com
Cc:     andrei.istodorescu@nxp.com, claudia.rosu@nxp.com,
        iulia.tanasescu@nxp.com, linux-bluetooth@vger.kernel.org,
        mihai-octavian.urzica@nxp.com, silviu.barbulescu@nxp.com,
        vlad.pruteanu@nxp.com
Subject: Re: [PATCH BlueZ 1/2] bap: Fix source+sink endpoint registration
Date:   Mon, 16 Oct 2023 16:27:01 +0300
Message-Id: <20231016132701.4793-1-claudia.rosu@nxp.com>
In-Reply-To: <CABBYNZL1UJkH8_bUv-qRW9TZSAwhrw29_-R5xjnz9B5b+EO5ng@mail.gmail.com>
References: <CABBYNZL1UJkH8_bUv-qRW9TZSAwhrw29_-R5xjnz9B5b+EO5ng@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZCtTVktJMGZkN0JjSHBOOHQvNExIdFo5M0ZTcmpkdjcyRkNvZTBkVnpoYmJX?=
 =?utf-8?B?d3oxK2hHalBqVTJCdGFTY1BDQU1pdXpGQlhHdUI3WVYwaUtDSWtXdEJMbUlE?=
 =?utf-8?B?czFsN0FHVFpmbmxJZy9XS1RjMi9hRENLZlRYSWxlVFRuZ2hCbkkrNG5xZUl6?=
 =?utf-8?B?OGlld2lMaWtBMlVWS2ZqeHJnYlI0QjZEb2srWWoyYjBzK1lXdHM3VlFFTWUx?=
 =?utf-8?B?bVFoNXJBRVpxeTMrYlVyTVZuUHRGbU1DWm5mclZ5YTBJaGUwaTBFc3FmblJR?=
 =?utf-8?B?cHhTRkZmZDY4Uk1oL210b04vVjR1R3Q5Um1OZ1ZMZTc4QkcyMVRTWmdhbjBl?=
 =?utf-8?B?MFRCL2VFOXZzbHZ2SjllVEdDSmJJaWxwK2JCMGtkc2JOU0xOeTUycmd3REkv?=
 =?utf-8?B?V3dLWUl1Wjh2YzYvRE1WbjNJdXlmR0U2anFHdTMxS0RISGt1R3hRRjJpOEIz?=
 =?utf-8?B?MlZwdDRDZ2Y1NGhmaDhXc1d3S0VsVTI2Q1hDcTYvT1BOZkR3RjNGOU8zYmFN?=
 =?utf-8?B?QTV2TDdrcUNJTDdZZjc5dCtCb0FYejZXaWhtTWliaXFNNXd5N0RoRitDQm0w?=
 =?utf-8?B?SGFRSEs3N2x2NkJyMFJLREsvekRDcURNSDdnbXlBTWdYcXMyZVN3NFV3UzY0?=
 =?utf-8?B?QTVKdHJ5RmxqR29kU0R6d3FTSnVBZytiUGdOcmxLbm9LNGpxRy82aDNleEpx?=
 =?utf-8?B?azhEbGtGRDZxaXR2OHBQclUrVVJnRHQ1T0hyeTRiVk9DVzlSejRoWllLaXRC?=
 =?utf-8?B?MGRCK2xmeVdkN3AxM0JHRUlCWHZWYXVyYWNSQS9GdVRpVkpKbTZsNzNtcUw2?=
 =?utf-8?B?cnJwbFYrY3pvL293bVJrMFR1cWtoUUVMT3J4YXJmTjVrL3BpY05OOTRMOHFC?=
 =?utf-8?B?dVE0cGFuT09HYVFKWm8wK2dzZ0t3UHVMbVZnQ2VIbVNURDFLemRiV1k1QVY5?=
 =?utf-8?B?eUpKazFZaVdmeDBBay9wNEJBdXFqNllJQnRQZTlrMHQ2T0JJUExoQVZxYVB1?=
 =?utf-8?B?b1pOWFIrVFIwT1orUDlML3FXUlQ5NStGMUpvMUJObzhYSXFLVnpaSlNIMHpn?=
 =?utf-8?B?QkxtREVtNUhod3hPbDI1bk1MSU1tRnZxMFUxZG16OEZwazdycUh2bmF1b2NY?=
 =?utf-8?B?Y0VnMWhkSTYzZXRUNVhTL1JRamdiMkxKT056SEViRUd2M21zQ0o4S0J5OFk5?=
 =?utf-8?B?Zk1MSmFxVUZQdE1BMEorOHh3VU1zZXhMRit5ZFloWUFRN0t4RWYzeFlIZjFi?=
 =?utf-8?B?M1YycEc0VWdpeDExMjhlZ05lRGdSWE1JR0JEdnNjR05ET1R6T1VuL2xPWGtY?=
 =?utf-8?B?b3pqbXJUbmUzL1VLdmE2aWZiV21PcmtpOU9ESEFDZExvR1N0cU85eExKSnkv?=
 =?utf-8?B?Y3FqWVh4TVpHVkJOdGQwaytxclVCUkU5V05pOHJpZUFWN1VuTHI5cVFuQ0Q1?=
 =?utf-8?B?U1BDNlM5WE1IZWxhYk9jTlFKY1FWTjVhZGZDNjg5dUdmMTNjamVCKzM0YUdl?=
 =?utf-8?B?NDQ5S0lXRVdzZFR0K2dGSHhoTGF4K2liUkgxeVhHT0JOUFRkUmVwd0EzLzVp?=
 =?utf-8?B?d29lZStTTDFSVzlmbmhXQ2c3TFJjSVExUVRzREtQdU11VWxvVi83NTNJSnFs?=
 =?utf-8?B?OXlIM1dCOThvU21pSGQvT2lsZ3RMdEw0cVFmUWpDc1lkWDBjdXdwRmxVblE1?=
 =?utf-8?B?OFJZQmVvUUhObFVMbGJLaWluenEyYmFVQWt5UGRBZVNWcjhETXBtdzhoWEFF?=
 =?utf-8?B?L0lIOGtDb3JyLzJZNzB3UGZaV2dVWndrSEdqcXFHei9iR1ArY0xiYVZWWnc5?=
 =?utf-8?B?VXhVbDdQVkRTaHpVbms4by9LQ1R5ZXhEQ3FjaTB1Ty9XekVKZGRISFVHdWtk?=
 =?utf-8?B?S3ovSnVSb0cxOW9POFFDTWdMUzg1ejgzRzl5WVNkTU51RFZyOVVwZW0wcU8r?=
 =?utf-8?B?MFNyeVF5RGYyRDZhUWxGbW14dnM0dWZ2aDBEYXNEUEYxZGtFUVBzUWtvWmJL?=
 =?utf-8?B?dWZ5RG1ibzkrZnlneGJnU3VXZ1hCV1d2Nnh0VGhGZDIxOXk2RmNGM1hXUGFU?=
 =?utf-8?B?a3ZDYjY1ckpETktESjU1L1JVem1iRGxxNTRSdDcyazArMnFVc1BHOXRtdkhM?=
 =?utf-8?Q?GIkxmIN/MZhdcmxQP/klp2Jfl?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 05db1052-fc78-497f-db43-08dbce4b95da
X-MS-Exchange-CrossTenant-AuthSource: GV1PR04MB9151.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Oct 2023 13:27:03.7613
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: zaq00KSPaMB3Crb0FeP/8W70uunlq+eeqmhLELVY4WeoONVRrmVksT263+9OoH6F+uVJCzjCgsd8rqmoQTpabQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR04MB9115
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email
Precedence: bulk
List-ID: <linux-bluetooth.vger.kernel.org>
X-Mailing-List: linux-bluetooth@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Mon, 16 Oct 2023 06:27:11 -0700 (PDT)

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