[v2] profiles/audio/ccp: Add initial code for ccp plugin

Commit Message

Shahid Vichhi Feb. 18, 2024, 11:54 p.m. UTC

This adds initial code for ccp plugin which handle call control profile and
Telephone Bearer Services and Generic Telephone Bearer Services for
client role.
---
 Makefile.plugins     |   5 +
 configure.ac         |   4 +
 profiles/audio/ccp.c | 238 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 247 insertions(+)
 create mode 100644 profiles/audio/ccp.c

Comments

Paul Menzel Feb. 19, 2024, 7:36 a.m. UTC | #1

Dear Shahid,


Thank you very much for your patch. Should the string BlueZ be part of 
the tag? Also, “Add … code” is redundant in my opinion, as that often 
done. Maybe:

     Add CCP plugin for Call Control Profile

Am 19.02.24 um 00:54 schrieb Shahid Vichhi:
> This adds initial code for ccp plugin which handle call control profile and

handle*s* and I would capitalize Call Control Profile

> Telephone Bearer Services and Generic Telephone Bearer Services for
> client role.

Please document, which specification you used, and how you tested this.

I comment some more nits below.

> ---
>   Makefile.plugins     |   5 +
>   configure.ac         |   4 +
>   profiles/audio/ccp.c | 238 +++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 247 insertions(+)
>   create mode 100644 profiles/audio/ccp.c
> 
> diff --git a/Makefile.plugins b/Makefile.plugins
> index e960eedeabd3..4aa2c9c92854 100644
> --- a/Makefile.plugins
> +++ b/Makefile.plugins
> @@ -138,6 +138,11 @@ builtin_modules += micp
>   builtin_sources += profiles/audio/micp.c
>   endif
>   
> +if CCP
> +builtin_modules += ccp
> +builtin_sources += profiles/audio/ccp.c
> +endif
> +
>   if CSIP
>   builtin_modules += csip
>   builtin_sources += profiles/audio/csip.c
> diff --git a/configure.ac b/configure.ac
> index 70e9d4be8127..c7dadf79a565 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -200,6 +200,10 @@ AC_ARG_ENABLE(mcp, AS_HELP_STRING([--disable-mcp],
>           [disable MCP profile]), [enable_mcp=${enableval}])
>   AM_CONDITIONAL(MCP, test "${enable_mcp}" != "no")
>   
> +AC_ARG_ENABLE(ccp, AS_HELP_STRING([--disable-ccp],
> +        [disable CCP profile]), [enable_ccp=${enableval}])

I would also add the long name in the help text: Call Control Profile.

