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.

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()")

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

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.

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()")

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

---
 drivers/bluetooth/hci_qca.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/bluetooth/hci_qca.c +0 -0

index 92fa20f5ac7d..739248c6d6b9 100644

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.

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()")

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

---
  drivers/bluetooth/hci_qca.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

drivers/bluetooth/hci_qca.c +4 -1

index 92fa20f5ac7d..739248c6d6b9 100644
@@ -2327,9 +2327,12 @@ 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) &&

Quoted 8 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.

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Quoted 10 lines · Wren Turkal

Quoted 8 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.

Quoted 6 lines

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

Bart

drivers/bluetooth/hci_qca.c +8 -3

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);

drivers/bluetooth/hci_qca.c +5 -2

 		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");

Quoted 66 lines · Bartosz Golaszewski

Quoted 10 lines · Wren Turkal

Quoted 8 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.

Quoted 6 lines

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

Bart

drivers/bluetooth/hci_qca.c +8 -3

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);

drivers/bluetooth/hci_qca.c +2 -2

 		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);
 		}
 

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");

i suggest stop here and request you code review for my changes, i found the issue and given fix for my concern.

Quoted 12 lines · Wren Turkal

Quoted 8 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.

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Quoted 3 lines

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

Quoted 16 lines · Bartosz Golaszewski

Quoted 15 lines · Wren Turkal

Quoted 8 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.

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Quoted 4 lines

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

I had a feeling that was what was going on. I'll give the patch a shot.

wt

Quoted 25 lines · Wren Turkal

Quoted 23 lines · Bartosz Golaszewski

Quoted 11 lines · Wren Turkal

Quoted 10 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.

Quoted 6 lines

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

Quoted 3 lines

I had a feeling that was what was going on. I'll give the patch a shot.

wt

Considering this patch is basically equivalent to patch 1/2 from Zijun, I am not surprised that is works similarly. I.e. on a cold boot, I can disable/re-enable bluetooth as many time as I want.

However, since this patch doesn't include the quirk fix from Zijun's patchset (patch 2/2), bluetooth fails to work after a warm boot.

@Zijun, this patch looks more idiomatic when I look at the surrounding code than your patch 1/2. Notice how it doesn't use the "else if" construct. It does the NULL test separately after checking for errors.

--
You're more amazing than you think!

Quoted 31 lines · Wren Turkal

Quoted 28 lines · Wren Turkal

Quoted 16 lines · Bartosz Golaszewski

Quoted 14 lines · Wren Turkal

Quoted 10 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.

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Quoted 7 lines

Quoted 3 lines

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

I had a feeling that was what was going on. I'll give the patch a shot.

wt

Quoted 4 lines

Considering this patch is basically equivalent to patch 1/2 from Zijun, I am not surprised that is works similarly. I.e. on a cold boot, I can disable/re-enable bluetooth as many time as I want.

Zijun didn't bail out on errors which is the issue the original patch tried to address and this one preserves.

Quoted 3 lines

However, since this patch doesn't include the quirk fix from Zijun's patchset (patch 2/2), bluetooth fails to work after a warm boot.

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

Quoted 6 lines

@Zijun, this patch looks more idiomatic when I look at the surrounding code than your patch 1/2. Notice how it doesn't use the "else if" construct. It does the NULL test separately after checking for errors.

--
You're more amazing than you think!

Bart

Quoted 27 lines · Wren Turkal

Quoted 25 lines · Wren Turkal

Quoted 23 lines · Bartosz Golaszewski

Quoted 11 lines · Wren Turkal

Quoted 10 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.

Quoted 6 lines

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

Quoted 8 lines

Quoted 3 lines

I had a feeling that was what was going on. I'll give the patch a shot.

wt

Considering this patch is basically equivalent to patch 1/2 from Zijun, I am not surprised that is works similarly. I.e. on a cold boot, I can disable/re-enable bluetooth as many time as I want.

Zijun didn't bail out on errors which is the issue the original patch tried to address and this one preserves.

Quoted 3 lines

However, since this patch doesn't include the quirk fix from Zijun's patchset (patch 2/2), bluetooth fails to work after a warm boot.

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

Quoted 6 lines

