[PATCH v2 1/9] Bluetooth: hci_uart_set_flow_control: Fix NULL deref when using serdev

[PATCH v2 1/9] Bluetooth: hci_uart_set_flow_control: Fix NULL deref when using serdev

[Hans de Goede]

Fix a NULL pointer deref (hu->tty) when calling hci_uart_set_flow_control
on hci_uart-s using serdev.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Also set RTS (Suggested-by: Sebastian Reichel <sre@kernel.org>)
---
 drivers/bluetooth/hci_ldisc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index a746627e784e..eec95019f15c 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -41,6 +41,7 @@
 #include <linux/ioctl.h>
 #include <linux/skbuff.h>
 #include <linux/firmware.h>
+#include <linux/serdev.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -298,6 +299,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
 	unsigned int set = 0;
 	unsigned int clear = 0;
 
+	if (hu->serdev) {
+		serdev_device_set_flow_control(hu->serdev, !enable);
+		serdev_device_set_rts(hu->serdev, !enable);
+		return;
+	}
+
 	if (enable) {
 		/* Disable hardware flow control */
 		ktermios = tty->termios;
-- 
2.14.2

[PATCH v2 2/9] Bluetooth: hci_bcm: Fix setting of irq trigger type

[Hans de Goede]

This commit fixes 2 issues with host-wake irq trigger type handling
in hci_bcm:

1) bcm_setup_sleep sets sleep_params.host_wake_active based on
bcm_device.irq_polarity, but bcm_request_irq was always requesting
IRQF_TRIGGER_RISING as trigger type independent of irq_polarity.

This was a problem when the irq is described as a GpioInt rather then
an Interrupt in the DSDT as for GpioInt-s the value passed to request_irq
is honored. This commit fixes this by requesting the correct trigger
type depending on bcm_device.irq_polarity.

2) bcm_device.irq_polarity was used to directly store an ACPI polarity
value (ACPI_ACTIVE_*). This is undesirable because hci_bcm is also
used with device-tree and checking for something like ACPI_ACTIVE_LOW
in a non ACPI specific function like bcm_request_irq feels wrong.

This commit fixes this by renaming irq_polarity to irq_active_low
and changing its type to a bool.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_bcm.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index c821234b9668..2285ca673ae3 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -68,7 +68,7 @@ struct bcm_device {
 	u32			init_speed;
 	u32			oper_speed;
 	int			irq;
-	u8			irq_polarity;
+	bool			irq_active_low;
 
 #ifdef CONFIG_PM
 	struct hci_uart		*hu;
@@ -213,7 +213,9 @@ static int bcm_request_irq(struct bcm_data *bcm)
 	}
 
 	err = devm_request_irq(&bdev->pdev->dev, bdev->irq, bcm_host_wake,
-			       IRQF_TRIGGER_RISING, "host_wake", bdev);
+			       bdev->irq_active_low ? IRQF_TRIGGER_FALLING :
+						      IRQF_TRIGGER_RISING,
+			       "host_wake", bdev);
 	if (err)
 		goto unlock;
 
@@ -253,7 +255,7 @@ static int bcm_setup_sleep(struct hci_uart *hu)
 	struct sk_buff *skb;
 	struct bcm_set_sleep_mode sleep_params = default_sleep_params;
 
-	sleep_params.host_wake_active = !bcm->dev->irq_polarity;
+	sleep_params.host_wake_active = !bcm->dev->irq_active_low;
 
 	skb = __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_params),
 			     &sleep_params, HCI_INIT_TIMEOUT);
@@ -690,10 +692,8 @@ static const struct acpi_gpio_mapping acpi_bcm_int_first_gpios[] = {
 };
 
 #ifdef CONFIG_ACPI
-static u8 acpi_active_low = ACPI_ACTIVE_LOW;
-
 /* IRQ polarity of some chipsets are not defined correctly in ACPI table. */
-static const struct dmi_system_id bcm_wrong_irq_dmi_table[] = {
+static const struct dmi_system_id bcm_active_low_irq_dmi_table[] = {
 	{
 		.ident = "Asus T100TA",
 		.matches = {
@@ -701,7 +701,6 @@ static const struct dmi_system_id bcm_wrong_irq_dmi_table[] = {
 					"ASUSTeK COMPUTER INC."),
 			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "T100TA"),
 		},
-		.driver_data = &acpi_active_low,
 	},
 	{
 		.ident = "Asus T100CHI",
@@ -710,7 +709,6 @@ static const struct dmi_system_id bcm_wrong_irq_dmi_table[] = {
 					"ASUSTeK COMPUTER INC."),
 			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "T100CHI"),
 		},
-		.driver_data = &acpi_active_low,
 	},
 	{	/* Handle ThinkPad 8 tablets with BCM2E55 chipset ACPI ID */
 		.ident = "Lenovo ThinkPad 8",
@@ -718,7 +716,6 @@ static const struct dmi_system_id bcm_wrong_irq_dmi_table[] = {
 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "LENOVO"),
 			DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "ThinkPad 8"),
 		},
-		.driver_data = &acpi_active_low,
 	},
 	{ }
 };
