* [Bluez PATCH v2 1/2] device: add device_remove_bonding function @ 2020-06-27 15:54 Archie Pusaka 2020-06-27 15:54 ` [Bluez PATCH v2 2/2] input: Remove bonding info when receiving virtual cable unplug Archie Pusaka 2020-07-07 18:02 ` [Bluez PATCH v2 1/2] device: add device_remove_bonding function Luiz Augusto von Dentz 0 siblings, 2 replies; 12+ messages in thread From: Archie Pusaka @ 2020-06-27 15:54 UTC (permalink / raw) To: linux-bluetooth, Luiz Augusto von Dentz; +Cc: Archie Pusaka, Alain Michaud From: Archie Pusaka <apusaka@chromium.org> This patch splits the "bonding removal" function in device.c, because we need to remove bonding information when receiving "virtual cable unplug" in HID profile. Reviewed-by: Alain Michaud <alainm@chromium.org> --- Changes in v2: None src/device.c | 25 +++++++++++++++---------- src/device.h | 1 + 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/device.c b/src/device.c index 7b0eb256e..9fb0e018c 100644 --- a/src/device.c +++ b/src/device.c @@ -4162,6 +4162,17 @@ static void delete_folder_tree(const char *dirname) rmdir(dirname); } +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type) +{ + if (bdaddr_type == BDADDR_BREDR) + device->bredr_state.bonded = false; + else + device->le_state.bonded = false; + + btd_adapter_remove_bonding(device->adapter, &device->bdaddr, + bdaddr_type); +} + static void device_remove_stored(struct btd_device *device) { char device_addr[18]; @@ -4170,17 +4181,11 @@ static void device_remove_stored(struct btd_device *device) char *data; gsize length = 0; - if (device->bredr_state.bonded) { - device->bredr_state.bonded = false; - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, - BDADDR_BREDR); - } + if (device->bredr_state.bonded) + device_remove_bonding(device, BDADDR_BREDR); - if (device->le_state.bonded) { - device->le_state.bonded = false; - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, - device->bdaddr_type); - } + if (device->le_state.bonded) + device_remove_bonding(device, device->bdaddr_type); device->bredr_state.paired = false; device->le_state.paired = false; diff --git a/src/device.h b/src/device.h index 06b100499..907c7c5c4 100644 --- a/src/device.h +++ b/src/device.h @@ -49,6 +49,7 @@ uint16_t btd_device_get_vendor(struct btd_device *device); uint16_t btd_device_get_vendor_src(struct btd_device *device); uint16_t btd_device_get_product(struct btd_device *device); uint16_t btd_device_get_version(struct btd_device *device); +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type); void device_remove(struct btd_device *device, gboolean remove_stored); int device_address_cmp(gconstpointer a, gconstpointer b); int device_bdaddr_cmp(gconstpointer a, gconstpointer b); -- 2.27.0.212.ge8ba1cc988-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Bluez PATCH v2 2/2] input: Remove bonding info when receiving virtual cable unplug 2020-06-27 15:54 [Bluez PATCH v2 1/2] device: add device_remove_bonding function Archie Pusaka @ 2020-06-27 15:54 ` Archie Pusaka 2020-07-07 9:34 ` Archie Pusaka 2020-07-07 18:02 ` [Bluez PATCH v2 1/2] device: add device_remove_bonding function Luiz Augusto von Dentz 1 sibling, 1 reply; 12+ messages in thread From: Archie Pusaka @ 2020-06-27 15:54 UTC (permalink / raw) To: linux-bluetooth, Luiz Augusto von Dentz; +Cc: Archie Pusaka, Alain Michaud From: Archie Pusaka <apusaka@chromium.org> From Bluetooth HID Profile 1.1 Spec: If a Virtual Cable is unplugged via a HID control Virtual Unplug command, then both the Bluetooth HID device and Bluetooth HID Host shall destroy or invalidate all Bluetooth bonding and Virtual Cable information that was previously stored in persistent memory for the respective Virtually Cabled devices and hosts. This patch removes the bonding information upon receiving and/or sending a "virtual cable unplug". Reviewed-by: Alain Michaud <alainm@chromium.org> --- Changes in v2: - Properly pass the correct argument to device_remove_bonding - rename unbond_on_disconnect to virtual_cable_unplug profiles/input/device.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/profiles/input/device.c b/profiles/input/device.c index d3724ed54..a76ab90bd 100644 --- a/profiles/input/device.c +++ b/profiles/input/device.c @@ -88,6 +88,7 @@ struct input_device { uint8_t report_req_pending; guint report_req_timer; uint32_t report_rsp_id; + bool virtual_cable_unplug; }; static int idle_timeout = 0; @@ -148,6 +149,14 @@ static void input_device_free(struct input_device *idev) g_free(idev); } +static void virtual_cable_unplug(struct input_device *idev) +{ + device_remove_bonding(idev->device, + btd_device_get_bdaddr_type(idev->device)); + + idev->virtual_cable_unplug = false; +} + static bool hidp_send_message(GIOChannel *chan, uint8_t hdr, const uint8_t *data, size_t size) { @@ -188,6 +197,9 @@ static bool hidp_send_message(GIOChannel *chan, uint8_t hdr, static bool hidp_send_ctrl_message(struct input_device *idev, uint8_t hdr, const uint8_t *data, size_t size) { + if (hdr == (HIDP_TRANS_HID_CONTROL | HIDP_CTRL_VIRTUAL_CABLE_UNPLUG)) + idev->virtual_cable_unplug = true; + return hidp_send_message(idev->ctrl_io, hdr, data, size); } @@ -344,6 +356,9 @@ static gboolean intr_watch_cb(GIOChannel *chan, GIOCondition cond, gpointer data /* Enter the auto-reconnect mode if needed */ input_device_enter_reconnect_mode(idev); + if (!idev->ctrl_io && idev->virtual_cable_unplug) + virtual_cable_unplug(idev); + return FALSE; } @@ -408,7 +423,7 @@ static void hidp_recv_ctrl_hid_control(struct input_device *idev, uint8_t param) DBG(""); if (param == HIDP_CTRL_VIRTUAL_CABLE_UNPLUG) - connection_disconnect(idev, 0); + connection_disconnect(idev, (1 << HIDP_VIRTUAL_CABLE_UNPLUG)); } static void hidp_recv_ctrl_data(struct input_device *idev, uint8_t param, @@ -532,6 +547,9 @@ static gboolean ctrl_watch_cb(GIOChannel *chan, GIOCondition cond, gpointer data if (idev->intr_io && !(cond & G_IO_NVAL)) g_io_channel_shutdown(idev->intr_io, TRUE, NULL); + if (!idev->intr_io && idev->virtual_cable_unplug) + virtual_cable_unplug(idev); + return FALSE; } @@ -1042,6 +1060,9 @@ static int connection_disconnect(struct input_device *idev, uint32_t flags) shutdown(sock, SHUT_WR); } + if (flags & (1 << HIDP_VIRTUAL_CABLE_UNPLUG)) + idev->virtual_cable_unplug = true; + if (idev->uhid) return 0; else -- 2.27.0.212.ge8ba1cc988-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Bluez PATCH v2 2/2] input: Remove bonding info when receiving virtual cable unplug 2020-06-27 15:54 ` [Bluez PATCH v2 2/2] input: Remove bonding info when receiving virtual cable unplug Archie Pusaka @ 2020-07-07 9:34 ` Archie Pusaka 0 siblings, 0 replies; 12+ messages in thread From: Archie Pusaka @ 2020-07-07 9:34 UTC (permalink / raw) To: linux-bluetooth, Luiz Augusto von Dentz; +Cc: Archie Pusaka, Alain Michaud Hi Bluez maintainers, Could you take a look at this patch? Thanks, Archie On Sat, 27 Jun 2020 at 23:54, Archie Pusaka <apusaka@google.com> wrote: > > From: Archie Pusaka <apusaka@chromium.org> > > From Bluetooth HID Profile 1.1 Spec: If a Virtual Cable is > unplugged via a HID control Virtual Unplug command, then both the > Bluetooth HID device and Bluetooth HID Host shall destroy or > invalidate all Bluetooth bonding and Virtual Cable information > that was previously stored in persistent memory for the respective > Virtually Cabled devices and hosts. > > This patch removes the bonding information upon receiving and/or > sending a "virtual cable unplug". > > Reviewed-by: Alain Michaud <alainm@chromium.org> > --- > > Changes in v2: > - Properly pass the correct argument to device_remove_bonding > - rename unbond_on_disconnect to virtual_cable_unplug > > profiles/input/device.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/profiles/input/device.c b/profiles/input/device.c > index d3724ed54..a76ab90bd 100644 > --- a/profiles/input/device.c > +++ b/profiles/input/device.c > @@ -88,6 +88,7 @@ struct input_device { > uint8_t report_req_pending; > guint report_req_timer; > uint32_t report_rsp_id; > + bool virtual_cable_unplug; > }; > > static int idle_timeout = 0; > @@ -148,6 +149,14 @@ static void input_device_free(struct input_device *idev) > g_free(idev); > } > > +static void virtual_cable_unplug(struct input_device *idev) > +{ > + device_remove_bonding(idev->device, > + btd_device_get_bdaddr_type(idev->device)); > + > + idev->virtual_cable_unplug = false; > +} > + > static bool hidp_send_message(GIOChannel *chan, uint8_t hdr, > const uint8_t *data, size_t size) > { > @@ -188,6 +197,9 @@ static bool hidp_send_message(GIOChannel *chan, uint8_t hdr, > static bool hidp_send_ctrl_message(struct input_device *idev, uint8_t hdr, > const uint8_t *data, size_t size) > { > + if (hdr == (HIDP_TRANS_HID_CONTROL | HIDP_CTRL_VIRTUAL_CABLE_UNPLUG)) > + idev->virtual_cable_unplug = true; > + > return hidp_send_message(idev->ctrl_io, hdr, data, size); > } > > @@ -344,6 +356,9 @@ static gboolean intr_watch_cb(GIOChannel *chan, GIOCondition cond, gpointer data > /* Enter the auto-reconnect mode if needed */ > input_device_enter_reconnect_mode(idev); > > + if (!idev->ctrl_io && idev->virtual_cable_unplug) > + virtual_cable_unplug(idev); > + > return FALSE; > } > > @@ -408,7 +423,7 @@ static void hidp_recv_ctrl_hid_control(struct input_device *idev, uint8_t param) > DBG(""); > > if (param == HIDP_CTRL_VIRTUAL_CABLE_UNPLUG) > - connection_disconnect(idev, 0); > + connection_disconnect(idev, (1 << HIDP_VIRTUAL_CABLE_UNPLUG)); > } > > static void hidp_recv_ctrl_data(struct input_device *idev, uint8_t param, > @@ -532,6 +547,9 @@ static gboolean ctrl_watch_cb(GIOChannel *chan, GIOCondition cond, gpointer data > if (idev->intr_io && !(cond & G_IO_NVAL)) > g_io_channel_shutdown(idev->intr_io, TRUE, NULL); > > + if (!idev->intr_io && idev->virtual_cable_unplug) > + virtual_cable_unplug(idev); > + > return FALSE; > } > > @@ -1042,6 +1060,9 @@ static int connection_disconnect(struct input_device *idev, uint32_t flags) > shutdown(sock, SHUT_WR); > } > > + if (flags & (1 << HIDP_VIRTUAL_CABLE_UNPLUG)) > + idev->virtual_cable_unplug = true; > + > if (idev->uhid) > return 0; > else > -- > 2.27.0.212.ge8ba1cc988-goog > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Bluez PATCH v2 1/2] device: add device_remove_bonding function 2020-06-27 15:54 [Bluez PATCH v2 1/2] device: add device_remove_bonding function Archie Pusaka 2020-06-27 15:54 ` [Bluez PATCH v2 2/2] input: Remove bonding info when receiving virtual cable unplug Archie Pusaka @ 2020-07-07 18:02 ` Luiz Augusto von Dentz 2020-07-08 4:30 ` Archie Pusaka 1 sibling, 1 reply; 12+ messages in thread From: Luiz Augusto von Dentz @ 2020-07-07 18:02 UTC (permalink / raw) To: Archie Pusaka; +Cc: linux-bluetooth, Archie Pusaka, Alain Michaud Hi Archie, On Sat, Jun 27, 2020 at 8:54 AM Archie Pusaka <apusaka@google.com> wrote: > > From: Archie Pusaka <apusaka@chromium.org> > > This patch splits the "bonding removal" function in device.c, > because we need to remove bonding information when receiving > "virtual cable unplug" in HID profile. > > Reviewed-by: Alain Michaud <alainm@chromium.org> > --- > > Changes in v2: None > > src/device.c | 25 +++++++++++++++---------- > src/device.h | 1 + > 2 files changed, 16 insertions(+), 10 deletions(-) > > diff --git a/src/device.c b/src/device.c > index 7b0eb256e..9fb0e018c 100644 > --- a/src/device.c > +++ b/src/device.c > @@ -4162,6 +4162,17 @@ static void delete_folder_tree(const char *dirname) > rmdir(dirname); > } > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type) > +{ > + if (bdaddr_type == BDADDR_BREDR) > + device->bredr_state.bonded = false; > + else > + device->le_state.bonded = false; > + > + btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > + bdaddr_type); > +} > + > static void device_remove_stored(struct btd_device *device) > { > char device_addr[18]; > @@ -4170,17 +4181,11 @@ static void device_remove_stored(struct btd_device *device) > char *data; > gsize length = 0; > > - if (device->bredr_state.bonded) { > - device->bredr_state.bonded = false; > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > - BDADDR_BREDR); > - } > + if (device->bredr_state.bonded) > + device_remove_bonding(device, BDADDR_BREDR); > > - if (device->le_state.bonded) { > - device->le_state.bonded = false; > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > - device->bdaddr_type); > - } > + if (device->le_state.bonded) > + device_remove_bonding(device, device->bdaddr_type); > > device->bredr_state.paired = false; > device->le_state.paired = false; > diff --git a/src/device.h b/src/device.h > index 06b100499..907c7c5c4 100644 > --- a/src/device.h > +++ b/src/device.h > @@ -49,6 +49,7 @@ uint16_t btd_device_get_vendor(struct btd_device *device); > uint16_t btd_device_get_vendor_src(struct btd_device *device); > uint16_t btd_device_get_product(struct btd_device *device); > uint16_t btd_device_get_version(struct btd_device *device); > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type); > void device_remove(struct btd_device *device, gboolean remove_stored); Is there any particular reason why device_remove is not enough here? I don't see any reason to leave the device object around after removing its bonding. > int device_address_cmp(gconstpointer a, gconstpointer b); > int device_bdaddr_cmp(gconstpointer a, gconstpointer b); > -- > 2.27.0.212.ge8ba1cc988-goog > -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Bluez PATCH v2 1/2] device: add device_remove_bonding function 2020-07-07 18:02 ` [Bluez PATCH v2 1/2] device: add device_remove_bonding function Luiz Augusto von Dentz @ 2020-07-08 4:30 ` Archie Pusaka 2020-07-14 4:50 ` Archie Pusaka 2020-07-14 17:09 ` Luiz Augusto von Dentz 0 siblings, 2 replies; 12+ messages in thread From: Archie Pusaka @ 2020-07-08 4:30 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Archie Pusaka, Alain Michaud Hi Luiz, As far as the spec is concerned, we can also remove the device by calling device_remove. However, I suppose it would be confusing for end users if they can no longer find their HID device on the device list just because the device previously sent a virtual cable disconnection. The HID 1.0 spec part 6.4.2 also gives an example of a possible scenario when a virtually cabled device is removed: "Unplugged devices shall be marked as known and put into a “most recently used list” of known devices to facilitate future re-connecting". Thanks, Archie On Wed, 8 Jul 2020 at 02:03, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Archie, > > On Sat, Jun 27, 2020 at 8:54 AM Archie Pusaka <apusaka@google.com> wrote: > > > > From: Archie Pusaka <apusaka@chromium.org> > > > > This patch splits the "bonding removal" function in device.c, > > because we need to remove bonding information when receiving > > "virtual cable unplug" in HID profile. > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > --- > > > > Changes in v2: None > > > > src/device.c | 25 +++++++++++++++---------- > > src/device.h | 1 + > > 2 files changed, 16 insertions(+), 10 deletions(-) > > > > diff --git a/src/device.c b/src/device.c > > index 7b0eb256e..9fb0e018c 100644 > > --- a/src/device.c > > +++ b/src/device.c > > @@ -4162,6 +4162,17 @@ static void delete_folder_tree(const char *dirname) > > rmdir(dirname); > > } > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type) > > +{ > > + if (bdaddr_type == BDADDR_BREDR) > > + device->bredr_state.bonded = false; > > + else > > + device->le_state.bonded = false; > > + > > + btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > + bdaddr_type); > > +} > > + > > static void device_remove_stored(struct btd_device *device) > > { > > char device_addr[18]; > > @@ -4170,17 +4181,11 @@ static void device_remove_stored(struct btd_device *device) > > char *data; > > gsize length = 0; > > > > - if (device->bredr_state.bonded) { > > - device->bredr_state.bonded = false; > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > - BDADDR_BREDR); > > - } > > + if (device->bredr_state.bonded) > > + device_remove_bonding(device, BDADDR_BREDR); > > > > - if (device->le_state.bonded) { > > - device->le_state.bonded = false; > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > - device->bdaddr_type); > > - } > > + if (device->le_state.bonded) > > + device_remove_bonding(device, device->bdaddr_type); > > > > device->bredr_state.paired = false; > > device->le_state.paired = false; > > diff --git a/src/device.h b/src/device.h > > index 06b100499..907c7c5c4 100644 > > --- a/src/device.h > > +++ b/src/device.h > > @@ -49,6 +49,7 @@ uint16_t btd_device_get_vendor(struct btd_device *device); > > uint16_t btd_device_get_vendor_src(struct btd_device *device); > > uint16_t btd_device_get_product(struct btd_device *device); > > uint16_t btd_device_get_version(struct btd_device *device); > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type); > > void device_remove(struct btd_device *device, gboolean remove_stored); > > Is there any particular reason why device_remove is not enough here? I > don't see any reason to leave the device object around after removing > its bonding. > > > int device_address_cmp(gconstpointer a, gconstpointer b); > > int device_bdaddr_cmp(gconstpointer a, gconstpointer b); > > -- > > 2.27.0.212.ge8ba1cc988-goog > > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Bluez PATCH v2 1/2] device: add device_remove_bonding function 2020-07-08 4:30 ` Archie Pusaka @ 2020-07-14 4:50 ` Archie Pusaka 2020-07-14 17:09 ` Luiz Augusto von Dentz 1 sibling, 0 replies; 12+ messages in thread From: Archie Pusaka @ 2020-07-14 4:50 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Archie Pusaka, Alain Michaud [Sorry, re-sending in plain text] Hi Luiz, Can I have your opinion on this? Thanks, Archie On Wed, 8 Jul 2020 at 12:30, Archie Pusaka <apusaka@google.com> wrote: > > Hi Luiz, > > As far as the spec is concerned, we can also remove the device by > calling device_remove. However, I suppose it would be confusing for > end users if they can no longer find their HID device on the device > list just because the device previously sent a virtual cable > disconnection. > The HID 1.0 spec part 6.4.2 also gives an example of a possible > scenario when a virtually cabled device is removed: "Unplugged devices > shall be marked as known and put into a “most recently used list” of > known devices to facilitate future re-connecting". > > Thanks, > Archie > > > On Wed, 8 Jul 2020 at 02:03, Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Archie, > > > > On Sat, Jun 27, 2020 at 8:54 AM Archie Pusaka <apusaka@google.com> wrote: > > > > > > From: Archie Pusaka <apusaka@chromium.org> > > > > > > This patch splits the "bonding removal" function in device.c, > > > because we need to remove bonding information when receiving > > > "virtual cable unplug" in HID profile. > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > --- > > > > > > Changes in v2: None > > > > > > src/device.c | 25 +++++++++++++++---------- > > > src/device.h | 1 + > > > 2 files changed, 16 insertions(+), 10 deletions(-) > > > > > > diff --git a/src/device.c b/src/device.c > > > index 7b0eb256e..9fb0e018c 100644 > > > --- a/src/device.c > > > +++ b/src/device.c > > > @@ -4162,6 +4162,17 @@ static void delete_folder_tree(const char *dirname) > > > rmdir(dirname); > > > } > > > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type) > > > +{ > > > + if (bdaddr_type == BDADDR_BREDR) > > > + device->bredr_state.bonded = false; > > > + else > > > + device->le_state.bonded = false; > > > + > > > + btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > + bdaddr_type); > > > +} > > > + > > > static void device_remove_stored(struct btd_device *device) > > > { > > > char device_addr[18]; > > > @@ -4170,17 +4181,11 @@ static void device_remove_stored(struct btd_device *device) > > > char *data; > > > gsize length = 0; > > > > > > - if (device->bredr_state.bonded) { > > > - device->bredr_state.bonded = false; > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > - BDADDR_BREDR); > > > - } > > > + if (device->bredr_state.bonded) > > > + device_remove_bonding(device, BDADDR_BREDR); > > > > > > - if (device->le_state.bonded) { > > > - device->le_state.bonded = false; > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > - device->bdaddr_type); > > > - } > > > + if (device->le_state.bonded) > > > + device_remove_bonding(device, device->bdaddr_type); > > > > > > device->bredr_state.paired = false; > > > device->le_state.paired = false; > > > diff --git a/src/device.h b/src/device.h > > > index 06b100499..907c7c5c4 100644 > > > --- a/src/device.h > > > +++ b/src/device.h > > > @@ -49,6 +49,7 @@ uint16_t btd_device_get_vendor(struct btd_device *device); > > > uint16_t btd_device_get_vendor_src(struct btd_device *device); > > > uint16_t btd_device_get_product(struct btd_device *device); > > > uint16_t btd_device_get_version(struct btd_device *device); > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type); > > > void device_remove(struct btd_device *device, gboolean remove_stored); > > > > Is there any particular reason why device_remove is not enough here? I > > don't see any reason to leave the device object around after removing > > its bonding. > > > > > int device_address_cmp(gconstpointer a, gconstpointer b); > > > int device_bdaddr_cmp(gconstpointer a, gconstpointer b); > > > -- > > > 2.27.0.212.ge8ba1cc988-goog > > > > > > > > > -- > > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Bluez PATCH v2 1/2] device: add device_remove_bonding function 2020-07-08 4:30 ` Archie Pusaka 2020-07-14 4:50 ` Archie Pusaka @ 2020-07-14 17:09 ` Luiz Augusto von Dentz 2020-07-15 14:15 ` Archie Pusaka 1 sibling, 1 reply; 12+ messages in thread From: Luiz Augusto von Dentz @ 2020-07-14 17:09 UTC (permalink / raw) To: Archie Pusaka; +Cc: linux-bluetooth, Archie Pusaka, Alain Michaud Hi Archie, On Tue, Jul 7, 2020 at 9:30 PM Archie Pusaka <apusaka@google.com> wrote: > > Hi Luiz, > > As far as the spec is concerned, we can also remove the device by > calling device_remove. However, I suppose it would be confusing for > end users if they can no longer find their HID device on the device > list just because the device previously sent a virtual cable > disconnection. > The HID 1.0 spec part 6.4.2 also gives an example of a possible > scenario when a virtually cabled device is removed: "Unplugged devices > shall be marked as known and put into a “most recently used list” of > known devices to facilitate future re-connecting". Then perhaps we shall have it marked as temporary as well, that said we do want to introduce disappearing logic so temporary devices are not left dangling for too long. > Thanks, > Archie > > > On Wed, 8 Jul 2020 at 02:03, Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Archie, > > > > On Sat, Jun 27, 2020 at 8:54 AM Archie Pusaka <apusaka@google.com> wrote: > > > > > > From: Archie Pusaka <apusaka@chromium.org> > > > > > > This patch splits the "bonding removal" function in device.c, > > > because we need to remove bonding information when receiving > > > "virtual cable unplug" in HID profile. > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > --- > > > > > > Changes in v2: None > > > > > > src/device.c | 25 +++++++++++++++---------- > > > src/device.h | 1 + > > > 2 files changed, 16 insertions(+), 10 deletions(-) > > > > > > diff --git a/src/device.c b/src/device.c > > > index 7b0eb256e..9fb0e018c 100644 > > > --- a/src/device.c > > > +++ b/src/device.c > > > @@ -4162,6 +4162,17 @@ static void delete_folder_tree(const char *dirname) > > > rmdir(dirname); > > > } > > > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type) > > > +{ > > > + if (bdaddr_type == BDADDR_BREDR) > > > + device->bredr_state.bonded = false; > > > + else > > > + device->le_state.bonded = false; > > > + > > > + btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > + bdaddr_type); > > > +} > > > + > > > static void device_remove_stored(struct btd_device *device) > > > { > > > char device_addr[18]; > > > @@ -4170,17 +4181,11 @@ static void device_remove_stored(struct btd_device *device) > > > char *data; > > > gsize length = 0; > > > > > > - if (device->bredr_state.bonded) { > > > - device->bredr_state.bonded = false; > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > - BDADDR_BREDR); > > > - } > > > + if (device->bredr_state.bonded) > > > + device_remove_bonding(device, BDADDR_BREDR); > > > > > > - if (device->le_state.bonded) { > > > - device->le_state.bonded = false; > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > - device->bdaddr_type); > > > - } > > > + if (device->le_state.bonded) > > > + device_remove_bonding(device, device->bdaddr_type); > > > > > > device->bredr_state.paired = false; > > > device->le_state.paired = false; > > > diff --git a/src/device.h b/src/device.h > > > index 06b100499..907c7c5c4 100644 > > > --- a/src/device.h > > > +++ b/src/device.h > > > @@ -49,6 +49,7 @@ uint16_t btd_device_get_vendor(struct btd_device *device); > > > uint16_t btd_device_get_vendor_src(struct btd_device *device); > > > uint16_t btd_device_get_product(struct btd_device *device); > > > uint16_t btd_device_get_version(struct btd_device *device); > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type); > > > void device_remove(struct btd_device *device, gboolean remove_stored); > > > > Is there any particular reason why device_remove is not enough here? I > > don't see any reason to leave the device object around after removing > > its bonding. > > > > > int device_address_cmp(gconstpointer a, gconstpointer b); > > > int device_bdaddr_cmp(gconstpointer a, gconstpointer b); > > > -- > > > 2.27.0.212.ge8ba1cc988-goog > > > > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Bluez PATCH v2 1/2] device: add device_remove_bonding function 2020-07-14 17:09 ` Luiz Augusto von Dentz @ 2020-07-15 14:15 ` Archie Pusaka 2020-07-15 17:31 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 12+ messages in thread From: Archie Pusaka @ 2020-07-15 14:15 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Archie Pusaka, Alain Michaud Hi Luiz, If we mark it as temporary, then the device will immediately get deleted upon disconnection. https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/adapter.c#n6875 This is the same situation as directly calling device_remove. May I know the reason why we want to put the device as temporary? If we currently don't have a way to keep a "previously connected but no longer bonded" device, then removing the device perhaps is the next best option. It still makes the user scan for the virtually disconnected device though. Thanks, Archie On Wed, 15 Jul 2020 at 01:10, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Archie, > > On Tue, Jul 7, 2020 at 9:30 PM Archie Pusaka <apusaka@google.com> wrote: > > > > Hi Luiz, > > > > As far as the spec is concerned, we can also remove the device by > > calling device_remove. However, I suppose it would be confusing for > > end users if they can no longer find their HID device on the device > > list just because the device previously sent a virtual cable > > disconnection. > > The HID 1.0 spec part 6.4.2 also gives an example of a possible > > scenario when a virtually cabled device is removed: "Unplugged devices > > shall be marked as known and put into a “most recently used list” of > > known devices to facilitate future re-connecting". > > Then perhaps we shall have it marked as temporary as well, that said > we do want to introduce disappearing logic so temporary devices are > not left dangling for too long. > > > Thanks, > > Archie > > > > > > On Wed, 8 Jul 2020 at 02:03, Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Archie, > > > > > > On Sat, Jun 27, 2020 at 8:54 AM Archie Pusaka <apusaka@google.com> wrote: > > > > > > > > From: Archie Pusaka <apusaka@chromium.org> > > > > > > > > This patch splits the "bonding removal" function in device.c, > > > > because we need to remove bonding information when receiving > > > > "virtual cable unplug" in HID profile. > > > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > > --- > > > > > > > > Changes in v2: None > > > > > > > > src/device.c | 25 +++++++++++++++---------- > > > > src/device.h | 1 + > > > > 2 files changed, 16 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/src/device.c b/src/device.c > > > > index 7b0eb256e..9fb0e018c 100644 > > > > --- a/src/device.c > > > > +++ b/src/device.c > > > > @@ -4162,6 +4162,17 @@ static void delete_folder_tree(const char *dirname) > > > > rmdir(dirname); > > > > } > > > > > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type) > > > > +{ > > > > + if (bdaddr_type == BDADDR_BREDR) > > > > + device->bredr_state.bonded = false; > > > > + else > > > > + device->le_state.bonded = false; > > > > + > > > > + btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > + bdaddr_type); > > > > +} > > > > + > > > > static void device_remove_stored(struct btd_device *device) > > > > { > > > > char device_addr[18]; > > > > @@ -4170,17 +4181,11 @@ static void device_remove_stored(struct btd_device *device) > > > > char *data; > > > > gsize length = 0; > > > > > > > > - if (device->bredr_state.bonded) { > > > > - device->bredr_state.bonded = false; > > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > - BDADDR_BREDR); > > > > - } > > > > + if (device->bredr_state.bonded) > > > > + device_remove_bonding(device, BDADDR_BREDR); > > > > > > > > - if (device->le_state.bonded) { > > > > - device->le_state.bonded = false; > > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > - device->bdaddr_type); > > > > - } > > > > + if (device->le_state.bonded) > > > > + device_remove_bonding(device, device->bdaddr_type); > > > > > > > > device->bredr_state.paired = false; > > > > device->le_state.paired = false; > > > > diff --git a/src/device.h b/src/device.h > > > > index 06b100499..907c7c5c4 100644 > > > > --- a/src/device.h > > > > +++ b/src/device.h > > > > @@ -49,6 +49,7 @@ uint16_t btd_device_get_vendor(struct btd_device *device); > > > > uint16_t btd_device_get_vendor_src(struct btd_device *device); > > > > uint16_t btd_device_get_product(struct btd_device *device); > > > > uint16_t btd_device_get_version(struct btd_device *device); > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type); > > > > void device_remove(struct btd_device *device, gboolean remove_stored); > > > > > > Is there any particular reason why device_remove is not enough here? I > > > don't see any reason to leave the device object around after removing > > > its bonding. > > > > > > > int device_address_cmp(gconstpointer a, gconstpointer b); > > > > int device_bdaddr_cmp(gconstpointer a, gconstpointer b); > > > > -- > > > > 2.27.0.212.ge8ba1cc988-goog > > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Bluez PATCH v2 1/2] device: add device_remove_bonding function 2020-07-15 14:15 ` Archie Pusaka @ 2020-07-15 17:31 ` Luiz Augusto von Dentz 2020-07-16 7:28 ` Archie Pusaka 0 siblings, 1 reply; 12+ messages in thread From: Luiz Augusto von Dentz @ 2020-07-15 17:31 UTC (permalink / raw) To: Archie Pusaka; +Cc: linux-bluetooth, Archie Pusaka, Alain Michaud Hi Archie, On Wed, Jul 15, 2020 at 7:15 AM Archie Pusaka <apusaka@google.com> wrote: > > Hi Luiz, > > If we mark it as temporary, then the device will immediately get > deleted upon disconnection. > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/adapter.c#n6875 > This is the same situation as directly calling device_remove. > > May I know the reason why we want to put the device as temporary? > > If we currently don't have a way to keep a "previously connected but > no longer bonded" device, then removing the device perhaps is the next > best option. It still makes the user scan for the virtually > disconnected device though. We keep a cache of previously known devices, but we only display them once they are actually found otherwise it may grow too big which is inconvenient, I wonder where this concept of "previously connected but no longer bonded" comes from though, we had the temporary to map devices that would not be persisted on the storage which I guess this is what it is about, that said we could perhaps have a timeout before setting it as temporary but we might want to integrate with the logic of detecting devices disappearing. > Thanks, > Archie > > On Wed, 15 Jul 2020 at 01:10, Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Archie, > > > > On Tue, Jul 7, 2020 at 9:30 PM Archie Pusaka <apusaka@google.com> wrote: > > > > > > Hi Luiz, > > > > > > As far as the spec is concerned, we can also remove the device by > > > calling device_remove. However, I suppose it would be confusing for > > > end users if they can no longer find their HID device on the device > > > list just because the device previously sent a virtual cable > > > disconnection. > > > The HID 1.0 spec part 6.4.2 also gives an example of a possible > > > scenario when a virtually cabled device is removed: "Unplugged devices > > > shall be marked as known and put into a “most recently used list” of > > > known devices to facilitate future re-connecting". > > > > Then perhaps we shall have it marked as temporary as well, that said > > we do want to introduce disappearing logic so temporary devices are > > not left dangling for too long. > > > > > Thanks, > > > Archie > > > > > > > > > On Wed, 8 Jul 2020 at 02:03, Luiz Augusto von Dentz > > > <luiz.dentz@gmail.com> wrote: > > > > > > > > Hi Archie, > > > > > > > > On Sat, Jun 27, 2020 at 8:54 AM Archie Pusaka <apusaka@google.com> wrote: > > > > > > > > > > From: Archie Pusaka <apusaka@chromium.org> > > > > > > > > > > This patch splits the "bonding removal" function in device.c, > > > > > because we need to remove bonding information when receiving > > > > > "virtual cable unplug" in HID profile. > > > > > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > > > --- > > > > > > > > > > Changes in v2: None > > > > > > > > > > src/device.c | 25 +++++++++++++++---------- > > > > > src/device.h | 1 + > > > > > 2 files changed, 16 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/src/device.c b/src/device.c > > > > > index 7b0eb256e..9fb0e018c 100644 > > > > > --- a/src/device.c > > > > > +++ b/src/device.c > > > > > @@ -4162,6 +4162,17 @@ static void delete_folder_tree(const char *dirname) > > > > > rmdir(dirname); > > > > > } > > > > > > > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type) > > > > > +{ > > > > > + if (bdaddr_type == BDADDR_BREDR) > > > > > + device->bredr_state.bonded = false; > > > > > + else > > > > > + device->le_state.bonded = false; > > > > > + > > > > > + btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > + bdaddr_type); > > > > > +} > > > > > + > > > > > static void device_remove_stored(struct btd_device *device) > > > > > { > > > > > char device_addr[18]; > > > > > @@ -4170,17 +4181,11 @@ static void device_remove_stored(struct btd_device *device) > > > > > char *data; > > > > > gsize length = 0; > > > > > > > > > > - if (device->bredr_state.bonded) { > > > > > - device->bredr_state.bonded = false; > > > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > - BDADDR_BREDR); > > > > > - } > > > > > + if (device->bredr_state.bonded) > > > > > + device_remove_bonding(device, BDADDR_BREDR); > > > > > > > > > > - if (device->le_state.bonded) { > > > > > - device->le_state.bonded = false; > > > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > - device->bdaddr_type); > > > > > - } > > > > > + if (device->le_state.bonded) > > > > > + device_remove_bonding(device, device->bdaddr_type); > > > > > > > > > > device->bredr_state.paired = false; > > > > > device->le_state.paired = false; > > > > > diff --git a/src/device.h b/src/device.h > > > > > index 06b100499..907c7c5c4 100644 > > > > > --- a/src/device.h > > > > > +++ b/src/device.h > > > > > @@ -49,6 +49,7 @@ uint16_t btd_device_get_vendor(struct btd_device *device); > > > > > uint16_t btd_device_get_vendor_src(struct btd_device *device); > > > > > uint16_t btd_device_get_product(struct btd_device *device); > > > > > uint16_t btd_device_get_version(struct btd_device *device); > > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type); > > > > > void device_remove(struct btd_device *device, gboolean remove_stored); > > > > > > > > Is there any particular reason why device_remove is not enough here? I > > > > don't see any reason to leave the device object around after removing > > > > its bonding. > > > > > > > > > int device_address_cmp(gconstpointer a, gconstpointer b); > > > > > int device_bdaddr_cmp(gconstpointer a, gconstpointer b); > > > > > -- > > > > > 2.27.0.212.ge8ba1cc988-goog > > > > > > > > > > > > > > > > > -- > > > > Luiz Augusto von Dentz > > > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Bluez PATCH v2 1/2] device: add device_remove_bonding function 2020-07-15 17:31 ` Luiz Augusto von Dentz @ 2020-07-16 7:28 ` Archie Pusaka 2020-07-16 16:14 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 12+ messages in thread From: Archie Pusaka @ 2020-07-16 7:28 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Archie Pusaka, Alain Michaud Hi Luiz, I think spec writers' idea is to have a list of previously connected devices which is ordered by most recently connected. The size of this list may be limited to a number, meaning that the least recently connected device will be removed from the list. The devices in this list may or may not be bonded. This list is accessible to users, so they can easily reconnect to the most recently used device. I don't suppose we currently have this list, so I'm happy with just removing the virtually unplugged device. Thanks, Archie On Thu, 16 Jul 2020 at 01:31, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Archie, > > On Wed, Jul 15, 2020 at 7:15 AM Archie Pusaka <apusaka@google.com> wrote: > > > > Hi Luiz, > > > > If we mark it as temporary, then the device will immediately get > > deleted upon disconnection. > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/adapter.c#n6875 > > This is the same situation as directly calling device_remove. > > > > May I know the reason why we want to put the device as temporary? > > > > If we currently don't have a way to keep a "previously connected but > > no longer bonded" device, then removing the device perhaps is the next > > best option. It still makes the user scan for the virtually > > disconnected device though. > > We keep a cache of previously known devices, but we only display them > once they are actually found otherwise it may grow too big which is > inconvenient, I wonder where this concept of "previously connected but > no longer bonded" comes from though, we had the temporary to map > devices that would not be persisted on the storage which I guess this > is what it is about, that said we could perhaps have a timeout before > setting it as temporary but we might want to integrate with the logic > of detecting devices disappearing. > > > Thanks, > > Archie > > > > On Wed, 15 Jul 2020 at 01:10, Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Archie, > > > > > > On Tue, Jul 7, 2020 at 9:30 PM Archie Pusaka <apusaka@google.com> wrote: > > > > > > > > Hi Luiz, > > > > > > > > As far as the spec is concerned, we can also remove the device by > > > > calling device_remove. However, I suppose it would be confusing for > > > > end users if they can no longer find their HID device on the device > > > > list just because the device previously sent a virtual cable > > > > disconnection. > > > > The HID 1.0 spec part 6.4.2 also gives an example of a possible > > > > scenario when a virtually cabled device is removed: "Unplugged devices > > > > shall be marked as known and put into a “most recently used list” of > > > > known devices to facilitate future re-connecting". > > > > > > Then perhaps we shall have it marked as temporary as well, that said > > > we do want to introduce disappearing logic so temporary devices are > > > not left dangling for too long. > > > > > > > Thanks, > > > > Archie > > > > > > > > > > > > On Wed, 8 Jul 2020 at 02:03, Luiz Augusto von Dentz > > > > <luiz.dentz@gmail.com> wrote: > > > > > > > > > > Hi Archie, > > > > > > > > > > On Sat, Jun 27, 2020 at 8:54 AM Archie Pusaka <apusaka@google.com> wrote: > > > > > > > > > > > > From: Archie Pusaka <apusaka@chromium.org> > > > > > > > > > > > > This patch splits the "bonding removal" function in device.c, > > > > > > because we need to remove bonding information when receiving > > > > > > "virtual cable unplug" in HID profile. > > > > > > > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > > > > --- > > > > > > > > > > > > Changes in v2: None > > > > > > > > > > > > src/device.c | 25 +++++++++++++++---------- > > > > > > src/device.h | 1 + > > > > > > 2 files changed, 16 insertions(+), 10 deletions(-) > > > > > > > > > > > > diff --git a/src/device.c b/src/device.c > > > > > > index 7b0eb256e..9fb0e018c 100644 > > > > > > --- a/src/device.c > > > > > > +++ b/src/device.c > > > > > > @@ -4162,6 +4162,17 @@ static void delete_folder_tree(const char *dirname) > > > > > > rmdir(dirname); > > > > > > } > > > > > > > > > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type) > > > > > > +{ > > > > > > + if (bdaddr_type == BDADDR_BREDR) > > > > > > + device->bredr_state.bonded = false; > > > > > > + else > > > > > > + device->le_state.bonded = false; > > > > > > + > > > > > > + btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > > + bdaddr_type); > > > > > > +} > > > > > > + > > > > > > static void device_remove_stored(struct btd_device *device) > > > > > > { > > > > > > char device_addr[18]; > > > > > > @@ -4170,17 +4181,11 @@ static void device_remove_stored(struct btd_device *device) > > > > > > char *data; > > > > > > gsize length = 0; > > > > > > > > > > > > - if (device->bredr_state.bonded) { > > > > > > - device->bredr_state.bonded = false; > > > > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > > - BDADDR_BREDR); > > > > > > - } > > > > > > + if (device->bredr_state.bonded) > > > > > > + device_remove_bonding(device, BDADDR_BREDR); > > > > > > > > > > > > - if (device->le_state.bonded) { > > > > > > - device->le_state.bonded = false; > > > > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > > - device->bdaddr_type); > > > > > > - } > > > > > > + if (device->le_state.bonded) > > > > > > + device_remove_bonding(device, device->bdaddr_type); > > > > > > > > > > > > device->bredr_state.paired = false; > > > > > > device->le_state.paired = false; > > > > > > diff --git a/src/device.h b/src/device.h > > > > > > index 06b100499..907c7c5c4 100644 > > > > > > --- a/src/device.h > > > > > > +++ b/src/device.h > > > > > > @@ -49,6 +49,7 @@ uint16_t btd_device_get_vendor(struct btd_device *device); > > > > > > uint16_t btd_device_get_vendor_src(struct btd_device *device); > > > > > > uint16_t btd_device_get_product(struct btd_device *device); > > > > > > uint16_t btd_device_get_version(struct btd_device *device); > > > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type); > > > > > > void device_remove(struct btd_device *device, gboolean remove_stored); > > > > > > > > > > Is there any particular reason why device_remove is not enough here? I > > > > > don't see any reason to leave the device object around after removing > > > > > its bonding. > > > > > > > > > > > int device_address_cmp(gconstpointer a, gconstpointer b); > > > > > > int device_bdaddr_cmp(gconstpointer a, gconstpointer b); > > > > > > -- > > > > > > 2.27.0.212.ge8ba1cc988-goog > > > > > > > > > > > > > > > > > > > > > -- > > > > > Luiz Augusto von Dentz > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Bluez PATCH v2 1/2] device: add device_remove_bonding function 2020-07-16 7:28 ` Archie Pusaka @ 2020-07-16 16:14 ` Luiz Augusto von Dentz 2020-07-16 18:10 ` Archie Pusaka 0 siblings, 1 reply; 12+ messages in thread From: Luiz Augusto von Dentz @ 2020-07-16 16:14 UTC (permalink / raw) To: Archie Pusaka; +Cc: linux-bluetooth, Archie Pusaka, Alain Michaud Hi Archie, On Thu, Jul 16, 2020 at 12:28 AM Archie Pusaka <apusaka@google.com> wrote: > > Hi Luiz, > > I think spec writers' idea is to have a list of previously connected > devices which is ordered by most recently connected. The size of this > list may be limited to a number, meaning that the least recently > connected device will be removed from the list. The devices in this > list may or may not be bonded. This list is accessible to users, so > they can easily reconnect to the most recently used device. > > I don't suppose we currently have this list, so I'm happy with just > removing the virtually unplugged device. So with the latest developments Im leaning towards using device_set_temporary since that would trigger a timer to remove the device after it has expired. > Thanks, > Archie > > On Thu, 16 Jul 2020 at 01:31, Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Archie, > > > > On Wed, Jul 15, 2020 at 7:15 AM Archie Pusaka <apusaka@google.com> wrote: > > > > > > Hi Luiz, > > > > > > If we mark it as temporary, then the device will immediately get > > > deleted upon disconnection. > > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/adapter.c#n6875 > > > This is the same situation as directly calling device_remove. > > > > > > May I know the reason why we want to put the device as temporary? > > > > > > If we currently don't have a way to keep a "previously connected but > > > no longer bonded" device, then removing the device perhaps is the next > > > best option. It still makes the user scan for the virtually > > > disconnected device though. > > > > We keep a cache of previously known devices, but we only display them > > once they are actually found otherwise it may grow too big which is > > inconvenient, I wonder where this concept of "previously connected but > > no longer bonded" comes from though, we had the temporary to map > > devices that would not be persisted on the storage which I guess this > > is what it is about, that said we could perhaps have a timeout before > > setting it as temporary but we might want to integrate with the logic > > of detecting devices disappearing. > > > > > Thanks, > > > Archie > > > > > > On Wed, 15 Jul 2020 at 01:10, Luiz Augusto von Dentz > > > <luiz.dentz@gmail.com> wrote: > > > > > > > > Hi Archie, > > > > > > > > On Tue, Jul 7, 2020 at 9:30 PM Archie Pusaka <apusaka@google.com> wrote: > > > > > > > > > > Hi Luiz, > > > > > > > > > > As far as the spec is concerned, we can also remove the device by > > > > > calling device_remove. However, I suppose it would be confusing for > > > > > end users if they can no longer find their HID device on the device > > > > > list just because the device previously sent a virtual cable > > > > > disconnection. > > > > > The HID 1.0 spec part 6.4.2 also gives an example of a possible > > > > > scenario when a virtually cabled device is removed: "Unplugged devices > > > > > shall be marked as known and put into a “most recently used list” of > > > > > known devices to facilitate future re-connecting". > > > > > > > > Then perhaps we shall have it marked as temporary as well, that said > > > > we do want to introduce disappearing logic so temporary devices are > > > > not left dangling for too long. > > > > > > > > > Thanks, > > > > > Archie > > > > > > > > > > > > > > > On Wed, 8 Jul 2020 at 02:03, Luiz Augusto von Dentz > > > > > <luiz.dentz@gmail.com> wrote: > > > > > > > > > > > > Hi Archie, > > > > > > > > > > > > On Sat, Jun 27, 2020 at 8:54 AM Archie Pusaka <apusaka@google.com> wrote: > > > > > > > > > > > > > > From: Archie Pusaka <apusaka@chromium.org> > > > > > > > > > > > > > > This patch splits the "bonding removal" function in device.c, > > > > > > > because we need to remove bonding information when receiving > > > > > > > "virtual cable unplug" in HID profile. > > > > > > > > > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > > > > > --- > > > > > > > > > > > > > > Changes in v2: None > > > > > > > > > > > > > > src/device.c | 25 +++++++++++++++---------- > > > > > > > src/device.h | 1 + > > > > > > > 2 files changed, 16 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > diff --git a/src/device.c b/src/device.c > > > > > > > index 7b0eb256e..9fb0e018c 100644 > > > > > > > --- a/src/device.c > > > > > > > +++ b/src/device.c > > > > > > > @@ -4162,6 +4162,17 @@ static void delete_folder_tree(const char *dirname) > > > > > > > rmdir(dirname); > > > > > > > } > > > > > > > > > > > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type) > > > > > > > +{ > > > > > > > + if (bdaddr_type == BDADDR_BREDR) > > > > > > > + device->bredr_state.bonded = false; > > > > > > > + else > > > > > > > + device->le_state.bonded = false; > > > > > > > + > > > > > > > + btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > > > + bdaddr_type); > > > > > > > +} > > > > > > > + > > > > > > > static void device_remove_stored(struct btd_device *device) > > > > > > > { > > > > > > > char device_addr[18]; > > > > > > > @@ -4170,17 +4181,11 @@ static void device_remove_stored(struct btd_device *device) > > > > > > > char *data; > > > > > > > gsize length = 0; > > > > > > > > > > > > > > - if (device->bredr_state.bonded) { > > > > > > > - device->bredr_state.bonded = false; > > > > > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > > > - BDADDR_BREDR); > > > > > > > - } > > > > > > > + if (device->bredr_state.bonded) > > > > > > > + device_remove_bonding(device, BDADDR_BREDR); > > > > > > > > > > > > > > - if (device->le_state.bonded) { > > > > > > > - device->le_state.bonded = false; > > > > > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > > > - device->bdaddr_type); > > > > > > > - } > > > > > > > + if (device->le_state.bonded) > > > > > > > + device_remove_bonding(device, device->bdaddr_type); > > > > > > > > > > > > > > device->bredr_state.paired = false; > > > > > > > device->le_state.paired = false; > > > > > > > diff --git a/src/device.h b/src/device.h > > > > > > > index 06b100499..907c7c5c4 100644 > > > > > > > --- a/src/device.h > > > > > > > +++ b/src/device.h > > > > > > > @@ -49,6 +49,7 @@ uint16_t btd_device_get_vendor(struct btd_device *device); > > > > > > > uint16_t btd_device_get_vendor_src(struct btd_device *device); > > > > > > > uint16_t btd_device_get_product(struct btd_device *device); > > > > > > > uint16_t btd_device_get_version(struct btd_device *device); > > > > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type); > > > > > > > void device_remove(struct btd_device *device, gboolean remove_stored); > > > > > > > > > > > > Is there any particular reason why device_remove is not enough here? I > > > > > > don't see any reason to leave the device object around after removing > > > > > > its bonding. > > > > > > > > > > > > > int device_address_cmp(gconstpointer a, gconstpointer b); > > > > > > > int device_bdaddr_cmp(gconstpointer a, gconstpointer b); > > > > > > > -- > > > > > > > 2.27.0.212.ge8ba1cc988-goog > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Luiz Augusto von Dentz > > > > > > > > > > > > > > > > -- > > > > Luiz Augusto von Dentz > > > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Bluez PATCH v2 1/2] device: add device_remove_bonding function 2020-07-16 16:14 ` Luiz Augusto von Dentz @ 2020-07-16 18:10 ` Archie Pusaka 0 siblings, 0 replies; 12+ messages in thread From: Archie Pusaka @ 2020-07-16 18:10 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Archie Pusaka, Alain Michaud Hi Luiz, I submitted a patch which calls device_set_temporary, but upon testing the device still immediately gets deleted after receiving virtual cable disconnect, since we delete temporary devices on adapter_remove_connection(). The timer doesn't make any difference this way. Thanks, Archie On Fri, 17 Jul 2020 at 00:14, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Archie, > > On Thu, Jul 16, 2020 at 12:28 AM Archie Pusaka <apusaka@google.com> wrote: > > > > Hi Luiz, > > > > I think spec writers' idea is to have a list of previously connected > > devices which is ordered by most recently connected. The size of this > > list may be limited to a number, meaning that the least recently > > connected device will be removed from the list. The devices in this > > list may or may not be bonded. This list is accessible to users, so > > they can easily reconnect to the most recently used device. > > > > I don't suppose we currently have this list, so I'm happy with just > > removing the virtually unplugged device. > > So with the latest developments Im leaning towards using > device_set_temporary since that would trigger a timer to remove the > device after it has expired. > > > Thanks, > > Archie > > > > On Thu, 16 Jul 2020 at 01:31, Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Archie, > > > > > > On Wed, Jul 15, 2020 at 7:15 AM Archie Pusaka <apusaka@google.com> wrote: > > > > > > > > Hi Luiz, > > > > > > > > If we mark it as temporary, then the device will immediately get > > > > deleted upon disconnection. > > > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/adapter.c#n6875 > > > > This is the same situation as directly calling device_remove. > > > > > > > > May I know the reason why we want to put the device as temporary? > > > > > > > > If we currently don't have a way to keep a "previously connected but > > > > no longer bonded" device, then removing the device perhaps is the next > > > > best option. It still makes the user scan for the virtually > > > > disconnected device though. > > > > > > We keep a cache of previously known devices, but we only display them > > > once they are actually found otherwise it may grow too big which is > > > inconvenient, I wonder where this concept of "previously connected but > > > no longer bonded" comes from though, we had the temporary to map > > > devices that would not be persisted on the storage which I guess this > > > is what it is about, that said we could perhaps have a timeout before > > > setting it as temporary but we might want to integrate with the logic > > > of detecting devices disappearing. > > > > > > > Thanks, > > > > Archie > > > > > > > > On Wed, 15 Jul 2020 at 01:10, Luiz Augusto von Dentz > > > > <luiz.dentz@gmail.com> wrote: > > > > > > > > > > Hi Archie, > > > > > > > > > > On Tue, Jul 7, 2020 at 9:30 PM Archie Pusaka <apusaka@google.com> wrote: > > > > > > > > > > > > Hi Luiz, > > > > > > > > > > > > As far as the spec is concerned, we can also remove the device by > > > > > > calling device_remove. However, I suppose it would be confusing for > > > > > > end users if they can no longer find their HID device on the device > > > > > > list just because the device previously sent a virtual cable > > > > > > disconnection. > > > > > > The HID 1.0 spec part 6.4.2 also gives an example of a possible > > > > > > scenario when a virtually cabled device is removed: "Unplugged devices > > > > > > shall be marked as known and put into a “most recently used list” of > > > > > > known devices to facilitate future re-connecting". > > > > > > > > > > Then perhaps we shall have it marked as temporary as well, that said > > > > > we do want to introduce disappearing logic so temporary devices are > > > > > not left dangling for too long. > > > > > > > > > > > Thanks, > > > > > > Archie > > > > > > > > > > > > > > > > > > On Wed, 8 Jul 2020 at 02:03, Luiz Augusto von Dentz > > > > > > <luiz.dentz@gmail.com> wrote: > > > > > > > > > > > > > > Hi Archie, > > > > > > > > > > > > > > On Sat, Jun 27, 2020 at 8:54 AM Archie Pusaka <apusaka@google.com> wrote: > > > > > > > > > > > > > > > > From: Archie Pusaka <apusaka@chromium.org> > > > > > > > > > > > > > > > > This patch splits the "bonding removal" function in device.c, > > > > > > > > because we need to remove bonding information when receiving > > > > > > > > "virtual cable unplug" in HID profile. > > > > > > > > > > > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > > > > > > --- > > > > > > > > > > > > > > > > Changes in v2: None > > > > > > > > > > > > > > > > src/device.c | 25 +++++++++++++++---------- > > > > > > > > src/device.h | 1 + > > > > > > > > 2 files changed, 16 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > > > diff --git a/src/device.c b/src/device.c > > > > > > > > index 7b0eb256e..9fb0e018c 100644 > > > > > > > > --- a/src/device.c > > > > > > > > +++ b/src/device.c > > > > > > > > @@ -4162,6 +4162,17 @@ static void delete_folder_tree(const char *dirname) > > > > > > > > rmdir(dirname); > > > > > > > > } > > > > > > > > > > > > > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type) > > > > > > > > +{ > > > > > > > > + if (bdaddr_type == BDADDR_BREDR) > > > > > > > > + device->bredr_state.bonded = false; > > > > > > > > + else > > > > > > > > + device->le_state.bonded = false; > > > > > > > > + > > > > > > > > + btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > > > > + bdaddr_type); > > > > > > > > +} > > > > > > > > + > > > > > > > > static void device_remove_stored(struct btd_device *device) > > > > > > > > { > > > > > > > > char device_addr[18]; > > > > > > > > @@ -4170,17 +4181,11 @@ static void device_remove_stored(struct btd_device *device) > > > > > > > > char *data; > > > > > > > > gsize length = 0; > > > > > > > > > > > > > > > > - if (device->bredr_state.bonded) { > > > > > > > > - device->bredr_state.bonded = false; > > > > > > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > > > > - BDADDR_BREDR); > > > > > > > > - } > > > > > > > > + if (device->bredr_state.bonded) > > > > > > > > + device_remove_bonding(device, BDADDR_BREDR); > > > > > > > > > > > > > > > > - if (device->le_state.bonded) { > > > > > > > > - device->le_state.bonded = false; > > > > > > > > - btd_adapter_remove_bonding(device->adapter, &device->bdaddr, > > > > > > > > - device->bdaddr_type); > > > > > > > > - } > > > > > > > > + if (device->le_state.bonded) > > > > > > > > + device_remove_bonding(device, device->bdaddr_type); > > > > > > > > > > > > > > > > device->bredr_state.paired = false; > > > > > > > > device->le_state.paired = false; > > > > > > > > diff --git a/src/device.h b/src/device.h > > > > > > > > index 06b100499..907c7c5c4 100644 > > > > > > > > --- a/src/device.h > > > > > > > > +++ b/src/device.h > > > > > > > > @@ -49,6 +49,7 @@ uint16_t btd_device_get_vendor(struct btd_device *device); > > > > > > > > uint16_t btd_device_get_vendor_src(struct btd_device *device); > > > > > > > > uint16_t btd_device_get_product(struct btd_device *device); > > > > > > > > uint16_t btd_device_get_version(struct btd_device *device); > > > > > > > > +void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type); > > > > > > > > void device_remove(struct btd_device *device, gboolean remove_stored); > > > > > > > > > > > > > > Is there any particular reason why device_remove is not enough here? I > > > > > > > don't see any reason to leave the device object around after removing > > > > > > > its bonding. > > > > > > > > > > > > > > > int device_address_cmp(gconstpointer a, gconstpointer b); > > > > > > > > int device_bdaddr_cmp(gconstpointer a, gconstpointer b); > > > > > > > > -- > > > > > > > > 2.27.0.212.ge8ba1cc988-goog > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > Luiz Augusto von Dentz > > > > > > > > > > > > > > > > > > > > -- > > > > > Luiz Augusto von Dentz > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-07-16 18:10 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-27 15:54 [Bluez PATCH v2 1/2] device: add device_remove_bonding function Archie Pusaka 2020-06-27 15:54 ` [Bluez PATCH v2 2/2] input: Remove bonding info when receiving virtual cable unplug Archie Pusaka 2020-07-07 9:34 ` Archie Pusaka 2020-07-07 18:02 ` [Bluez PATCH v2 1/2] device: add device_remove_bonding function Luiz Augusto von Dentz 2020-07-08 4:30 ` Archie Pusaka 2020-07-14 4:50 ` Archie Pusaka 2020-07-14 17:09 ` Luiz Augusto von Dentz 2020-07-15 14:15 ` Archie Pusaka 2020-07-15 17:31 ` Luiz Augusto von Dentz 2020-07-16 7:28 ` Archie Pusaka 2020-07-16 16:14 ` Luiz Augusto von Dentz 2020-07-16 18:10 ` Archie Pusaka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).