[PATCH v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86 platforms.

[PATCH v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86 platforms.

[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 <bgodavar@codeaurora.org>
---
 drivers/bluetooth/hci_qca.c | 47 ++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 237aea34b69f..0a5c98d46864 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -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;
 	struct sk_buff *skb;
 	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
 
@@ -985,18 +985,19 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 	skb_queue_tail(&qca->txq, skb);
 	hci_uart_tx_wakeup(hu);
 
-	qcadev = serdev_device_get_drvdata(hu->serdev);
+	if (hu->serdev)
+		qcadev = serdev_device_get_drvdata(hu->serdev);
 
 	/* Wait for the baudrate change request to be sent */
-
 	while (!skb_queue_empty(&qca->txq))
 		usleep_range(100, 200);
 
-	serdev_device_wait_until_sent(hu->serdev,
-		      msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
+	if (hu->serdev)
+		serdev_device_wait_until_sent(hu->serdev,
+				msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
 
 	/* Give the controller time to process the request */
-	if (qcadev->btsoc_type == QCA_WCN3990)
+	if (qcadev && qcadev->btsoc_type == QCA_WCN3990)
 		msleep(10);
 	else
 		msleep(300);
@@ -1072,10 +1073,12 @@ static unsigned int qca_get_speed(struct hci_uart *hu,
 
 static int qca_check_speeds(struct hci_uart *hu)
 {
-	struct qca_serdev *qcadev;
+	struct qca_serdev *qcadev = NULL;
 
-	qcadev = serdev_device_get_drvdata(hu->serdev);
-	if (qcadev->btsoc_type == QCA_WCN3990) {
+	if (hu->serdev)
+		qcadev = serdev_device_get_drvdata(hu->serdev);
+
+	if (qcadev && qcadev->btsoc_type == QCA_WCN3990) {
 		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
 		    !qca_get_speed(hu, QCA_OPER_SPEED))
 			return -EINVAL;
@@ -1091,7 +1094,7 @@ static int qca_check_speeds(struct hci_uart *hu)
 static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 {
 	unsigned int speed, qca_baudrate;
-	struct qca_serdev *qcadev;
+	struct qca_serdev *qcadev = NULL;
 	int ret = 0;
 
 	if (speed_type == QCA_INIT_SPEED) {
@@ -1106,8 +1109,10 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 		/* Disable flow control for wcn3990 to deassert RTS while
 		 * changing the baudrate of chip and host.
 		 */
-		qcadev = serdev_device_get_drvdata(hu->serdev);
-		if (qcadev->btsoc_type == QCA_WCN3990)
+		if (hu->serdev)
+			qcadev = serdev_device_get_drvdata(hu->serdev);
+
+		if (qcadev && qcadev->btsoc_type == QCA_WCN3990)
 			hci_uart_set_flow_control(hu, true);
 
 		qca_baudrate = qca_get_baudrate_value(speed);
@@ -1119,7 +1124,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 		host_set_baudrate(hu, speed);
 
 error:
-		if (qcadev->btsoc_type == QCA_WCN3990)
+		if (qcadev && qcadev->btsoc_type == QCA_WCN3990)
 			hci_uart_set_flow_control(hu, false);
 	}
 
@@ -1181,11 +1186,15 @@ static int qca_setup(struct hci_uart *hu)
 	struct hci_dev *hdev = hu->hdev;
 	struct qca_data *qca = hu->priv;
 	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
-	struct qca_serdev *qcadev;
+	struct qca_serdev *qcadev = NULL;
 	int ret;
 	int soc_ver = 0;
+	enum qca_btsoc_type btsoc_type = QCA_ROME;
 
-	qcadev = serdev_device_get_drvdata(hu->serdev);
+	if (hu->serdev) {
+		qcadev = serdev_device_get_drvdata(hu->serdev);
+		btsoc_type = qcadev->btsoc_type;
+	}
 
 	ret = qca_check_speeds(hu);
 	if (ret)
@@ -1194,7 +1203,7 @@ static int qca_setup(struct hci_uart *hu)
 	/* Patch downloading has to be done without IBS mode */
 	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
 
-	if (qcadev->btsoc_type == QCA_WCN3990) {
+	if (qcadev && btsoc_type == QCA_WCN3990) {
 		bt_dev_info(hdev, "setting up wcn3990");
 
 		/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to execute
@@ -1225,7 +1234,7 @@ static int qca_setup(struct hci_uart *hu)
 		qca_baudrate = qca_get_baudrate_value(speed);
 	}
 
-	if (qcadev->btsoc_type != QCA_WCN3990) {
+	if (btsoc_type != QCA_WCN3990) {
 		/* Get QCA version information */
 		ret = qca_read_soc_version(hdev, &soc_ver);
 		if (ret)
@@ -1234,7 +1243,7 @@ static int qca_setup(struct hci_uart *hu)
 
 	bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver);
 	/* Setup patch / NVM configurations */
-	ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, soc_ver);
+	ret = qca_uart_setup(hdev, qca_baudrate, btsoc_type, soc_ver);
 	if (!ret) {
 		set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
 		qca_debugfs_init(hdev);
@@ -1250,7 +1259,7 @@ static int qca_setup(struct hci_uart *hu)
 	}
 
 	/* Setup bdaddr */
-	if (qcadev->btsoc_type == QCA_WCN3990)
+	if (btsoc_type == QCA_WCN3990)
 		hu->hdev->set_bdaddr = qca_set_bdaddr;
 	else
 		hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86 platforms.

[Matthias Kaehlcke]

Hi Balakrishna,

On Thu, Mar 07, 2019 at 03:47:22PM +0530, Balakrishna Godavarthi wrote:
> 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 <bgodavar@codeaurora.org>

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

> ---
>  drivers/bluetooth/hci_qca.c | 47 ++++++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 237aea34b69f..0a5c98d46864 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -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

Re: [PATCH v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86 platforms.

[Balakrishna Godavarthi]

Hi Matthias,

On 2019-03-08 02:12, Matthias Kaehlcke wrote:
> Hi Balakrishna,
> 
> On Thu, Mar 07, 2019 at 03:47:22PM +0530, Balakrishna Godavarthi wrote:
>> 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 <bgodavar@codeaurora.org>
> 
> Oh, I wasn't aware of the instantiation through ldisc and was actually
> considering to *remove* some of the seemingly unnecessary serdev
> checks.
> 
>> ---
>>  drivers/bluetooth/hci_qca.c | 47 
>> ++++++++++++++++++++++---------------
>>  1 file changed, 28 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 237aea34b69f..0a5c98d46864 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -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.

> 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.
> 
[Bala]: will note this improvement point and do it with the above 
change.

> Thanks
> 
> Matthias

-- 
Regards
Balakrishna.

Re: [PATCH v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86 platforms.

[Matthias Kaehlcke]

On Fri, Mar 08, 2019 at 10:43:14AM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2019-03-08 02:12, Matthias Kaehlcke wrote:
> > Hi Balakrishna,
> > 
> > On Thu, Mar 07, 2019 at 03:47:22PM +0530, Balakrishna Godavarthi wrote:
> > > 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 <bgodavar@codeaurora.org>
> > 
> > Oh, I wasn't aware of the instantiation through ldisc and was actually
> > considering to *remove* some of the seemingly unnecessary serdev
> > checks.
> > 
> > > ---
> > >  drivers/bluetooth/hci_qca.c | 47
> > > ++++++++++++++++++++++---------------
> > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index 237aea34b69f..0a5c98d46864 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > @@ -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.

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

Re: [PATCH v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86 platforms.

[rjliao]

在 2019-03-09 02:52，Matthias Kaehlcke 写道：
> On Fri, Mar 08, 2019 at 10:43:14AM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2019-03-08 02:12, Matthias Kaehlcke wrote:
>> > Hi Balakrishna,
>> >
>> > On Thu, Mar 07, 2019 at 03:47:22PM +0530, Balakrishna Godavarthi wrote:
>> > > 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 <bgodavar@codeaurora.org>
>> >
>> > Oh, I wasn't aware of the instantiation through ldisc and was actually
>> > considering to *remove* some of the seemingly unnecessary serdev
>> > checks.
>> >
>> > > ---
>> > >  drivers/bluetooth/hci_qca.c | 47
>> > > ++++++++++++++++++++++---------------
>> > >  1 file changed, 28 insertions(+), 19 deletions(-)
>> > >
>> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > index 237aea34b69f..0a5c98d46864 100644
>> > > --- a/drivers/bluetooth/hci_qca.c
>> > > +++ b/drivers/bluetooth/hci_qca.c
>> > > @@ -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.
> 
> 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

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/?

Thanks,
Rocky

Re: [PATCH v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86 platforms.

[Matthias Kaehlcke]

Hi Rocky,

On Tue, Mar 12, 2019 at 05:01:59PM +0800, rjliao@codeaurora.org wrote:
> 在 2019-03-09 02:52，Matthias Kaehlcke 写道：
> > On Fri, Mar 08, 2019 at 10:43:14AM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2019-03-08 02:12, Matthias Kaehlcke wrote:
> > > > Hi Balakrishna,
> > > >
> > > > On Thu, Mar 07, 2019 at 03:47:22PM +0530, Balakrishna Godavarthi wrote:
> > > > > 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 <bgodavar@codeaurora.org>
> > > >
> > > > Oh, I wasn't aware of the instantiation through ldisc and was actually
> > > > considering to *remove* some of the seemingly unnecessary serdev
> > > > checks.
> > > >
> > > > > ---
> > > > >  drivers/bluetooth/hci_qca.c | 47
> > > > > ++++++++++++++++++++++---------------
> > > > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > > > index 237aea34b69f..0a5c98d46864 100644
> > > > > --- a/drivers/bluetooth/hci_qca.c
> > > > > +++ b/drivers/bluetooth/hci_qca.c
> > > > > @@ -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.
> > 
> > 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
> 
> 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!

> 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

Re: [PATCH v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86 platforms.

[rjliao]

在 2019-03-12 23:52，Matthias Kaehlcke 写道：
> Hi Rocky,
> 
> On Tue, Mar 12, 2019 at 05:01:59PM +0800, rjliao@codeaurora.org wrote:
>> 在 2019-03-09 02:52，Matthias Kaehlcke 写道：
>> > On Fri, Mar 08, 2019 at 10:43:14AM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Matthias,
>> > >
>> > > On 2019-03-08 02:12, Matthias Kaehlcke wrote:
>> > > > Hi Balakrishna,
>> > > >
>> > > > On Thu, Mar 07, 2019 at 03:47:22PM +0530, Balakrishna Godavarthi wrote:
>> > > > > 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 <bgodavar@codeaurora.org>
>> > > >
>> > > > Oh, I wasn't aware of the instantiation through ldisc and was actually
>> > > > considering to *remove* some of the seemingly unnecessary serdev
>> > > > checks.
>> > > >
>> > > > > ---
>> > > > >  drivers/bluetooth/hci_qca.c | 47
>> > > > > ++++++++++++++++++++++---------------
>> > > > >  1 file changed, 28 insertions(+), 19 deletions(-)
>> > > > >
>> > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > > > index 237aea34b69f..0a5c98d46864 100644
>> > > > > --- a/drivers/bluetooth/hci_qca.c
>> > > > > +++ b/drivers/bluetooth/hci_qca.c
>> > > > > @@ -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.
>> >
>> > 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
>> 
>> 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!
> 
>> 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

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));
---
>       if (hu->serdev)
>               serdev_device_wait_until_sent(hu->serdev,
>                               msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));

Re: [PATCH v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86 platforms.

[Matthias Kaehlcke]

On Wed, Mar 13, 2019 at 05:43:14PM +0800, rjliao@codeaurora.org wrote:
> 在 2019-03-12 23:52，Matthias Kaehlcke 写道：
> > Hi Rocky,
> > 
> > On Tue, Mar 12, 2019 at 05:01:59PM +0800, rjliao@codeaurora.org wrote:
> > > 在 2019-03-09 02:52，Matthias Kaehlcke 写道：
> > > > On Fri, Mar 08, 2019 at 10:43:14AM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Matthias,
> > > > >
> > > > > On 2019-03-08 02:12, Matthias Kaehlcke wrote:
> > > > > > Hi Balakrishna,
> > > > > >
> > > > > > On Thu, Mar 07, 2019 at 03:47:22PM +0530, Balakrishna Godavarthi wrote:
> > > > > > > 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 <bgodavar@codeaurora.org>
> > > > > >
> > > > > > Oh, I wasn't aware of the instantiation through ldisc and was actually
> > > > > > considering to *remove* some of the seemingly unnecessary serdev
> > > > > > checks.
> > > > > >
> > > > > > > ---
> > > > > > >  drivers/bluetooth/hci_qca.c | 47
> > > > > > > ++++++++++++++++++++++---------------
> > > > > > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > > > > > index 237aea34b69f..0a5c98d46864 100644
> > > > > > > --- a/drivers/bluetooth/hci_qca.c
> > > > > > > +++ b/drivers/bluetooth/hci_qca.c
> > > > > > > @@ -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.
> > > >
> > > > 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
> > > 
> > > 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!
> > 
> > > 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
> 
> 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));
> ---
> >       if (hu->serdev)
> >               serdev_device_wait_until_sent(hu->serdev,
> >                               msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));

argh, I shouldn't have missed this, thanks for testing!

Here is a fix:

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