> +AM_CONDITIONAL(CCP, test "${enable_ccp}" != "no")
> +
>   AC_ARG_ENABLE(vcp, AS_HELP_STRING([--disable-vcp],
>   		[disable VCP profile]), [enable_vcp=${enableval}])
>   AM_CONDITIONAL(VCP, test "${enable_vcp}" != "no")
> diff --git a/profiles/audio/ccp.c b/profiles/audio/ccp.c
> new file mode 100644
> index 000000000000..fe97911e75f4
> --- /dev/null
> +++ b/profiles/audio/ccp.c
> @@ -0,0 +1,238 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2024  Intel Corporation. All rights reserved.
> + *
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#define _GNU_SOURCE
> +
> +#include <ctype.h>
> +#include <stdbool.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +
> +#include "gdbus/gdbus.h"
> +
> +#include "lib/bluetooth.h"
> +#include "lib/hci.h"
> +#include "lib/sdp.h"
> +#include "lib/uuid.h"
> +
> +#include "src/dbus-common.h"
> +#include "src/shared/util.h"
> +#include "src/shared/att.h"
> +#include "src/shared/queue.h"
> +#include "src/shared/gatt-db.h"
> +#include "src/shared/gatt-client.h"
> +#include "src/shared/gatt-server.h"
> +#include "src/shared/ccp.h"
> +
> +#include "btio/btio.h"
> +#include "src/plugin.h"
> +#include "src/adapter.h"
> +#include "src/gatt-database.h"
> +#include "src/device.h"
> +#include "src/profile.h"
> +#include "src/service.h"
> +#include "src/log.h"
> +#include "src/error.h"
> +
> +#define GTBS_UUID_STR "0000184C-0000-1000-8000-00805f9b34fb"
> +
> +struct ccp_data {
> +	struct btd_device *device;
> +	struct btd_service *service;
> +	struct bt_ccp *ccp;
> +	unsigned int state_id;
> +};
> +
> +static void ccp_debug(const char *str, void *user_data)
> +{
> +	DBG_IDX(0xffff, "%s", str);
> +}
> +
> +static struct ccp_data *ccp_data_new(struct btd_device *device)
> +{
> +	struct ccp_data *data;
> +
> +	data = new0(struct ccp_data, 1);
> +	data->device = device;
> +
> +	return data;
> +}
> +
> +static int ccp_probe(struct btd_service *service)
> +{
> +	struct btd_device *device = btd_service_get_device(service);
> +	struct btd_adapter *adapter = device_get_adapter(device);
> +	struct btd_gatt_database *database = btd_adapter_get_database(adapter);
> +	struct ccp_data *data = btd_service_get_user_data(service);
> +	char addr[18];
> +
> +	ba2str(device_get_address(device), addr);
> +	DBG("%s", addr);
> +
> +	/* Ignore, if we were probed for this device already */

The comment is redundant, as the error message says the same.

> +	if (data) {
> +		error("Profile probed twice for the same device!");
> +		return -EINVAL;
> +	}
> +
> +	data = ccp_data_new(device);
> +	data->service = service;
> +
> +	data->ccp = bt_ccp_new(btd_gatt_database_get_db(database),
> +			       btd_device_get_gatt_db(device));
> +
> +	bt_ccp_set_debug(data->ccp, ccp_debug, NULL, NULL);
> +	btd_service_set_user_data(service, data);
> +
> +	return 0;
> +}
> +
> +static void ccp_data_free(struct ccp_data *data)
> +{
> +	if (data->service) {
> +		btd_service_set_user_data(data->service, NULL);
> +		bt_ccp_set_user_data(data->ccp, NULL);
> +	}
> +
> +	bt_ccp_unref(data->ccp);
> +	free(data);
> +}
> +
> +static void ccp_data_remove(struct ccp_data *data)
> +{
> +	DBG("data %p", data);
> +
> +	ccp_data_free(data);
> +}
> +
> +static void ccp_remove(struct btd_service *service)
> +{
> +	struct btd_device *device = btd_service_get_device(service);
> +	struct ccp_data *data;
> +	char addr[18];
> +
> +	ba2str(device_get_address(device), addr);
> +	DBG("%s", addr);
> +
> +	data = btd_service_get_user_data(service);
> +	if (!data) {
> +		error("CCP service not handled by profile");
> +		return;
> +	}
> +
> +	ccp_data_remove(data);
> +}
> +
> +static int ccp_accept(struct btd_service *service)
> +{
> +	struct btd_device *device = btd_service_get_device(service);
> +	struct bt_gatt_client *client = btd_device_get_gatt_client(device);
> +	struct ccp_data *data = btd_service_get_user_data(service);
> +	char addr[18];
> +
> +	ba2str(device_get_address(device), addr);
> +	DBG("%s", addr);
> +
> +	if (!bt_ccp_attach(data->ccp, client)) {
> +		error("VCP unable to attach");

What is VCP?

> +		return -EINVAL;
> +	}
> +
> +	/*TODO: register telephony operations here*/

I’d add a space after /* and before */.

> +
> +	btd_service_connecting_complete(service, 0);
> +
> +	return 0;
> +}
> +
> +static int ccp_connect(struct btd_service *service)
> +{
> +	struct btd_device *device = btd_service_get_device(service);
> +	char addr[18];
> +
> +	ba2str(device_get_address(device), addr);
> +	DBG("%s", addr);
> +
> +	return 0;
> +}
> +
> +static int ccp_disconnect(struct btd_service *service)
> +{
> +	struct btd_device *device = btd_service_get_device(service);
> +	struct ccp_data *data = btd_service_get_user_data(service);
> +	char addr[18];
> +
> +	ba2str(device_get_address(device), addr);
> +	DBG("%s", addr);
> +
> +	bt_ccp_detach(data->ccp);
> +
> +	btd_service_disconnecting_complete(service, 0);
> +
> +	return 0;
> +}
> +
> +static int
> +ccp_server_probe(struct btd_profile *p,
> +		 struct btd_adapter *adapter)

