Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()

PATCH Applied Bartosz Golaszewski 8 participants 28 messages Apr 24, 2024 – Apr 26, 2024

Subsystems (2)

Bluetooth drivers Qualcomm Bluetooth driver

lore

Share & tools

lore

Bartosz Golaszewski

author

2024-04-24 12:29 UTC #0 lore raw

PATCH v2 1/1 Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()

From: Bartosz Golaszewski <[email protected]>

Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.

Reported-by Wren Turkal <[email protected]>

Reported-by Zijun Hu <[email protected]>

Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here

Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")

Reviewed-by Krzysztof Kozlowski <[email protected]>

Signed-off-by Bartosz Golaszewski <[email protected]>

---
v1 -> v2:
- also restore the previous behavior for QCA6390 and other models that fall under the default: label in the affected switch case
- bail-out on errors when getting the swctrl GPIO too

 drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

drivers/bluetooth/hci_qca.c +13 -5

index 92fa20f5ac7d..0e98ad2c0c9d 100644
@@ -2327,16 +2327,21 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855)) {
 			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
-			power_ctrl_enabled = false;
+			return PTR_ERR(qcadev->bt_en);
 		}
 
+		if (!qcadev->bt_en)
+			power_ctrl_enabled = false;
+
 		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
 					       GPIOD_IN);
 		if (IS_ERR(qcadev->sw_ctrl) &&
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855 ||
-		     data->soc_type == QCA_WCN7850))
-			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+		     data->soc_type == QCA_WCN7850)) {
+			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+			return PTR_ERR(qcadev->sw_ctrl);
+		}
 
 		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
 		if (IS_ERR(qcadev->susclk)) {

@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
 					       GPIOD_OUT_LOW);
 		if (IS_ERR(qcadev->bt_en)) {
-			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-			power_ctrl_enabled = false;
+			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+			return PTR_ERR(qcadev->bt_en);
 		}
 
+		if (!qcadev->bt_en)
+			power_ctrl_enabled = false;
+
 		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
 		if (IS_ERR(qcadev->susclk)) {
 			dev_warn(&serdev->dev, "failed to acquire clk\n");

Signature · 1 line

2.40.1

quic_zijuhu

2024-04-24 12:35 UTC #1 lore raw

Quoted 71 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.
Reported-by Wren Turkal <[email protected]>
Reported-by Zijun Hu <[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>
---
v1 -> v2:
- also restore the previous behavior for QCA6390 and other models that fall under the default: label in the affected switch case
- bail-out on errors when getting the swctrl GPIO too
 drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
drivers/bluetooth/hci_qca.c +13 -5
index 92fa20f5ac7d..0e98ad2c0c9d 100644
@@ -2327,16 +2327,21 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855)) {
 			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
-			power_ctrl_enabled = false;
+			return PTR_ERR(qcadev->bt_en);
 		}
 
+		if (!qcadev->bt_en)
+			power_ctrl_enabled = false;
+
 		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
 					       GPIOD_IN);
 		if (IS_ERR(qcadev->sw_ctrl) &&
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855 ||
-		     data->soc_type == QCA_WCN7850))
-			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+		     data->soc_type == QCA_WCN7850)) {
+			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+			return PTR_ERR(qcadev->sw_ctrl);
+		}
 
 		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
 		if (IS_ERR(qcadev->susclk)) {
@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
 					       GPIOD_OUT_LOW);
 		if (IS_ERR(qcadev->bt_en)) {
-			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-			power_ctrl_enabled = false;
+			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+			return PTR_ERR(qcadev->bt_en);
 		}
 
+		if (!qcadev->bt_en)
+			power_ctrl_enabled = false;
+
 		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
 		if (IS_ERR(qcadev->susclk)) {
 			dev_warn(&serdev->dev, "failed to acquire clk\n");NAK, you don't answer my question for your v1 before send v2

quic_zijuhu

2024-04-24 12:59 UTC #2 lore raw

In reply to Bartosz Golaszewski

Quoted 11 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.

Quoted 5 lines

Reported-by Wren Turkal <[email protected]>
Reported-by Zijun Hu <[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski <[email protected]>

is it really reviewed-by Krzysztof?
suggest reviewer give explicit review-by tag by public way, then you add this tag.

Quoted 15 lines

Signed-off-by Bartosz Golaszewski <[email protected]>
---
v1 -> v2:
- also restore the previous behavior for QCA6390 and other models that fall under the default: label in the affected switch case
- bail-out on errors when getting the swctrl GPIO too
 drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
drivers/bluetooth/hci_qca.c +0 -0
index 92fa20f5ac7d..0e98ad2c0c9d 100644

drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2330

Quoted 5 lines

drivers/bluetooth/hci_qca.c +1 -1

@@ -2327,16 +2327,21 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855)) {
 			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
-			power_ctrl_enabled = false;
+

think about what will happen for present lunched products if this type error really happens.
BT don't work at all with your change. BT can be used mostly without your change.

			return PTR_ERR(qcadev->bt_en);

drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2335

Quoted 5 lines

drivers/bluetooth/hci_qca.c +3 -0

 		}
 
+		if (!qcadev->bt_en)
+			power_ctrl_enabled = false;
+

you don't answer me how to treat a required enable is not configured by user

Quoted 15 lines

