lkml.org 
[lkml]   [2019]   [Jan]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC,v2] Bluetooth: hci_qca: Collect controller memory dump during SSR
Hi Marcel,

On 2019-01-18 14:24, Marcel Holtmann wrote:
> Hi Balakrishna,
>
>>>>>> We will collect the ramdump of BT controller when hardware error
>>>>>> event
>>>>>> received before rebooting the HCI layer. Before restarting a
>>>>>> subsystem
>>>>>> or a process running on a subsystem, it is often required to
>>>>>> request
>>>>>> either a subsystem or a process to perform proper cache dump and
>>>>>> software failure reason into a memory buffer which application
>>>>>> processor can retrieve afterwards. SW developers can often provide
>>>>>> initial investigation by looking into that debugging information.
>>>>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>>>>>> ---
>>>>>> changes v2:
>>>>>> * entirely an newer approach handling with an work queue.
>>>>>> ---
>>>>>> drivers/bluetooth/hci_qca.c | 289
>>>>>> ++++++++++++++++++++++++++++++++++--
>>>>>> 1 file changed, 278 insertions(+), 11 deletions(-)
>>>>>> diff --git a/drivers/bluetooth/hci_qca.c
>>>>>> b/drivers/bluetooth/hci_qca.c
>>>>>> index f036c8f98ea3..62b768bc32ec 100644
>>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>>> @@ -32,6 +32,7 @@
>>>>>> #include <linux/clk.h>
>>>>>> #include <linux/debugfs.h>
>>>>>> #include <linux/delay.h>
>>>>>> +#include <linux/devcoredump.h>
>>>>>> #include <linux/device.h>
>>>>>> #include <linux/gpio/consumer.h>
>>>>>> #include <linux/mod_devicetable.h>
>>>>> don't we have some crashdump core facility that could be utilized
>>>>> here?
>>>> [Bala]: no i don't think so. that is reason calling devcoredump.h
>>> what is wrong with using devcoredump.h?
>>
>> [Bala]: using the devcoredump.h
>>
>>>>>> @@ -57,9 +58,10 @@
>>>>>> /* Controller states */
>>>>>> #define STATE_IN_BAND_SLEEP_ENABLED 1
>>>>>> -#define IBS_WAKE_RETRANS_TIMEOUT_MS 100
>>>>>> +#define IBS_WAKE_RETRANS_TIMEOUT_MS 100
>>>>>> #define IBS_TX_IDLE_TIMEOUT_MS 2000
>>>>>> -#define BAUDRATE_SETTLE_TIMEOUT_MS 300
>>>>>> +#define BAUDRATE_SETTLE_TIMEOUT_MS 300
>>>>>> +#define MEMDUMP_COLLECTION_TIMEOUT_MS 8000
>>>>>> /* susclk rate */
>>>>>> #define SUSCLK_RATE_32KHZ 32768
>>>>>> @@ -67,6 +69,13 @@
>>>>>> /* Controller debug log header */
>>>>>> #define QCA_DEBUG_HANDLE 0x2EDC
>>>>>> +/* Controller dump header */
>>>>>> +#define QCA_SSR_DUMP_HANDLE 0x0108
>>>>>> +#define QCA_DUMP_PACKET_SIZE 255
>>>>>> +#define QCA_LAST_SEQUENCE_NUM 0xFFFF
>>>>>> +#define QCA_CRASHBYTE_PACKET_LEN 1100
>>>>>> +#define QCA_MEMDUMP_BYTE 0xFB
>>>>>> +
>>>>>> /* HCI_IBS transmit side sleep protocol states */
>>>>>> enum tx_ibs_states {
>>>>>> HCI_IBS_TX_ASLEEP,
>>>>>> @@ -89,12 +98,41 @@ enum hci_ibs_clock_state_vote {
>>>>>> HCI_IBS_RX_VOTE_CLOCK_OFF,
>>>>>> };
>>>>>> +/* Controller memory dump states */
>>>>>> +enum qca_memdump_states {
>>>>>> + QCA_MEMDUMP_IDLE,
>>>>>> + QCA_MEMDUMP_COLLECTING,
>>>>>> + QCA_MEMDUMP_COLLECTED,
>>>>>> + QCA_MEMDUMP_TIMEOUT,
>>>>>> +};
>>>>>> +
>>>>>> +struct qca_memdump_data {
>>>>>> + char *memdump_buf_head;
>>>>>> + char *memdump_buf_tail;
>>>>>> + u32 current_seq_no;
>>>>>> + u32 received_dump;
>>>>>> +};
>>>>>> +
>>>>>> +struct qca_memdump_event_hdr {
>>>>>> + __u8 evt;
>>>>>> + __u8 plen;
>>>>>> + __u16 opcode;
>>>>>> + __u16 seq_no;
>>>>>> + __u8 reserved;
>>>>>> +} __packed;
>>>>>> +
>>>>>> +
>>>>>> +struct qca_dump_size {
>>>>>> + u32 dump_size;
>>>>>> +} __packed;
>>>>>> +
>>>>>> struct qca_data {
>>>>>> struct hci_uart *hu;
>>>>>> struct sk_buff *rx_skb;
>>>>>> struct sk_buff_head txq;
>>>>>> - struct sk_buff_head tx_wait_q; /* HCI_IBS wait queue */
>>>>>> - spinlock_t hci_ibs_lock; /* HCI_IBS state lock */
>>>>>> + struct sk_buff_head tx_wait_q; /* HCI_IBS wait queue */
>>>>>> + struct sk_buff_head rx_memdump_q; /* Memdump wait queue */
>>>>>> + spinlock_t hci_ibs_lock; /* HCI_IBS state lock */
>>>>>> u8 tx_ibs_state; /* HCI_IBS transmit side power state*/
>>>>>> u8 rx_ibs_state; /* HCI_IBS receive side power state */
>>>>>> bool tx_vote; /* Clock must be on for TX */
>>>>>> @@ -103,12 +141,17 @@ struct qca_data {
>>>>>> u32 tx_idle_delay;
>>>>>> struct timer_list wake_retrans_timer;
>>>>>> u32 wake_retrans;
>>>>>> + struct timer_list memdump_timer;
>>>>>> + u32 memdump_delay;
>>>>>> struct workqueue_struct *workqueue;
>>>>>> struct work_struct ws_awake_rx;
>>>>>> struct work_struct ws_awake_device;
>>>>>> struct work_struct ws_rx_vote_off;
>>>>>> struct work_struct ws_tx_vote_off;
>>>>>> + struct work_struct ctrl_memdump_evt;
>>>>>> + struct qca_memdump_data *qca_memdump;
>>>>>> unsigned long flags;
>>>>>> + enum qca_memdump_states memdump_state;
>>>>>> /* For debugging purpose */
>>>>>> u64 ibs_sent_wacks;
>>>>>> @@ -173,6 +216,7 @@ struct qca_serdev {
>>>>>> static int qca_power_setup(struct hci_uart *hu, bool on);
>>>>>> static void qca_power_shutdown(struct hci_uart *hu);
>>>>>> static int qca_power_off(struct hci_dev *hdev);
>>>>>> +static void qca_controller_memdump(struct work_struct *work);
>>>>>> static void __serial_clock_on(struct tty_struct *tty)
>>>>>> {
>>>>>> @@ -446,6 +490,21 @@ static void
>>>>>> hci_ibs_wake_retrans_timeout(struct timer_list *t)
>>>>>> hci_uart_tx_wakeup(hu);
>>>>>> }
>>>>>> +static void hci_memdump_timeout(struct timer_list *t)
>>>>>> +{
>>>>>> + struct qca_data *qca = from_timer(qca, t, tx_idle_timer);
>>>>>> + struct hci_uart *hu = qca->hu;
>>>>>> + struct qca_memdump_data *qca_memdump = qca->qca_memdump;
>>>>>> + char *memdump_buf = qca_memdump->memdump_buf_tail;
>>>>>> +
>>>>>> + bt_dev_err(hu->hdev, "clearing allocated memory due to memdump
>>>>>> timeout");
>>>>>> + kfree(memdump_buf);
>>>>>> + kfree(qca_memdump);
>>>>>> + qca->memdump_state = QCA_MEMDUMP_TIMEOUT;
>>>>>> + del_timer(&qca->memdump_timer);
>>>>>> + cancel_work_sync(&qca->ctrl_memdump_evt);
>>>>>> +}
>>>>>> +
>>>>>> /* Initialize protocol */
>>>>>> static int qca_open(struct hci_uart *hu)
>>>>>> {
>>>>>> @@ -461,6 +520,7 @@ static int qca_open(struct hci_uart *hu)
>>>>>> skb_queue_head_init(&qca->txq);
>>>>>> skb_queue_head_init(&qca->tx_wait_q);
>>>>>> + skb_queue_head_init(&qca->rx_memdump_q);
>>>>>> spin_lock_init(&qca->hci_ibs_lock);
>>>>>> qca->workqueue = alloc_ordered_workqueue("qca_wq", 0);
>>>>>> if (!qca->workqueue) {
>>>>>> @@ -473,6 +533,7 @@ static int qca_open(struct hci_uart *hu)
>>>>>> INIT_WORK(&qca->ws_awake_device, qca_wq_awake_device);
>>>>>> INIT_WORK(&qca->ws_rx_vote_off, qca_wq_serial_rx_clock_vote_off);
>>>>>> INIT_WORK(&qca->ws_tx_vote_off, qca_wq_serial_tx_clock_vote_off);
>>>>>> + INIT_WORK(&qca->ctrl_memdump_evt, qca_controller_memdump);
>>>>>> qca->hu = hu;
>>>>>> @@ -500,7 +561,7 @@ static int qca_open(struct hci_uart *hu)
>>>>>> qca->tx_votes_off = 0;
>>>>>> qca->rx_votes_on = 0;
>>>>>> qca->rx_votes_off = 0;
>>>>>> -
>>>>>> + qca->memdump_state = QCA_MEMDUMP_IDLE;
>>>>>> hu->priv = qca;
>>>>>> if (hu->serdev) {
>>>>>> @@ -528,6 +589,9 @@ static int qca_open(struct hci_uart *hu)
>>>>>> timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
>>>>>> qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
>>>>>> + timer_setup(&qca->memdump_timer, hci_memdump_timeout, 0);
>>>>>> + qca->memdump_delay = MEMDUMP_COLLECTION_TIMEOUT_MS;
>>>>>> +
>>>>>> BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
>>>>>> qca->tx_idle_delay, qca->wake_retrans);
>>>>>> @@ -605,8 +669,10 @@ static int qca_close(struct hci_uart *hu)
>>>>>> skb_queue_purge(&qca->tx_wait_q);
>>>>>> skb_queue_purge(&qca->txq);
>>>>>> + skb_queue_purge(&qca->rx_memdump_q);
>>>>>> del_timer(&qca->tx_idle_timer);
>>>>>> del_timer(&qca->wake_retrans_timer);
>>>>>> + del_timer(&qca->memdump_timer);
>>>>>> destroy_workqueue(qca->workqueue);
>>>>>> qca->hu = NULL;
>>>>>> @@ -866,6 +932,141 @@ static int qca_recv_acl_data(struct hci_dev
>>>>>> *hdev, struct sk_buff *skb)
>>>>>> return hci_recv_frame(hdev, skb);
>>>>>> }
>>>>>> +static void qca_controller_memdump(struct work_struct *work)
>>>>>> +{
>>>>>> + struct qca_data *qca = container_of(work, struct qca_data,
>>>>>> + ctrl_memdump_evt);
>>>>>> + struct hci_uart *hu = qca->hu;
>>>>>> + struct sk_buff *skb;
>>>>>> + struct qca_memdump_event_hdr *cmd_hdr;
>>>>>> + struct qca_memdump_data *qca_memdump = qca->qca_memdump;
>>>>>> + struct qca_dump_size *dump;
>>>>>> + char *memdump_buf;
>>>>>> + char nullBuff[QCA_DUMP_PACKET_SIZE] = { 0 };
>>>>>> + u16 opcode, seq_no;
>>>>>> + u32 dump_size;
>>>>>> +
>>>>>> + while ((skb = skb_dequeue(&qca->rx_memdump_q))) {
>>>>>> +
>>>>>> + if (!qca_memdump) {
>>>>>> + qca_memdump = kzalloc(sizeof(struct qca_memdump_data),
>>>>>> + GFP_ATOMIC);
>>>>>> + if (!qca_memdump)
>>>>>> + return;
>>>>>> +
>>>>>> + qca->qca_memdump = qca_memdump;
>>>>>> + }
>>>>>> +
>>>>>> + qca->memdump_state = QCA_MEMDUMP_COLLECTING;
>>>>>> + cmd_hdr = (void *) skb->data;
>>>>>> + opcode = __le16_to_cpu(cmd_hdr->opcode);
>>>>>> + seq_no = __le16_to_cpu(cmd_hdr->seq_no);
>>>>>> + skb_pull(skb, sizeof(struct qca_memdump_event_hdr));
>>>>>> +
>>>>>> + if (!seq_no) {
>>>>>> +
>>>>>> + /* This is the first frame of memdump packet from
>>>>>> + * the controller, Disable IBS to recevie dump
>>>>>> + * with out any interruption, ideally time required for
>>>>>> + * the controller to send the dump is 8 seconds. let us
>>>>>> + * start timer to handle this asynchronous activity.
>>>>>> + */
>>>>>> + clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>>>>>> +
>>>>>> + dump = (void *) skb->data;
>>>>>> + dump_size = __le32_to_cpu(dump->dump_size);
>>>>>> + if (!(dump_size)) {
>>>>>> + bt_dev_err(hu->hdev, "QCA invalid memdump size");
>>>>>> + kfree_skb(skb);
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + bt_dev_info(hu->hdev, "QCA: collecting memdump started"
>>>>>> + "of size:%u", dump_size);
>>>>>> + mod_timer(&qca->memdump_timer, (jiffies +
>>>>>> + msecs_to_jiffies(qca->memdump_delay)));
>>>>>> +
>>>>>> + skb_pull(skb, sizeof(dump_size));
>>>>>> + memdump_buf = vmalloc(dump_size);
>>>>>> + qca_memdump->memdump_buf_head = memdump_buf;
>>>>>> + qca_memdump->memdump_buf_tail = memdump_buf;
>>>>>> + }
>>>>>> +
>>>>>> + memdump_buf = qca_memdump->memdump_buf_tail;
>>>>>> +
>>>>>> + /* If sequence no 0 is missed then there is no point in
>>>>>> + * accepting the other sequences.
>>>>>> + */
>>>>>> + if (!memdump_buf) {
>>>>>> + bt_dev_err(hu->hdev, "QCA: Discarding other packets");
>>>>>> + kfree(qca_memdump);
>>>>>> + kfree_skb(skb);
>>>>>> + qca->qca_memdump = NULL;
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + /* There could be chance of missing some packets from
>>>>>> + * the controller. In such cases let us store the dummy
>>>>>> + * packets in the buffer.
>>>>>> + */
>>>>>> + while ((seq_no > qca_memdump->current_seq_no + 1) &&
>>>>>> + seq_no != QCA_LAST_SEQUENCE_NUM) {
>>>>>> + bt_dev_err(hu->hdev, "QCA controller missed packet:%d",
>>>>>> + qca_memdump->current_seq_no);
>>>>>> + memcpy(memdump_buf, nullBuff, QCA_DUMP_PACKET_SIZE);
>>>>>> + memdump_buf = memdump_buf + QCA_DUMP_PACKET_SIZE;
>>>>>> + qca_memdump->received_dump += QCA_DUMP_PACKET_SIZE;
>>>>>> + qca_memdump->current_seq_no++;
>>>>>> + }
>>>>>> +
>>>>>> + memcpy(memdump_buf, (unsigned char *) skb->data, skb->len);
>>>>>> + memdump_buf = memdump_buf + skb->len;
>>>>>> + qca_memdump->memdump_buf_tail = memdump_buf;
>>>>>> + qca_memdump->current_seq_no = seq_no + 1;
>>>>>> + qca_memdump->received_dump += skb->len;
>>>>>> + qca->qca_memdump = qca_memdump;
>>>>>> + kfree_skb(skb);
>>>>>> + if (seq_no == QCA_LAST_SEQUENCE_NUM) {
>>>>>> + bt_dev_info(hu->hdev, "QCA writing crash dump of size %d
>>>>>> bytes",
>>>>>> + qca_memdump->received_dump);
>>>>>> + memdump_buf = qca_memdump->memdump_buf_head;
>>>>>> + dev_coredumpv(&hu->serdev->dev, memdump_buf,
>>>>>> + qca_memdump->received_dump, GFP_KERNEL);
>>>>>> + // Revisit for free(memdump) if needed.
>>>>>> + del_timer(&qca->memdump_timer);
>>>>>> + kfree(qca->qca_memdump);
>>>>>> + qca->qca_memdump = NULL;
>>>>>> + qca->memdump_state = QCA_MEMDUMP_COLLECTED;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> +}
>>>>>> +
>>>>>> +int qca_controller_memdump_event(struct hci_dev *hdev, struct
>>>>>> sk_buff *skb)
>>>>>> +{
>>>>>> + struct hci_uart *hu = hci_get_drvdata(hdev);
>>>>>> + struct qca_data *qca = hu->priv;
>>>>>> +
>>>>>> + skb_queue_tail(&qca->rx_memdump_q, skb);
>>>>>> + queue_work(qca->workqueue, &qca->ctrl_memdump_evt);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int qca_recv_event_data(struct hci_dev *hdev, struct
>>>>>> sk_buff *skb)
>>>>>> +{
>>>>>> + /* We receive chip memory dump as an event packet, With a
>>>>>> dedicated
>>>>>> + * handler followed by a hardware error event. When this event
>>>>>> is
>>>>>> + * received we store dump into a file before closing hci. This
>>>>>> + * dump will help in triaging the issues.
>>>>>> + */
>>>>>> + if ((skb->data[0] == HCI_VENDOR_PKT) &&
>>>>>> + (get_unaligned_be16((skb->data) + 2) ==
>>>>>> QCA_SSR_DUMP_HANDLE))
>>>>> The (skb->data) is pointless, just do skb->data.
>>>> [Bala]: will update.
>>>>>> + return qca_controller_memdump_event(hdev, skb);
>>>>>> +
>>>>>> + return hci_recv_frame(hdev, skb);
>>>>>> +}
>>>>>> +
>>>>>> #define QCA_IBS_SLEEP_IND_EVENT \
>>>>>> .type = HCI_IBS_SLEEP_IND, \
>>>>>> .hlen = 0, \
>>>>>> @@ -888,12 +1089,12 @@ static int qca_recv_acl_data(struct hci_dev
>>>>>> *hdev, struct sk_buff *skb)
>>>>>> .maxlen = HCI_MAX_IBS_SIZE
>>>>>> static const struct h4_recv_pkt qca_recv_pkts[] = {
>>>>>> - { H4_RECV_ACL, .recv = qca_recv_acl_data },
>>>>>> - { H4_RECV_SCO, .recv = hci_recv_frame },
>>>>>> - { H4_RECV_EVENT, .recv = hci_recv_frame },
>>>>>> - { QCA_IBS_WAKE_IND_EVENT, .recv = qca_ibs_wake_ind },
>>>>>> - { QCA_IBS_WAKE_ACK_EVENT, .recv = qca_ibs_wake_ack },
>>>>>> - { QCA_IBS_SLEEP_IND_EVENT, .recv = qca_ibs_sleep_ind },
>>>>>> + { H4_RECV_ACL, .recv = qca_recv_acl_data },
>>>>>> + { H4_RECV_SCO, .recv = hci_recv_frame },
>>>>>> + { H4_RECV_EVENT, .recv = qca_recv_event_data },
>>>>> call it qca_recv_event and don't realign the whole table.
>>>> [Bala]: will update.
>>>>>> + { QCA_IBS_WAKE_IND_EVENT, .recv = qca_ibs_wake_ind },
>>>>>> + { QCA_IBS_WAKE_ACK_EVENT, .recv = qca_ibs_wake_ack },
>>>>>> + { QCA_IBS_SLEEP_IND_EVENT, .recv = qca_ibs_sleep_ind },
>>>>>> };
>>>>>> static int qca_recv(struct hci_uart *hu, const void *data, int
>>>>>> count)
>>>>>> @@ -1114,6 +1315,68 @@ static int qca_set_speed(struct hci_uart
>>>>>> *hu, enum qca_speed_type speed_type)
>>>>>> return 0;
>>>>>> }
>>>>>> +static int qca_send_crashbuffer(struct hci_uart *hu)
>>>>>> +{
>>>>>> + struct qca_data *qca = hu->priv;
>>>>>> + struct sk_buff *skb;
>>>>>> + unsigned char buf[QCA_CRASHBYTE_PACKET_LEN];
>>>>>> +
>>>>>> + bt_dev_dbg(hu->hdev, "sending soc crash buffer to controller");
>>>>>> +
>>>>>> + /* We forcefully crash the controller, by sending 0xfb byte for
>>>>>> + * 1024 times. We also might have chance of losing data, To be
>>>>>> + * on safer side we send 1100 bytes to the SoC.
>>>>>> + */
>>>>>> + memset(buf, QCA_MEMDUMP_BYTE, QCA_CRASHBYTE_PACKET_LEN);
>>>>>> +
>>>>>> + skb = bt_skb_alloc(QCA_CRASHBYTE_PACKET_LEN, GFP_KERNEL);
>>>>>> + if (!skb) {
>>>>>> + bt_dev_err(hu->hdev, "Failed to allocate memory for skb
>>>>>> packet");
>>>>>> + return -ENOMEM;
>>>>>> + }
>>>>>> +
>>>>>> + skb_put_data(skb, buf, QCA_CRASHBYTE_PACKET_LEN);
>>>>>> + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
>>>>>> +
>>>>>> + skb_queue_tail(&qca->txq, skb);
>>>>>> + hci_uart_tx_wakeup(hu);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static void qca_hw_error(struct hci_dev *hdev, u8 code)
>>>>>> +{
>>>>>> + struct hci_uart *hu = hci_get_drvdata(hdev);
>>>>>> + struct qca_data *qca = hu->priv;
>>>>>> +
>>>>>> + bt_dev_info(hdev, "mem_dump_status: %d", qca->memdump_state);
>>>>>> +
>>>>>> + if (qca->memdump_state == QCA_MEMDUMP_IDLE) {
>>>>>> + /* If hardware error event received for other than QCA
>>>>>> + * soc memory dump event, then we need to crash the SOC
>>>>>> + * and wait here for 8 seconds to get the dump packets.
>>>>>> + * This will block main thread to be on hold until we
>>>>>> + * collect dump.
>>>>>> + */
>>>>>> + mod_timer(&qca->memdump_timer,
>>>>>> + (jiffies + msecs_to_jiffies(qca->memdump_delay)));
>>>>> Why a timer that then ends up in interrupt context?
>>>> [Bala]: some times we only receive hardware error from the chip, so
>>>> we are sending an special buffer to request the
>>>> chip for memory dump. we are using an timer to check whether
>>>> the data memdump is collected. if the timer
>>>> over flows we are cleaning up all the assigned memory and
>>>> unblock the main thread,
>>>> in short this timer is a watch dog timer for the memory dump
>>>> collection.
>>> I saw that you used a bunch of timers actually. My question is why
>>> are
>>> these not delayed_wq instead? I wonder if we really want to keep
>>> using
>>> timers and handling interrupt context locking if we could move
>>> everything to wq instead.
>>
>> [Bala]: Let me brief you the requirement.
>> This patch is used to collect the memory dump received from the
>> chip and store it into a file.
>> We receive chip memory dump in two cases
>> 1. When chip goes into the bad state it sends the dump followed
>> by and hardware error event.
>> 2. Some time when we send some garbage or un recognized command
>> to chip.
>> it will respond with a hardware error event. So we send an
>> 0xfb bytes to the chip requesting it to send the memory dump.
>>
>> In both cases, chip will go into the bad state once it sends the dump.
>> so we need to turn OFF and ON chip and perform the init procedure.
>> So keeping the above cases in the view, we register an handle for
>> hardware error event. So that in both the cases hw error routine will
>> execute.
>>
>> As the dump as some specific headers we will queue the received data
>> into work queue.
>> We are blocking the main thread until the total dump is stored into
>> the file.
>> Here is the answer for the question, yes we check the status of the
>> work queue but for handling case 2 we require timers.
>> But let us assume the wq is taking more time than expected. Then we
>> also need a timer.
>> we are using timer so that we will not block the main thread
>> indefinitely.
>> They are some cases where when we request a memory dump, after some
>> packet sent by the chip.
>> it may go into a bad state where it will not pump the remaining dump,
>> so we are using a timer to unblock the main thread.
>> so during timeout, we are restarting the flags to unblock the main
>> thread.
>>
>>
>>>>>> + qca_send_crashbuffer(hu);
>>>>>> +
>>>>>> + while (qca->memdump_state != QCA_MEMDUMP_COLLECTED &&
>>>>>> + qca->memdump_state != QCA_MEMDUMP_TIMEOUT)
>>>>>> + ;
>>>>> What is this? Busy loop? I rather not do that.
>>>> [Bala]: this busy required to collected the dump from the chip
>>>> before we close an reopen the hci.
>>>> >> + } else if (qca->memdump_state == QCA_MEMDUMP_COLLECTING)
>>>> {
>>> Why not use some wakeup handling here. I am really against any kind
>>> of
>>> busy loops in a driver.
>> [Bala]: this busy loop is require. pls refer my comments to the above
>> query.
>> this will help in mainly in case 2, where we will block the
>> main thread until we have dump collected.
>>
>> let us assume that we received an hardware error event, hw
>> error routine we execute which sends an
>> special byte to the chip requesting for memory dump. it starts
>> sending the dump, but we move forward and turn off the
>> chip so we will miss the dump. so here the busy loop is to wait
>> until we have dump.
>> but the we are not waiting indefinitely we have timer to
>> unblock them.
>
> I do not see how any of this is correct. Can you not use some wakeup
> helper like everybody else does. I also do not see how these two
> threads are correctly protected. It looks really hackish to me.
>
> Regards
>
> Marcel

Ok sure, will check and update accordingly in the incremental patch.

--
Regards
Balakrishna.

\
 
 \ /
  Last update: 2019-01-21 15:41    [W:0.060 / U:2.864 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site