On Thu, 30 Apr 2015 10:48:09 +0300
Eyal Reizer <[email protected]> wrote:
> tty_hci driver exposes a /dev/hci_tty character device node, that intends
> to emulate a generic /dev/ttyX device that would be used by the user-space
> Bluetooth stacks to send/receive data to/from the WL combo-connectivity
> chipsets.
We have an in kernel bluetooth stack, why do we care about this - how
will people test and debug driver interactions with binary only bluetooth
stacks in userspace ?
That aside
Either
- Use the tty layer to provide your interface with a correct "emulation",
or
- Don't pretend to be a tty device in the first place
Your code behaviour is nothing like a tty or indeed much like anything
else sane.
> +int hci_tty_open(struct inode *inod, struct file *file)
> +{
> + int i = 0, err = 0;
> + unsigned long timeleft;
> + struct ti_st *hst;
> +
> + pr_info("inside %s (%p, %p)\n", __func__, inod, file);
> +
> + hst = kzalloc(sizeof(*hst), GFP_KERNEL);
> + file->private_data = hst;
> + hst = file->private_data;
Just crashed if you ran out of memory
> + pr_debug("waiting for registration completion signal from ST");
> + timeleft = wait_for_completion_timeout
> + (&hst->wait_reg_completion,
> + msecs_to_jiffies(BT_REGISTER_TIMEOUT));
> + if (!timeleft) {
> + pr_err("Timeout(%d sec),didn't get reg completion signal from ST",
> + BT_REGISTER_TIMEOUT / 1000);
> + err = -ETIMEDOUT;
> + goto error;
Without de-registering stuff ?
+
> +/** hci_tty_read Function
> + *
> + * Parameters :
> + * @file : File pointer for BT char driver
> + * @data : Data which needs to be passed to APP
> + * @size : Length of the data passesd
> + * offset :
> + * Returns Size of packet received - on success
> + * else suitable error code
> + */
> +ssize_t hci_tty_read(struct file *file, char __user *data, size_t size,
> + loff_t *offset)
> +{
> + int len = 0, tout;
> + struct sk_buff *skb = NULL, *rskb = NULL;
> + struct ti_st *hst;
> +
> + pr_debug("inside %s (%p, %p, %u, %p)\n",
> + __func__, file, data, size, offset);
> +
> + /* Validate input parameters */
> + if ((NULL == file) || (((NULL == data) || (0 == size)))) {
> + pr_err("Invalid input params passed to %s", __func__);
> + return -EINVAL;
> + }
Why ... if they are broken here then the kernel is already crashed.
> +
> + hst = file->private_data;
> +
> + /* cannot come here if poll-ed before reading
> + * if not poll-ed wait on the same wait_q
> + */
> + tout = wait_event_interruptible_timeout(hst->data_q,
> + !skb_queue_empty(&hst->rx_list),
> + msecs_to_jiffies(1000));
> + /* Check for timed out condition */
> + if (0 == tout) {
> + pr_err("Device Read timed out\n");
> + return -ETIMEDOUT;
> + }
> +
> + /* hst->rx_list not empty skb already present */
> + skb = skb_dequeue(&hst->rx_list);
> + if (!skb) {
> + pr_err("dequed skb is null?\n");
> + return -EIO;
> + }
> +
> +#ifdef VERBOSE
> + print_hex_dump(KERN_INFO, ">in>", DUMP_PREFIX_NONE,
> + 16, 1, skb->data, skb->len, 0);
> +#endif
> +
> + /* Forward the data to the user */
> + if (skb->len >= size) {
> + pr_err("FIONREAD not done before read\n");
> + return -ENOMEM;
Even if the user did an FIONREAD this wouldn't work with two threads.
ENOMEM is a nonsense return for it
The semantics of read/write on a tty are not those you have implemented
You don't want to allow users to fill the log with error messages
> + }
> + /* returning skb */
> + rskb = alloc_skb(size, GFP_KERNEL);
> + if (!rskb)
> + return -ENOMEM;
> +
> + /* cb[0] has the pkt_type 0x04 or 0x02 or 0x03 */
> + memcpy(skb_put(rskb, 1), &skb->cb[0], 1);
> + memcpy(skb_put(rskb, skb->len), skb->data, skb->len);
> +
> + if (copy_to_user(data, rskb->data, rskb->len)) {
> + pr_err("unable to copy to user space\n");
Same problem with error messages
> + /* Queue the skb back to head */
> + skb_queue_head(&hst->rx_list, skb);
You've just re-ordered your list if threaded
> +/** hci_tty_ioctl Function
> + * This will peform the functions as directed by the command and command
> + * argument.
> + *
> + * Parameters :
> + * @file : File pointer for BT char driver
> + * @cmd : IOCTL Command
> + * @arg : Command argument for IOCTL command
> + * Returns 0 on success
> + * else suitable error code
> + */
> +static long hci_tty_ioctl(struct file *file,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct sk_buff *skb = NULL;
> + int retcode = 0;
> + struct ti_st *hst;
> +
> + pr_debug("inside %s (%p, %u, %lx)", __func__, file, cmd, arg);
> +
> + /* Validate input parameters */
> + if ((NULL == file) || (0 == cmd)) {
> + pr_err("invalid input parameters passed to %s", __func__);
> + return -EINVAL;
> + }
More nonsense validation
> +
> + hst = file->private_data;
> +
> + switch (cmd) {
> + case FIONREAD:
> + /* Deque the SKB from the head if rx_list is not empty
> + * update the argument with skb->len to provide amount of data
> + * available in the available SKB +1 for the PKT_TYPE
> + * field not provided in data by TI-ST.
> + */
> + skb = skb_dequeue(&hst->rx_list);
> + if (skb) {
> + *(unsigned int *)arg = skb->len + 1;
arg is in userspace is it not - if so you've just added some interesting
holes because you should be using the proper user access functions.
> + /* Re-Store the SKB for furtur Read operations */
> + skb_queue_head(&hst->rx_list, skb);
Re-ordering problems again here.
> + } else {
> + *(unsigned int *)arg = 0;
> + }
> + pr_debug("returning %d\n", *(unsigned int *)arg);
> + break;
> + default:
> + pr_debug("Un-Identified IOCTL %d", cmd);
> + retcode = 0;
> + break;
> + }
> +
> + return retcode;
> +}
> +
> +/** hci_tty_poll Function
> + * This function will wait till some data is received to the hci_tty driver from ST
> + *
> + * Parameters :
> + * @file : File pointer for BT char driver
> + * @wait : POLL wait information
> + * Returns status of POLL on success
> + * else suitable error code
> + */
> +static unsigned int hci_tty_poll(struct file *file, poll_table *wait)
> +{
> + struct ti_st *hst = file->private_data;
> + unsigned long mask = 0;
> +
> + pr_debug("@ %s\n", __func__);
> +
> + /* wait to be completed by st_receive */
> + poll_wait(file, &hst->data_q, wait);
> + pr_debug("poll broke\n");
Why "broke" ?
> +
> + if (!skb_queue_empty(&hst->rx_list)) {
> + pr_debug("rx list que !empty\n");
> + mask |= POLLIN; /* TODO: check app for mask */
> + }
> +
Alan
Hi Dotan,
>>>>>> tty_hci driver exposes a /dev/hci_tty character device node, that
>>>>>> intends to emulate a generic /dev/ttyX device that would be used by
>>>>>> the user-space Bluetooth stacks to send/receive data to/from the WL
>>>>>> combo-connectivity chipsets.
>>>>>
>>>>> We have an in kernel bluetooth stack, why do we care about this -
>>>>> how will people test and debug driver interactions with binary only
>>>>> bluetooth stacks in userspace ?
>>>>
>>>> NAK to this driver.
>>>>
>>>> If you want to run an external Bluetooth stack, then use HCI User
>>>> Channel. The functionality already exists.
>>>
>>> This is a pretty old driver that has been written a couple of years back and is
>> being used by customers (including android that use a userspace based stack).
>>> If there is another driver already available that provides same functionality,
>> of course we will be happy to use it instead.
>>> Any idea of where we can get info on the HCI User Channel?
>>
>> search for my BlueZ for Android presentation from Android Builders Summit
>> from 2014. It has an explanation on how HCI User Channel works.
>>
>> Otherwise look into bluez.git since it contains a bunch of examples on how to
>> use the HCI User Channel. It is a generic functionality for raw access to the HCI
>> layer. Look for HCI_CHANNEL_USER.
>>
>>>> We do not want to have two drivers for the same hardware. Work with
>>>> the Bluetooth stack that is present in the kernel and please stop hacking
>> around it.
>>>>
>>> There are in fact quite a few products that use commercial user space based
>> Bluetooth stacks.
>>> One reason is that bluez has not been certified but I assume they have
>> other reasons as well.
>>
>> Please stop this FUD. BlueZ has been qualified multiple times.
>>
>> And let me tell you this, when doing BlueZ for Android, we went through a
>> lengthy period of qualification and fixing PTS with 65 errata today. I don't
>> know if any commercial stack was actually such a good citizen in ensuring that
>> the certification tools get fixed. And we documented all of it.
>>
>
> No argue here, as a matter of fact, we (TI) certified BlueZ several times in the past. Our customers are using large variety of Kernel versions and might use a different version of BlueZ. Our BQE requested us to certify the stack per kernel and per BlueZ version which is practically impossible (due to certification costs and the time it takes).
> If you are familiar with a way to bypass it, that would be great.
that is why we documented everything for BlueZ for Android in such detail so that the whole host stack can be qualified using PTS. It still means that you have to run 1000+ test cases, but at least you have the full documentation with detailed instructions for it.
Automating PTS is work in progress actually. So that you can just start the process and once it is done, you get your evidence for qualification.
Regards
Marcel
Hi Marcel
>-----Original Message-----
>From: Marcel Holtmann [mailto:[email protected]]
>Sent: Thursday, April 30, 2015 5:58 PM
>To: Reizer, Eyal
>Cc: One Thousand Gnomes; Eyal Reizer; [email protected]; Pavan
>Savoy; Mahaveer, Vishal; [email protected]; Ziv, Dotan
>Subject: Re: [PATCH] Bluetooth: Add tty HCI driver
>
>Hi Eyal,
>
>>>>> tty_hci driver exposes a /dev/hci_tty character device node, that
>>>>> intends to emulate a generic /dev/ttyX device that would be used by
>>>>> the user-space Bluetooth stacks to send/receive data to/from the WL
>>>>> combo-connectivity chipsets.
>>>>
>>>> We have an in kernel bluetooth stack, why do we care about this -
>>>> how will people test and debug driver interactions with binary only
>>>> bluetooth stacks in userspace ?
>>>
>>> NAK to this driver.
>>>
>>> If you want to run an external Bluetooth stack, then use HCI User
>>> Channel. The functionality already exists.
>>
>> This is a pretty old driver that has been written a couple of years back=
and is
>being used by customers (including android that use a userspace based stac=
k).
>> If there is another driver already available that provides same function=
ality,
>of course we will be happy to use it instead.
>> Any idea of where we can get info on the HCI User Channel?
>
>search for my BlueZ for Android presentation from Android Builders Summit
>from 2014. It has an explanation on how HCI User Channel works.
>
>Otherwise look into bluez.git since it contains a bunch of examples on how=
to
>use the HCI User Channel. It is a generic functionality for raw access to =
the HCI
>layer. Look for HCI_CHANNEL_USER.
>
>>> We do not want to have two drivers for the same hardware. Work with
>>> the Bluetooth stack that is present in the kernel and please stop hacki=
ng
>around it.
>>>
>> There are in fact quite a few products that use commercial user space ba=
sed
>Bluetooth stacks.
>> One reason is that bluez has not been certified but I assume they have
>other reasons as well.
>
>Please stop this FUD. BlueZ has been qualified multiple times.
>
>And let me tell you this, when doing BlueZ for Android, we went through a
>lengthy period of qualification and fixing PTS with 65 errata today. I don=
't
>know if any commercial stack was actually such a good citizen in ensuring =
that
>the certification tools get fixed. And we documented all of it.
>
No argue here, as a matter of fact, we (TI) certified BlueZ several times i=
n the past. Our customers are using large variety of Kernel versions and mi=
ght use a different version of BlueZ. Our BQE requested us to certify the s=
tack per kernel and per BlueZ version which is practically impossible (due =
to certification costs and the time it takes).=20
If you are familiar with a way to bypass it, that would be great.
>Clone the bluez.git tree and look for android/{pts,pics,pixit}-*.txt files=
. They
>contain all details for qualifying BlueZ for Android stack.
>
>Regards
>
>Marcel
Regards,
Dotan
Hi Eyal,
>>>> tty_hci driver exposes a /dev/hci_tty character device node, that
>>>> intends to emulate a generic /dev/ttyX device that would be used by
>>>> the user-space Bluetooth stacks to send/receive data to/from the WL
>>>> combo-connectivity chipsets.
>>>
>>> We have an in kernel bluetooth stack, why do we care about this - how
>>> will people test and debug driver interactions with binary only
>>> bluetooth stacks in userspace ?
>>
>> NAK to this driver.
>>
>> If you want to run an external Bluetooth stack, then use HCI User Channel. The
>> functionality already exists.
>
> This is a pretty old driver that has been written a couple of years back and is being used by customers (including android that use a userspace based stack).
> If there is another driver already available that provides same functionality, of course we will be happy to use it instead.
> Any idea of where we can get info on the HCI User Channel?
search for my BlueZ for Android presentation from Android Builders Summit from 2014. It has an explanation on how HCI User Channel works.
Otherwise look into bluez.git since it contains a bunch of examples on how to use the HCI User Channel. It is a generic functionality for raw access to the HCI layer. Look for HCI_CHANNEL_USER.
>> We do not want to have two drivers for the same hardware. Work with the
>> Bluetooth stack that is present in the kernel and please stop hacking around it.
>>
> There are in fact quite a few products that use commercial user space based Bluetooth stacks.
> One reason is that bluez has not been certified but I assume they have other reasons as well.
Please stop this FUD. BlueZ has been qualified multiple times.
And let me tell you this, when doing BlueZ for Android, we went through a lengthy period of qualification and fixing PTS with 65 errata today. I don't know if any commercial stack was actually such a good citizen in ensuring that the certification tools get fixed. And we documented all of it.
Clone the bluez.git tree and look for android/{pts,pics,pixit}-*.txt files. They contain all details for qualifying BlueZ for Android stack.
Regards
Marcel
Hi,
> -----Original Message-----
> From: Marcel Holtmann [mailto:[email protected]]
> Sent: Thursday, April 30, 2015 5:25 PM
> To: One Thousand Gnomes
> Cc: Eyal Reizer; [email protected]; Reizer, Eyal; Pavan Savoy;
> Mahaveer, Vishal; [email protected]
> Subject: Re: [PATCH] Bluetooth: Add tty HCI driver
>=20
> Hi,
>=20
> >> tty_hci driver exposes a /dev/hci_tty character device node, that
> >> intends to emulate a generic /dev/ttyX device that would be used by
> >> the user-space Bluetooth stacks to send/receive data to/from the WL
> >> combo-connectivity chipsets.
> >
> > We have an in kernel bluetooth stack, why do we care about this - how
> > will people test and debug driver interactions with binary only
> > bluetooth stacks in userspace ?
>=20
> NAK to this driver.
>=20
> If you want to run an external Bluetooth stack, then use HCI User Channel=
. The
> functionality already exists.
This is a pretty old driver that has been written a couple of years back an=
d is being used by customers (including android that use a userspace based =
stack).
If there is another driver already available that provides same functionali=
ty, of course we will be happy to use it instead.=20
Any idea of where we can get info on the HCI User Channel?
>=20
> We do not want to have two drivers for the same hardware. Work with the
> Bluetooth stack that is present in the kernel and please stop hacking aro=
und it.
>=20
There are in fact quite a few products that use commercial user space based=
Bluetooth stacks.
One reason is that bluez has not been certified but I assume they have othe=
r reasons as well.
Best Regards,
Eyal
Hi,
>> tty_hci driver exposes a /dev/hci_tty character device node, that intends
>> to emulate a generic /dev/ttyX device that would be used by the user-space
>> Bluetooth stacks to send/receive data to/from the WL combo-connectivity
>> chipsets.
>
> We have an in kernel bluetooth stack, why do we care about this - how
> will people test and debug driver interactions with binary only bluetooth
> stacks in userspace ?
NAK to this driver.
If you want to run an external Bluetooth stack, then use HCI User Channel. The functionality already exists.
We do not want to have two drivers for the same hardware. Work with the Bluetooth stack that is present in the kernel and please stop hacking around it.
Regards
Marcel