| Message ID | 20190307101722.25871-1-bgodavar@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | [v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86 platforms. | expand |
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
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
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
在 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
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
在 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));
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/
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;
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(-)