| Message ID | 1629091302-7893-1-git-send-email-bgodavar@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Bluetooth: hci_qca: Set SSR triggered flags when SSR command is sent out | expand |
Hi Balakrishna, > This change sets SSR triggered flags when QCA SSR command is sent to > SoC. After the SSR command sent, driver discards the incoming data from > the upper layers. This way will ensure to read full dumps from the > BT SoC without any flow control issues due to excess of data receiving > from the HOST in audio usecases. > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> > --- > drivers/bluetooth/hci_qca.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index 53deea2..5cbed6a 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -69,6 +69,8 @@ > #define QCA_LAST_SEQUENCE_NUM 0xFFFF > #define QCA_CRASHBYTE_PACKET_LEN 1096 > #define QCA_MEMDUMP_BYTE 0xFB > +#define QCA_SSR_OPCODE 0xFC0C > +#define QCA_SSR_PKT_LEN 5 > > enum qca_flags { > QCA_IBS_DISABLED, > @@ -871,6 +873,14 @@ static int qca_enqueue(struct hci_uart *hu, struct sk_buff *skb) > /* Prepend skb with frame type */ > memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > > + if (hci_skb_pkt_type(skb) == HCI_COMMAND_PKT && > + skb->len == QCA_SSR_PKT_LEN && > + hci_skb_opcode(skb) == QCA_SSR_OPCODE) { > + bt_dev_info(hu->hdev, "Triggering ssr"); > + set_bit(QCA_SSR_TRIGGERED, &qca->flags); > + set_bit(QCA_MEMDUMP_COLLECTION, &qca->flags); > + } > + can we please stop hacking around by parsing opcodes in an enqueue function. Sounds like someone is injecting raw HCI vendor commands and then having a driver react to it. Regards Marcel
Hi Marcel, On 2021-08-16 21:37, Marcel Holtmann wrote: > Hi Balakrishna, > >> This change sets SSR triggered flags when QCA SSR command is sent to >> SoC. After the SSR command sent, driver discards the incoming data >> from >> the upper layers. This way will ensure to read full dumps from the >> BT SoC without any flow control issues due to excess of data receiving >> from the HOST in audio usecases. >> >> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> >> --- >> drivers/bluetooth/hci_qca.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index 53deea2..5cbed6a 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -69,6 +69,8 @@ >> #define QCA_LAST_SEQUENCE_NUM 0xFFFF >> #define QCA_CRASHBYTE_PACKET_LEN 1096 >> #define QCA_MEMDUMP_BYTE 0xFB >> +#define QCA_SSR_OPCODE 0xFC0C >> +#define QCA_SSR_PKT_LEN 5 >> >> enum qca_flags { >> QCA_IBS_DISABLED, >> @@ -871,6 +873,14 @@ static int qca_enqueue(struct hci_uart *hu, >> struct sk_buff *skb) >> /* Prepend skb with frame type */ >> memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); >> >> + if (hci_skb_pkt_type(skb) == HCI_COMMAND_PKT && >> + skb->len == QCA_SSR_PKT_LEN && >> + hci_skb_opcode(skb) == QCA_SSR_OPCODE) { >> + bt_dev_info(hu->hdev, "Triggering ssr"); >> + set_bit(QCA_SSR_TRIGGERED, &qca->flags); >> + set_bit(QCA_MEMDUMP_COLLECTION, &qca->flags); >> + } >> + > > can we please stop hacking around by parsing opcodes in an enqueue > function. Sounds like someone is injecting raw HCI vendor commands and > then having a driver react to it. > [Bala]: yes this opcode is injected via hcitool to test BT SoC dump procedure or to collect the dumps to debug the issue during issue cases. When audio usecases are running, HOST sends ACL packets to SoC, in meantime if this command is sent to SoC using hcitool to collect dumps at particular point, With out this check HOST is pumping continues data to SoC and SoC RFR line goes high, sometimes SoC become unresponsive and driver starts logging command timeout error. Instead here, once a cmd with this opcode is sent, timer is started to ensure that SSR is in progress. If no response from SoC for 8 seconds. Driver will be restarted. > Regards > > Marcel
Hi Balakrishna, >>> This change sets SSR triggered flags when QCA SSR command is sent to >>> SoC. After the SSR command sent, driver discards the incoming data from >>> the upper layers. This way will ensure to read full dumps from the >>> BT SoC without any flow control issues due to excess of data receiving >>> from the HOST in audio usecases. >>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> >>> --- >>> drivers/bluetooth/hci_qca.c | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >>> index 53deea2..5cbed6a 100644 >>> --- a/drivers/bluetooth/hci_qca.c >>> +++ b/drivers/bluetooth/hci_qca.c >>> @@ -69,6 +69,8 @@ >>> #define QCA_LAST_SEQUENCE_NUM 0xFFFF >>> #define QCA_CRASHBYTE_PACKET_LEN 1096 >>> #define QCA_MEMDUMP_BYTE 0xFB >>> +#define QCA_SSR_OPCODE 0xFC0C >>> +#define QCA_SSR_PKT_LEN 5 >>> enum qca_flags { >>> QCA_IBS_DISABLED, >>> @@ -871,6 +873,14 @@ static int qca_enqueue(struct hci_uart *hu, struct sk_buff *skb) >>> /* Prepend skb with frame type */ >>> memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); >>> + if (hci_skb_pkt_type(skb) == HCI_COMMAND_PKT && >>> + skb->len == QCA_SSR_PKT_LEN && >>> + hci_skb_opcode(skb) == QCA_SSR_OPCODE) { >>> + bt_dev_info(hu->hdev, "Triggering ssr"); >>> + set_bit(QCA_SSR_TRIGGERED, &qca->flags); >>> + set_bit(QCA_MEMDUMP_COLLECTION, &qca->flags); >>> + } >>> + >> can we please stop hacking around by parsing opcodes in an enqueue >> function. Sounds like someone is injecting raw HCI vendor commands and >> then having a driver react to it. > [Bala]: yes this opcode is injected via hcitool to test BT SoC dump procedure or > to collect the dumps to debug the issue during issue cases. When audio usecases are running, > HOST sends ACL packets to SoC, in meantime if this command is sent to SoC using hcitool > to collect dumps at particular point, With out this check HOST is pumping continues data to > SoC and SoC RFR line goes high, sometimes SoC become unresponsive and driver starts logging > command timeout error. Instead here, once a cmd with this opcode is sent, timer is started > to ensure that SSR is in progress. If no response from SoC for 8 seconds. Driver will be restarted. so why would I add a kernel work-around for this? Design a proper interface for this and don’t rely on injecting HCI commands via HCI raw channel. Regards Marcel
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index 53deea2..5cbed6a 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -69,6 +69,8 @@ #define QCA_LAST_SEQUENCE_NUM 0xFFFF #define QCA_CRASHBYTE_PACKET_LEN 1096 #define QCA_MEMDUMP_BYTE 0xFB +#define QCA_SSR_OPCODE 0xFC0C +#define QCA_SSR_PKT_LEN 5 enum qca_flags { QCA_IBS_DISABLED, @@ -871,6 +873,14 @@ static int qca_enqueue(struct hci_uart *hu, struct sk_buff *skb) /* Prepend skb with frame type */ memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); + if (hci_skb_pkt_type(skb) == HCI_COMMAND_PKT && + skb->len == QCA_SSR_PKT_LEN && + hci_skb_opcode(skb) == QCA_SSR_OPCODE) { + bt_dev_info(hu->hdev, "Triggering ssr"); + set_bit(QCA_SSR_TRIGGERED, &qca->flags); + set_bit(QCA_MEMDUMP_COLLECTION, &qca->flags); + } + spin_lock_irqsave(&qca->hci_ibs_lock, flags); /* Don't go to sleep in middle of patch download or
This change sets SSR triggered flags when QCA SSR command is sent to SoC. After the SSR command sent, driver discards the incoming data from the upper layers. This way will ensure to read full dumps from the BT SoC without any flow control issues due to excess of data receiving from the HOST in audio usecases. Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> --- drivers/bluetooth/hci_qca.c | 10 ++++++++++ 1 file changed, 10 insertions(+)