diff mbox series

[v2,3/4] Bluetooth: hci_ldisc: Add a ioctl HCIUARTSETPROTODATA

Message ID 1713358336-29619-4-git-send-email-quic_zijuhu@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Fix 2 tool btattach issues for QCA controllers | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/SubjectPrefix success Gitlint PASS

Commit Message

quic_zijuhu April 17, 2024, 12:52 p.m. UTC 
HCIUARTSETPROTODATA is introduced to specify protocol specific settings
prior to HCIUARTSETPROTO, for protocal QCA, it is used by tool btattch
to specify soc type.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/bluetooth/hci_ldisc.c | 10 ++++++++++
 drivers/bluetooth/hci_uart.h  |  3 +++
 2 files changed, 13 insertions(+)

Comments

Luiz Augusto von Dentz April 17, 2024, 9:27 p.m. UTC | #1 
Hi Zijun,

On Wed, Apr 17, 2024 at 8:53 AM Zijun Hu <quic_zijuhu@quicinc.com> wrote:
>
> HCIUARTSETPROTODATA is introduced to specify protocol specific settings
> prior to HCIUARTSETPROTO, for protocal QCA, it is used by tool btattch
> to specify soc type.
>
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/bluetooth/hci_ldisc.c | 10 ++++++++++
>  drivers/bluetooth/hci_uart.h  |  3 +++
>  2 files changed, 13 insertions(+)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index a26367e9fb19..4be09c61bae5 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -506,6 +506,7 @@ static int hci_uart_tty_open(struct tty_struct *tty)
>         /* disable alignment support by default */
>         hu->alignment = 1;
>         hu->padding = 0;
> +       hu->proto_data = 0;
>
>         INIT_WORK(&hu->init_ready, hci_uart_init_work);
>         INIT_WORK(&hu->write_work, hci_uart_write_work);
> @@ -795,6 +796,15 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
>                 err = hu->hdev_flags;
>                 break;
>
> +       case HCIUARTSETPROTODATA:
> +               if (test_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> +                       err = -EBUSY;
> +               } else {
> +                       hu->proto_data = arg;
> +                       BT_DBG("HCIUARTSETPROTODATA %lu okay.", arg);
> +               }
> +               break;
> +
>         default:
>                 err = n_tty_ioctl_helper(tty, cmd, arg);
>                 break;
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 68c8c7e95d64..fc35e9bd4398 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -18,6 +18,8 @@
>  #define HCIUARTGETDEVICE       _IOR('U', 202, int)
>  #define HCIUARTSETFLAGS                _IOW('U', 203, int)
>  #define HCIUARTGETFLAGS                _IOR('U', 204, int)
> +/* Used prior to HCIUARTSETPROTO */
> +#define HCIUARTSETPROTODATA    _IOW('U', 205, unsigned long)

Don't think this will gonna fly, Im not going to introduce vendor
specific like this, besides if the kernel is not able to discover this
data why would userspace be?

