Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3704-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <85d93186-f792-4b53-b885-fb2630dd9c6c@quicinc.com>
Date: Thu, 18 Apr 2024 08:44:00 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 3/4] Bluetooth: hci_ldisc: Add a ioctl
 HCIUARTSETPROTODATA
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
CC: <luiz.von.dentz@intel.com>, <marcel@holtmann.org>,
        <linux-bluetooth@vger.kernel.org>, <",linux-kernel"@vger.kernel.org>
References: <1710914907-30012-1-git-send-email-quic_zijuhu@quicinc.com>
 <1713358336-29619-1-git-send-email-quic_zijuhu@quicinc.com>
 <1713358336-29619-4-git-send-email-quic_zijuhu@quicinc.com>
 <CABBYNZKPK8d2yKKDnpNrr0vYj1u4wygvL-bSuDva31mY-4BwEw@mail.gmail.com>
Content-Language: en-US
From: quic_zijuhu <quic_zijuhu@quicinc.com>
In-Reply-To: <CABBYNZKPK8d2yKKDnpNrr0vYj1u4wygvL-bSuDva31mY-4BwEw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

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