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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>> ---
>> 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
>>
>
>