@Zijun, this patch looks more idiomatic when I look at the surrounding code than your patch 1/2. Notice how it doesn't use the "else if" construct. It does the NULL test separately after checking for errors.

--
You're more amazing than you think!

Bart

Quoted 40 lines · Bartosz Golaszewski

Quoted 27 lines · Wren Turkal

Quoted 25 lines · Wren Turkal

Quoted 23 lines · Bartosz Golaszewski

Quoted 11 lines · Wren Turkal

Quoted 10 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.

Quoted 6 lines

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

Quoted 8 lines

Quoted 3 lines

I had a feeling that was what was going on. I'll give the patch a shot.

wt

Considering this patch is basically equivalent to patch 1/2 from Zijun, I am not surprised that is works similarly. I.e. on a cold boot, I can disable/re-enable bluetooth as many time as I want.

Zijun didn't bail out on errors which is the issue the original patch tried to address and this one preserves.

Quoted 6 lines

Quoted 3 lines

However, since this patch doesn't include the quirk fix from Zijun's patchset (patch 2/2), bluetooth fails to work after a warm boot.

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Quoted 8 lines

Quoted 6 lines

@Zijun, this patch looks more idiomatic when I look at the surrounding code than your patch 1/2. Notice how it doesn't use the "else if" construct. It does the NULL test separately after checking for errors.

--
You're more amazing than you think!

Bart

--
You're more amazing than you think!

Quoted 49 lines · Wren Turkal

Quoted 46 lines · Bartosz Golaszewski

Quoted 27 lines · Wren Turkal

Quoted 25 lines · Wren Turkal

Quoted 23 lines · Bartosz Golaszewski

Quoted 11 lines · Wren Turkal

Quoted 10 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.

Quoted 6 lines

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

Quoted 8 lines

Quoted 3 lines

I had a feeling that was what was going on. I'll give the patch a shot.

wt

Considering this patch is basically equivalent to patch 1/2 from Zijun, I am not surprised that is works similarly. I.e. on a cold boot, I can disable/re-enable bluetooth as many time as I want.

Zijun didn't bail out on errors which is the issue the original patch tried to address and this one preserves.

Quoted 3 lines

However, since this patch doesn't include the quirk fix from Zijun's patchset (patch 2/2), bluetooth fails to work after a warm boot.

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

Quoted 2 lines

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Bart

Quoted 46 lines · Wren Turkal

Quoted 44 lines · Bartosz Golaszewski

Quoted 27 lines · Wren Turkal

Quoted 25 lines · Wren Turkal

Quoted 23 lines · Bartosz Golaszewski

Quoted 11 lines · Wren Turkal

Quoted 10 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.

Quoted 6 lines

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

Quoted 8 lines

Quoted 3 lines

I had a feeling that was what was going on. I'll give the patch a shot.

wt

Considering this patch is basically equivalent to patch 1/2 from Zijun, I am not surprised that is works similarly. I.e. on a cold boot, I can disable/re-enable bluetooth as many time as I want.

Zijun didn't bail out on errors which is the issue the original patch tried to address and this one preserves.

Quoted 3 lines

However, since this patch doesn't include the quirk fix from Zijun's patchset (patch 2/2), bluetooth fails to work after a warm boot.

Quoted 5 lines

Quoted 2 lines

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Bart

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

@Zijun, this patch looks more idiomatic when I look at the surrounding code than your patch 1/2. Notice how it doesn't use the "else if" construct. It does the NULL test separately after checking for errors.

--
You're more amazing than you think!

Bart

--
You're more amazing than you think!

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Quoted 2 lines

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

To be specific, I have applied your patch and Zijun's 2/2 only.

Quoted 12 lines · Wren Turkal

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

Quoted 2 lines

To be specific, I have applied your patch and Zijun's 2/2 only.

No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes Krzysztof's changes and replaces them with a different if else. This patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let Krzysztof handle it.

Quoted 13 lines · Bartosz Golaszewski

Quoted 12 lines · Wren Turkal

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

Quoted 7 lines

Quoted 2 lines

To be specific, I have applied your patch and Zijun's 2/2 only.

No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes Krzysztof's changes and replaces them with a different if else. This patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let Krzysztof handle it.

Got it. Thx.

BTW, should this patch's commit message include the following?

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

If so, please feel free to add it.

