MIME-Version: 1.0
References: <20180316221221.130478-1-marcel@holtmann.org>
In-Reply-To: <20180316221221.130478-1-marcel@holtmann.org>
From:   Emil Lenngren <emil.lenngren@gmail.com>
Date:   Fri, 4 Jan 2019 11:24:29 +0100
Message-ID: <CAO1O6scQR_3t28DPUQsY-yONc1JMVaFHZejgGAK2kv-vHY=Qtw@mail.gmail.com>
Subject: Re: [RFC] Bluetooth: Add new serdev based driver for 3-Wire attached controllers
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Bluez mailing list <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Marcel,
a few questions below

Den fre 16 mars 2018 kl 23:13 skrev Marcel Holtmann <marcel@holtmann.org>:
>
> This is a from scratch written driver to run H:5 aka 3-Wire on serdev based
> system with a Bluetooth controller attached via an UART. It is currently
> tested on RPi3. It works only without Broadcom integration and that means
> firmware loading is not supported. It is easy to add, but in that case
> some packet errors happen that are not yet debugged.
>
> It does support data integrity check, but it does not actually support
> the sliding window. It is currently limit to 1 packet and on the
> receiver side there is no packet loss detection.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> +static void bt3wire_tx_work(struct work_struct *work)
> +{
> +       struct bt3wire_dev *bdev = container_of(work, struct bt3wire_dev,
> +                                              tx_work);
> +       struct serdev_device *serdev = bdev->serdev;
> +       struct hci_dev *hdev = bdev->hdev;
> +
> +       while (1) {
> +               clear_bit(BT3WIRE_TX_STATE_WAKEUP, &bdev->tx_state);
> +
> +               while (1) {
> +                       struct sk_buff *skb = skb_dequeue(&bdev->tx_queue);
> +                       int len;
> +
> +                       if (!skb)
> +                               break;
> +
> +                       len = serdev_device_write_buf(serdev, skb->data,
> +                                                     skb->len);
> +                       hdev->stat.byte_tx += len;
> +
> +                       skb_pull(skb, len);
> +                       if (skb->len > 0) {
> +                               skb_queue_head(&bdev->tx_queue, skb);
> +                               break;
> +                       }
> +
> +                       kfree_skb(skb);
> +               }
> +
> +               if (!test_bit(BT3WIRE_TX_STATE_WAKEUP, &bdev->tx_state))
> +                       break;
> +       }
> +
> +       clear_bit(BT3WIRE_TX_STATE_ACTIVE, &bdev->tx_state);
> +}
> +
> +static int bt3wire_tx_wakeup(struct bt3wire_dev *bdev)
> +{
> +       if (test_and_set_bit(BT3WIRE_TX_STATE_ACTIVE, &bdev->tx_state)) {
> +               set_bit(BT3WIRE_TX_STATE_WAKEUP, &bdev->tx_state);
> +               return 0;
> +       }
> +
> +       schedule_work(&bdev->tx_work);
> +       return 0;
> +}
> +
> +static int bt3wire_open(struct hci_dev *hdev)
> +{
> +       struct bt3wire_dev *bdev = hci_get_drvdata(hdev);
> +       int err;
> +
> +       bdev->tx_seq = 0;
> +       bdev->tx_ack = 0;
> +
> +       bdev->rx_slip_state = SLIP_WAIT_DELIM;
> +       bdev->rx_skb = NULL;
> +
> +       err = serdev_device_open(bdev->serdev);
> +       if (err) {
> +               bt_dev_err(hdev, "Unable to open UART device %s",
> +                          dev_name(&bdev->serdev->dev));
> +               return err;
> +       }
> +
> +       serdev_device_set_baudrate(bdev->serdev, 115200);
> +
> +       bdev->link_state = LINK_UNINITIALIZED;
> +       schedule_delayed_work(&bdev->link_timer, 0);
> +
> +       if (!wait_event_interruptible_timeout(bdev->link_wait,
> +                                             bdev->link_state == LINK_ACTIVE,
> +                                             LINK_ACTIVATION_TIMEOUT)) {
> +               bt_dev_err(hdev, "Link activation timeout");
> +               err = -ETIMEDOUT;
> +
> +               cancel_delayed_work_sync(&bdev->link_timer);
> +               serdev_device_close(bdev->serdev);
> +       } else {
> +               bt_dev_info(hdev, "Link activation successful");
> +               err = 0;
> +       }
> +
> +       return err;
> +}
> +
> +static int bt3wire_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +       struct bt3wire_dev *bdev = hci_get_drvdata(hdev);
> +
> +       switch (hci_skb_pkt_type(skb)) {
> +       case HCI_COMMAND_PKT:
> +               hdev->stat.cmd_tx++;
> +               break;
> +       case HCI_ACLDATA_PKT:
> +               hdev->stat.acl_tx++;
> +               break;
> +       case HCI_SCODATA_PKT:
> +               hdev->stat.sco_tx++;
> +               break;
> +       }
> +
> +       bt3wire_queue_pkt(bdev, hci_skb_pkt_type(skb), skb->data, skb->len);
> +       kfree_skb(skb);
> +
> +       bt3wire_tx_wakeup(bdev);
> +       return 0;
> +}
> +
> +static void bt3wire_write_wakeup(struct serdev_device *serdev)
> +{
> +       struct bt3wire_dev *bdev = serdev_device_get_drvdata(serdev);
> +
> +       bt3wire_tx_wakeup(bdev);
> +}
> +

Any specific reason this draft as well as
https://patchwork.kernel.org/patch/10490651/ uses its own code above
for tx work, send frame, set baud rate etc. when there seems to be a
common lib for such things in hci_serdev.c?

I'm also wondering about having the tx work in a separate
workqueue/thread. Wouldn't it be better (fewer context switches) if
the serdev_device_write_buf call was done from the same threads that
1. put the message in the queue and 2. the serdev wake-up callback
(protected by some lock)? It seems serdev_device_write_buf is async
and shouldn't block if I'm correct.
In the current code (whose logic seems to be used by many drivers) I
see a potential race condition if there is a context switch just
before the line "clear_bit(BT3WIRE_TX_STATE_ACTIVE,
&bdev->tx_state);", then another thread calls bt3wire_tx_wakeup, then
the tx work will never run (unless a new packet is enqueued).

About the baud rate, would it be a good idea to control that via a
device tree parameter?

/Emil