Breaking the line like this looks inconsistent with above.

> +{
> +	struct btd_gatt_database *database = btd_adapter_get_database(adapter);
> +
> +	bt_ccp_register(btd_gatt_database_get_db(database));
> +
> +	return 0;
> +}
> +
> +static void
> +ccp_server_remove(struct btd_profile *p,
> +		  struct btd_adapter *adapter)
> +{
> +	DBG("CCP remove Adapter");

I’d use lower-case adapter.

> +}
> +
> +static struct btd_profile ccp_profile = {
> +	.name			= "ccp",
> +	.priority		= BTD_PROFILE_PRIORITY_MEDIUM,
> +	.remote_uuid	= GTBS_UUID_STR,
> +	.device_probe	= ccp_probe,
> +	.device_remove	= ccp_remove,
> +	.accept			= ccp_accept,
> +	.connect		= ccp_connect,
> +	.disconnect		= ccp_disconnect,
> +
> +	.adapter_probe	= ccp_server_probe,
> +	.adapter_remove = ccp_server_remove,
> +
> +	.experimental	= true,
> +};
> +
> +static int ccp_init(void)
> +{
> +	return btd_profile_register(&ccp_profile);
> +}
> +
> +static void ccp_exit(void)
> +{
> +	btd_profile_unregister(&ccp_profile);
> +}
> +
> +BLUETOOTH_PLUGIN_DEFINE(ccp, VERSION, BLUETOOTH_PLUGIN_PRIORITY_DEFAULT,
> +			ccp_init, ccp_exit)


Kind regards,

Paul

[e-mail] Feb. 19, 2024, 8:09 a.m. UTC | #2

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=827344

---Test result---

Test Summary:
CheckPatch                    PASS      0.36 seconds
GitLint                       PASS      0.19 seconds
BuildEll                      PASS      24.11 seconds
BluezMake                     PASS      745.45 seconds
MakeCheck                     PASS      11.58 seconds
MakeDistcheck                 PASS      163.30 seconds
CheckValgrind                 PASS      225.72 seconds
CheckSmatch                   PASS      330.98 seconds
bluezmakeextell               PASS      107.32 seconds
IncrementalBuild              PASS      685.96 seconds
ScanBuild                     WARNING   990.69 seconds

Details
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
Makefile:13722: warning: overriding recipe for target 'install-data-hook'
Makefile:13717: warning: ignoring old recipe for target 'install-data-hook'
Makefile:13722: warning: overriding recipe for target 'install-data-hook'
Makefile:13717: warning: ignoring old recipe for target 'install-data-hook'
src/shared/gatt-client.c:451:21: warning: Use of memory after it is freed
        gatt_db_unregister(op->client->db, op->db_id);
                           ^~~~~~~~~~