wt
--
You're more amazing than you think!

Quoted 12 lines · Wren Turkal

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

Quoted 2 lines

To be specific, I have applied your patch and Zijun's 2/2 only.

No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes Krzysztof's changes and replaces them with a different if else. This patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let Krzysztof handle it.

Quoted 12 lines · Wren Turkal

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

Quoted 2 lines

To be specific, I have applied your patch and Zijun's 2/2 only.

No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes Krzysztof's changes and replaces them with a different if else. This patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let Krzysztof handle it.

Bart

Quoted 13 lines · Bartosz Golaszewski

Quoted 12 lines · Wren Turkal

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

Quoted 8 lines

Quoted 2 lines

To be specific, I have applied your patch and Zijun's 2/2 only.

No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes Krzysztof's changes and replaces them with a different if else. This patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let Krzysztof handle it.

do you really realize what do you talk about?
do you really listen what do @Wren says?

he says that my patch 2/2 is right based on several verification results.

BTW, my 2/2 fix don't have anything about DTS usage.

he maybe be a DTS expert but not BT from his present fix history for bluetooth system.

Quoted 23 lines · quic_zijuhu

Quoted 14 lines · Bartosz Golaszewski

Quoted 13 lines · Wren Turkal

Quoted 10 lines

Quoted 7 lines

Quoted 4 lines

That's OK, we have the first part right. Let's now see if we can reuse
patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

Quoted 8 lines

Quoted 2 lines

To be specific, I have applied your patch and Zijun's 2/2 only.

No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes Krzysztof's changes and replaces them with a different if else. This patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let Krzysztof handle it.

Quoted 4 lines

do you really realize what do you talk about?
do you really listen what do @Wren says?

he says that my patch 2/2 is right based on several verification results.

she, not he

Quoted 1 line

BTW, my 2/2 fix don't have anything about DTS usage.

I think the problem with your 2/2 patch is that it removes the conditional bailing if the device is shutdown or not open.

Maybe this patch instead?

drivers/bluetooth/hci_qca.c +1 -0

index 2f7ae38d85eb..fcac44ae7898 100644
@@ -2456,6 +2456,10 @@ static void qca_serdev_shutdown(struct device *dev)
                     !test_bit(HCI_RUNNING, &hdev->flags))
                         return;
 
+               if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP,

&hdev->quirks) ||
+                   hci_dev_test_flag(hdev, HCI_SETUP))
+                       return;
+
                serdev_device_write_flush(serdev);
                ret = serdev_device_write_buf(serdev, ibs_wake_cmd, sizeof(ibs_wake_cmd));

Quoted 2 lines

he maybe be a DTS expert but not BT from his present fix history for bluetooth system.

Quoted 55 lines · Wren Turkal

Quoted 23 lines · quic_zijuhu

Quoted 14 lines · Bartosz Golaszewski

Quoted 13 lines · Wren Turkal

Quoted 10 lines

Quoted 7 lines

Quoted 4 lines

That's OK, we have the first part right. Let's now see if we can reuse
patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

Quoted 8 lines

Quoted 2 lines

To be specific, I have applied your patch and Zijun's 2/2 only.

No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes Krzysztof's changes and replaces them with a different if else. This patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let Krzysztof handle it.

Quoted 4 lines

do you really realize what do you talk about?
do you really listen what do @Wren says?

he says that my patch 2/2 is right based on several verification results.

she, not he

Quoted 1 line

BTW, my 2/2 fix don't have anything about DTS usage.

I think the problem with your 2/2 patch is that it removes the conditional bailing if the device is shutdown or not open.

Maybe this patch instead?

drivers/bluetooth/hci_qca.c +1 -0

index 2f7ae38d85eb..fcac44ae7898 100644
@@ -2456,6 +2456,10 @@ static void qca_serdev_shutdown(struct device *dev)
                     !test_bit(HCI_RUNNING, &hdev->flags))
                         return;
 
+               if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP,

&hdev->quirks) ||
+                   hci_dev_test_flag(hdev, HCI_SETUP))
+                       return;
+
                serdev_device_write_flush(serdev);
                ret = serdev_device_write_buf(serdev, ibs_wake_cmd, sizeof(ibs_wake_cmd));

