[PATCH AUTOSEL for 4.15 099/124] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO

[Sasha Levin]

From: Lukas Wunner 

[ Upstream commit 3e81a4ca51a1172253078ca7abd6a91040b8fcf4 ]

Commit 0395ffc1ee05 ("Bluetooth: hci_bcm: Add PM for BCM devices")
amended this driver to request a shutdown and device wake GPIO on probe,
but mandated that only one of them need to be present:

/* 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, "invalid platform data\n");
return -EINVAL;
}

However the same commit added a call to bcm_gpio_set_power() to the
->probe hook, which unconditionally accesses *both* GPIOs.  Luckily,
the resulting NULL pointer deref was never reported, suggesting there's
no machine where either GPIO is missing.

Commit 8a92056837fd ("Bluetooth: hci_bcm: Add (runtime)pm support to the
serdev driver") removed the check whether at least one of the GPIOs is
present without specifying a reason.

Because commit 62aaefa7d038 ("Bluetooth: hci_bcm: improve use of gpios
API") refactored the driver to use devm_gpiod_get_optional() instead of
devm_gpiod_get(), one is now tempted to believe that the driver doesn't
require *any* of the two GPIOs.

Which is wrong, the driver still requires both GPIOs to avoid a NULL
pointer deref.  To this end, establish the status quo ante and request
the GPIOs with devm_gpiod_get() again.  Bail out of ->probe if either
of them is missing.

Oddly enough, whereas bcm_gpio_set_power() accesses the device wake pin
unconditionally, bcm_suspend_device() and bcm_resume_device() do check
for its presence before accessing it.  Those checks are superfluous,
so remove them.

Cc: Frédéric Danis 
Cc: Loic Poulain 
Cc: Hans de Goede 
Cc: Uwe Kleine-König 
Cc: Linus Walleij 
Reviewed-by: Andy Shevchenko 
Signed-off-by: Lukas Wunner 
Signed-off-by: Marcel Holtmann 
Signed-off-by: Sasha Levin 
---
 drivers/bluetooth/hci_bcm.c | 24 +---
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 707c2d1b84c7..b8eeac4e6f35 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -577,11 +577,9 @@ static int bcm_suspend_device(struct device *dev)
}
 
/* Suspend the device */
-   if (bdev->device_wakeup) {
-   gpiod_set_value(bdev->device_wakeup, false);
-   bt_dev_dbg(bdev, "suspend, delaying 15 ms");
-   mdelay(15);
-   }
+   gpiod_set_value(bdev->device_wakeup, false);
+   bt_dev_dbg(bdev, "suspend, delaying 15 ms");
+   mdelay(15);
 
return 0;
 }
@@ -592,11 +590,9 @@ static int bcm_resume_device(struct device *dev)
 
bt_dev_dbg(bdev, "");
 
-   if (bdev->device_wakeup) {
-   gpiod_set_value(bdev->device_wakeup, true);
-   bt_dev_dbg(bdev, "resume, delaying 15 ms");
-   mdelay(15);
-   }
+   gpiod_set_value(bdev->device_wakeup, true);
+   bt_dev_dbg(bdev, "resume, delaying 15 ms");
+   mdelay(15);
 
