Re: [PATCH v4 1/2] Bluetooth: Implement support for Mesh

["Gix, Brian" <brian.gix@intel.com>]

Hi Luiz,

On Wed, 2022-05-11 at 14:08 -0700, Luiz Augusto von Dentz wrote:
> Hi Brian,
> 
> On Wed, May 11, 2022 at 8:54 AM Brian Gix <brian.gix@intel.com>
> wrote:
> > [...]

> > 
> > +/* Use ext advertising if supported and not running Mesh */
> > +#define use_ext_adv(dev) (ext_adv_capable(dev) && \
> > +                         !hci_dev_test_flag(dev, HCI_MESH))
> 
> Im not really following why you don't want to use EA with MESH, afaik
> that work perfectly fine with either one besides there is actually an
> added benefit since we can use a dedicated instance/set for mesh so
> it
> can coexist with other instances, and even on controllers that does
> not support it we can still use the software based rotation using
> adv_info instances.

The current mesh spec only uses standard LE Advertising packets. I
haven't been able to test with any controllers that support Extended
Advertising for outbound advertisements, so I am unsure if they would
work or not for Mesh packets.

If a requested advertisement fits within a legacy advertisement, will a
newer controller still send it out as a Legacy ADV?

[...]

> >  
> >         hci_dev_unlock(hdev);
> > diff --git a/net/bluetooth/hci_request.c
> > b/net/bluetooth/hci_request.c
> > index f4afe482e300..261e71581c57 100644
> > --- a/net/bluetooth/hci_request.c
> > +++ b/net/bluetooth/hci_request.c
> 
> We shouldn't be using hci_request.c anymore, so if you are changing
> it
> because this code is actually being used for mesh than we have to
> convert the code using it to use hci_sync.c instead.
> 

I do not know if anyone is still using hci_request. However, incoming
ADV filtering absolutely breaks Mesh implimentations, especially during
Provisioning. It seems prudent to me to keep Filtering disabled if mesh
is being used on the controller, regardless of why or where
HCI_OP_SET_SCAN_ENABLE is being called from.  That said, I would
happily get rid of the entire hci_req_start_scan() function if
possible.

[...]

> > +int hci_mesh_send_sync(struct hci_dev *hdev, bdaddr_t *bdaddr, u8
> > bdaddr_type,
> > +                      u8 *data, u8 len)
> > +{
> > +       struct hci_cp_le_set_adv_data cp_data;
> > +       struct hci_cp_le_set_adv_param cp_param;
> > +       u8 own_addr_type, enable;
> > +       int err;
> > +
> > +       memset(&cp_data, 0, sizeof(cp_data));
> > +       cp_data.length = len + 1;
> > +       cp_data.data[0] = len;
> > +       memcpy(cp_data.data + 1, data, len);
> > +
> > +       hci_update_random_address_sync(hdev, true, false,
> > &own_addr_type);
> > +
> > +       memset(&cp_param, 0, sizeof(cp_param));
> > +       cp_param.type = LE_ADV_NONCONN_IND;
> > +       cp_param.min_interval =
> > cpu_to_le16(DISCOV_LE_ADV_MESH_MIN);
> > +       cp_param.max_interval =
> > cpu_to_le16(DISCOV_LE_ADV_MESH_MAX);
> > +       cp_param.own_address_type = 1;
> > +       cp_param.channel_map = 7;
> > +       cp_param.filter_policy = 3;
> > +
>  which > +       __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_ADV_PARAM,
> > +                             sizeof(cp_param), &cp_param,
> > HCI_CMD_TIMEOUT);
> 
> We should be using the hci_adv_info instead so we have a dedicated
> instance for mesh rather than using HCI_OP_LE_SET_ADV_PARAM which btw
> will fail if there is already anything using the extended version of
> these commands.
> 
> Perhaps you should something like:
> 
> https://patchwork.kernel.org/project/bluetooth/patch/20220506215743.3870212-5-luiz.dentz@gmail.com/
> 
> Ive introduced hci_add_per_instance so perhaps we could have
> something
> like hci_add_mesh_instance and then have a flag indicating it is for
> mesh so you can lookup for existing instance when you want to send
> the
> next packet you just update its data.
> 
> 

I will take a closer look at this.  Part of my opinion will be based on
what you can tell me about the behavior of Extended Advertising when we
want to send Legacy ADVs. There are a couple things that are possibly
unique about Mesh ADVs, such as timing and fine control over (the
usually but not always Random) BDADDR.

--Brian