Bluetooth: hci_qca: Enable the ldisc for ROME for x86 platforms.

When using btattach to setup Rome over ldisc we observed a crash in qca_setup as it will try to access the serdev which is not available in the ldisc proto. This patch will fix the crash by support both the ldisc and serdev way in the qca hci_uart driver.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
 drivers/bluetooth/hci_qca.c | 47 ++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 19 deletions(-)

drivers/bluetooth/hci_qca.c +0 -0

index 237aea34b69f..0a5c98d46864 100644

Hi Balakrishna,

Quoted 6 lines · Balakrishna Godavarthi

When using btattach to setup Rome over ldisc we observed a crash in qca_setup as it will try to access the serdev which is not available in the ldisc proto. This patch will fix the crash by support both the ldisc and serdev way in the qca hci_uart driver.

Signed-off-by Balakrishna Godavarthi <[email protected]>

Oh, I wasn't aware of the instantiation through ldisc and was actually considering to *remove* some of the seemingly unnecessary serdev checks.

Quoted 5 lines

---
 drivers/bluetooth/hci_qca.c | 47 
++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 19 deletions(-)

drivers/bluetooth/hci_qca.c static int qca_set_baudrate(struct hci_dev *hdev, · line 967

Quoted 11 lines

drivers/bluetooth/hci_qca.c +1 -1

index 237aea34b69f..0a5c98d46864 100644
@@ -963,7 +963,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
 uint8_t baudrate)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
 	struct qca_data *qca = hu->priv;
-	struct qca_serdev *qcadev;
+	struct qca_serdev *qcadev = NULL;

In many cases the only field that is accessed is qcadev->btsoc_type. I think something like 'qca_get_soc_type(struct hci_dev *hdev / struct hci_uart *hu)' would make things more readable.

IMO the whole 'qcadev' vs 'qca(_data)' is confusing anyway, in this sense even better if we can make most of the 'qcadev' references disappear.

Thanks

Matthias

Hi Matthias,

Quoted 19 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 6 lines · Balakrishna Godavarthi

When using btattach to setup Rome over ldisc we observed a crash in qca_setup as it will try to access the serdev which is not available in the ldisc proto. This patch will fix the crash by support both the ldisc and serdev way in the qca hci_uart driver.

Signed-off-by Balakrishna Godavarthi <[email protected]>

Oh, I wasn't aware of the instantiation through ldisc and was actually considering to *remove* some of the seemingly unnecessary serdev checks.

Quoted 5 lines

---
 drivers/bluetooth/hci_qca.c | 47
++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 19 deletions(-)

Quoted 16 lines

