| Message ID | 1713358336-29619-1-git-send-email-quic_zijuhu@quicinc.com |
|---|---|
| Headers | show |
| Series | Fix 2 tool btattach issues for QCA controllers | expand |
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 >
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 >> > >
BT chip vendor and customer often needs to use tool btattach to perform various development,verification and evaluation test for vendor's BT module, but tool btattach is not working fine for any QCA BT controller currently, this patch series are to solve this issue. There are many QCA soc types defined by drivers/bluetooth/btqca.h enum qca_btsoc_type { QCA_INVALID = -1, QCA_AR3002, QCA_ROME, QCA_WCN3988, QCA_WCN3990, QCA_WCN3998, QCA_WCN3991, QCA_QCA2066, QCA_QCA6390, QCA_WCN6750, QCA_WCN6855, QCA_WCN7850, QCA_MAX, }; and every soc type stands for a kind of QCA BT controller, this patch series are to solve below 2 tool btattach issues for QCA soc types: 1) tool btattach will cause kernel crash when used for QCA_ROME 2) tool btattach does not support any other soc type except QCA_ROME For hci_uart derived from tool btattach, it is allocated by hci_ldisc and is Non-serdev device, its @serdev is NULL and its soc type is currenlty hard coded as QCA_ROME. The 1st issue is caused by dereferencing nullptr hu->serdev, in order to solve the 2nd issue, a new ioctl is introduced for user to specify soc type by a new added tool btattach option. Changes v1 -> v2 - Add patch 2/4 - Correct cover letter Zijun Hu (4): Bluetooth: qca: Fix crash caused by tool btattach for QCA_ROME Bluetooth: qca: Fix nullptr dereference for non-serdev devices Bluetooth: hci_ldisc: Add a ioctl HCIUARTSETPROTODATA Bluetooth: qca: Fix wrong soc type returned for tool btattach drivers/bluetooth/btqca.h | 1 + drivers/bluetooth/hci_ldisc.c | 10 ++++++++++ drivers/bluetooth/hci_qca.c | 19 +++++++++++++------ drivers/bluetooth/hci_uart.h | 3 +++ 4 files changed, 27 insertions(+), 6 deletions(-)