drivers/bluetooth/hci_qca.c +4 -2

 		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
 					       GPIOD_IN);
 		if (IS_ERR(qcadev->sw_ctrl) &&
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855 ||
-		     data->soc_type == QCA_WCN7850))
-			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+		     data->soc_type == QCA_WCN7850)) {
+			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+			return PTR_ERR(qcadev->sw_ctrl);have the same question as above.
+		}
 
 		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
 		if (IS_ERR(qcadev->susclk)) {

drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2366

Quoted 9 lines

drivers/bluetooth/hci_qca.c +2 -2

@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
 					       GPIOD_OUT_LOW);
 		if (IS_ERR(qcadev->bt_en)) {
-			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-			power_ctrl_enabled = false;
+			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+			return PTR_ERR(qcadev->bt_en);
 		}

have the same question as above.
is it right for such prompt ?

drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2372

Quoted 6 lines

drivers/bluetooth/hci_qca.c +3 -0

+		if (!qcadev->bt_en)
+			power_ctrl_enabled = false;
+
 		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
 		if (IS_ERR(qcadev->susclk)) {
 			dev_warn(&serdev->dev, "failed to acquire clk\n");

have the same question as above.

how do you known the root cause of the issue reported without my earlier debugging and fix?

do my fix regarding the issue i concerned have any fault?

NAK by me.

Wren Turkal

2024-04-24 13:16 UTC #3 lore raw

Quoted 85 lines · quic_zijuhu

Quoted 11 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.
Quoted 5 lines
Reported-by Wren Turkal <[email protected]>
Reported-by Zijun Hu <[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski <[email protected]>
is it really reviewed-by Krzysztof?
suggest reviewer give explicit review-by tag by public way, then you add this tag.
Quoted 15 lines
Signed-off-by Bartosz Golaszewski <[email protected]>
---
v1 -> v2:
- also restore the previous behavior for QCA6390 and other models that fall under the default: label in the affected switch case
- bail-out on errors when getting the swctrl GPIO too
  drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
  1 file changed, 13 insertions(+), 5 deletions(-)
drivers/bluetooth/hci_qca.c +0 -0
index 92fa20f5ac7d..0e98ad2c0c9d 100644
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2330
Quoted 5 lines
drivers/bluetooth/hci_qca.c +1 -1
@@ -2327,16 +2327,21 @@ static int qca_serdev_probe(struct serdev_device *serdev)
  		    (data->soc_type == QCA_WCN6750 ||
  		     data->soc_type == QCA_WCN6855)) {
  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
-			power_ctrl_enabled = false;
+
think about what will happen for present lunched products if this type error really happens.
BT don't work at all with your change. BT can be used mostly without your change.
			return PTR_ERR(qcadev->bt_en);
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2335
Quoted 5 lines
drivers/bluetooth/hci_qca.c +3 -0
  		}
 
+		if (!qcadev->bt_en)
+			power_ctrl_enabled = false;
+
you don't answer me how to treat a required enable is not configured by user
Quoted 15 lines
drivers/bluetooth/hci_qca.c +4 -2
  		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
  					       GPIOD_IN);
  		if (IS_ERR(qcadev->sw_ctrl) &&
  		    (data->soc_type == QCA_WCN6750 ||
  		     data->soc_type == QCA_WCN6855 ||
-		     data->soc_type == QCA_WCN7850))
-			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+		     data->soc_type == QCA_WCN7850)) {
+			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+			return PTR_ERR(qcadev->sw_ctrl);have the same question as above.
+		}
 
  		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
  		if (IS_ERR(qcadev->susclk)) {
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2366
Quoted 9 lines
drivers/bluetooth/hci_qca.c +2 -2
@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
  		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
  					       GPIOD_OUT_LOW);
  		if (IS_ERR(qcadev->bt_en)) {
-			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-			power_ctrl_enabled = false;
+			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+			return PTR_ERR(qcadev->bt_en);
  		}
 