src/shared/gatt-client.c:696:2: warning: Use of memory after it is freed
        discovery_op_complete(op, false, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:993:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1099:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1291:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1356:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1631:6: warning: Use of memory after it is freed
        if (read_db_hash(op)) {
            ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1636:2: warning: Use of memory after it is freed
        discover_all(op);
        ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2140:6: warning: Use of memory after it is freed
        if (read_db_hash(op)) {
            ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2148:8: warning: Use of memory after it is freed
                                                        discovery_op_ref(op),
                                                        ^~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3237:2: warning: Use of memory after it is freed
        complete_write_long_op(req, success, 0, false);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3259:2: warning: Use of memory after it is freed
        request_unref(req);
        ^~~~~~~~~~~~~~~~~~
12 warnings generated.



---
Regards,
Linux Bluetooth

Patch

diff --git a/Makefile.plugins b/Makefile.plugins
index e960eedeabd3..4aa2c9c92854 100644
--- a/Makefile.plugins
+++ b/Makefile.plugins
@@ -138,6 +138,11 @@  builtin_modules += micp
 builtin_sources += profiles/audio/micp.c
 endif
 
+if CCP
+builtin_modules += ccp
+builtin_sources += profiles/audio/ccp.c
+endif
+
 if CSIP
 builtin_modules += csip
 builtin_sources += profiles/audio/csip.c
diff --git a/configure.ac b/configure.ac
index 70e9d4be8127..c7dadf79a565 100644
--- a/configure.ac
+++ b/configure.ac
@@ -200,6 +200,10 @@  AC_ARG_ENABLE(mcp, AS_HELP_STRING([--disable-mcp],
         [disable MCP profile]), [enable_mcp=${enableval}])
 AM_CONDITIONAL(MCP, test "${enable_mcp}" != "no")
 
+AC_ARG_ENABLE(ccp, AS_HELP_STRING([--disable-ccp],
+        [disable CCP profile]), [enable_ccp=${enableval}])
+AM_CONDITIONAL(CCP, test "${enable_ccp}" != "no")
+
 AC_ARG_ENABLE(vcp, AS_HELP_STRING([--disable-vcp],
 		[disable VCP profile]), [enable_vcp=${enableval}])
 AM_CONDITIONAL(VCP, test "${enable_vcp}" != "no")
diff --git a/profiles/audio/ccp.c b/profiles/audio/ccp.c
new file mode 100644
index 000000000000..fe97911e75f4
--- /dev/null
+++ b/profiles/audio/ccp.c
@@ -0,0 +1,238 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2024  Intel Corporation. All rights reserved.
+ *
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+
+#include <ctype.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <errno.h>
+
+#include <glib.h>
+
+#include "gdbus/gdbus.h"
+
+#include "lib/bluetooth.h"
+#include "lib/hci.h"
+#include "lib/sdp.h"
+#include "lib/uuid.h"
+
+#include "src/dbus-common.h"
+#include "src/shared/util.h"
+#include "src/shared/att.h"
+#include "src/shared/queue.h"
+#include "src/shared/gatt-db.h"
+#include "src/shared/gatt-client.h"
+#include "src/shared/gatt-server.h"
+#include "src/shared/ccp.h"
+
+#include "btio/btio.h"
+#include "src/plugin.h"
+#include "src/adapter.h"
+#include "src/gatt-database.h"
+#include "src/device.h"
+#include "src/profile.h"
+#include "src/service.h"
+#include "src/log.h"
+#include "src/error.h"
+
+#define GTBS_UUID_STR "0000184C-0000-1000-8000-00805f9b34fb"
+
+struct ccp_data {
+	struct btd_device *device;
+	struct btd_service *service;
+	struct bt_ccp *ccp;
+	unsigned int state_id;
+};
+
+static void ccp_debug(const char *str, void *user_data)
+{
+	DBG_IDX(0xffff, "%s", str);
+}
+
+static struct ccp_data *ccp_data_new(struct btd_device *device)
+{
+	struct ccp_data *data;
+
+	data = new0(struct ccp_data, 1);
+	data->device = device;
+
+	return data;
+}
+
+static int ccp_probe(struct btd_service *service)
+{
+	struct btd_device *device = btd_service_get_device(service);
+	struct btd_adapter *adapter = device_get_adapter(device);
+	struct btd_gatt_database *database = btd_adapter_get_database(adapter);
+	struct ccp_data *data = btd_service_get_user_data(service);
+	char addr[18];
+
+	ba2str(device_get_address(device), addr);
+	DBG("%s", addr);
+
+	/* Ignore, if we were probed for this device already */
+	if (data) {
+		error("Profile probed twice for the same device!");
+		return -EINVAL;
+	}
+
+	data = ccp_data_new(device);
+	data->service = service;
+
+	data->ccp = bt_ccp_new(btd_gatt_database_get_db(database),
+			       btd_device_get_gatt_db(device));
+
+	bt_ccp_set_debug(data->ccp, ccp_debug, NULL, NULL);
+	btd_service_set_user_data(service, data);
+
+	return 0;
+}
+
+static void ccp_data_free(struct ccp_data *data)
+{
+	if (data->service) {
+		btd_service_set_user_data(data->service, NULL);
+		bt_ccp_set_user_data(data->ccp, NULL);
+	}
+
+	bt_ccp_unref(data->ccp);
+	free(data);
+}
+
+static void ccp_data_remove(struct ccp_data *data)
+{
+	DBG("data %p", data);
+
+	ccp_data_free(data);
+}
+
+static void ccp_remove(struct btd_service *service)
+{
+	struct btd_device *device = btd_service_get_device(service);
+	struct ccp_data *data;
+	char addr[18];
+
+	ba2str(device_get_address(device), addr);
+	DBG("%s", addr);
+
+	data = btd_service_get_user_data(service);
+	if (!data) {
+		error("CCP service not handled by profile");
+		return;
+	}
+
+	ccp_data_remove(data);
+}
+
+static int ccp_accept(struct btd_service *service)
+{
+	struct btd_device *device = btd_service_get_device(service);
+	struct bt_gatt_client *client = btd_device_get_gatt_client(device);
+	struct ccp_data *data = btd_service_get_user_data(service);
+	char addr[18];
+
+	ba2str(device_get_address(device), addr);
+	DBG("%s", addr);
+
+	if (!bt_ccp_attach(data->ccp, client)) {
+		error("VCP unable to attach");
+		return -EINVAL;
+	}
+
+	/*TODO: register telephony operations here*/
+
+	btd_service_connecting_complete(service, 0);
+
+	return 0;
+}
+
+static int ccp_connect(struct btd_service *service)
+{
+	struct btd_device *device = btd_service_get_device(service);
+	char addr[18];
+
+	ba2str(device_get_address(device), addr);
+	DBG("%s", addr);
+
+	return 0;
+}
+
+static int ccp_disconnect(struct btd_service *service)
+{
+	struct btd_device *device = btd_service_get_device(service);
+	struct ccp_data *data = btd_service_get_user_data(service);
+	char addr[18];
+
+	ba2str(device_get_address(device), addr);
+	DBG("%s", addr);
+
+	bt_ccp_detach(data->ccp);
+
+	btd_service_disconnecting_complete(service, 0);
+
+	return 0;
+}
+
+static int
+ccp_server_probe(struct btd_profile *p,
+		 struct btd_adapter *adapter)
+{
+	struct btd_gatt_database *database = btd_adapter_get_database(adapter);
+
+	bt_ccp_register(btd_gatt_database_get_db(database));
+
+	return 0;
+}
+
+static void
+ccp_server_remove(struct btd_profile *p,
+		  struct btd_adapter *adapter)
+{
+	DBG("CCP remove Adapter");
+}
+
+static struct btd_profile ccp_profile = {
+	.name			= "ccp",
+	.priority		= BTD_PROFILE_PRIORITY_MEDIUM,
+	.remote_uuid	= GTBS_UUID_STR,
+	.device_probe	= ccp_probe,
+	.device_remove	= ccp_remove,
+	.accept			= ccp_accept,
+	.connect		= ccp_connect,
+	.disconnect		= ccp_disconnect,
+
+	.adapter_probe	= ccp_server_probe,
+	.adapter_remove = ccp_server_remove,
+
+	.experimental	= true,
+};
+
+static int ccp_init(void)
+{
+	return btd_profile_register(&ccp_profile);
+}
+
+static void ccp_exit(void)
+{
+	btd_profile_unregister(&ccp_profile);
+}
+
+BLUETOOTH_PLUGIN_DEFINE(ccp, VERSION, BLUETOOTH_PLUGIN_PRIORITY_DEFAULT,
+			ccp_init, ccp_exit)