drivers/bluetooth/hci_qca.c static int qca_set_baudrate(struct hci_dev · line 967

Quoted 11 lines

drivers/bluetooth/hci_qca.c +1 -1

index 237aea34b69f..0a5c98d46864 100644
@@ -963,7 +963,7 @@ static int qca_set_baudrate(struct hci_dev
 *hdev, uint8_t baudrate)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
 	struct qca_data *qca = hu->priv;
-	struct qca_serdev *qcadev;
+	struct qca_serdev *qcadev = NULL;

In many cases the only field that is accessed is qcadev->btsoc_type. I think something like 'qca_get_soc_type(struct hci_dev *hdev / struct hci_uart *hu)' would make things more readable.

[Bala]: sure will update this in other patch once this change is landed as this has to
go in priority as we have crash coming.

Quoted 38 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 19 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 6 lines · Balakrishna Godavarthi

When using btattach to setup Rome over ldisc we observed a crash in qca_setup as it will try to access the serdev which is not available in the ldisc proto. This patch will fix the crash by support both the ldisc and serdev way in the qca hci_uart driver.

Signed-off-by Balakrishna Godavarthi <[email protected]>

Oh, I wasn't aware of the instantiation through ldisc and was actually considering to *remove* some of the seemingly unnecessary serdev checks.

Quoted 5 lines

---
 drivers/bluetooth/hci_qca.c | 47
++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 19 deletions(-)

Quoted 16 lines

drivers/bluetooth/hci_qca.c static int qca_set_baudrate(struct hci_dev · line 967

Quoted 11 lines

drivers/bluetooth/hci_qca.c +1 -1

index 237aea34b69f..0a5c98d46864 100644
@@ -963,7 +963,7 @@ static int qca_set_baudrate(struct hci_dev
 *hdev, uint8_t baudrate)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
 	struct qca_data *qca = hu->priv;
-	struct qca_serdev *qcadev;
+	struct qca_serdev *qcadev = NULL;

In many cases the only field that is accessed is qcadev->btsoc_type. I think something like 'qca_get_soc_type(struct hci_dev *hdev / struct hci_uart *hu)' would make things more readable.

Quoted 3 lines

[Bala]: sure will update this in other patch once this change is landed as this has to
go in priority as we have crash coming.

That's not how things should work, especially for fairly trivial changes. It requires reviewers to first spent time to review the patch that adds clutter and later spend more time to review the one that removes it. It's also easier to get a clean patch merged in the first place, rather than a noisy one.

Anyway, here is my take at it: https://lore.kernel.org/patchwork/patch/1049014/

Please help with testing for ROME, unless you disagree with the approach.

Thanks

Matthias

Quoted 58 lines · 在 2019-03-09 02:52，Matthias Kaehlcke 写道：

Quoted 38 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 19 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 6 lines · Balakrishna Godavarthi

When using btattach to setup Rome over ldisc we observed a crash in qca_setup as it will try to access the serdev which is not available in the ldisc proto. This patch will fix the crash by support both the ldisc and serdev way in the qca hci_uart driver.

Signed-off-by Balakrishna Godavarthi <[email protected]>

Oh, I wasn't aware of the instantiation through ldisc and was actually considering to *remove* some of the seemingly unnecessary serdev checks.

Quoted 5 lines

---
 drivers/bluetooth/hci_qca.c | 47
++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 19 deletions(-)

Quoted 16 lines

drivers/bluetooth/hci_qca.c static int qca_set_baudrate(struct hci_dev · line 967

Quoted 11 lines

drivers/bluetooth/hci_qca.c +1 -1

index 237aea34b69f..0a5c98d46864 100644
@@ -963,7 +963,7 @@ static int qca_set_baudrate(struct hci_dev
 *hdev, uint8_t baudrate)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
 	struct qca_data *qca = hu->priv;
-	struct qca_serdev *qcadev;
+	struct qca_serdev *qcadev = NULL;

In many cases the only field that is accessed is qcadev->btsoc_type. I think something like 'qca_get_soc_type(struct hci_dev *hdev / struct hci_uart *hu)' would make things more readable.

Quoted 4 lines

[Bala]: sure will update this in other patch once this change is landed as
this has to
go in priority as we have crash coming.

That's not how things should work, especially for fairly trivial changes. It requires reviewers to first spent time to review the patch that adds clutter and later spend more time to review the one that removes it. It's also easier to get a clean patch merged in the first place, rather than a noisy one.

Anyway, here is my take at it:
https://lore.kernel.org/patchwork/patch/1049014/

Please help with testing for ROME, unless you disagree with the approach.

Thanks

Quoted 1 line

Matthias

Hi Matthias,

I will test your patch and update to you, and you are correct that AR3002 is not part of Rome family, you should use QCA_ROME as the default return of qca_soc_type.

Could you also loop me in
https://lore.kernel.org/patchwork/patch/1049014/?

Hi Rocky,

Quoted 59 lines · On Tue, Mar 12, 2019 at 05:01:59PM +0800, [email protected] wrote:

Quoted 58 lines · 在 2019-03-09 02:52，Matthias Kaehlcke 写道：

Quoted 38 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 19 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 6 lines · Balakrishna Godavarthi

When using btattach to setup Rome over ldisc we observed a crash in qca_setup as it will try to access the serdev which is not available in the ldisc proto. This patch will fix the crash by support both the ldisc and serdev way in the qca hci_uart driver.

Signed-off-by Balakrishna Godavarthi <[email protected]>

Oh, I wasn't aware of the instantiation through ldisc and was actually considering to *remove* some of the seemingly unnecessary serdev checks.

Quoted 5 lines

---
 drivers/bluetooth/hci_qca.c | 47
++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 19 deletions(-)

Quoted 16 lines

drivers/bluetooth/hci_qca.c static int qca_set_baudrate(struct hci_dev · line 967

Quoted 11 lines

drivers/bluetooth/hci_qca.c +1 -1

index 237aea34b69f..0a5c98d46864 100644
@@ -963,7 +963,7 @@ static int qca_set_baudrate(struct hci_dev
 *hdev, uint8_t baudrate)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
 	struct qca_data *qca = hu->priv;
-	struct qca_serdev *qcadev;
+	struct qca_serdev *qcadev = NULL;

In many cases the only field that is accessed is qcadev->btsoc_type. I think something like 'qca_get_soc_type(struct hci_dev *hdev / struct hci_uart *hu)' would make things more readable.

Quoted 4 lines

[Bala]: sure will update this in other patch once this change is landed as
this has to
go in priority as we have crash coming.

That's not how things should work, especially for fairly trivial changes. It requires reviewers to first spent time to review the patch that adds clutter and later spend more time to review the one that removes it. It's also easier to get a clean patch merged in the first place, rather than a noisy one.

Anyway, here is my take at it:
https://lore.kernel.org/patchwork/patch/1049014/

Please help with testing for ROME, unless you disagree with the approach.

Thanks

Quoted 7 lines

Quoted 1 line

Matthias

Hi Matthias,

I will test your patch and update to you, and you are correct that AR3002 is not part of Rome family, you should use QCA_ROME as the default return of qca_soc_type.

Thanks for the confirmation!

Quoted 2 lines

Could you also loop me in
https://lore.kernel.org/patchwork/patch/1049014/?

This patch has been superseded by a newer version:

https://lore.kernel.org/patchwork/patch/1049696/

It already landed in bluetooth-next.

Testing with Rome and ldisc would still be appreciated, since I don't have such a configuration.

Thanks

Matthias

      if (hu->serdev)
              serdev_device_wait_until_sent(hu->serdev,
                              msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));

Quoted 81 lines · 在 2019-03-12 23:52，Matthias Kaehlcke 写道：

Hi Rocky,

Quoted 61 lines · On Tue, Mar 12, 2019 at 05:01:59PM +0800, [email protected] wrote:

Quoted 53 lines · 在 2019-03-09 02:52，Matthias Kaehlcke 写道：

Quoted 38 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 19 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 6 lines · Balakrishna Godavarthi

When using btattach to setup Rome over ldisc we observed a crash in qca_setup as it will try to access the serdev which is not available in the ldisc proto. This patch will fix the crash by support both the ldisc and serdev way in the qca hci_uart driver.

Signed-off-by Balakrishna Godavarthi <[email protected]>

Oh, I wasn't aware of the instantiation through ldisc and was actually considering to *remove* some of the seemingly unnecessary serdev checks.

Quoted 5 lines

---
 drivers/bluetooth/hci_qca.c | 47
++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 19 deletions(-)

Quoted 16 lines

drivers/bluetooth/hci_qca.c static int qca_set_baudrate(struct hci_dev · line 967

Quoted 11 lines

drivers/bluetooth/hci_qca.c +1 -1

index 237aea34b69f..0a5c98d46864 100644
@@ -963,7 +963,7 @@ static int qca_set_baudrate(struct hci_dev
 *hdev, uint8_t baudrate)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
 	struct qca_data *qca = hu->priv;
-	struct qca_serdev *qcadev;
+	struct qca_serdev *qcadev = NULL;

In many cases the only field that is accessed is qcadev->btsoc_type. I think something like 'qca_get_soc_type(struct hci_dev *hdev / struct hci_uart *hu)' would make things more readable.

Quoted 4 lines

[Bala]: sure will update this in other patch once this change is landed as
this has to
go in priority as we have crash coming.

That's not how things should work, especially for fairly trivial changes. It requires reviewers to first spent time to review the patch that adds clutter and later spend more time to review the one that removes it. It's also easier to get a clean patch merged in the first place, rather than a noisy one.

Anyway, here is my take at it:
https://lore.kernel.org/patchwork/patch/1049014/

Quoted 6 lines

Please help with testing for ROME, unless you disagree with the approach.

Thanks

Matthias

Quoted 7 lines

Hi Matthias,

I will test your patch and update to you, and you are correct that AR3002 is
not part of Rome family, you should use QCA_ROME as the default return of
qca_soc_type.

Thanks for the confirmation!

Quoted 2 lines

Could you also loop me in
https://lore.kernel.org/patchwork/patch/1049014/?

This patch has been superseded by a newer version:

https://lore.kernel.org/patchwork/patch/1049696/

Quoted 8 lines

It already landed in bluetooth-next.

Testing with Rome and ldisc would still be appreciated, since I don't have such a configuration.

Thanks

Matthias

Hi Matthias,

I verified your change and found there is another deference to serdev in the qca_set_baudrate() function while running the ldisc proto, it will cause a crash and need to add a check as below. Could you help to add this change?

< serdev_device_wait_until_sent(hu->serdev,
< msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
---

Quoted 3 lines

      if (hu->serdev)
              serdev_device_wait_until_sent(hu->serdev,
                              msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));