/* When this executes, the device has woken up already */
if (bdev->is_suspended && bdev->hu) {
@@ -779,14 +775,12 @@ static int bcm_get_resources(struct bcm_device *dev)
 
dev->clk = devm_clk_get(dev->dev, NULL);
 
-   dev->device_wakeup = devm_gpiod_get_optional(dev->dev,
-"device-wakeup",
-GPIOD_OUT_LOW);
+   dev->device_wakeup = devm_gpiod_get(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(dev->dev, "shutdown",
-   GPIOD_OUT_LOW);
+   dev->shutdown = devm_gpiod_get(dev->dev, "shutdown", GPIOD_OUT_LOW);
if (IS_ERR(dev->shutdown))
return PTR_ERR(dev->shutdown);
 
-- 
2.14.1

[PATCH AUTOSEL for 4.15 099/124] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO

[Sasha Levin]

From: Lukas Wunner 

[ Upstream commit 3e81a4ca51a1172253078ca7abd6a91040b8fcf4 ]

Commit 0395ffc1ee05 ("Bluetooth: hci_bcm: Add PM for BCM devices")
amended this driver to request a shutdown and device wake GPIO on probe,
but mandated that only one of them need to be present:

/* 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, "invalid platform data\n");
return -EINVAL;
}

However the same commit added a call to bcm_gpio_set_power() to the
->probe hook, which unconditionally accesses *both* GPIOs.  Luckily,
the resulting NULL pointer deref was never reported, suggesting there's
no machine where either GPIO is missing.

Commit 8a92056837fd ("Bluetooth: hci_bcm: Add (runtime)pm support to the
serdev driver") removed the check whether at least one of the GPIOs is
present without specifying a reason.

Because commit 62aaefa7d038 ("Bluetooth: hci_bcm: improve use of gpios
API") refactored the driver to use devm_gpiod_get_optional() instead of
devm_gpiod_get(), one is now tempted to believe that the driver doesn't
require *any* of the two GPIOs.

Which is wrong, the driver still requires both GPIOs to avoid a NULL
pointer deref.  To this end, establish the status quo ante and request
the GPIOs with devm_gpiod_get() again.  Bail out of ->probe if either
of them is missing.

Oddly enough, whereas bcm_gpio_set_power() accesses the device wake pin
unconditionally, bcm_suspend_device() and bcm_resume_device() do check
for its presence before accessing it.  Those checks are superfluous,
so remove them.

Cc: Frédéric Danis 
Cc: Loic Poulain 
Cc: Hans de Goede 
Cc: Uwe Kleine-König 
Cc: Linus Walleij 
Reviewed-by: Andy Shevchenko 
Signed-off-by: Lukas Wunner 
Signed-off-by: Marcel Holtmann 
Signed-off-by: Sasha Levin 
---
 drivers/bluetooth/hci_bcm.c | 24 +---
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 707c2d1b84c7..b8eeac4e6f35 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -577,11 +577,9 @@ static int bcm_suspend_device(struct device *dev)
}
 
/* Suspend the device */
-   if (bdev->device_wakeup) {
-   gpiod_set_value(bdev->device_wakeup, false);
-   bt_dev_dbg(bdev, "suspend, delaying 15 ms");
-   mdelay(15);
-   }
+   gpiod_set_value(bdev->device_wakeup, false);
+   bt_dev_dbg(bdev, "suspend, delaying 15 ms");
+   mdelay(15);
 
return 0;
 }
@@ -592,11 +590,9 @@ static int bcm_resume_device(struct device *dev)
 
bt_dev_dbg(bdev, "");
 
-   if (bdev->device_wakeup) {
-   gpiod_set_value(bdev->device_wakeup, true);
-   bt_dev_dbg(bdev, "resume, delaying 15 ms");
-   mdelay(15);
-   }
+   gpiod_set_value(bdev->device_wakeup, true);
+   bt_dev_dbg(bdev, "resume, delaying 15 ms");
+   mdelay(15);
 
/* When this executes, the device has woken up already */
if (bdev->is_suspended && bdev->hu) {
@@ -779,14 +775,12 @@ static int bcm_get_resources(struct bcm_device *dev)
 
dev->clk = devm_clk_get(dev->dev, NULL);
 
-   dev->device_wakeup = devm_gpiod_get_optional(dev->dev,
-"device-wakeup",
-GPIOD_OUT_LOW);
+   dev->device_wakeup = devm_gpiod_get(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(dev->dev, "shutdown",
-   GPIOD_OUT_LOW);
+   dev->shutdown = devm_gpiod_get(dev->dev, "shutdown", GPIOD_OUT_LOW);
if (IS_ERR(dev->shutdown))
return PTR_ERR(dev->shutdown);
 
-- 
2.14.1

Re: [PATCH AUTOSEL for 4.15 099/124] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO

[Lukas Wunner]

On Mon, Mar 19, 2018 at 03:48:50PM +, Sasha Levin wrote:
> From: Lukas Wunner 
> 
> [ Upstream commit 3e81a4ca51a1172253078ca7abd6a91040b8fcf4 ]
>

Unfortunately this commit had to be fixed up with ab2f336cb7e6
("Bluetooth: hci_bcm: Make shutdown and device wake GPIO optional")
by Stefan Wahren (+cc).

Please be sure to apply that one on top.  Thanks!


> Commit 0395ffc1ee05 ("Bluetooth: hci_bcm: Add PM for BCM devices")
> amended this driver to request a shutdown and device wake GPIO on probe,
> but mandated that only one of them need to be present:
> 
>   /* 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, "invalid platform data\n");
>   return -EINVAL;
>   }
> 
> However the same commit added a call to bcm_gpio_set_power() to the
> ->probe hook, which unconditionally accesses *both* GPIOs.  Luckily,
> the resulting NULL pointer deref was never reported, suggesting there's
> no machine where either GPIO is missing.
> 
> Commit 8a92056837fd ("Bluetooth: hci_bcm: Add (runtime)pm support to the
> serdev driver") removed the check whether at least one of the GPIOs is
> present without specifying a reason.
> 
> Because commit 62aaefa7d038 ("Bluetooth: hci_bcm: improve use of gpios
> API") refactored the driver to use devm_gpiod_get_optional() instead of
> devm_gpiod_get(), one is now tempted to believe that the driver doesn't
> require *any* of the two GPIOs.
> 
> Which is wrong, the driver still requires both GPIOs to avoid a NULL
> pointer deref.  To this end, establish the status quo ante and request
> the GPIOs with devm_gpiod_get() again.  Bail out of ->probe if either
> of them is missing.
> 
> Oddly enough, whereas bcm_gpio_set_power() accesses the device wake pin
> unconditionally, bcm_suspend_device() and bcm_resume_device() do check
> for its presence before accessing it.  Those checks are superfluous,
> so remove them.
> 
> Cc: Frédéric Danis 
> Cc: Loic Poulain 
> Cc: Hans de Goede 
> Cc: Uwe Kleine-König 
> Cc: Linus Walleij 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Lukas Wunner 
> Signed-off-by: Marcel Holtmann 
> Signed-off-by: Sasha Levin 
> ---
>  drivers/bluetooth/hci_bcm.c | 24 +---
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 707c2d1b84c7..b8eeac4e6f35 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -577,11 +577,9 @@ static int bcm_suspend_device(struct device *dev)
>   }
>  
>   /* Suspend the device */
> - if (bdev->device_wakeup) {
> - gpiod_set_value(bdev->device_wakeup, false);
> - bt_dev_dbg(bdev, "suspend, delaying 15 ms");
> - mdelay(15);
> - }
> + gpiod_set_value(bdev->device_wakeup, false);
> + bt_dev_dbg(bdev, "suspend, delaying 15 ms");
> + mdelay(15);
>  
>   return 0;
>  }
> @@ -592,11 +590,9 @@ static int bcm_resume_device(struct device *dev)
>  
>   bt_dev_dbg(bdev, "");
>  
> - if (bdev->device_wakeup) {
> - gpiod_set_value(bdev->device_wakeup, true);
> - bt_dev_dbg(bdev, "resume, delaying 15 ms");
> - mdelay(15);
> - }
> + gpiod_set_value(bdev->device_wakeup, true);
> + bt_dev_dbg(bdev, "resume, delaying 15 ms");
> + mdelay(15);
>  
>   /* When this executes, the device has woken up already */
>   if (bdev->is_suspended && bdev->hu) {
> @@ -779,14 +775,12 @@ static int bcm_get_resources(struct bcm_device *dev)
>  
>   dev->clk = devm_clk_get(dev->dev, NULL);
>  
> - dev->device_wakeup = devm_gpiod_get_optional(dev->dev,
> -  "device-wakeup",
> -  GPIOD_OUT_LOW);
> + dev->device_wakeup = devm_gpiod_get(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(dev->dev, "shutdown",
> - GPIOD_OUT_LOW);
> + dev->shutdown = devm_gpiod_get(dev->dev, "shutdown", GPIOD_OUT_LOW);
>   if (IS_ERR(dev->shutdown))
>   return PTR_ERR(dev->shutdown);
>  
> -- 
> 2.14.1

Re: [PATCH AUTOSEL for 4.15 099/124] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO

[Lukas Wunner]

On Mon, Mar 19, 2018 at 03:48:50PM +, Sasha Levin wrote:
> From: Lukas Wunner 
> 
> [ Upstream commit 3e81a4ca51a1172253078ca7abd6a91040b8fcf4 ]
>

Unfortunately this commit had to be fixed up with ab2f336cb7e6
("Bluetooth: hci_bcm: Make shutdown and device wake GPIO optional")
by Stefan Wahren (+cc).

Please be sure to apply that one on top.  Thanks!


> Commit 0395ffc1ee05 ("Bluetooth: hci_bcm: Add PM for BCM devices")
> amended this driver to request a shutdown and device wake GPIO on probe,
> but mandated that only one of them need to be present:
> 
>   /* 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, "invalid platform data\n");
>   return -EINVAL;
>   }
> 
> However the same commit added a call to bcm_gpio_set_power() to the
> ->probe hook, which unconditionally accesses *both* GPIOs.  Luckily,
> the resulting NULL pointer deref was never reported, suggesting there's
> no machine where either GPIO is missing.
> 
> Commit 8a92056837fd ("Bluetooth: hci_bcm: Add (runtime)pm support to the
> serdev driver") removed the check whether at least one of the GPIOs is
> present without specifying a reason.
> 
> Because commit 62aaefa7d038 ("Bluetooth: hci_bcm: improve use of gpios
> API") refactored the driver to use devm_gpiod_get_optional() instead of
> devm_gpiod_get(), one is now tempted to believe that the driver doesn't
> require *any* of the two GPIOs.
> 
> Which is wrong, the driver still requires both GPIOs to avoid a NULL
> pointer deref.  To this end, establish the status quo ante and request
> the GPIOs with devm_gpiod_get() again.  Bail out of ->probe if either
> of them is missing.
> 
> Oddly enough, whereas bcm_gpio_set_power() accesses the device wake pin
> unconditionally, bcm_suspend_device() and bcm_resume_device() do check
> for its presence before accessing it.  Those checks are superfluous,
> so remove them.
> 
> Cc: Frédéric Danis 
> Cc: Loic Poulain 
> Cc: Hans de Goede 
> Cc: Uwe Kleine-König 
> Cc: Linus Walleij 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Lukas Wunner 
> Signed-off-by: Marcel Holtmann 
> Signed-off-by: Sasha Levin 
> ---
>  drivers/bluetooth/hci_bcm.c | 24 +---
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 707c2d1b84c7..b8eeac4e6f35 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -577,11 +577,9 @@ static int bcm_suspend_device(struct device *dev)
>   }
>  
>   /* Suspend the device */
> - if (bdev->device_wakeup) {
> - gpiod_set_value(bdev->device_wakeup, false);
> - bt_dev_dbg(bdev, "suspend, delaying 15 ms");
> - mdelay(15);
> - }
> + gpiod_set_value(bdev->device_wakeup, false);
> + bt_dev_dbg(bdev, "suspend, delaying 15 ms");
> + mdelay(15);
>  
>   return 0;
>  }
> @@ -592,11 +590,9 @@ static int bcm_resume_device(struct device *dev)
>  
>   bt_dev_dbg(bdev, "");
>  
> - if (bdev->device_wakeup) {
> - gpiod_set_value(bdev->device_wakeup, true);
> - bt_dev_dbg(bdev, "resume, delaying 15 ms");
> - mdelay(15);
> - }
> + gpiod_set_value(bdev->device_wakeup, true);
> + bt_dev_dbg(bdev, "resume, delaying 15 ms");
> + mdelay(15);
>  
>   /* When this executes, the device has woken up already */
>   if (bdev->is_suspended && bdev->hu) {
> @@ -779,14 +775,12 @@ static int bcm_get_resources(struct bcm_device *dev)
>  
>   dev->clk = devm_clk_get(dev->dev, NULL);
>  
> - dev->device_wakeup = devm_gpiod_get_optional(dev->dev,
> -  "device-wakeup",
> -  GPIOD_OUT_LOW);
> + dev->device_wakeup = devm_gpiod_get(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(dev->dev, "shutdown",
> - GPIOD_OUT_LOW);
> + dev->shutdown = devm_gpiod_get(dev->dev, "shutdown", GPIOD_OUT_LOW);
>   if (IS_ERR(dev->shutdown))
>   return PTR_ERR(dev->shutdown);
>  
> -- 
> 2.14.1