Hi Loic,
>> A platform device can be used to provide some specific resources in
>> order to manage the controller. In this first patch we retrieve the
>> reset gpio which is used to power on/off the controller.
>>
>> The main issue is to match the current tty with the correct pdev.
>> In case of ACPI, we can easily find the right tty/pdev pair because
>> they are both child of the same UART port.
>>
>> If controller is powered-on from the driver, we need to wait for a
>> HCI boot event before being able to send any command.
>>
>> Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxx>
>> ---
>> v2: v3: none, align patch version with patch 1/2 "Intel baudrate" patch
>> v4: remove device tree support, will be part of a different patch
>> v5: static declaration fixes
>> rework locking usage, remove confusing intel_device_lock/unlock
>> Add intel_acpi_probe, align acpi device table name
>> v6: intel_set_power refactoring
>> move idev search (intel_get_idev) direclty in intel_open
>>
>> drivers/bluetooth/hci_intel.c | 216 +++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 215 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
>> index da3192a..c060fd3 100644
>> --- a/drivers/bluetooth/hci_intel.c
>> +++ b/drivers/bluetooth/hci_intel.c
>> @@ -26,6 +26,10 @@
>> #include <linux/skbuff.h>
>> #include <linux/firmware.h>
>> #include <linux/wait.h>
>> +#include <linux/tty.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/acpi.h>
>>
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> @@ -39,10 +43,20 @@
>> #define STATE_FIRMWARE_FAILED 3
>> #define STATE_BOOTING 4
>>
>> +struct intel_device {
>> + struct list_head list;
>> + struct platform_device *pdev;
>> + struct gpio_desc *reset;
>> + struct hci_uart *hu;
>
> tell me where you are actually using hci_uart in the context of intel_device. I can not make out its usage and you are creating a race condition when you try to clear this. So as long as you do not need it, do not add this. From what I can tell this is dead code at the moment.
>
>> +};
>> +static LIST_HEAD(intel_device_list);
>> +static DEFINE_SPINLOCK(intel_device_list_lock);
>> +
>> struct intel_data {
>> struct sk_buff *rx_skb;
>> struct sk_buff_head txq;
>> unsigned long flags;
>> + struct intel_device *idev;
>> };
>>
>> static u8 intel_convert_speed(unsigned int speed)
>> @@ -77,9 +91,52 @@ static u8 intel_convert_speed(unsigned int speed)
>> }
>> }
>>
>> +static bool intel_device_exists(struct intel_device *idev)
>> +{
>> + struct list_head *p;
>> +
>> + list_for_each(p, &intel_device_list) {
>> + struct intel_device *dev = list_entry(p, struct intel_device,
>> + list);
>> + if (dev == idev)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int intel_set_power(struct hci_uart *hu, bool powered)
>> +{
>> + struct intel_data *intel = hu->priv;
>> + struct intel_device *idev = intel->idev;
>> + int err = 0;
>> +
>> + spin_lock(&intel_device_list_lock);
>> +
>> + if (!intel_device_exists(idev)) {
>> + err = -ENODEV;
>> + goto done;
>> + }
>> +
>> + if (!idev->reset) {
>> + err = -EINVAL;
>> + goto done;
>> + }
>> +
>> + BT_DBG("%s: set power %d", hu->hdev->name, powered);
>> +
>> + gpiod_set_value(idev->reset, powered);
>> +
>> +done:
>> + spin_unlock(&intel_device_list_lock);
>> +
>> + return err;
>> +}
>> +
>> static int intel_open(struct hci_uart *hu)
>> {
>> struct intel_data *intel;
>> + struct list_head *p;
>>
>> BT_DBG("hu %p", hu);
>>
>> @@ -90,6 +147,29 @@ static int intel_open(struct hci_uart *hu)
>> skb_queue_head_init(&intel->txq);
>>
>> hu->priv = intel;
>> +
>> + spin_lock(&intel_device_list_lock);
>> + list_for_each(p, &intel_device_list) {
>> + struct intel_device *idev;
>> +
>> + idev = list_entry(p, struct intel_device, list);
>> +
>> + /* tty device and pdev device should share the same parent
>> + * which is the UART port.
>> + */
>> + if (hu->tty->dev->parent == idev->pdev->dev.parent) {
>> + BT_INFO("hu %p, Found compatible pm device (%s)", hu,
>> + dev_name(&idev->pdev->dev));
>> + intel->idev = idev;
>> + intel->idev->hu = hu;
>> + break;
>> + }
>> + }
>> + spin_unlock(&intel_device_list_lock);
>> +
>> + if (!intel_set_power(hu, true))
>> + set_bit(STATE_BOOTING, &intel->flags);
>> +
>> return 0;
>> }
>>
>> @@ -99,6 +179,13 @@ static int intel_close(struct hci_uart *hu)
>>
>> BT_DBG("hu %p", hu);
>>
>> + intel_set_power(hu, false);
>> +
>> + spin_lock(&intel_device_list_lock);
>> + if (intel_device_exists(intel->idev))
>> + intel->idev->hu = NULL;
>> + spin_unlock(&intel_device_list_lock);
>> +
>> skb_queue_purge(&intel->txq);
>> kfree_skb(intel->rx_skb);
>> kfree(intel);
>> @@ -149,6 +236,26 @@ static int intel_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> struct hci_dev *hdev = hu->hdev;
>> u8 speed_cmd[] = { 0x06, 0xfc, 0x01, 0x00 };
>> struct sk_buff *skb;
>> + int err;
>> +
>> + /* This can be the first command sent to the chip, check controller is
>> + * ready
>> + */
>> + err = wait_on_bit_timeout(&intel->flags, STATE_BOOTING,
>> + TASK_INTERRUPTIBLE,
>> + msecs_to_jiffies(1000));
>> +
>> + if (err == 1) {
>> + BT_ERR("%s: Device boot interrupted", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> + return -EINTR;
>> + }
>> +
>> + if (err) {
>> + BT_ERR("%s: Device boot timeout", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> + /* Try to continue anyway */
>> + }
>>
>> BT_INFO("%s: Change controller speed to %d", hdev->name, speed);
>>
>> @@ -231,6 +338,23 @@ static int intel_setup(struct hci_uart *hu)
>> if (oper_speed && init_speed && oper_speed != init_speed)
>> speed_change = 1;
>>
>> + /* Check controller is ready */
>> + err = wait_on_bit_timeout(&intel->flags, STATE_BOOTING,
>> + TASK_INTERRUPTIBLE,
>> + msecs_to_jiffies(1000));
>> +
>> + if (err == 1) {
>> + BT_ERR("%s: Device boot interrupted", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> + return -EINTR;
>> + }
>> +
>> + if (err) {
>> + BT_ERR("%s: Device boot timeout", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> + /* Try to continue anyway */
>> + }
>> +
>> set_bit(STATE_BOOTLOADER, &intel->flags);
>>
>> /* Read the Intel version information to determine if the device
>> @@ -546,11 +670,13 @@ done:
>>
>> if (err == 1) {
>> BT_ERR("%s: Device boot interrupted", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> return -EINTR;
>> }
>>
>> if (err) {
>> BT_ERR("%s: Device boot timeout", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> return -ETIMEDOUT;
>> }
>>
>> @@ -584,7 +710,8 @@ static int intel_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
>> struct intel_data *intel = hu->priv;
>> struct hci_event_hdr *hdr;
>>
>> - if (!test_bit(STATE_BOOTLOADER, &intel->flags))
>> + if (!test_bit(STATE_BOOTLOADER, &intel->flags) &&
>> + !test_bit(STATE_BOOTING, &intel->flags))
>> goto recv;
>>
>> hdr = (void *)skb->data;
>> @@ -700,12 +827,99 @@ static const struct hci_uart_proto intel_proto = {
>> .dequeue = intel_dequeue,
>> };
>>
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id intel_acpi_match[] = {
>> + { "INT33E1", 0 },
>> + { },
>> +};
>> +
>> +static int intel_acpi_probe(struct intel_device *idev)
>> +{
>> + const struct acpi_device_id *id;
>> +
>> + id = acpi_match_device(intel_acpi_match, &idev->pdev->dev);
>> + if (!id)
>> + return -ENODEV;
>> +
>> + return 0;
>> +}
>> +#else
>> +static int intel_acpi_probe(struct intel_device *idev)
>> +{
>> + return -ENODEV;
>> +}
>> +#endif
>> +
>> +static int intel_probe(struct platform_device *pdev)
>> +{
>> + struct intel_device *idev;
>> +
>> + idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL);
>> + if (!idev)
>> + return -ENOMEM;
>> +
>> + idev->pdev = pdev;
>> +
>> + if (ACPI_HANDLE(&pdev->dev)) {
>> + int err = intel_acpi_probe(idev);
>> + if (err)
>> + return err;
>> + } else {
>> + return -ENODEV;
>> + }
>> +
>> + idev->reset = devm_gpiod_get_optional(&pdev->dev, "reset",
>> + GPIOD_OUT_LOW);
>> + if (IS_ERR(idev->reset)) {
>> + dev_err(&pdev->dev, "Unable to retrieve gpio\n");
>> + return PTR_ERR(idev->reset);
>> + }
>> +
>> + platform_set_drvdata(pdev, idev);
>> +
>> + /* Place this instance on the device list */
>> + spin_lock(&intel_device_list_lock);
>> + list_add_tail(&idev->list, &intel_device_list);
>> + spin_unlock(&intel_device_list_lock);
>> +
>> + dev_info(&pdev->dev, "registered.\n");
>> +
>> + return 0;
>> +}
>> +
>> +static int intel_remove(struct platform_device *pdev)
>> +{
>> + struct intel_device *idev = platform_get_drvdata(pdev);
>> +
>> + spin_lock(&intel_device_list_lock);
>> + list_del(&idev->list);
>
> This is dangerous. If intel->idev still holds a reference, then this is blowing up in your face in case the platform device goes away first.
>
> I really wonder that instead of storing a reference in intel->idev, just don't do that and always try to find the device when you want to use it. I mean they are only two cases right now. When you attach the line discipline and when you release it.
>
> So what I would do is keep it simple. On intel_open / intel_close, you hold the list spinlock, then find a matching device, if found, set GPIO, release spinlock. So the set powered helper can do the whole thing.
>
> spin_lock(&intel_device_list_lock);
> list_for_each(p, &intel_device_list) {
> struct intel_device *idev;
>
> idev = list_entry(p, struct intel_device, list);
>
> /* tty device and pdev device should share the same parent
> * which is the UART port.
> */
> if (hu->tty->dev->parent == idev->pdev->dev.parent) {
> BT_INFO("hu %p, Switching compatible pm device (%s) to %u", hu,
> dev_name(&idev->pdev->dev), powered);
> gpiod_set_value(idev->reset, powered);
> break;
> }
> }
> spin_unlock(&intel_device_list_lock);
>
> At this point it turns into calling the set_powered with true or false from intel_open and intel_close.
actually I updated your patch to do it like this and posted it. Look at v2 since I forgot the module device table macro that is needed for module auto-loading.
It looks now pretty simple to me. The only thing that I don't know is if we can call gpiod_set_value while holding a spinlock. Then again, I am pretty sure we could just convert this into a mutex anyway since normally probe functions are allowed to sleep.
Please give the v2 patch a spin on real hardware. I do not have a board where I can test this on.
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
