| Message ID | 20210106172647.Bluez.v1.1.If4056891c866780eba7b210887c6db468b8b55e9@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | [Bluez,v1] adapter: Remove temporary devices before power off | expand |
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=409877 ---Test result--- ############################## Test: CheckPatch - PASS ############################## Test: CheckGitLint - PASS ############################## Test: CheckBuild - PASS ############################## Test: MakeCheck - PASS --- Regards, Linux Bluetooth
Hi Archie, On Wed, Jan 6, 2021 at 1:27 AM Archie Pusaka <apusaka@google.com> wrote: > > From: Archie Pusaka <apusaka@chromium.org> > > If adapter is powered off when a currently connected device is > being removed, there is a possibility that we haven't finish waiting > for the disconnection but the adapter is already powered down. > > When this happens, the kernel would fail to clean the device's > information, for example the pairing information. This causes > disagreement between the user space and the kernel about whether the > device is already paired, because the device is successfully removed > from the user space's perspective. > > This patch enforces the removal of such devices before allowing the > adapter to power off. > --- > > src/adapter.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/src/adapter.c b/src/adapter.c > index ec6a6a64c5..92d1cb2232 100644 > --- a/src/adapter.c > +++ b/src/adapter.c > @@ -517,6 +517,7 @@ static void adapter_stop(struct btd_adapter *adapter); > static void trigger_passive_scanning(struct btd_adapter *adapter); > static bool set_mode(struct btd_adapter *adapter, uint16_t opcode, > uint8_t mode); > +static void remove_temporary_devices(struct btd_adapter *adapter); I would have the function above declared just before the first user that way we don't have to use a forward declaration like above. > static void settings_changed(struct btd_adapter *adapter, uint32_t settings) > { > @@ -622,6 +623,8 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode, > switch (mode) { > case MGMT_OP_SET_POWERED: > setting = MGMT_SETTING_POWERED; > + if (!mode) > + remove_temporary_devices(adapter); > break; > case MGMT_OP_SET_CONNECTABLE: > setting = MGMT_SETTING_CONNECTABLE; > @@ -2888,8 +2891,10 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting, > param = &mode; > len = sizeof(mode); > > - if (!mode) > + if (!mode) { > clear_discoverable(adapter); > + remove_temporary_devices(adapter); > + } > > break; > case MGMT_SETTING_DISCOVERABLE: > @@ -5304,6 +5309,19 @@ static void remove_discovery_list(struct btd_adapter *adapter) > adapter->discovery_list = NULL; > } > > +static void remove_temporary_devices(struct btd_adapter *adapter) > +{ > + GSList *l, *next; > + > + for (l = adapter->devices; l; l = next) { > + struct btd_device *dev = l->data; > + > + next = g_slist_next(l); > + if (device_is_temporary(dev)) > + btd_adapter_remove_device(adapter, dev); > + } > +} > + > static void adapter_free(gpointer user_data) > { > struct btd_adapter *adapter = user_data; > -- > 2.29.2.729.g45daf8777d-goog >
Hi Archie, On Wed, Jan 6, 2021 at 10:24 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Archie, > > On Wed, Jan 6, 2021 at 1:27 AM Archie Pusaka <apusaka@google.com> wrote: > > > > From: Archie Pusaka <apusaka@chromium.org> > > > > If adapter is powered off when a currently connected device is > > being removed, there is a possibility that we haven't finish waiting > > for the disconnection but the adapter is already powered down. > > > > When this happens, the kernel would fail to clean the device's > > information, for example the pairing information. This causes > > disagreement between the user space and the kernel about whether the > > device is already paired, because the device is successfully removed > > from the user space's perspective. > > > > This patch enforces the removal of such devices before allowing the > > adapter to power off. > > --- > > > > src/adapter.c | 20 +++++++++++++++++++- > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/src/adapter.c b/src/adapter.c > > index ec6a6a64c5..92d1cb2232 100644 > > --- a/src/adapter.c > > +++ b/src/adapter.c > > @@ -517,6 +517,7 @@ static void adapter_stop(struct btd_adapter *adapter); > > static void trigger_passive_scanning(struct btd_adapter *adapter); > > static bool set_mode(struct btd_adapter *adapter, uint16_t opcode, > > uint8_t mode); > > +static void remove_temporary_devices(struct btd_adapter *adapter); > > I would have the function above declared just before the first user > that way we don't have to use a forward declaration like above. > > > static void settings_changed(struct btd_adapter *adapter, uint32_t settings) > > { > > @@ -622,6 +623,8 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode, > > switch (mode) { > > case MGMT_OP_SET_POWERED: > > setting = MGMT_SETTING_POWERED; > > + if (!mode) > > + remove_temporary_devices(adapter); > > break; > > case MGMT_OP_SET_CONNECTABLE: > > setting = MGMT_SETTING_CONNECTABLE; > > @@ -2888,8 +2891,10 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting, > > param = &mode; > > len = sizeof(mode); > > > > - if (!mode) > > + if (!mode) { > > clear_discoverable(adapter); > > + remove_temporary_devices(adapter); > > + } > > > > break; > > case MGMT_SETTING_DISCOVERABLE: > > @@ -5304,6 +5309,19 @@ static void remove_discovery_list(struct btd_adapter *adapter) > > adapter->discovery_list = NULL; > > } > > > > +static void remove_temporary_devices(struct btd_adapter *adapter) > > +{ > > + GSList *l, *next; > > + > > + for (l = adapter->devices; l; l = next) { > > + struct btd_device *dev = l->data; > > + > > + next = g_slist_next(l); > > + if (device_is_temporary(dev)) > > + btd_adapter_remove_device(adapter, dev); > > + } > > +} > > + > > static void adapter_free(gpointer user_data) > > { > > struct btd_adapter *adapter = user_data; > > -- > > 2.29.2.729.g45daf8777d-goog > > > > > -- > Luiz Augusto von Dentz Applied, thanks. I went ahead and did the changes myself.
diff --git a/src/adapter.c b/src/adapter.c index ec6a6a64c5..92d1cb2232 100644 --- a/src/adapter.c +++ b/src/adapter.c @@ -517,6 +517,7 @@ static void adapter_stop(struct btd_adapter *adapter); static void trigger_passive_scanning(struct btd_adapter *adapter); static bool set_mode(struct btd_adapter *adapter, uint16_t opcode, uint8_t mode); +static void remove_temporary_devices(struct btd_adapter *adapter); static void settings_changed(struct btd_adapter *adapter, uint32_t settings) { @@ -622,6 +623,8 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode, switch (mode) { case MGMT_OP_SET_POWERED: setting = MGMT_SETTING_POWERED; + if (!mode) + remove_temporary_devices(adapter); break; case MGMT_OP_SET_CONNECTABLE: setting = MGMT_SETTING_CONNECTABLE; @@ -2888,8 +2891,10 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting, param = &mode; len = sizeof(mode); - if (!mode) + if (!mode) { clear_discoverable(adapter); + remove_temporary_devices(adapter); + } break; case MGMT_SETTING_DISCOVERABLE: @@ -5304,6 +5309,19 @@ static void remove_discovery_list(struct btd_adapter *adapter) adapter->discovery_list = NULL; } +static void remove_temporary_devices(struct btd_adapter *adapter) +{ + GSList *l, *next; + + for (l = adapter->devices; l; l = next) { + struct btd_device *dev = l->data; + + next = g_slist_next(l); + if (device_is_temporary(dev)) + btd_adapter_remove_device(adapter, dev); + } +} + static void adapter_free(gpointer user_data) { struct btd_adapter *adapter = user_data;
From: Archie Pusaka <apusaka@chromium.org> If adapter is powered off when a currently connected device is being removed, there is a possibility that we haven't finish waiting for the disconnection but the adapter is already powered down. When this happens, the kernel would fail to clean the device's information, for example the pairing information. This causes disagreement between the user space and the kernel about whether the device is already paired, because the device is successfully removed from the user space's perspective. This patch enforces the removal of such devices before allowing the adapter to power off. --- src/adapter.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)