Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CABBYNZJTvEFAESfLqebuUrq6mUAw1ZcQbSMqpfgbMLkGbH_k8A@mail.gmail.com>
 <20230322152347.42109-1-iulia.tanasescu@nxp.com>
In-Reply-To: <20230322152347.42109-1-iulia.tanasescu@nxp.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 22 Mar 2023 10:38:24 -0700
Message-ID: <CABBYNZJfkdbXyJUfg-i88Uorj5rp97ZpbXcRZ2oGt8PmW4tgRw@mail.gmail.com>
Subject: Re: [PATCH 1/1] Bluetooth: Split bt_iso_qos into dedicated structures
To:     Iulia Tanasescu <iulia.tanasescu@nxp.com>
Cc:     linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Iulia,

On Wed, Mar 22, 2023 at 8:24=E2=80=AFAM Iulia Tanasescu <iulia.tanasescu@nx=
p.com> wrote:
>
> Hi Luiz,
>
> I have investigated some possible ways to update my patch using your
> suggestions.
>
> I think the most convenient method would be to implement the structure
> as an union of dedicated parameters and to add an additional parameter
> that would indicate the type of QoS the structure is holding.
>
> It would be something like this:
>
> struct bt_iso_bcast_qos {
>     union {
>         struct bt_iso_bcast_src_qos bsrc;
>         struct bt_iso_bcast_snk_qos bsnk;
>     };
> };
>
> enum {
>     BT_ISO_UCAST_QOS,
>     BT_ISO_BCAST_SRC_QOS,
>     BT_ISO_BCAST_SNK_QOS,
> };

We don't really need a type like that because the socket can already
tell what type it is based on the address:

https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.gi=
t/tree/net/bluetooth/iso.c#n853

> struct bt_iso_qos {
>     int qos_type;
>     union {
>         struct bt_iso_ucast_qos ucast;
>         struct bt_iso_bcast_qos bcast;
>     };
> };
>
> The flow would be something like this:
>
> At socket creation, some default unicast QoS parameters are loaded
> in the qos field of the socket structure.
>
> When the "setsockopt" function is called on an ISO socket from user
> space, the user will provide a bt_iso_qos structure as defined above,
> containing the type of QoS to set and the desired parameters.
> The kernel will validate the parameters depending on their type, and
> it will overwrite the unicast defaults if the check is succesful.
>
> When the user calls other ISO socket APIs, like connect or listen,
> and the procedures to execute are broadcast related, the kernel will
> either use the QoS options that had been previously set by the user,
> or, if the user did not set any options, the unicast defaults will
> be replaced with broadcast defaults, and the procedures will start
> this way.

In case of iso_sock_alloc being called from parent you might be able
to allocate it with proper defaults since you know what address was
used in the parent thus can detect if it is a unicast or broadcast,
also you may need to detect if BT_ISO_QOS is used before bind we can
either reject it or just copy as is but then check_qos shall be run on
bind/connect since only at that point we know the destination address.

> Do you think this is a good design? Please let me know if I should
> proceed with this implementation.

Yep, apart from the comments above.

> Regards,
> Iulia
>


--=20
Luiz Augusto von Dentz