>  /* UART protocols */
>  #define HCI_UART_MAX_PROTO     12
> @@ -71,6 +73,7 @@ struct hci_uart {
>         struct work_struct      init_ready;
>         struct work_struct      write_work;
>
> +       unsigned long proto_data;
>         const struct hci_uart_proto *proto;
>         struct percpu_rw_semaphore proto_lock;  /* Stop work for proto close */
>         void                    *priv;
> --
> 2.7.4
>
quic_zijuhu April 18, 2024, 12:44 a.m. UTC | #2 
On 4/18/2024 5:27 AM, Luiz Augusto von Dentz wrote:
> Hi Zijun,
> 
> On Wed, Apr 17, 2024 at 8:53 AM Zijun Hu <quic_zijuhu@quicinc.com> wrote:
>>
>> HCIUARTSETPROTODATA is introduced to specify protocol specific settings
>> prior to HCIUARTSETPROTO, for protocal QCA, it is used by tool btattch
>> to specify soc type.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/bluetooth/hci_ldisc.c | 10 ++++++++++
>>  drivers/bluetooth/hci_uart.h  |  3 +++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index a26367e9fb19..4be09c61bae5 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -506,6 +506,7 @@ static int hci_uart_tty_open(struct tty_struct *tty)
>>         /* disable alignment support by default */
>>         hu->alignment = 1;
>>         hu->padding = 0;
>> +       hu->proto_data = 0;
>>
>>         INIT_WORK(&hu->init_ready, hci_uart_init_work);
>>         INIT_WORK(&hu->write_work, hci_uart_write_work);
>> @@ -795,6 +796,15 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
>>                 err = hu->hdev_flags;
>>                 break;
>>
>> +       case HCIUARTSETPROTODATA:
>> +               if (test_bit(HCI_UART_PROTO_SET, &hu->flags)) {
>> +                       err = -EBUSY;
>> +               } else {
>> +                       hu->proto_data = arg;
>> +                       BT_DBG("HCIUARTSETPROTODATA %lu okay.", arg);
>> +               }
>> +               break;
>> +
>>         default:
>>                 err = n_tty_ioctl_helper(tty, cmd, arg);
>>                 break;
>> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
>> index 68c8c7e95d64..fc35e9bd4398 100644
>> --- a/drivers/bluetooth/hci_uart.h
>> +++ b/drivers/bluetooth/hci_uart.h
>> @@ -18,6 +18,8 @@
>>  #define HCIUARTGETDEVICE       _IOR('U', 202, int)
>>  #define HCIUARTSETFLAGS                _IOW('U', 203, int)
>>  #define HCIUARTGETFLAGS                _IOR('U', 204, int)
>> +/* Used prior to HCIUARTSETPROTO */
>> +#define HCIUARTSETPROTODATA    _IOW('U', 205, unsigned long)
> 
> Don't think this will gonna fly, Im not going to introduce vendor
> specific like this, besides if the kernel is not able to discover this
> data why would userspace be?
> 
i don't think so as explained below.
1)
For the final product, BT device will get many configuration info from board configuration such DT|ACPI during
driver probe phase, But for tool btattach case, it has no way to get such configuration info due to derived from hci_ldisc.
so i introduce a new ioctl to let user specify some such required info when possible to make btattach work.

2) present ioctl HCIUARTSETPROTO has been introduced specify vendor protocol, why can't introduce a new ioctl to specify
protocol specific settings ? is HCIUARTSETPROTO vendor specific?

3) ioctl()'s designed purpose is for variant non-standard settings, do you have suggestions about how to specify device driver specify settings from user
if ioct() is not suitable?

4)
hci_ldisc driver don't parse and touch such user specified settings and pass it into vendor driver directly
does it has any problem?

>>  /* UART protocols */
>>  #define HCI_UART_MAX_PROTO     12
>> @@ -71,6 +73,7 @@ struct hci_uart {
>>         struct work_struct      init_ready;
>>         struct work_struct      write_work;
>>
>> +       unsigned long proto_data;
>>         const struct hci_uart_proto *proto;
>>         struct percpu_rw_semaphore proto_lock;  /* Stop work for proto close */
>>         void                    *priv;
>> --
>> 2.7.4
>>
> 
>
diff mbox series

Patch

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index a26367e9fb19..4be09c61bae5 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -506,6 +506,7 @@  static int hci_uart_tty_open(struct tty_struct *tty)
 	/* disable alignment support by default */
 	hu->alignment = 1;
 	hu->padding = 0;
+	hu->proto_data = 0;
 
 	INIT_WORK(&hu->init_ready, hci_uart_init_work);
 	INIT_WORK(&hu->write_work, hci_uart_write_work);
@@ -795,6 +796,15 @@  static int hci_uart_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
 		err = hu->hdev_flags;
 		break;
 
+	case HCIUARTSETPROTODATA:
+		if (test_bit(HCI_UART_PROTO_SET, &hu->flags)) {
+			err = -EBUSY;
+		} else {
+			hu->proto_data = arg;
+			BT_DBG("HCIUARTSETPROTODATA %lu okay.", arg);
+		}
+		break;
+
 	default:
 		err = n_tty_ioctl_helper(tty, cmd, arg);
 		break;
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 68c8c7e95d64..fc35e9bd4398 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -18,6 +18,8 @@ 
 #define HCIUARTGETDEVICE	_IOR('U', 202, int)
 #define HCIUARTSETFLAGS		_IOW('U', 203, int)
 #define HCIUARTGETFLAGS		_IOR('U', 204, int)
+/* Used prior to HCIUARTSETPROTO */
+#define HCIUARTSETPROTODATA	_IOW('U', 205, unsigned long)
 
 /* UART protocols */
 #define HCI_UART_MAX_PROTO	12
@@ -71,6 +73,7 @@  struct hci_uart {
 	struct work_struct	init_ready;
 	struct work_struct	write_work;
 
+	unsigned long proto_data;
 	const struct hci_uart_proto *proto;
 	struct percpu_rw_semaphore proto_lock;	/* Stop work for proto close */
 	void			*priv;