have the same question as above.
is it right for such prompt ?
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2372
Quoted 6 lines
drivers/bluetooth/hci_qca.c +3 -0
+		if (!qcadev->bt_en)
+			power_ctrl_enabled = false;
+
  		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
  		if (IS_ERR(qcadev->susclk)) {
  			dev_warn(&serdev->dev, "failed to acquire clk\n");
have the same question as above.

Quoted 2 lines

how do you known the root cause of the issue reported without my earlier debugging and fix?

Without your debugging, this fix would not have been possible.

Quoted 8 lines

do my fix regarding the issue i concerned have any fault?
NAK by me.

Signature · 1 line

You're more amazing than you think!

Wren Turkal

Tested-by on v2 1/1

2024-04-24 13:10 UTC #4 lore raw

In reply to Bartosz Golaszewski

Quoted 11 lines · Bartosz Golaszewski

From: Bartosz Golaszewski<[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.

Quoted 6 lines

Reported-by Wren Turkal<[email protected]>
Reported-by Zijun Hu<[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski<[email protected]>
Signed-off-by Bartosz Golaszewski<[email protected]>

Tested-by "Wren Turkal" <[email protected]>

Like this?
wt

Signature · 1 line

You're more amazing than you think!

Bartosz Golaszewski

Reviewed-by Krzysztof Kozlowski

2024-04-24 13:19 UTC #5 lore raw

Quoted 20 lines · Wren Turkal

Quoted 11 lines · Bartosz Golaszewski
From: Bartosz Golaszewski<[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.
Quoted 6 lines
Reported-by Wren Turkal<[email protected]>
Reported-by Zijun Hu<[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski<[email protected]>
Signed-off-by Bartosz Golaszewski<[email protected]>

Quoted 4 lines

Tested-by "Wren Turkal" <[email protected]>
Like this?

Yes, awesome, thanks.

This is how reviewing works too in the kernel, look at what Krzysztof did under v1, he just wrote:

Reviewed-by Krzysztof Kozlowski<[email protected]>

And mailing list tools will pick it up.

Bartosz

quic_zijuhu

2024-04-24 13:22 UTC #6 lore raw

Quoted 24 lines · Bartosz Golaszewski

Quoted 23 lines · Wren Turkal
Quoted 11 lines · Bartosz Golaszewski
From: Bartosz Golaszewski<[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.
Quoted 6 lines
Reported-by Wren Turkal<[email protected]>
Reported-by Zijun Hu<[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski<[email protected]>
Signed-off-by Bartosz Golaszewski<[email protected]>
Tested-by "Wren Turkal" <[email protected]>

Quoted 9 lines

Quoted 1 line
Like this?
Yes, awesome, thanks.
This is how reviewing works too in the kernel, look at what Krzysztof did under v1, he just wrote:
Reviewed-by Krzysztof Kozlowski<[email protected]>

v1 have obvious something wrong as i pointed and verified. so i think it is not suitable to attach v1's review-by tag to v2 anyway.

Quoted 3 lines

And mailing list tools will pick it up.
Bartosz

Wren Turkal

2024-04-24 13:31 UTC #7 lore raw

Quoted 29 lines · quic_zijuhu

Quoted 28 lines · Bartosz Golaszewski
Quoted 20 lines · Wren Turkal
Quoted 11 lines · Bartosz Golaszewski
From: Bartosz Golaszewski<[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.
Quoted 6 lines
Reported-by Wren Turkal<[email protected]>
Reported-by Zijun Hu<[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski<[email protected]>
Signed-off-by Bartosz Golaszewski<[email protected]>
Quoted 4 lines
Tested-by "Wren Turkal" <[email protected]>
Like this?
Yes, awesome, thanks.

Quoted 7 lines

Quoted 5 lines
This is how reviewing works too in the kernel, look at what Krzysztof did under v1, he just wrote:
Reviewed-by Krzysztof Kozlowski<[email protected]>
v1 have obvious something wrong as i pointed and verified. so i think it is not suitable to attach v1's review-by tag to v2 anyway.

@Zijun, your concern is that current DTs may not define the gpio and this will cause the bluetooth not to work?

Would that not more appropriately be fixed by machine-specific fixups for the DT?

Quoted 5 lines

Quoted 4 lines
And mailing list tools will pick it up.
Bartosz

Signature · 1 line

You're more amazing than you think!

quic_zijuhu

2024-04-24 13:36 UTC #8 lore raw

Quoted 38 lines · Wren Turkal

Quoted 37 lines · quic_zijuhu
Quoted 36 lines · Bartosz Golaszewski
Quoted 23 lines · Wren Turkal
Quoted 13 lines · Bartosz Golaszewski
From: Bartosz Golaszewski<[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth:
hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.
Quoted 7 lines
Reported-by Wren Turkal<[email protected]>
Reported-by Zijun Hu<[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use
IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski<[email protected]>
Signed-off-by Bartosz Golaszewski<[email protected]>
Quoted 4 lines
Tested-by "Wren Turkal" <[email protected]>
Like this?
Yes, awesome, thanks.
This is how reviewing works too in the kernel, look at what Krzysztof did under v1, he just wrote:
Reviewed-by Krzysztof Kozlowski<[email protected]>

Quoted 9 lines

Quoted 2 lines
v1 have obvious something wrong as i pointed and verified. so i think it is not suitable to attach v1's review-by tag to v2 anyway.
@Zijun, your concern is that current DTs may not define the gpio and this will cause the bluetooth not to work?
Would that not more appropriately be fixed by machine-specific fixups for the DT?

for lunched production, it is difficult or not possible to change such config.

Quoted 6 lines

Quoted 5 lines
Quoted 4 lines
And mailing list tools will pick it up.
Bartosz

Wren Turkal

2024-04-24 13:40 UTC #9 lore raw

Quoted 42 lines · quic_zijuhu

Quoted 41 lines · Wren Turkal
Quoted 39 lines · quic_zijuhu
Quoted 27 lines · Bartosz Golaszewski
Quoted 26 lines · Wren Turkal
Quoted 13 lines · Bartosz Golaszewski
From: Bartosz Golaszewski<[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth:
hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.
Quoted 7 lines
Reported-by Wren Turkal<[email protected]>
Reported-by Zijun Hu<[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use
IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski<[email protected]>
Signed-off-by Bartosz Golaszewski<[email protected]>
Tested-by "Wren Turkal" <[email protected]>
Quoted 9 lines
Quoted 1 line
Like this?
Yes, awesome, thanks.
This is how reviewing works too in the kernel, look at what Krzysztof did under v1, he just wrote:
Reviewed-by Krzysztof Kozlowski<[email protected]>
v1 have obvious something wrong as i pointed and verified. so i think it is not suitable to attach v1's review-by tag to v2 anyway.

Quoted 8 lines

Quoted 6 lines
@Zijun, your concern is that current DTs may not define the gpio and this will cause the bluetooth not to work?
Would that not more appropriately be fixed by machine-specific fixups for the DT?
for lunched production, it is difficult or not possible to change such config.

I am not talking about the DT in the device. I am talking about the mechanism the kernel has for applying fixups to DTs.

If a dev builds a new kernel for a dev and finds it not to work, the kernel would then have a fixup added, like described here: https://docs.kernel.org/devicetree/usage-model.html#platform-identification

Quoted 8 lines

Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
And mailing list tools will pick it up.
Bartosz

Signature · 1 line

You're more amazing than you think!

Bartosz Golaszewski

author

2024-04-24 13:40 UTC #10 lore raw

In reply to Wren Turkal

Quoted 34 lines · Wren Turkal

Quoted 32 lines · quic_zijuhu
Quoted 31 lines · Bartosz Golaszewski
Quoted 20 lines · Wren Turkal
Quoted 11 lines · Bartosz Golaszewski
From: Bartosz Golaszewski<[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.
Quoted 6 lines
Reported-by Wren Turkal<[email protected]>
Reported-by Zijun Hu<[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski<[email protected]>
Signed-off-by Bartosz Golaszewski<[email protected]>
Quoted 4 lines
Tested-by "Wren Turkal" <[email protected]>
Like this?
Yes, awesome, thanks.
This is how reviewing works too in the kernel, look at what Krzysztof did under v1, he just wrote:

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Reviewed-by Krzysztof Kozlowski<[email protected]>
v1 have obvious something wrong as i pointed and verified. so i think it is not suitable to attach v1's review-by tag to v2 anyway.
@Zijun, your concern is that current DTs may not define the gpio and this will cause the bluetooth not to work?

This is simply not true. If the GPIO is not specified, gpiod_get_optional() will return NULL and GPIO APIs will work just fine.

That being said: the contract for whether a GPIO is needed or not is not in the driver C code or released DT sources. It's in the bindings documents under Documentation/devicetree/bindings/. This is the source of truth. So if the binding for a given model has always said "required: enable-gpios" then we're absolutely in our rights to fix the driver to conform to that even if previously omitted. If you think otherwise - relax the bindings first.

Bart

Quoted 11 lines

Would that not more appropriately be fixed by machine-specific fixups for the DT?
Quoted 5 lines
Quoted 4 lines
And mailing list tools will pick it up.
Bartosz
--
You're more amazing than you think!

[email protected]

2024-04-24 13:16 UTC #11 lore raw

In reply to Bartosz Golaszewski

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=847442

---Test result---

Test Summary:

CheckPatch                    FAIL      0.94 seconds
GitLint                       FAIL      0.51 seconds
SubjectPrefix                 PASS      0.10 seconds
BuildKernel                   PASS      29.92 seconds
CheckAllWarning               PASS      32.99 seconds
CheckSparse                   PASS      39.01 seconds
CheckSmatch                   FAIL      36.66 seconds
BuildKernel32                 PASS      29.51 seconds
TestRunnerSetup               PASS      537.50 seconds
TestRunner_l2cap-tester       PASS      21.34 seconds
TestRunner_iso-tester         PASS      31.05 seconds
TestRunner_bnep-tester        PASS      4.70 seconds
TestRunner_mgmt-tester        FAIL      113.32 seconds
TestRunner_rfcomm-tester      PASS      7.47 seconds
TestRunner_sco-tester         PASS      15.20 seconds
TestRunner_ioctl-tester       PASS      7.94 seconds
TestRunner_mesh-tester        PASS      5.88 seconds
TestRunner_smp-tester         PASS      7.04 seconds
TestRunner_userchan-tester    PASS      5.00 seconds
IncrementalBuild              PASS      29.56 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #108:

Reported-by Wren Turkal <[email protected]>

Reported-by Zijun Hu <[email protected]>

total: 0 errors, 1 warnings, 39 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13641795.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (84>80): "[v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()"
16: B1 Line exceeds max length (106>80): "Closes: https://lore.kernel.org/linux-bluetooth/[email protected]/"
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139 make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2 make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139 make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bpa10x.o] Error 139 make[4]: *** Deleting file 'drivers/bluetooth/bpa10x.o'
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2 make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2

Failed Test Cases
LL Privacy - Remove Device 4 (Disable Adv) Timed out 2.174 seconds

---
Regards,
Linux Bluetooth

Krzysztof Kozlowski

2024-04-24 14:46 UTC #12 lore raw

In reply to Bartosz Golaszewski

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

drivers/bluetooth/hci_qca.c +0 -0

 		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
 		if (IS_ERR(qcadev->susclk)) {

@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",

drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2368

Quoted 10 lines

drivers/bluetooth/hci_qca.c +4 -2

 					       GPIOD_OUT_LOW);
 		if (IS_ERR(qcadev->bt_en)) {
-			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-			power_ctrl_enabled = false;
+			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+			return PTR_ERR(qcadev->bt_en);
 		}
 
+		if (!qcadev->bt_en)
+			power_ctrl_enabled = false;

This looks duplicated - you already have such check earlier.

Best regards,
Krzysztof

Bartosz Golaszewski

2024-04-24 14:52 UTC #13 lore raw

Quoted 17 lines · Krzysztof Kozlowski

Quoted 15 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

              qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
              if (IS_ERR(qcadev->susclk)) {

drivers/bluetooth/hci_qca.c +2 -2

@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
              qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
                                             GPIOD_OUT_LOW);
              if (IS_ERR(qcadev->bt_en)) {
-                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-                     power_ctrl_enabled = false;
+                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+                     return PTR_ERR(qcadev->bt_en);
              }

Quoted 5 lines

Review static int qca_serdev_probe(struct serdev_device *serdev) · line 2368
Quoted 2 lines
(quoted diff) +2 -0
+             if (!qcadev->bt_en)
+                     power_ctrl_enabled = false;
This looks duplicated - you already have such check earlier.

It's under a different switch case!

Bartosz

Krzysztof Kozlowski

Reviewed-by

2024-04-24 15:05 UTC #14 lore raw

Quoted 19 lines · Bartosz Golaszewski

Quoted 17 lines · Krzysztof Kozlowski

Quoted 15 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

              qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
              if (IS_ERR(qcadev->susclk)) {

drivers/bluetooth/hci_qca.c +2 -2

@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
              qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
                                             GPIOD_OUT_LOW);
              if (IS_ERR(qcadev->bt_en)) {
-                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-                     power_ctrl_enabled = false;
+                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+                     return PTR_ERR(qcadev->bt_en);
              }

Quoted 7 lines

Quoted 5 lines
Review static int qca_serdev_probe(struct serdev_device *serdev) · line 2368
Quoted 2 lines
(quoted diff) +2 -0
+             if (!qcadev->bt_en)
+                     power_ctrl_enabled = false;
This looks duplicated - you already have such check earlier.
It's under a different switch case!

Damn diff. Yeah, looks ok.

Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

quic_zijuhu

2024-04-24 15:24 UTC #15 lore raw

In reply to Bartosz Golaszewski

Quoted 7 lines · Bartosz Golaszewski

Quoted 5 lines · Krzysztof Kozlowski
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>

Quoted 10 lines

drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2364

Quoted 10 lines

              qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
              if (IS_ERR(qcadev->susclk)) {

drivers/bluetooth/hci_qca.c +2 -2

@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
              qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
                                             GPIOD_OUT_LOW);
              if (IS_ERR(qcadev->bt_en)) {
-                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-                     power_ctrl_enabled = false;
+                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+                     return PTR_ERR(qcadev->bt_en);

please think about for QCA2066. if it is returned from here. BT will not working at all. if you don't return here. i will be working fine for every BT functionality.
NAK again by me.

Quoted 11 lines

Quoted 7 lines
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2334
Quoted 4 lines
drivers/bluetooth/hci_qca.c +2 -0
              }
 
+             if (!qcadev->bt_en)
+                     power_ctrl_enabled = false;
This looks duplicated - you already have such check earlier.
It's under a different switch case!
Bartosz

quic_zijuhu

2024-04-24 15:30 UTC #16 lore raw

Quoted 8 lines · quic_zijuhu

Quoted 7 lines · Bartosz Golaszewski
Quoted 5 lines · Krzysztof Kozlowski
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>

Quoted 13 lines

Quoted 10 lines

drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2364

Quoted 10 lines

              qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
              if (IS_ERR(qcadev->susclk)) {

drivers/bluetooth/hci_qca.c +2 -2

@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
              qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
                                             GPIOD_OUT_LOW);
              if (IS_ERR(qcadev->bt_en)) {
-                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-                     power_ctrl_enabled = false;
+                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+                     return PTR_ERR(qcadev->bt_en);

please think about for QCA2066. if it is returned from here. BT will not working at all. if you don't return here. i will be working fine for every BT functionality.

sorry, correct a type error, it is QCA6390 not QCA2066.

Quoted 15 lines

NAK again by me.
Quoted 11 lines
Quoted 7 lines
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2334
Quoted 4 lines
drivers/bluetooth/hci_qca.c +2 -0
              }
 
+             if (!qcadev->bt_en)
+                     power_ctrl_enabled = false;
This looks duplicated - you already have such check earlier.
It's under a different switch case!
Bartosz

Bartosz Golaszewski

author

2024-04-24 15:31 UTC #17 lore raw

Quoted 23 lines · quic_zijuhu

Quoted 8 lines · quic_zijuhu
Quoted 7 lines · Bartosz Golaszewski
Quoted 5 lines · Krzysztof Kozlowski
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Quoted 13 lines
Quoted 10 lines
Quoted 10 lines
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2364
Quoted 10 lines
              qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
              if (IS_ERR(qcadev->susclk)) {
drivers/bluetooth/hci_qca.c +2 -2
@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
              qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
                                             GPIOD_OUT_LOW);
              if (IS_ERR(qcadev->bt_en)) {
-                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-                     power_ctrl_enabled = false;
+                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+                     return PTR_ERR(qcadev->bt_en);
please think about for QCA2066. if it is returned from here. BT will not working at all. if you don't return here. i will be working fine for every BT functionality.

Quoted 1 line

sorry, correct a type error, it is QCA6390 not QCA2066.

Doesn't matter. If enable-gpios is not there, gpiod_get_optional() will return NULL and we will not return.

Bart

Quoted 16 lines

Quoted 15 lines
NAK again by me.
Quoted 11 lines
Quoted 7 lines
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2334
Quoted 4 lines
drivers/bluetooth/hci_qca.c +2 -0
              }
 
+             if (!qcadev->bt_en)
+                     power_ctrl_enabled = false;
This looks duplicated - you already have such check earlier.
It's under a different switch case!
Bartosz

quic_zijuhu

2024-04-24 15:35 UTC #18 lore raw

Quoted 24 lines · Bartosz Golaszewski

Quoted 23 lines · quic_zijuhu
Quoted 21 lines · quic_zijuhu
Quoted 7 lines · Bartosz Golaszewski
Quoted 5 lines · Krzysztof Kozlowski
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Quoted 10 lines
Quoted 10 lines
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2364
Quoted 10 lines
              qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
              if (IS_ERR(qcadev->susclk)) {
drivers/bluetooth/hci_qca.c +2 -2
@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
              qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
                                             GPIOD_OUT_LOW);
              if (IS_ERR(qcadev->bt_en)) {
-                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-                     power_ctrl_enabled = false;
+                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+                     return PTR_ERR(qcadev->bt_en);
please think about for QCA2066. if it is returned from here. BT will not working at all. if you don't return here. i will be working fine for every BT functionality.

Quoted 5 lines

Quoted 1 line
sorry, correct a type error, it is QCA6390 not QCA2066.
Doesn't matter. If enable-gpios is not there, gpiod_get_optional() will return NULL and we will not return.

i think i need to explain more for my consider case. let me take QCA6390, if the property enable-gpios is configured. but IS_ERR(qcadev->bt_en) case happens, your change will do error return, so BT will not work at all

but if you don't do error return, BT will working fine.

so your fix is not right regarding QCA6390.

Quoted 18 lines

Bart
Quoted 16 lines
Quoted 15 lines
NAK again by me.
Quoted 11 lines
Quoted 7 lines
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2334
Quoted 4 lines
drivers/bluetooth/hci_qca.c +2 -0
              }
 
+             if (!qcadev->bt_en)
+                     power_ctrl_enabled = false;
This looks duplicated - you already have such check earlier.
It's under a different switch case!
Bartosz

Luiz Augusto von Dentz

maintainer

2024-04-24 15:41 UTC #19 lore raw

Hi Quic_zijuhu,

Quoted 31 lines · quic_zijuhu

Quoted 24 lines · Bartosz Golaszewski
Quoted 23 lines · quic_zijuhu
Quoted 21 lines · quic_zijuhu
Quoted 7 lines · Bartosz Golaszewski
Quoted 5 lines · Krzysztof Kozlowski
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Quoted 10 lines
Quoted 10 lines
Review static int qca_serdev_probe(struct serdev_device *serdev) · line 2364
Quoted 10 lines
              qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
              if (IS_ERR(qcadev->susclk)) {
(quoted diff) +2 -2
@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
              qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
                                             GPIOD_OUT_LOW);
              if (IS_ERR(qcadev->bt_en)) {
-                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-                     power_ctrl_enabled = false;
+                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+                     return PTR_ERR(qcadev->bt_en);
please think about for QCA2066. if it is returned from here. BT will not working at all. if you don't return here. i will be working fine for every BT functionality.
Quoted 5 lines
Quoted 1 line
sorry, correct a type error, it is QCA6390 not QCA2066.
Doesn't matter. If enable-gpios is not there, gpiod_get_optional() will return NULL and we will not return.

Quoted 8 lines

i think i need to explain more for my consider case. let me take QCA6390, if the property enable-gpios is configured. but IS_ERR(qcadev->bt_en) case happens, your change will do error return, so BT will not work at all
but if you don't do error return, BT will working fine.
so your fix is not right regarding QCA6390.

Actually I'd say he is probably right with respect to DT because that seems to require GPIO, we probably need a clearer way to differentiate if a device is being set up via DT or not (btattach), in any case DT is probably preferable thus why I went ahead and applied this one.

Quoted 19 lines

Quoted 18 lines
Bart
Quoted 16 lines
Quoted 15 lines
NAK again by me.
Quoted 11 lines
Quoted 7 lines
Review static int qca_serdev_probe(struct serdev_device *serdev) · line 2368
Quoted 4 lines
(quoted diff) +2 -0
              }
 
+             if (!qcadev->bt_en)
+                     power_ctrl_enabled = false;
This looks duplicated - you already have such check earlier.
It's under a different switch case!
Bartosz

Signature · 1 line

Luiz Augusto von Dentz

quic_zijuhu

2024-04-24 15:47 UTC #20 lore raw

Quoted 39 lines · Luiz Augusto von Dentz

Hi Quic_zijuhu,
Quoted 36 lines · quic_zijuhu
Quoted 24 lines · Bartosz Golaszewski
Quoted 23 lines · quic_zijuhu
Quoted 21 lines · quic_zijuhu
Quoted 7 lines · Bartosz Golaszewski
Quoted 5 lines · Krzysztof Kozlowski
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Quoted 10 lines
Quoted 10 lines
Review static int qca_serdev_probe(struct serdev_device *serdev) · line 2364
Quoted 10 lines
              qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
              if (IS_ERR(qcadev->susclk)) {
(quoted diff) +2 -2
@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
              qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
                                             GPIOD_OUT_LOW);
              if (IS_ERR(qcadev->bt_en)) {
-                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-                     power_ctrl_enabled = false;
+                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+                     return PTR_ERR(qcadev->bt_en);
please think about for QCA2066. if it is returned from here. BT will not working at all. if you don't return here. i will be working fine for every BT functionality.
Quoted 5 lines
Quoted 1 line
sorry, correct a type error, it is QCA6390 not QCA2066.
Doesn't matter. If enable-gpios is not there, gpiod_get_optional() will return NULL and we will not return.
i think i need to explain more for my consider case. let me take QCA6390, if the property enable-gpios is configured. but IS_ERR(qcadev->bt_en) case happens, your change will do error return, so BT will not work at all

Quoted 9 lines

Quoted 3 lines
but if you don't do error return, BT will working fine.
so your fix is not right regarding QCA6390.
Actually I'd say he is probably right with respect to DT because that seems to require GPIO, we probably need a clearer way to differentiate if a device is being set up via DT or not (btattach), in any case DT is probably preferable thus why I went ahead and applied this one.

for QCA6390, i can confirm that enable-gpios is optional. it is bring-up by my team. i can confirm reporter's machine don't config the GPIO pin. DTS binding spec also don't mark it as required.

that is why i change my changing from initial reverting the whole change to only focus on QCA6390 that is the machine reported the issue.

Quoted 21 lines

Quoted 19 lines
Quoted 18 lines
Bart
Quoted 16 lines
Quoted 15 lines
NAK again by me.
Quoted 11 lines
Quoted 7 lines
Review static int qca_serdev_probe(struct serdev_device *serdev) · line 2368
Quoted 4 lines
(quoted diff) +2 -0
              }
 
+             if (!qcadev->bt_en)
+                     power_ctrl_enabled = false;
This looks duplicated - you already have such check earlier.
It's under a different switch case!
Bartosz

quic_zijuhu

2024-04-24 15:57 UTC #21 lore raw

Quoted 49 lines · quic_zijuhu

Quoted 39 lines · Luiz Augusto von Dentz
Hi Quic_zijuhu,
Quoted 36 lines · quic_zijuhu
Quoted 24 lines · Bartosz Golaszewski
Quoted 23 lines · quic_zijuhu
Quoted 21 lines · quic_zijuhu
Quoted 7 lines · Bartosz Golaszewski
Quoted 5 lines · Krzysztof Kozlowski
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Quoted 10 lines
Quoted 10 lines
Review static int qca_serdev_probe(struct serdev_device *serdev) · line 2364
Quoted 10 lines
              qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
              if (IS_ERR(qcadev->susclk)) {
(quoted diff) +2 -2
@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
              qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
                                             GPIOD_OUT_LOW);
              if (IS_ERR(qcadev->bt_en)) {
-                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-                     power_ctrl_enabled = false;
+                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+                     return PTR_ERR(qcadev->bt_en);
please think about for QCA2066. if it is returned from here. BT will not working at all. if you don't return here. i will be working fine for every BT functionality.
Quoted 5 lines
Quoted 1 line
sorry, correct a type error, it is QCA6390 not QCA2066.
Doesn't matter. If enable-gpios is not there, gpiod_get_optional() will return NULL and we will not return.
i think i need to explain more for my consider case. let me take QCA6390, if the property enable-gpios is configured. but IS_ERR(qcadev->bt_en) case happens, your change will do error return, so BT will not work at all
Quoted 9 lines
Quoted 3 lines
but if you don't do error return, BT will working fine.
so your fix is not right regarding QCA6390.
Actually I'd say he is probably right with respect to DT because that seems to require GPIO, we probably need a clearer way to differentiate if a device is being set up via DT or not (btattach), in any case DT is probably preferable thus why I went ahead and applied this one.

Quoted 31 lines

for QCA6390, i can confirm that enable-gpios is optional. it is bring-up by my team. i can confirm reporter's machine don't config the GPIO pin. DTS binding spec also don't mark it as required.
that is why i change my changing from initial reverting the whole change to only focus on QCA6390 that is the machine reported the issue.
Quoted 21 lines
Quoted 19 lines
Quoted 18 lines
Bart
Quoted 16 lines
Quoted 15 lines
NAK again by me.
Quoted 11 lines
Quoted 7 lines
Review static int qca_serdev_probe(struct serdev_device *serdev) · line 2368
Quoted 4 lines
(quoted diff) +2 -0
              }
 
+             if (!qcadev->bt_en)
+                     power_ctrl_enabled = false;
This looks duplicated - you already have such check earlier.
It's under a different switch case!
Bartosz

i find the change have been merged.
i think it is not good manner to merge change in this way even if i give reasonable doubt

Bartosz Golaszewski

author

2024-04-24 15:30 UTC #22 lore raw

In reply to quic_zijuhu

Quoted 9 lines · quic_zijuhu

Quoted 7 lines · Bartosz Golaszewski
Quoted 5 lines · Krzysztof Kozlowski
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>

Quoted 15 lines

Quoted 10 lines

drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2364

Quoted 10 lines

              qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
              if (IS_ERR(qcadev->susclk)) {

drivers/bluetooth/hci_qca.c +2 -2

@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
              qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
                                             GPIOD_OUT_LOW);
              if (IS_ERR(qcadev->bt_en)) {
-                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-                     power_ctrl_enabled = false;
+                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+                     return PTR_ERR(qcadev->bt_en);

please think about for QCA2066. if it is returned from here. BT will not working at all. if you don't return here. i will be working fine for every BT functionality.
NAK again by me.

Luiz,

This in turn is an example of Zijun making a claim that looks like a legitimate review but is simply untrue. He's done it several times. I'm afraid that it may affect your judgment due to the confidence the claims are made with. As Krzysztof said multiple times: the device-tree bindings for QCA2066 are very clear: the enable-gpios property is *required* and so returning an error on failure here is correct. Even changing gpiod_get_optional() to just gpiod_get() would be in line with what the contract in the binding document says. Even if we relaxed the bindings, returning here stil *IS CORRECT* as if the enable-gpios is not defined, GPIOLIB will return NULL and we will NOT return.

Bartosz

Quoted 12 lines

Quoted 11 lines
Quoted 7 lines
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2334
Quoted 4 lines
drivers/bluetooth/hci_qca.c +2 -0
              }
 
+             if (!qcadev->bt_en)
+                     power_ctrl_enabled = false;
This looks duplicated - you already have such check earlier.
It's under a different switch case!
Bartosz

Krzysztof Kozlowski

2024-04-24 15:32 UTC #23 lore raw

In reply to quic_zijuhu

Quoted 8 lines · quic_zijuhu

Quoted 7 lines · Bartosz Golaszewski
Quoted 5 lines · Krzysztof Kozlowski
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>

Quoted 11 lines

Quoted 10 lines

drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 2364

Quoted 10 lines

              qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
              if (IS_ERR(qcadev->susclk)) {

drivers/bluetooth/hci_qca.c +2 -2

@@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
              qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
                                             GPIOD_OUT_LOW);
              if (IS_ERR(qcadev->bt_en)) {
-                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-                     power_ctrl_enabled = false;
+                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+                     return PTR_ERR(qcadev->bt_en);

please think about for QCA2066. if it is returned from here. BT will

Which is correct. QCA2066 requires GPIO. Look at the bindings.

Quoted 3 lines

not working at all. if you don't return here. i will be working fine for every BT functionality.
NAK again by me.

Yeah, my bad, I taught you that sentence and you keep repeating it.

Best regards,
Krzysztof

[email protected]

CI Applied

2024-04-24 15:40 UTC #24 lore raw

In reply to Bartosz Golaszewski

This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <[email protected]>:

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <[email protected]>:

Quoted 11 lines · On Wed, 24 Apr 2024 14:29:32 +0200 you wrote:

From: Bartosz Golaszewski <[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.

Quoted 1 line

[...]

Here is the summary with links:
- [v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()

    https://git.kernel.org/bluetooth/bluetooth-next/c/48a9e64a533b

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Bartosz Golaszewski

author Reviewed-by Krzysztof Kozlowski Tested-by Wren Turkal Reviewed-by Krzysztof Kozlowski Reviewed-by Krzysztof Kozlowski

2024-04-26 14:37 UTC #25 lore raw

Quoted 19 lines · On Wed, 24 Apr 2024 17:40:27 +0200, [email protected] said:

Hello:
This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <[email protected]>:
Quoted 11 lines · On Wed, 24 Apr 2024 14:29:32 +0200 you wrote:
From: Bartosz Golaszewski <[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.
Quoted 1 line
[...]

Quoted 11 lines

Here is the summary with links:
- [v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()
    https://git.kernel.org/bluetooth/bluetooth-next/c/48a9e64a533b
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Luiz,

I think patchwork borked when picking up this one, here's what the commit trailer looks like in next:

Reported-by Wren Turkal <[email protected]>

Reported-by Zijun Hu <[email protected]>

Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here

Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use

IS_ERR_OR_NULL() with gpiod_get_optional()")

Reviewed-by Krzysztof Kozlowski <[email protected]>

Signed-off-by Bartosz Golaszewski <[email protected]>

Tested-by Wren Turkal" <[email protected]>

Reported-by Wren Turkal <[email protected]>

Reported-by Zijun Hu <[email protected]>

Reviewed-by Krzysztof Kozlowski<[email protected]>

Reviewed-by Krzysztof Kozlowski <[email protected]>

Signed-off-by Luiz Augusto von Dentz <[email protected]>

Reported-by and Reviewed-by tags are duplicated. One of the RB tags is missing a space.

Bartosz

Luiz Augusto von Dentz

maintainer

2024-04-26 15:09 UTC #26 lore raw

Hi Bartosz,

Quoted 52 lines · Bartosz Golaszewski

Quoted 19 lines · On Wed, 24 Apr 2024 17:40:27 +0200, [email protected] said:
Hello:
This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <[email protected]>:
Quoted 11 lines · On Wed, 24 Apr 2024 14:29:32 +0200 you wrote:
From: Bartosz Golaszewski <[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.
Quoted 1 line
[...]
Quoted 11 lines
Here is the summary with links:
- [v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()
    https://git.kernel.org/bluetooth/bluetooth-next/c/48a9e64a533b
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Luiz,
I think patchwork borked when picking up this one, here's what the commit trailer looks like in next:
Reported-by Wren Turkal <[email protected]>
Reported-by Zijun Hu <[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use
IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>
Tested-by Wren Turkal" <[email protected]>
Reported-by Wren Turkal <[email protected]>
Reported-by Zijun Hu <[email protected]>
Reviewed-by Krzysztof Kozlowski<[email protected]>
Reviewed-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Luiz Augusto von Dentz <[email protected]>

Quoted 2 lines

Reported-by and Reviewed-by tags are duplicated. One of the RB tags is missing a space.

Oh crap, should probably not trust patchwork would pick up the tags properly, that said the pull-request was already merged, not sure if we can do something about it now?

Signature · 1 line

Luiz Augusto von Dentz

Bartosz Golaszewski

2024-04-26 17:23 UTC #27 lore raw

Quoted 56 lines · Luiz Augusto von Dentz

Hi Bartosz,
Quoted 52 lines · Bartosz Golaszewski
Quoted 19 lines · On Wed, 24 Apr 2024 17:40:27 +0200, [email protected] said:
Hello:
This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <[email protected]>:
Quoted 11 lines · On Wed, 24 Apr 2024 14:29:32 +0200 you wrote:
From: Bartosz Golaszewski <[email protected]>
Any return value from gpiod_get_optional() other than a pointer to a GPIO descriptor or a NULL-pointer is an error and the driver should abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets power_ctrl_enabled on NULL-pointer returned by devm_gpiod_get_optional(). Restore this behavior but bail-out on errors. While at it: also bail-out on error returned when trying to get the "swctrl" GPIO.
Quoted 1 line
[...]
Quoted 11 lines
Here is the summary with links:
- [v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()
    https://git.kernel.org/bluetooth/bluetooth-next/c/48a9e64a533b
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Luiz,
I think patchwork borked when picking up this one, here's what the commit trailer looks like in next:
Reported-by Wren Turkal <[email protected]>
Reported-by Zijun Hu <[email protected]>
Closes https://lore.kernel.org/linux-bluetooth/[email protected]/ open here
Fixes 56d074d26c58 ("Bluetooth: hci_qca: don't use
IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>
Tested-by Wren Turkal" <[email protected]>
Reported-by Wren Turkal <[email protected]>
Reported-by Zijun Hu <[email protected]>
Reviewed-by Krzysztof Kozlowski<[email protected]>
Reviewed-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Luiz Augusto von Dentz <[email protected]>

Quoted 7 lines

Quoted 2 lines
Reported-by and Reviewed-by tags are duplicated. One of the RB tags is missing a space.
Oh crap, should probably not trust patchwork would pick up the tags properly, that said the pull-request was already merged, not sure if we can do something about it now?

Nope, if it's gone upstream then it's too late.

BTW As a fresh b4 convert I highly recommend it for managing patches. :)

Bart

Quoted 2 lines

--
Luiz Augusto von Dentz