Quoted 4 lines

Quoted 2 lines

he maybe be a DTS expert but not BT from his present fix history for bluetooth system.

Hi Wren,
i think i don't need to care about why wrong condition cause wrong results.

Quoted 22 lines · quic_zijuhu

Quoted 13 lines · Bartosz Golaszewski

Quoted 12 lines · Wren Turkal

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

Quoted 8 lines

Quoted 2 lines

To be specific, I have applied your patch and Zijun's 2/2 only.

No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes Krzysztof's changes and replaces them with a different if else. This patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let Krzysztof handle it.

Quoted 4 lines

do you really realize what do you talk about?
do you really listen what do @Wren says?

he says that my patch 2/2 is right based on several verification results.

she, not he

Quoted 1 line

BTW, my 2/2 fix don't have anything about DTS usage.

I think the problem with your 2/2 patch is that it removes the conditional bailing if the device is shutdown or not open.

Maybe this patch instead?

drivers/bluetooth/hci_qca.c +1 -0

index 2f7ae38d85eb..fcac44ae7898 100644
@@ -2456,6 +2456,10 @@ static void qca_serdev_shutdown(struct device *dev)
                     !test_bit(HCI_RUNNING, &hdev->flags))
                         return;
 
+               if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP,

&hdev->quirks) ||

drivers/bluetooth/hci_qca.c +3 -0

+                   hci_dev_test_flag(hdev, HCI_SETUP))
+                       return;
+
                 serdev_device_write_flush(serdev);
                 ret = serdev_device_write_buf(serdev, ibs_wake_cmd,
                                               sizeof(ibs_wake_cmd));

Quoted 2 lines

he maybe be a DTS expert but not BT from his present fix history for bluetooth system.

Quoted 55 lines · Wren Turkal

Quoted 22 lines · quic_zijuhu

Quoted 13 lines · Bartosz Golaszewski

Quoted 12 lines · Wren Turkal

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

That's OK, we have the first part right. Let's now see if we can reuse patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

Quoted 8 lines

Quoted 2 lines

To be specific, I have applied your patch and Zijun's 2/2 only.

No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes Krzysztof's changes and replaces them with a different if else. This patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let Krzysztof handle it.

Quoted 4 lines

do you really realize what do you talk about?
do you really listen what do @Wren says?

he says that my patch 2/2 is right based on several verification results.

she, not he

Quoted 1 line

BTW, my 2/2 fix don't have anything about DTS usage.

I think the problem with your 2/2 patch is that it removes the conditional bailing if the device is shutdown or not open.

Maybe this patch instead?

drivers/bluetooth/hci_qca.c +1 -0

index 2f7ae38d85eb..fcac44ae7898 100644
@@ -2456,6 +2456,10 @@ static void qca_serdev_shutdown(struct device *dev)
                      !test_bit(HCI_RUNNING, &hdev->flags))
                          return;
 
+               if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP,

&hdev->quirks) ||

drivers/bluetooth/hci_qca.c +3 -0

+                   hci_dev_test_flag(hdev, HCI_SETUP))
+                       return;
+
                  serdev_device_write_flush(serdev);
                  ret = serdev_device_write_buf(serdev, ibs_wake_cmd,
                                                sizeof(ibs_wake_cmd));

Quoted 4 lines

Quoted 2 lines

he maybe be a DTS expert but not BT from his present fix history for bluetooth system.

Did you test it? Does it work? If so, please consider sending it upstream for review.

You can keep Zijun's authorship but add your own SoB tag at the end and mention what you did. Something like this:

