Hi Luiz,
On Tue, Jan 28, 2020 at 12:29 PM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Abhishek,
>
> On Tue, Jan 28, 2020 at 10:44 AM Abhishek Pandit-Subedi
> <abhishekpandit@xxxxxxxxxxxx> wrote:
> >
> > If a device is capable of waking the host system from suspend, it should
> > be marked as wake capable. We introduce a new management operation here
> > to set this property and implement the API needed to call it. We also
> > add the dbus endpoint to allow the wake capable setting to be
> > controlled.
> >
> > ---
> >
> > Changes in v3:
> > * Added profile_wake_support and made wake_capable dependent on it
> >
> > Changes in v2:
> > * Added dbus api "WakeCapable" to set value
> > * Update device_set_wake_capable to be called by
> > adapter_set_wake_capable_complete so we can emit property changed
> >
> > lib/mgmt.h | 9 ++++++
> > src/adapter.c | 65 ++++++++++++++++++++++++++++++++++++++++
> > src/adapter.h | 2 ++
> > src/device.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > src/device.h | 5 ++++
> > 5 files changed, 164 insertions(+)
> >
> > diff --git a/lib/mgmt.h b/lib/mgmt.h
> > index 276445d0a..cf19dd086 100644
> > --- a/lib/mgmt.h
> > +++ b/lib/mgmt.h
> > @@ -599,6 +599,13 @@ struct mgmt_cp_set_blocked_keys {
> > struct mgmt_blocked_key_info keys[0];
> > } __packed;
> >
> > +#define MGMT_OP_SET_WAKE_CAPABLE 0x0047
> > +#define MGMT_SET_WAKE_CAPABLE_SIZE 8
> > +struct mgmt_cp_set_wake_capable {
> > + struct mgmt_addr_info addr;
> > + uint8_t wake_enable;
> > +} __packed;
> > +
> > #define MGMT_EV_CMD_COMPLETE 0x0001
> > struct mgmt_ev_cmd_complete {
> > uint16_t opcode;
> > @@ -893,6 +900,8 @@ static const char *mgmt_op[] = {
> > "Set Appearance",
> > "Get PHY Configuration",
> > "Set PHY Configuration",
> > + "Set Blocked Keys",
> > + "Set Wake Capable",
> > };
> >
> > static const char *mgmt_ev[] = {
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 329c3ae0b..294a9c9e4 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -4685,6 +4685,71 @@ void adapter_whitelist_add(struct btd_adapter *adapter, struct btd_device *dev)
> > add_whitelist_complete, adapter, NULL);
> > }
> >
> > +static void set_wake_capable_complete(uint8_t status, uint16_t length,
> > + const void *param, void *user_data)
> > +{
> > + const struct mgmt_cp_set_wake_capable *rp = param;
> > + struct btd_adapter *adapter = user_data;
> > + struct btd_device *dev;
>
> Extra empty line not needed.
My git merging skills have failed me yet again. Will fix in next patch.
>
> > + char addr[18];
> > +
> > + if (length < sizeof(*rp)) {
> > + btd_error(adapter->dev_id,
> > + "Too small Set Wake Capable complete event");
> > + return;
> > + }
> > +
> > + ba2str(&rp->addr.bdaddr, addr);
> > +
> > + dev = btd_adapter_find_device(adapter, &rp->addr.bdaddr,
> > + rp->addr.type);
> > + if (!dev) {
> > + btd_error(adapter->dev_id,
> > + "Set Wake Capable complete for unknown device %s",
> > + addr);
> > + return;
> > + }
> > +
> > + if (status != MGMT_STATUS_SUCCESS) {
> > + btd_error(adapter->dev_id,
> > + "Failed to set wake capable %s(%u) = %d: %s (0x%02x)",
> > + addr, rp->addr.type, rp->wake_enable,
> > + mgmt_errstr(status), status);
> > + return;
> > + }
> > +
> > + device_set_wake_capable(dev, rp->wake_enable);
> > +
> > + DBG("Set wake capable complete %s (%u)", addr, rp->addr.type);
> > +}
> > +
> > +void adapter_set_wake_capable(struct btd_adapter* adapter,
> > + struct btd_device* dev,
> > + bool wake_enable)
> > +{
> > + struct mgmt_cp_set_wake_capable cp;
> > + char addr[18];
>
> Shouldn't there be a way to check if the kernel does actually support
> the command first? We shouldn't be attempting to use it with kernels
> that don't support it and the property should also not appear in that
> case, so it is only visible if:
>
> 1. Kernel reports that Wake Capable command is supported
> 2. A driver/plugin/profile calls device_set_wake_support
>
> Also in case those 2 conditions are attended bu the command fails we
> should reply with an error since there won't be any PropertiesChanged
> to indicate something went wrong.
That sounds reasonable. Are all supported management apis reported
somewhere? (I may discover the answer to this just by reading code)
Also, what's a good way to report an error on PropertiesChanged? Just
logging the error (i.e. error("Failed to set wake capable property"))
or actually calling something like g_dbus_emit_property_error (I don't
think this is real; just using an example)?
>
> > + memset(&cp, 0, sizeof(cp));
> > + bacpy(&cp.addr.bdaddr, device_get_address(dev));
> > + cp.addr.type = btd_device_get_bdaddr_type(dev);
> > + cp.wake_enable = device_get_profile_wake_support(dev) && wake_enable;
> > +
> > + ba2strlc(&cp.addr.bdaddr, addr);
> > +
> > + if (!mgmt_send(adapter->mgmt, MGMT_OP_SET_WAKE_CAPABLE, adapter->dev_id,
> > + sizeof(cp), &cp, set_wake_capable_complete, adapter,
> > + NULL)) {
> > + btd_warn(adapter->dev_id,
> > + "Could not set wake capable = %u on %s (%u)",
> > + cp.wake_enable, addr, cp.addr.type);
> > + }
> > +
> > + DBG("Setting %s (%u) to wake capable = %u", addr,
> > + cp.addr.type, cp.wake_enable);
> > +}
> > +
> > static void remove_whitelist_complete(uint8_t status, uint16_t length,
> > const void *param, void *user_data)
> > {
> > diff --git a/src/adapter.h b/src/adapter.h
> > index d0a5253bd..e990279ed 100644
> > --- a/src/adapter.h
> > +++ b/src/adapter.h
> > @@ -221,6 +221,8 @@ void adapter_whitelist_add(struct btd_adapter *adapter,
> > struct btd_device *dev);
> > void adapter_whitelist_remove(struct btd_adapter *adapter,
> > struct btd_device *dev);
> > +void adapter_set_wake_capable(struct btd_adapter* adapter,
> > + struct btd_device* dev, bool wake_enable);
> >
> > void btd_adapter_set_oob_handler(struct btd_adapter *adapter,
> > struct oob_handler *handler);
> > diff --git a/src/device.c b/src/device.c
> > index a4fe10980..7a455e1e9 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -189,6 +189,8 @@ struct btd_device {
> > bool le;
> > bool pending_paired; /* "Paired" waiting for SDP */
> > bool svc_refreshed;
> > + bool profile_wake_support; /* Profile supports wake */
>
> I guess we can drop the profile term here since that is implicit already.
Will change in next patch.
>
> > + bool wake_capable; /* Can wake from suspend */
> > GSList *svc_callbacks;
> > GSList *eir_uuids;
> > struct bt_ad *ad;
> > @@ -415,6 +417,9 @@ static gboolean store_device_info_cb(gpointer user_data)
> > g_key_file_set_boolean(key_file, "General", "Blocked",
> > device->blocked);
> >
> > + g_key_file_set_boolean(key_file, "General", "WakeCapable",
> > + device->wake_capable);
> > +
> > if (device->uuids) {
> > GSList *l;
> > int i;
> > @@ -1318,6 +1323,47 @@ dev_property_advertising_data_exist(const GDBusPropertyTable *property,
> > return bt_ad_has_data(device->ad, NULL);
> > }
> >
> > +static gboolean
> > +dev_property_get_wake_capable(const GDBusPropertyTable *property,
> > + DBusMessageIter *iter, void *data)
> > +{
> > + struct btd_device *device = data;
> > + dbus_bool_t wake_capable =
> > + device_get_wake_capable(device) ? TRUE : FALSE;
> > +
> > + dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &wake_capable);
> > +
> > + return TRUE;
> > +}
> > +
> > +static void dev_property_set_wake_capable(const GDBusPropertyTable *property,
> > + DBusMessageIter *value,
> > + GDBusPendingPropertySet id, void *data)
> > +{
> > + struct btd_device *device = data;
> > + dbus_bool_t b;
> > +
> > + if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN) {
> > + g_dbus_pending_property_error(id,
> > + ERROR_INTERFACE ".InvalidArguments",
> > + "Invalid arguments in method call");
> > + return;
> > + }
> > +
> > + dbus_message_iter_get_basic(value, &b);
> > +
> > + adapter_set_wake_capable(device->adapter, device, b == TRUE);
> > + g_dbus_pending_property_success(id);
> > +}
> > +
> > +static gboolean dev_property_wake_capable_exist(
> > + const GDBusPropertyTable *property, void *data)
> > +{
> > + struct btd_device *device = data;
> > +
> > + return device_get_profile_wake_support(device) ? TRUE : FALSE;
> > +}
> > +
> > static gboolean disconnect_all(gpointer user_data)
> > {
> > struct btd_device *device = user_data;
> > @@ -1509,6 +1555,34 @@ void device_set_ltk_enc_size(struct btd_device *device, uint8_t enc_size)
> > bt_att_set_enc_key_size(device->att, device->ltk_enc_size);
> > }
> >
> > +bool device_get_profile_wake_support(struct btd_device *device)
> > +{
> > + return device->profile_wake_support;
> > +}
> > +
> > +void device_set_profile_wake_support(struct btd_device *device,
> > + bool wake_support)
> > +{
> > + device->profile_wake_support = wake_support;
> > + /* WakeCapable is only visible when wake_support is true */
> > + g_dbus_emit_property_changed(dbus_conn, device->path, DEVICE_INTERFACE,
> > + "WakeCapable");
> > +}
> > +
> > +bool device_get_wake_capable(struct btd_device *device)
> > +{
> > + return device->wake_capable;
> > +}
> > +
> > +void device_set_wake_capable(struct btd_device *device, bool wake_capable)
> > +{
> > + device->wake_capable = wake_capable;
> > +
> > + store_device_info(device);
> > + g_dbus_emit_property_changed(dbus_conn, device->path, DEVICE_INTERFACE,
> > + "WakeCapable");
> > +}
> > +
> > static void device_set_auto_connect(struct btd_device *device, gboolean enable)
> > {
> > char addr[18];
> > @@ -2779,6 +2853,9 @@ static const GDBusPropertyTable device_properties[] = {
> > { "AdvertisingData", "a{yv}", dev_property_get_advertising_data,
> > NULL, dev_property_advertising_data_exist,
> > G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
> > + { "WakeCapable", "b", dev_property_get_wake_capable,
> > + dev_property_set_wake_capable,
> > + dev_property_wake_capable_exist },
> > { }
> > };
> >
> > @@ -3030,6 +3107,7 @@ static void load_info(struct btd_device *device, const char *local,
> > char *str;
> > gboolean store_needed = FALSE;
> > gboolean blocked;
> > + gboolean wake_capable;
> > char **uuids;
> > int source, vendor, product, version;
> > char **techno, **t;
> > @@ -3141,6 +3219,11 @@ next:
> > btd_device_set_pnpid(device, source, vendor, product, version);
> > }
> >
> > + /* Mark wake capable */
> > + wake_capable = g_key_file_get_boolean(key_file, "General",
> > + "WakeCapable", NULL);
> > + adapter_set_wake_capable(device->adapter, device, wake_capable == TRUE);
> > +
> > if (store_needed)
> > store_device_info(device);
> > }
> > diff --git a/src/device.h b/src/device.h
> > index 06b100499..43f633e38 100644
> > --- a/src/device.h
> > +++ b/src/device.h
> > @@ -139,6 +139,11 @@ void device_store_svc_chng_ccc(struct btd_device *device, uint8_t bdaddr_type,
> > uint16_t value);
> > void device_load_svc_chng_ccc(struct btd_device *device, uint16_t *ccc_le,
> > uint16_t *ccc_bredr);
> > +bool device_get_profile_wake_support(struct btd_device *device);
> > +void device_set_profile_wake_support(struct btd_device *device,
> > + bool wake_support);
> > +bool device_get_wake_capable(struct btd_device *device);
> > +void device_set_wake_capable(struct btd_device *device, bool wake_capable);
> >
> > typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
> > void *user_data);
> > --
> > 2.25.0.341.g760bfbb309-goog
> >
>
>
> --
> Luiz Augusto von Dentz
--
Thanks,
Abhishek