@@ -733,13 +730,13 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
 	switch (ares->type) {
 	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
 		irq = &ares->data.extended_irq;
-		dev->irq_polarity = irq->polarity;
+		dev->irq_active_low = irq->polarity == ACPI_ACTIVE_LOW;
 		break;
 
 	case ACPI_RESOURCE_TYPE_GPIO:
 		gpio = &ares->data.gpio;
 		if (gpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT)
-			dev->irq_polarity = gpio->polarity;
+			dev->irq_active_low = gpio->polarity == ACPI_ACTIVE_LOW;
 		break;
 
 	case ACPI_RESOURCE_TYPE_SERIAL_BUS:
@@ -834,11 +831,11 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 		return ret;
 	acpi_dev_free_resource_list(&resources);
 
-	dmi_id = dmi_first_match(bcm_wrong_irq_dmi_table);
+	dmi_id = dmi_first_match(bcm_active_low_irq_dmi_table);
 	if (dmi_id) {
 		bt_dev_warn(dev, "%s: Overwriting IRQ polarity to active low",
 			    dmi_id->ident);
-		dev->irq_polarity = *(u8 *)dmi_id->driver_data;
+		dev->irq_active_low = true;
 	}
 
 	return 0;
-- 
2.14.2

[PATCH v2 3/9] Bluetooth: hci_bcm: Move bcm_platform_probe call out of bcm_acpi_probe

[Hans de Goede]

Since bcm_acpi_probe calls bcm_platform_probe, bcm_probe always ends up
calling bcm_platform_probe.

This commit simplifies things by making bcm_probe always call
bcm_platform_probe itself.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_bcm.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 2285ca673ae3..00103307a776 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -820,10 +820,6 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 	if (ret)
 		return ret;
 
-	ret = bcm_platform_probe(dev);
-	if (ret)
-		return ret;
-
 	/* Retrieve UART ACPI info */
 	ret = acpi_dev_get_resources(ACPI_COMPANION(&dev->pdev->dev),
 				     &resources, bcm_resource, dev);
@@ -858,10 +854,13 @@ static int bcm_probe(struct platform_device *pdev)
 
 	dev->pdev = pdev;
 
-	if (has_acpi_companion(&pdev->dev))
+	if (has_acpi_companion(&pdev->dev)) {
 		ret = bcm_acpi_probe(dev);
-	else
-		ret = bcm_platform_probe(dev);
+		if (ret)
+			return ret;
+	}
+
+	ret = bcm_platform_probe(dev);
 	if (ret)
 		return ret;
 
-- 
2.14.2

[PATCH v2 4/9] Bluetooth: hci_bcm: Move platform_get_irq call to bcm_probe

[Hans de Goede]

The ACPI subsys is going to move over to instantiating ACPI enumerated
HCIs as serdevs, rather then as platform devices.

Most of the code in bcm_platform_probe is actually not platform
specific and will work with any struct device passed to it, the one
platform specific call in bcm_platform_probe is platform_get_irq.

This commit moves platform_get_irq call to the platform-driver's bcm_probe
function, this is a preparation patch for adding (runtime)pm support to
the serdev path.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_bcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 00103307a776..ea530a56778d 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -776,7 +776,6 @@ static int bcm_platform_probe(struct bcm_device *dev)
 		return PTR_ERR(dev->shutdown);
 
 	/* IRQ can be declared in ACPI table as Interrupt or GpioInt */
-	dev->irq = platform_get_irq(pdev, 0);
 	if (dev->irq <= 0) {
 		struct gpio_desc *gpio;
 
@@ -853,6 +852,7 @@ static int bcm_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	dev->pdev = pdev;
+	dev->irq = platform_get_irq(pdev, 0);
 
 	if (has_acpi_companion(&pdev->dev)) {
 		ret = bcm_acpi_probe(dev);
-- 
2.14.2

[PATCH v2 5/9] Bluetooth: hci_bcm: Store device pointer instead of platform_device pointer

[Hans de Goede]

The ACPI subsys is going to move over to instantiating ACPI enumerated
HCIs as serdevs, rather then as platform devices.

This means that the serdev driver paths of hci_bcm.c also need to start
supporting (runtime)pm through GPIOs and a host-wake IRQ.

The hci_bcm code is already mostly independent of how the HCI gets
instantiated, but even though the code only cares about pdev->dev, it
was storing pdev itself in struct bcm_device.

This commit stores pdev->dev rather then pdev in struct bcm_device, this
is a preparation patch for adding (runtime)pm support to the serdev path.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_bcm.c | 73 ++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 38 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index ea530a56778d..cfc87fb5051c 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -56,7 +56,7 @@
 struct bcm_device {
 	struct list_head	list;
 
-	struct platform_device	*pdev;
+	struct device		*dev;
 
 	const char		*name;
 	struct gpio_desc	*device_wakeup;
@@ -188,9 +188,9 @@ static irqreturn_t bcm_host_wake(int irq, void *data)
 
 	bt_dev_dbg(bdev, "Host wake IRQ");
 
-	pm_runtime_get(&bdev->pdev->dev);
-	pm_runtime_mark_last_busy(&bdev->pdev->dev);
-	pm_runtime_put_autosuspend(&bdev->pdev->dev);
+	pm_runtime_get(bdev->dev);
+	pm_runtime_mark_last_busy(bdev->dev);
+	pm_runtime_put_autosuspend(bdev->dev);
 
 	return IRQ_HANDLED;
 }
@@ -212,20 +212,20 @@ static int bcm_request_irq(struct bcm_data *bcm)
 		goto unlock;
 	}
 
-	err = devm_request_irq(&bdev->pdev->dev, bdev->irq, bcm_host_wake,
+	err = devm_request_irq(bdev->dev, bdev->irq, bcm_host_wake,
 			       bdev->irq_active_low ? IRQF_TRIGGER_FALLING :
 						      IRQF_TRIGGER_RISING,
 			       "host_wake", bdev);
 	if (err)
 		goto unlock;
 
-	device_init_wakeup(&bdev->pdev->dev, true);
+	device_init_wakeup(bdev->dev, true);
 
-	pm_runtime_set_autosuspend_delay(&bdev->pdev->dev,
+	pm_runtime_set_autosuspend_delay(bdev->dev,
 					 BCM_AUTOSUSPEND_DELAY);
-	pm_runtime_use_autosuspend(&bdev->pdev->dev);
-	pm_runtime_set_active(&bdev->pdev->dev);
-	pm_runtime_enable(&bdev->pdev->dev);
+	pm_runtime_use_autosuspend(bdev->dev);
+	pm_runtime_set_active(bdev->dev);
+	pm_runtime_enable(bdev->dev);
 
 unlock:
 	mutex_unlock(&bcm_device_lock);
@@ -332,7 +332,7 @@ static int bcm_open(struct hci_uart *hu)
 		 * platform device (saved during device probe) and
 		 * parent of tty device used by hci_uart
 		 */
-		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
+		if (hu->tty->dev->parent == dev->dev->parent) {
 			bcm->dev = dev;
 			hu->init_speed = dev->init_speed;
 			hu->oper_speed = dev->oper_speed;
@@ -367,12 +367,12 @@ static int bcm_close(struct hci_uart *hu)
 	if (bcm_device_exists(bdev)) {
 		bcm_gpio_set_power(bdev, false);
 #ifdef CONFIG_PM
-		pm_runtime_disable(&bdev->pdev->dev);
-		pm_runtime_set_suspended(&bdev->pdev->dev);
+		pm_runtime_disable(bdev->dev);
+		pm_runtime_set_suspended(bdev->dev);
 
-		if (device_can_wakeup(&bdev->pdev->dev)) {
-			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
-			device_init_wakeup(&bdev->pdev->dev, false);
+		if (device_can_wakeup(bdev->dev)) {
+			devm_free_irq(bdev->dev, bdev->irq, bdev);
+			device_init_wakeup(bdev->dev, false);
 		}
 
 		bdev->hu = NULL;
@@ -506,9 +506,9 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
 		/* Delay auto-suspend when receiving completed packet */
 		mutex_lock(&bcm_device_lock);
 		if (bcm->dev && bcm_device_exists(bcm->dev)) {
-			pm_runtime_get(&bcm->dev->pdev->dev);
-			pm_runtime_mark_last_busy(&bcm->dev->pdev->dev);
-			pm_runtime_put_autosuspend(&bcm->dev->pdev->dev);
+			pm_runtime_get(bcm->dev->dev);
+			pm_runtime_mark_last_busy(bcm->dev->dev);
+			pm_runtime_put_autosuspend(bcm->dev->dev);
 		}
 		mutex_unlock(&bcm_device_lock);
 	}
@@ -539,15 +539,15 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 
 	if (bcm_device_exists(bcm->dev)) {
 		bdev = bcm->dev;
-		pm_runtime_get_sync(&bdev->pdev->dev);
+		pm_runtime_get_sync(bdev->dev);
 		/* Shall be resumed here */
 	}
 
 	skb = skb_dequeue(&bcm->txq);
 
 	if (bdev) {
-		pm_runtime_mark_last_busy(&bdev->pdev->dev);
-		pm_runtime_put_autosuspend(&bdev->pdev->dev);
+		pm_runtime_mark_last_busy(bdev->dev);
+		pm_runtime_put_autosuspend(bdev->dev);
 	}
 
 	mutex_unlock(&bcm_device_lock);
@@ -623,7 +623,7 @@ static int bcm_suspend(struct device *dev)
 	if (pm_runtime_active(dev))
 		bcm_suspend_device(dev);
 
-	if (device_may_wakeup(&bdev->pdev->dev)) {
+	if (device_may_wakeup(dev)) {
 		error = enable_irq_wake(bdev->irq);
 		if (!error)
 			bt_dev_dbg(bdev, "BCM irq: enabled");
@@ -651,7 +651,7 @@ static int bcm_resume(struct device *dev)
 	if (!bdev->hu)
 		goto unlock;
 
-	if (device_may_wakeup(&bdev->pdev->dev)) {
+	if (device_may_wakeup(dev)) {
 		disable_irq_wake(bdev->irq);
 		bt_dev_dbg(bdev, "BCM irq: disabled");
 	}
@@ -758,19 +758,17 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
 
 static int bcm_platform_probe(struct bcm_device *dev)
 {
-	struct platform_device *pdev = dev->pdev;
+	dev->name = dev_name(dev->dev);
 
-	dev->name = dev_name(&pdev->dev);
+	dev->clk = devm_clk_get(dev->dev, NULL);
 
-	dev->clk = devm_clk_get(&pdev->dev, NULL);
-
-	dev->device_wakeup = devm_gpiod_get_optional(&pdev->dev,
+	dev->device_wakeup = devm_gpiod_get_optional(dev->dev,
 						     "device-wakeup",
 						     GPIOD_OUT_LOW);
 	if (IS_ERR(dev->device_wakeup))
 		return PTR_ERR(dev->device_wakeup);
 
-	dev->shutdown = devm_gpiod_get_optional(&pdev->dev, "shutdown",
+	dev->shutdown = devm_gpiod_get_optional(dev->dev, "shutdown",
 						GPIOD_OUT_LOW);
 	if (IS_ERR(dev->shutdown))
 		return PTR_ERR(dev->shutdown);
@@ -779,7 +777,7 @@ static int bcm_platform_probe(struct bcm_device *dev)
 	if (dev->irq <= 0) {
 		struct gpio_desc *gpio;
 
-		gpio = devm_gpiod_get_optional(&pdev->dev, "host-wakeup",
+		gpio = devm_gpiod_get_optional(dev->dev, "host-wakeup",
 					       GPIOD_IN);
 		if (IS_ERR(gpio))
 			return PTR_ERR(gpio);
@@ -787,13 +785,13 @@ static int bcm_platform_probe(struct bcm_device *dev)
 		dev->irq = gpiod_to_irq(gpio);
 	}
 
-	dev_info(&pdev->dev, "BCM irq: %d\n", dev->irq);
+	dev_info(dev->dev, "BCM irq: %d\n", dev->irq);
 
 	/* Make sure at-least one of the GPIO is defined and that
 	 * a name is specified for this instance
 	 */
 	if ((!dev->device_wakeup && !dev->shutdown) || !dev->name) {
-		dev_err(&pdev->dev, "invalid platform data\n");
+		dev_err(dev->dev, "invalid platform data\n");
 		return -EINVAL;
 	}
 
@@ -803,7 +801,6 @@ static int bcm_platform_probe(struct bcm_device *dev)
 #ifdef CONFIG_ACPI
 static int bcm_acpi_probe(struct bcm_device *dev)
 {
-	struct platform_device *pdev = dev->pdev;
 	LIST_HEAD(resources);
 	const struct dmi_system_id *dmi_id;
 	const struct acpi_gpio_mapping *gpio_mapping = acpi_bcm_int_last_gpios;
@@ -811,16 +808,16 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 	int ret;
 
 	/* Retrieve GPIO data */
-	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
+	id = acpi_match_device(dev->dev->driver->acpi_match_table, dev->dev);
 	if (id)
 		gpio_mapping = (const struct acpi_gpio_mapping *) id->driver_data;
 
-	ret = devm_acpi_dev_add_driver_gpios(&pdev->dev, gpio_mapping);
+	ret = devm_acpi_dev_add_driver_gpios(dev->dev, gpio_mapping);
 	if (ret)
 		return ret;
 
 	/* Retrieve UART ACPI info */
-	ret = acpi_dev_get_resources(ACPI_COMPANION(&dev->pdev->dev),
+	ret = acpi_dev_get_resources(ACPI_COMPANION(dev->dev),
 				     &resources, bcm_resource, dev);
 	if (ret < 0)
 		return ret;
@@ -851,7 +848,7 @@ static int bcm_probe(struct platform_device *pdev)
 	if (!dev)
 		return -ENOMEM;
 
-	dev->pdev = pdev;
+	dev->dev = &pdev->dev;
 	dev->irq = platform_get_irq(pdev, 0);
 
 	if (has_acpi_companion(&pdev->dev)) {
-- 
2.14.2

[PATCH v2 6/9] Bluetooth: hci_bcm: Rename bcm_platform_probe to bcm_get_resources

[Hans de Goede]

After our previous changes, there is nothing platform specific about
bcm_platform_probe anymore, rename it to bcm_get_resources.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_bcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index cfc87fb5051c..5c8371d8aace 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -756,7 +756,7 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
 }
 #endif /* CONFIG_ACPI */
 
-static int bcm_platform_probe(struct bcm_device *dev)
+static int bcm_get_resources(struct bcm_device *dev)
 {
 	dev->name = dev_name(dev->dev);
 
@@ -857,7 +857,7 @@ static int bcm_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	ret = bcm_platform_probe(dev);
+	ret = bcm_get_resources(dev);
 	if (ret)
 		return ret;
 
-- 
2.14.2

[PATCH v2 7/9] Bluetooth: hci_bcm: Make acpi_probe get irq from ACPI resources

[Hans de Goede]

The ACPI subsys is going to move over to instantiating ACPI enumerated
HCIs as serdevs, rather then as platform devices.

So we need to make bcm_acpi_probe() suitable for use on non platform-
devices too, which means that we cannot rely on platform_get_irq()
getting called.

This commit modifies bcm_acpi_probe() to directly get the irq from
the ACPI resources, this is a preparation patch for adding (runtime)pm
support to the serdev path.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Make bcm_resource return 0 so that acpi_dev_get_resources() does not
 return an empty list (Suggested-by: Frédéric Danis)
---
 drivers/bluetooth/hci_bcm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 5c8371d8aace..e577547c4acf 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -751,8 +751,7 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
 		break;
 	}
 
-	/* Always tell the ACPI core to skip this resource */
-	return 1;
+	return 0;
 }
 #endif /* CONFIG_ACPI */
 
@@ -805,6 +804,7 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 	const struct dmi_system_id *dmi_id;
 	const struct acpi_gpio_mapping *gpio_mapping = acpi_bcm_int_last_gpios;
 	const struct acpi_device_id *id;
+	struct resource_entry *entry;
 	int ret;
 
 	/* Retrieve GPIO data */
@@ -821,6 +821,13 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 				     &resources, bcm_resource, dev);
 	if (ret < 0)
 		return ret;
+
+	resource_list_for_each_entry(entry, &resources) {
+		if (resource_type(entry->res) == IORESOURCE_IRQ) {
+			dev->irq = entry->res->start;
+			break;
+		}
+	}
 	acpi_dev_free_resource_list(&resources);
 
 	dmi_id = dmi_first_match(bcm_active_low_irq_dmi_table);
-- 
2.14.2

[PATCH v2 8/9] Bluetooth: hci_bcm: Make suspend/resume functions platform_dev independent

[Hans de Goede]

Use dev_get_drvdata instead of platform_get_drvdata in the suspend /
resume functions. This is a preparation patch for adding (runtime)pm
support to the serdev path.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_bcm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index e577547c4acf..e7494efb56e9 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -558,7 +558,7 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 #ifdef CONFIG_PM
 static int bcm_suspend_device(struct device *dev)
 {
-	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
+	struct bcm_device *bdev = dev_get_drvdata(dev);
 
 	bt_dev_dbg(bdev, "");
 
@@ -581,7 +581,7 @@ static int bcm_suspend_device(struct device *dev)
 
 static int bcm_resume_device(struct device *dev)
 {
-	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
+	struct bcm_device *bdev = dev_get_drvdata(dev);
 
 	bt_dev_dbg(bdev, "");
 
@@ -606,7 +606,7 @@ static int bcm_resume_device(struct device *dev)
 /* Platform suspend callback */
 static int bcm_suspend(struct device *dev)
 {
-	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
+	struct bcm_device *bdev = dev_get_drvdata(dev);
 	int error;
 
 	bt_dev_dbg(bdev, "suspend: is_suspended %d", bdev->is_suspended);
@@ -638,7 +638,7 @@ static int bcm_suspend(struct device *dev)
 /* Platform resume callback */
 static int bcm_resume(struct device *dev)
 {
-	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
+	struct bcm_device *bdev = dev_get_drvdata(dev);
 
 	bt_dev_dbg(bdev, "resume: is_suspended %d", bdev->is_suspended);
 
-- 
2.14.2

[PATCH v2 9/9] Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver

[Hans de Goede]

Make the serdev driver use struct bcm_device as its driver data and share
all the pm / GPIO / IRQ related code paths with the platform driver.

After this commit the 2 drivers are in essence the same and the serdev
driver interface can be used for all ACPI enumerated HCI UARTs.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/bluetooth/hci_bcm.c | 118 +++++++++++++++++++++++++-------------------
 1 file changed, 68 insertions(+), 50 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index e7494efb56e9..cff91930f0b1 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -52,8 +52,10 @@
 
 #define BCM_AUTOSUSPEND_DELAY	5000 /* default autosleep delay */
 
-/* platform device driver resources */
+/* device driver resources */
 struct bcm_device {
+	/* Must be the first member, hci_serdev.c expects this. */
+	struct hci_uart		serdev_hu;
 	struct list_head	list;
 
 	struct device		*dev;
@@ -76,11 +78,6 @@ struct bcm_device {
 #endif
 };
 
-/* serdev driver resources */
-struct bcm_serdev {
-	struct hci_uart hu;
-};
-
 /* generic bcm uart resources */
 struct bcm_data {
 	struct sk_buff		*rx_skb;
@@ -155,6 +152,10 @@ static bool bcm_device_exists(struct bcm_device *device)
 {
 	struct list_head *p;
 
+	/* Devices using serdev always exist */
+	if (device && device->hu && device->hu->serdev)
+		return true;
+
 	list_for_each(p, &bcm_device_list) {
 		struct bcm_device *dev = list_entry(p, struct bcm_device, list);
 
@@ -200,7 +201,6 @@ static int bcm_request_irq(struct bcm_data *bcm)
 	struct bcm_device *bdev = bcm->dev;
 	int err;
 
-	/* If this is not a platform device, do not enable PM functionalities */
 	mutex_lock(&bcm_device_lock);
 	if (!bcm_device_exists(bdev)) {
 		err = -ENODEV;
@@ -313,18 +313,17 @@ static int bcm_open(struct hci_uart *hu)
 
 	hu->priv = bcm;
 
-	/* If this is a serdev defined device, then only use
-	 * serdev open primitive and skip the rest.
-	 */
+	mutex_lock(&bcm_device_lock);
+
 	if (hu->serdev) {
 		serdev_device_open(hu->serdev);
+		bcm->dev = serdev_device_get_drvdata(hu->serdev);
 		goto out;
 	}
 
 	if (!hu->tty->dev)
 		goto out;
 
-	mutex_lock(&bcm_device_lock);
 	list_for_each(p, &bcm_device_list) {
 		struct bcm_device *dev = list_entry(p, struct bcm_device, list);
 
@@ -334,37 +333,45 @@ static int bcm_open(struct hci_uart *hu)
 		 */
 		if (hu->tty->dev->parent == dev->dev->parent) {
 			bcm->dev = dev;
-			hu->init_speed = dev->init_speed;
-			hu->oper_speed = dev->oper_speed;
 #ifdef CONFIG_PM
 			dev->hu = hu;
 #endif
-			bcm_gpio_set_power(bcm->dev, true);
 			break;
 		}
 	}
 
-	mutex_unlock(&bcm_device_lock);
 out:
+	if (bcm->dev) {
+		hu->init_speed = bcm->dev->init_speed;
+		hu->oper_speed = bcm->dev->oper_speed;
+		bcm_gpio_set_power(bcm->dev, true);
+	}
+
+	mutex_unlock(&bcm_device_lock);
 	return 0;
 }
 
 static int bcm_close(struct hci_uart *hu)
 {
 	struct bcm_data *bcm = hu->priv;
-	struct bcm_device *bdev = bcm->dev;
+	struct bcm_device *bdev = NULL;
 
 	bt_dev_dbg(hu->hdev, "hu %p", hu);
 
-	/* If this is a serdev defined device, only use serdev
-	 * close primitive and then continue as usual.
-	 */
-	if (hu->serdev)
-		serdev_device_close(hu->serdev);
-
 	/* Protect bcm->dev against removal of the device or driver */
 	mutex_lock(&bcm_device_lock);
-	if (bcm_device_exists(bdev)) {
+
+	if (hu->serdev) {
+		serdev_device_close(hu->serdev);
+		bdev = serdev_device_get_drvdata(hu->serdev);
+	} else if (bcm_device_exists(bcm->dev)) {
+		bdev = bcm->dev;
+#ifdef CONFIG_PM
+		bdev->hu = NULL;
+#endif
+	}
+
+	if (bdev) {
 		bcm_gpio_set_power(bdev, false);
 #ifdef CONFIG_PM
 		pm_runtime_disable(bdev->dev);
@@ -374,8 +381,6 @@ static int bcm_close(struct hci_uart *hu)
 			devm_free_irq(bdev->dev, bdev->irq, bdev);
 			device_init_wakeup(bdev->dev, false);
 		}
-
-		bdev->hu = NULL;
 #endif
 	}
 	mutex_unlock(&bcm_device_lock);
@@ -603,7 +608,7 @@ static int bcm_resume_device(struct device *dev)
 #endif
 
 #ifdef CONFIG_PM_SLEEP
-/* Platform suspend callback */
+/* suspend callback */
 static int bcm_suspend(struct device *dev)
 {
 	struct bcm_device *bdev = dev_get_drvdata(dev);
@@ -611,8 +616,10 @@ static int bcm_suspend(struct device *dev)
 
 	bt_dev_dbg(bdev, "suspend: is_suspended %d", bdev->is_suspended);
 
-	/* bcm_suspend can be called at any time as long as platform device is
-	 * bound, so it should use bcm_device_lock to protect access to hci_uart
+	/*
+	 * When used with a device instantiated as platform_device, bcm_suspend
+	 * can be called at any time as long as the platform device is bound,
+	 * so it should use bcm_device_lock to protect access to hci_uart
 	 * and device_wake-up GPIO.
 	 */
 	mutex_lock(&bcm_device_lock);
@@ -635,15 +642,17 @@ static int bcm_suspend(struct device *dev)
 	return 0;
 }
 
-/* Platform resume callback */
+/* resume callback */
 static int bcm_resume(struct device *dev)
 {
 	struct bcm_device *bdev = dev_get_drvdata(dev);
 
 	bt_dev_dbg(bdev, "resume: is_suspended %d", bdev->is_suspended);
 
-	/* bcm_resume can be called at any time as long as platform device is
-	 * bound, so it should use bcm_device_lock to protect access to hci_uart
+	/*
+	 * When used with a device instantiated as platform_device, bcm_resume
+	 * can be called at any time as long as platform device is bound,
+	 * so it should use bcm_device_lock to protect access to hci_uart
 	 * and device_wake-up GPIO.
 	 */
 	mutex_lock(&bcm_device_lock);
@@ -785,15 +794,6 @@ static int bcm_get_resources(struct bcm_device *dev)
 	}
 
 	dev_info(dev->dev, "BCM irq: %d\n", dev->irq);
-
-	/* Make sure at-least one of the GPIO is defined and that
-	 * a name is specified for this instance
-	 */
-	if ((!dev->device_wakeup && !dev->shutdown) || !dev->name) {
-		dev_err(dev->dev, "invalid platform data\n");
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
@@ -846,6 +846,12 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 }
 #endif /* CONFIG_ACPI */
 
+static int bcm_of_probe(struct bcm_device *bdev)
+{
+	device_property_read_u32(bdev->dev, "max-speed", &bdev->oper_speed);
+	return 0;
+}
+
 static int bcm_probe(struct platform_device *pdev)
 {
 	struct bcm_device *dev;
@@ -934,7 +940,7 @@ static const struct acpi_device_id bcm_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
 #endif
 
-/* Platform suspend and resume callbacks */
+/* suspend and resume callbacks */
 static const struct dev_pm_ops bcm_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
 	SET_RUNTIME_PM_OPS(bcm_suspend_device, bcm_resume_device, NULL)
@@ -952,29 +958,39 @@ static struct platform_driver bcm_driver = {
 
 static int bcm_serdev_probe(struct serdev_device *serdev)
 {
-	struct bcm_serdev *bcmdev;
-	u32 speed;
+	struct bcm_device *bcmdev;
 	int err;
 
 	bcmdev = devm_kzalloc(&serdev->dev, sizeof(*bcmdev), GFP_KERNEL);
 	if (!bcmdev)
 		return -ENOMEM;
 
-	bcmdev->hu.serdev = serdev;
+	bcmdev->dev = &serdev->dev;
+	bcmdev->hu = &bcmdev->serdev_hu;
+	bcmdev->serdev_hu.serdev = serdev;
 	serdev_device_set_drvdata(serdev, bcmdev);
 
-	err = device_property_read_u32(&serdev->dev, "max-speed", &speed);
-	if (!err)
-		bcmdev->hu.oper_speed = speed;
+	if (has_acpi_companion(&serdev->dev))
+		err = bcm_acpi_probe(bcmdev);
+	else
+		err = bcm_of_probe(bcmdev);
+	if (err)
+		return err;
 
-	return hci_uart_register_device(&bcmdev->hu, &bcm_proto);
+	err = bcm_get_resources(bcmdev);
+	if (err)
+		return err;
+
+	bcm_gpio_set_power(bcmdev, false);
+
+	return hci_uart_register_device(&bcmdev->serdev_hu, &bcm_proto);
 }
 
 static void bcm_serdev_remove(struct serdev_device *serdev)
 {
-	struct bcm_serdev *bcmdev = serdev_device_get_drvdata(serdev);
+	struct bcm_device *bcmdev = serdev_device_get_drvdata(serdev);
 
-	hci_uart_unregister_device(&bcmdev->hu);
+	hci_uart_unregister_device(&bcmdev->serdev_hu);
 }
 
 #ifdef CONFIG_OF
@@ -991,6 +1007,8 @@ static struct serdev_device_driver bcm_serdev_driver = {
 	.driver = {
 		.name = "hci_uart_bcm",
 		.of_match_table = of_match_ptr(bcm_bluetooth_of_match),
+		.acpi_match_table = ACPI_PTR(bcm_acpi_match),
+		.pm = &bcm_pm_ops,
 	},
 };
 
-- 
2.14.2

Re: [PATCH v2 9/9] Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver

[Marcel Holtmann]

Hi Hans,

> Make the serdev driver use struct bcm_device as its driver data and share
> all the pm / GPIO / IRQ related code paths with the platform driver.
> 
> After this commit the 2 drivers are in essence the same and the serdev
> driver interface can be used for all ACPI enumerated HCI UARTs.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/bluetooth/hci_bcm.c | 118 +++++++++++++++++++++++++-------------------
> 1 file changed, 68 insertions(+), 50 deletions(-)

all 9 patches have been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH v2 9/9] Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver

[Hans de Goede]

Hi,

On 05-10-17 12:45, Marcel Holtmann wrote:
> Hi Hans,
> 
>> Make the serdev driver use struct bcm_device as its driver data and share
>> all the pm / GPIO / IRQ related code paths with the platform driver.
>>
>> After this commit the 2 drivers are in essence the same and the serdev
>> driver interface can be used for all ACPI enumerated HCI UARTs.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> drivers/bluetooth/hci_bcm.c | 118 +++++++++++++++++++++++++-------------------
>> 1 file changed, 68 insertions(+), 50 deletions(-)
> 
> all 9 patches have been applied to bluetooth-next tree.

Excellent, thank you!

So I guess this means we can also move forward with getting
the 2 patches from Frédéric Danis merged ? There is a bit
of a bisect-ability problem there, if the acpi pull-req
gets merged first then uart attached bcm bt will stop
working until the bluetooth subsys is also merged.

But I don't think this will impact a lot of users
(also given the need for a manual btattach so far),
so I don't think this is a big problem... ?

Regards,

Hans

Re: [PATCH v2 9/9] Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver

[Marcel Holtmann]

Hi Hans,

>>> Make the serdev driver use struct bcm_device as its driver data and share
>>> all the pm / GPIO / IRQ related code paths with the platform driver.
>>> 
>>> After this commit the 2 drivers are in essence the same and the serdev
>>> driver interface can be used for all ACPI enumerated HCI UARTs.
>>> 
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> drivers/bluetooth/hci_bcm.c | 118 +++++++++++++++++++++++++-------------------
>>> 1 file changed, 68 insertions(+), 50 deletions(-)
>> all 9 patches have been applied to bluetooth-next tree.
> 
> Excellent, thank you!
> 
> So I guess this means we can also move forward with getting
> the 2 patches from Frédéric Danis merged ? There is a bit
> of a bisect-ability problem there, if the acpi pull-req
> gets merged first then uart attached bcm bt will stop
> working until the bluetooth subsys is also merged.
> 
> But I don't think this will impact a lot of users
> (also given the need for a manual btattach so far),
> so I don't think this is a big problem... ?

I wonder if we should just do this as all-in-one change for 4.15 kernel. We surely can get the ACPI changes into 4.15 and the Bluetooth changes as well. Then it should just work.

So I am leaning to just adding the revert patch into the bluetooth-next tree and then just add the ACPI PnP ID for the GPD device in the same pull request. And if the ACPI changes make it then nobody will know the difference that we switched from USB to UART.

Regards

Marcel

Re: [PATCH v2 9/9] Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver

[Hans de Goede]

Hi,

On 05-10-17 17:15, Marcel Holtmann wrote:
> Hi Hans,
> 
>>>> Make the serdev driver use struct bcm_device as its driver data and share
>>>> all the pm / GPIO / IRQ related code paths with the platform driver.
>>>>
>>>> After this commit the 2 drivers are in essence the same and the serdev
>>>> driver interface can be used for all ACPI enumerated HCI UARTs.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>> drivers/bluetooth/hci_bcm.c | 118 +++++++++++++++++++++++++-------------------
>>>> 1 file changed, 68 insertions(+), 50 deletions(-)
>>> all 9 patches have been applied to bluetooth-next tree.
>>
>> Excellent, thank you!
>>
>> So I guess this means we can also move forward with getting
>> the 2 patches from Frédéric Danis merged ? There is a bit
>> of a bisect-ability problem there, if the acpi pull-req
>> gets merged first then uart attached bcm bt will stop
>> working until the bluetooth subsys is also merged.
>>
>> But I don't think this will impact a lot of users
>> (also given the need for a manual btattach so far),
>> so I don't think this is a big problem... ?
> 
> I wonder if we should just do this as all-in-one change for 4.15 kernel. We surely can get the ACPI changes into 4.15 and the Bluetooth changes as well. Then it should just work.
> 
> So I am leaning to just adding the revert patch into the bluetooth-next tree and then just add the ACPI PnP ID for the GPD device in the same pull request. And if the ACPI changes make it then nobody will know the difference that we switched from USB to UART.

Yes that should work fine.

Regards,

Hans

Re: [PATCH v2 9/9] Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver

[Marcel Holtmann]

Hi Hans,

>>>>> Make the serdev driver use struct bcm_device as its driver data and share
>>>>> all the pm / GPIO / IRQ related code paths with the platform driver.
>>>>> 
>>>>> After this commit the 2 drivers are in essence the same and the serdev
>>>>> driver interface can be used for all ACPI enumerated HCI UARTs.
>>>>> 
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> drivers/bluetooth/hci_bcm.c | 118 +++++++++++++++++++++++++-------------------
>>>>> 1 file changed, 68 insertions(+), 50 deletions(-)
>>>> all 9 patches have been applied to bluetooth-next tree.
>>> 
>>> Excellent, thank you!
>>> 
>>> So I guess this means we can also move forward with getting
>>> the 2 patches from Frédéric Danis merged ? There is a bit
>>> of a bisect-ability problem there, if the acpi pull-req
>>> gets merged first then uart attached bcm bt will stop
>>> working until the bluetooth subsys is also merged.
>>> 
>>> But I don't think this will impact a lot of users
>>> (also given the need for a manual btattach so far),
>>> so I don't think this is a big problem... ?
>> I wonder if we should just do this as all-in-one change for 4.15 kernel. We surely can get the ACPI changes into 4.15 and the Bluetooth changes as well. Then it should just work.
>> So I am leaning to just adding the revert patch into the bluetooth-next tree and then just add the ACPI PnP ID for the GPD device in the same pull request. And if the ACPI changes make it then nobody will know the difference that we switched from USB to UART.
> 
> Yes that should work fine.

I think that is what I am going for. Mainly since the GPD USB hack already seems to be in 4.13 and thus a little to late to fix anyway.

Regards

Marcel

Re: [PATCH v2 9/9] Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver

[Johan Hovold]

On Thu, Oct 05, 2017 at 05:15:05PM +0200, Marcel Holtmann wrote:
> Hi Hans,
> 
> >>> Make the serdev driver use struct bcm_device as its driver data and share
> >>> all the pm / GPIO / IRQ related code paths with the platform driver.
> >>> 
> >>> After this commit the 2 drivers are in essence the same and the serdev
> >>> driver interface can be used for all ACPI enumerated HCI UARTs.
> >>> 
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> ---
> >>> drivers/bluetooth/hci_bcm.c | 118 +++++++++++++++++++++++++-------------------
> >>> 1 file changed, 68 insertions(+), 50 deletions(-)
> >> all 9 patches have been applied to bluetooth-next tree.
> > 
> > Excellent, thank you!

Nice work here, Hans, allowing serdev to coexist with the
platform-device hacks until the transition is complete and those hacks
can finally be removed.

> > So I guess this means we can also move forward with getting
> > the 2 patches from Frédéric Danis merged ? There is a bit
> > of a bisect-ability problem there, if the acpi pull-req
> > gets merged first then uart attached bcm bt will stop
> > working until the bluetooth subsys is also merged.
> > 
> > But I don't think this will impact a lot of users
> > (also given the need for a manual btattach so far),
> > so I don't think this is a big problem... ?
> 
> I wonder if we should just do this as all-in-one change for 4.15
> kernel. We surely can get the ACPI changes into 4.15 and the Bluetooth
> changes as well. Then it should just work.

However, there are of course a couple of caveats. Once Frederic's ACPI
patches land, there will be no more platform child devices. Unless
serdev support is then compiled in, this means that PM will break
(silently). And if serdev is enabled, of course the tty class device is
gone and hciattach (btattach) will fail, but I guess everyone is aware
of that issue by now.

Should BT_HCIUART_BCM start depending on SERIAL_DEV_CTRL_TTYPORT (when
ACPI is enabled) to avoid such silent breakage once ACPI-support is
merged?

Johan

Re: [PATCH v2 9/9] Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver

[Hans de Goede]

Hi,

On 07-10-17 16:26, Johan Hovold wrote:
> On Thu, Oct 05, 2017 at 05:15:05PM +0200, Marcel Holtmann wrote:
>> Hi Hans,
>>
>>>>> Make the serdev driver use struct bcm_device as its driver data and share
>>>>> all the pm / GPIO / IRQ related code paths with the platform driver.
>>>>>
>>>>> After this commit the 2 drivers are in essence the same and the serdev
>>>>> driver interface can be used for all ACPI enumerated HCI UARTs.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> drivers/bluetooth/hci_bcm.c | 118 +++++++++++++++++++++++++-------------------
>>>>> 1 file changed, 68 insertions(+), 50 deletions(-)
>>>> all 9 patches have been applied to bluetooth-next tree.
>>>
>>> Excellent, thank you!
> 
> Nice work here, Hans, allowing serdev to coexist with the
> platform-device hacks until the transition is complete and those hacks
> can finally be removed.
> 
>>> So I guess this means we can also move forward with getting
>>> the 2 patches from Frédéric Danis merged ? There is a bit
>>> of a bisect-ability problem there, if the acpi pull-req
>>> gets merged first then uart attached bcm bt will stop
>>> working until the bluetooth subsys is also merged.
>>>
>>> But I don't think this will impact a lot of users
>>> (also given the need for a manual btattach so far),
>>> so I don't think this is a big problem... ?
>>
>> I wonder if we should just do this as all-in-one change for 4.15
>> kernel. We surely can get the ACPI changes into 4.15 and the Bluetooth
>> changes as well. Then it should just work.
> 
> However, there are of course a couple of caveats. Once Frederic's ACPI
> patches land, there will be no more platform child devices. Unless
> serdev support is then compiled in, this means that PM will break
> (silently). And if serdev is enabled, of course the tty class device is
> gone and hciattach (btattach) will fail, but I guess everyone is aware
> of that issue by now.
> 
> Should BT_HCIUART_BCM start depending on SERIAL_DEV_CTRL_TTYPORT (when
> ACPI is enabled) to avoid such silent breakage once ACPI-support is
> merged?

It seems that this was answered already in further discussions and
your recent patch to add a default y to SERIAL_DEV_CTRL_TTYPORT fixes
this, right ?

Regards,

Hans

Re: [PATCH v2 9/9] Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver

[Johan Hovold]

On Mon, Oct 09, 2017 at 04:02:24PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 07-10-17 16:26, Johan Hovold wrote:

> > However, there are of course a couple of caveats. Once Frederic's ACPI
> > patches land, there will be no more platform child devices. Unless
> > serdev support is then compiled in, this means that PM will break
> > (silently). And if serdev is enabled, of course the tty class device is
> > gone and hciattach (btattach) will fail, but I guess everyone is aware
> > of that issue by now.
> > 
> > Should BT_HCIUART_BCM start depending on SERIAL_DEV_CTRL_TTYPORT (when
> > ACPI is enabled) to avoid such silent breakage once ACPI-support is
> > merged?
> 
> It seems that this was answered already in further discussions and
> your recent patch to add a default y to SERIAL_DEV_CTRL_TTYPORT fixes
> this, right ?

Only partially, as the serdev bus code could still be built as a module,
which would prevent SERIAL_DEV_CTRL_TTYPORT from being selected.

We could consider having BT_HCIUART_BCM depend on
SERIAL_DEV_CTRL_TTYPORT (or !ACPI || SERIAL_DEV_CTRL_TTYPORT) just to
avoid any hard-to-detect regressions.

Johan

Re: [PATCH v2 1/9] Bluetooth: hci_uart_set_flow_control: Fix NULL deref when using serdev

[Johan Hovold]

On Wed, Oct 04, 2017 at 08:43:35PM +0200, Hans de Goede wrote:
> Fix a NULL pointer deref (hu->tty) when calling hci_uart_set_flow_control
> on hci_uart-s using serdev.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Also set RTS (Suggested-by: Sebastian Reichel <sre@kernel.org>)
> ---
>  drivers/bluetooth/hci_ldisc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index a746627e784e..eec95019f15c 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -41,6 +41,7 @@
>  #include <linux/ioctl.h>
>  #include <linux/skbuff.h>
>  #include <linux/firmware.h>
> +#include <linux/serdev.h>

I know this is already merged, but do we really want to add a dependency
on serdev from hci_ldisc.c?

There is already a helper function host_set_baudrate() in hci_bcm to
handle another case like this. If more drivers will need to support
both then these could be moved to a common header, but we should at
least try to be consistent here.

>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
> @@ -298,6 +299,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
>  	unsigned int set = 0;
>  	unsigned int clear = 0;
>  
> +	if (hu->serdev) {
> +		serdev_device_set_flow_control(hu->serdev, !enable);
> +		serdev_device_set_rts(hu->serdev, !enable);

The order here may matter; in the non-serdev case, rts is raised before
enabling flow control.

> +		return;
> +	}
> +
>  	if (enable) {
>  		/* Disable hardware flow control */
>  		ktermios = tty->termios;

Johan

Re: [PATCH v2 1/9] Bluetooth: hci_uart_set_flow_control: Fix NULL deref when using serdev

[Hans de Goede]

Hi,

On 07-10-17 16:36, Johan Hovold wrote:
> On Wed, Oct 04, 2017 at 08:43:35PM +0200, Hans de Goede wrote:
>> Fix a NULL pointer deref (hu->tty) when calling hci_uart_set_flow_control
>> on hci_uart-s using serdev.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Also set RTS (Suggested-by: Sebastian Reichel <sre@kernel.org>)
>> ---
>>   drivers/bluetooth/hci_ldisc.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index a746627e784e..eec95019f15c 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -41,6 +41,7 @@
>>   #include <linux/ioctl.h>
>>   #include <linux/skbuff.h>
>>   #include <linux/firmware.h>
>> +#include <linux/serdev.h>
> 
> I know this is already merged, but do we really want to add a dependency
> on serdev from hci_ldisc.c?
> 
> There is already a helper function host_set_baudrate() in hci_bcm to
> handle another case like this. If more drivers will need to support
> both then these could be moved to a common header, but we should at
> least try to be consistent here.

That is a valiud question, Marcel do you want me to do a follow
up series which:

1) Reverts this commit; and
2) Add a host_set_flow_control helper to hci_bcm.c ?

>>   #include <net/bluetooth/bluetooth.h>
>>   #include <net/bluetooth/hci_core.h>
>> @@ -298,6 +299,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
>>   	unsigned int set = 0;
>>   	unsigned int clear = 0;
>>   
>> +	if (hu->serdev) {
>> +		serdev_device_set_flow_control(hu->serdev, !enable);
>> +		serdev_device_set_rts(hu->serdev, !enable);
> 
> The order here may matter; in the non-serdev case, rts is raised before
> enabling flow control.

AFAIK it should not matter, if RTS is not set before enabling, then CTS
may not be set by the other side yet and the hw flow-control will
wait for CTS to get raised.

Regards,

Hans





> 
>> +		return;
>> +	}
>> +
>>   	if (enable) {
>>   		/* Disable hardware flow control */
>>   		ktermios = tty->termios;
> 
> Johan
> 

Re: [PATCH v2 1/9] Bluetooth: hci_uart_set_flow_control: Fix NULL deref when using serdev

[Johan Hovold]

On Mon, Oct 09, 2017 at 04:06:02PM +0200, Hans de Goede wrote:

> On 07-10-17 16:36, Johan Hovold wrote:
> > On Wed, Oct 04, 2017 at 08:43:35PM +0200, Hans de Goede wrote:

> >> @@ -298,6 +299,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> >>   	unsigned int set = 0;
> >>   	unsigned int clear = 0;
> >>   
> >> +	if (hu->serdev) {
> >> +		serdev_device_set_flow_control(hu->serdev, !enable);
> >> +		serdev_device_set_rts(hu->serdev, !enable);
> > 
> > The order here may matter; in the non-serdev case, rts is raised before
> > enabling flow control.
> 
> AFAIK it should not matter, if RTS is not set before enabling, then CTS
> may not be set by the other side yet and the hw flow-control will
> wait for CTS to get raised.

That's not necessarily how hardware flow control is implemented. When
using auto-RTS, RTS will simply be asserted whenever there's room in the
receive FIFO.

So in fact it seems the hci_ldisc should be doing this in the reverse
order (e.g. deassert RTS before disabling auto-RTS).

Probably doesn't matter in practice here, but I would at least expect
the enable and disable cases to be each others inverses (and for the
ldisc and serdev implementations to match).

Johan

Re: [PATCH v2 1/9] Bluetooth: hci_uart_set_flow_control: Fix NULL deref when using serdev

[Marcel Holtmann]

Hi Hans,

>>> Fix a NULL pointer deref (hu->tty) when calling hci_uart_set_flow_control
>>> on hci_uart-s using serdev.
>>> 
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> -Also set RTS (Suggested-by: Sebastian Reichel <sre@kernel.org>)
>>> ---
>>>  drivers/bluetooth/hci_ldisc.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>> 
>>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>>> index a746627e784e..eec95019f15c 100644
>>> --- a/drivers/bluetooth/hci_ldisc.c
>>> +++ b/drivers/bluetooth/hci_ldisc.c
>>> @@ -41,6 +41,7 @@
>>>  #include <linux/ioctl.h>
>>>  #include <linux/skbuff.h>
>>>  #include <linux/firmware.h>
>>> +#include <linux/serdev.h>
>> I know this is already merged, but do we really want to add a dependency
>> on serdev from hci_ldisc.c?
>> There is already a helper function host_set_baudrate() in hci_bcm to
>> handle another case like this. If more drivers will need to support
>> both then these could be moved to a common header, but we should at
>> least try to be consistent here.
> 
> That is a valiud question, Marcel do you want me to do a follow
> up series which:
> 
> 1) Reverts this commit; and
> 2) Add a host_set_flow_control helper to hci_bcm.c ?

don’t bother with a revert, lets just improve this with follow up patches.

Regards

Marcel