[Wren: kept Krzysztof's fix]

Signed-off-by Wren...

Bartosz

Quoted 65 lines · Bartosz Golaszewski

Quoted 63 lines · Wren Turkal

Quoted 24 lines · quic_zijuhu

Quoted 15 lines · Bartosz Golaszewski

Quoted 13 lines · Wren Turkal

Quoted 10 lines

Quoted 7 lines

Quoted 4 lines

That's OK, we have the first part right. Let's now see if we can reuse
patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

Quoted 8 lines

Quoted 2 lines

To be specific, I have applied your patch and Zijun's 2/2 only.

No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes Krzysztof's changes and replaces them with a different if else. This patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let Krzysztof handle it.

Quoted 5 lines

do you really realize what do you talk about?
do you really listen what do @Wren says?

he says that my patch 2/2 is right based on several verification results.

she, not he

Quoted 1 line

BTW, my 2/2 fix don't have anything about DTS usage.

I think the problem with your 2/2 patch is that it removes the conditional bailing if the device is shutdown or not open.

Maybe this patch instead?

drivers/bluetooth/hci_qca.c +1 -0

index 2f7ae38d85eb..fcac44ae7898 100644
@@ -2456,6 +2456,10 @@ static void qca_serdev_shutdown(struct device
 *dev)
                       !test_bit(HCI_RUNNING, &hdev->flags))
                           return;
 
+               if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP,

&hdev->quirks) ||
+                   hci_dev_test_flag(hdev, HCI_SETUP))
+                       return;
+
                  serdev_device_write_flush(serdev);
                  ret = serdev_device_write_buf(serdev, ibs_wake_cmd, sizeof(ibs_wake_cmd));

Quoted 2 lines

he maybe be a DTS expert but not BT from his present fix history for bluetooth system.

Quoted 6 lines

Did you test it? Does it work? If so, please consider sending it upstream for review.

You can keep Zijun's authorship but add your own SoB tag at the end and mention what you did. Something like this:

Quoted 4 lines

[Wren: kept Krzysztof's fix]

Signed-off-by Wren...

Bartosz

@Bartosz, I have tested this, and it works functionally for my setup. I cannot detect a difference between this and Zijun's logic when I compile a kernel with this patch.

@Zijun, I think you have objections to this patch. I would like to make sure I hear your concern. Can you please take through it like I'm a 5 year old who barely knows C? In retrospect, I guess that I would be a pretty precocious 5 year old. LOL.

In all seriousness, @Zijun, I really appreciate the work you did on this. I would like to understand why you assert that removing the logic of Krzysztof is appropriate. Again, I am not a kernel developer, so this stuff is really outside my wheelhouse. Having said that, by my reading, which may very well be worng, it seems like you are just adding another case that is orthogonal to K's conditions. I'd like to truly understand you position to know if the patch I am suggesting is somehow harmful. This is an earnest question. I really want to respect your expertise here, and I really want you to know how much I appreciate your work. you maybe see all replies of [2/2] patch for this issue within below

wt

Quoted 66 lines · Wren Turkal

Quoted 65 lines · Bartosz Golaszewski

Quoted 63 lines · Wren Turkal

Quoted 24 lines · quic_zijuhu

Quoted 15 lines · Bartosz Golaszewski

Quoted 13 lines · Wren Turkal

Quoted 10 lines

Quoted 7 lines

Quoted 4 lines

That's OK, we have the first part right. Let's now see if we can reuse
patch 2/2 from Zijun.

I'm compiling it right now. Be back soon.

Well I doubt it's correct as it removed Krzysztof's fix which looks right. If I were to guess I'd say we need some mix of both.

Patch 2/2 remove K's fix? I thought only 1/2 did that.

Quoted 8 lines

Quoted 2 lines

To be specific, I have applied your patch and Zijun's 2/2 only.

No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes Krzysztof's changes and replaces them with a different if else. This patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let Krzysztof handle it.

Quoted 5 lines

do you really realize what do you talk about?
do you really listen what do @Wren says?

he says that my patch 2/2 is right based on several verification results.

she, not he

Quoted 1 line

BTW, my 2/2 fix don't have anything about DTS usage.

I think the problem with your 2/2 patch is that it removes the conditional bailing if the device is shutdown or not open.

Maybe this patch instead?

drivers/bluetooth/hci_qca.c +1 -0

index 2f7ae38d85eb..fcac44ae7898 100644
@@ -2456,6 +2456,10 @@ static void qca_serdev_shutdown(struct device
 *dev)
                       !test_bit(HCI_RUNNING, &hdev->flags))
                           return;
 
+               if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP,

&hdev->quirks) ||
+                   hci_dev_test_flag(hdev, HCI_SETUP))
+                       return;
+
                  serdev_device_write_flush(serdev);
                  ret = serdev_device_write_buf(serdev, ibs_wake_cmd, sizeof(ibs_wake_cmd));

Quoted 2 lines

he maybe be a DTS expert but not BT from his present fix history for bluetooth system.

Quoted 6 lines

Quoted 6 lines

Did you test it? Does it work? If so, please consider sending it upstream for review.

You can keep Zijun's authorship but add your own SoB tag at the end and mention what you did. Something like this:

[V7 2/2] as shown by below link is the formal fix.
https://patchwork.kernel.org/project/bluetooth/patch/[email protected]/

this fix logic was introduced from the very beginning when i saw your issue description as shown by below link
https://lore.kernel.org/all/[email protected]/#t

Quoted 14 lines

Quoted 4 lines

[Wren: kept Krzysztof's fix]

Signed-off-by Wren...

Bartosz

@Bartosz, I have tested this, and it works functionally for my setup. I cannot detect a difference between this and Zijun's logic when I compile a kernel with this patch.

@Zijun, I think you have objections to this patch. I would like to make sure I hear your concern. Can you please take through it like I'm a 5 year old who barely knows C? In retrospect, I guess that I would be a pretty precocious 5 year old. LOL.

Quoted 10 lines

In all seriousness, @Zijun, I really appreciate the work you did on this. I would like to understand why you assert that removing the logic of Krzysztof is appropriate. Again, I am not a kernel developer, so this stuff is really outside my wheelhouse. Having said that, by my reading, which may very well be worng, it seems like you are just adding another case that is orthogonal to K's conditions. I'd like to truly understand you position to know if the patch I am suggesting is somehow harmful. This is an earnest question. I really want to respect your expertise here, and I really want you to know how much I appreciate your work. you maybe see all replies of [2/2] patch for this issue within below

link. i believe you will understand it. the bottom of the link includes all reply history.
https://lore.kernel.org/all/[email protected]/

Quoted 12 lines · Wren Turkal

Quoted 8 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.

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Quoted 3 lines

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

Quoted 16 lines · Bartosz Golaszewski

Quoted 15 lines · Wren Turkal

Quoted 8 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.

Nack. This patch does fixes neither the disable/re-enable problem nor the warm boot problem.

Zijun replied to this patch also with what I think is the proper reasoning for why it doesn't fix my setup.

Quoted 4 lines

Indeed, I only addressed a single issue here and not the code under the
default: label of the switch case. Sorry.

Could you give the following diff a try?

I am compiling a kernel the patch right now, but I did want to let you know that the patch got corrupted by extra line wrapping. I'm not sure how you're sending it, but I thought you might want to know.

This must be your email client wrapping lines over a certain limit. Try and get the diff from lore[1], it should be fine.

Bart

[1]https://lore.kernel.org/lkml/CAMRc=MepDwUbAKrWgm0CXKObqy8=igtug0QDgo-CgwxjZCAC2Q@mail.gmail.com/

Quoted 5 lines · Bartosz Golaszewski

This must be your email client wrapping lines over a certain limit. Try and get the diff from lore[1], it should be fine.

Bart

Quoted 1 line

[1]https://lore.kernel.org/lkml/CAMRc=MepDwUbAKrWgm0CXKObqy8=igtug0QDgo-CgwxjZCAC2Q@mail.gmail.com/

I don't think it's my client. The extra newlines are right there in the lore link.

Look at the line that starts with "@@". That line is wrapped. The following line ("serdev_device *serdev)") should be at the end of the previous line. The same thing happened on the second "@@" line as well.

Quoted 7 lines · Wren Turkal

Quoted 5 lines · Bartosz Golaszewski

This must be your email client wrapping lines over a certain limit. Try and get the diff from lore[1], it should be fine.

Bart

Quoted 9 lines

Quoted 1 line

[1]https://lore.kernel.org/lkml/CAMRc=MepDwUbAKrWgm0CXKObqy8=igtug0QDgo-CgwxjZCAC2Q@mail.gmail.com/

I don't think it's my client. The extra newlines are right there in the lore link.

Look at the line that starts with "@@". That line is wrapped. The following line ("serdev_device *serdev)") should be at the end of the previous line. The same thing happened on the second "@@" line as well.

Indeed. I just noticed that it applies fine with git apply and figured the output must be right. Anyway, this is not a proper patch, I will send one once I adapt Zijun's code.

Bart