Hi Abhishek, > Per our earlier review on chromium gerrit: > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1992966 > > I'm not too keen on the change from mutex to spinlock because it's > made the code more complex. > > Also, it has been a couple weeks since my last review and I've lost > the context of what order of events are supposed to happen (making > reviewing the sequencing hard). > > Good case: > > Memdump event from firmware > Some number of memdump events with seq # > Hw error event > Reset > > Timeout case: > > Memdump event from firmware > Some number of memdump events with seq # > Timeout schedules hw_error_event > hw_error_event clears the memdump activity > reset > > Software memdump: > > hw_error_event sends memdump command to firmware and waits for completion > memdump event with seq# > hw error event > reset > > Does this look right? Could you add this to either the commit message > or as a comment in one of the functions so that it's easier to > understand what is the expected order of events. > > On Tue, Feb 4, 2020 at 8:16 AM Venkata Lakshmi Narayana Gubba > <gubbaven@xxxxxxxxxxxxxx> wrote: >> >> This patch will fix the below issues >> 1.Fixed race conditions while accessing memory dump state flags. >> 2.Updated with actual context of timer in hci_memdump_timeout() >> 3.Updated injecting hardware error event if the dumps failed to receive. >> 4.Once timeout is triggered, stopping the memory dump collections. >> >> Fixes: d841502c79e3 ("Bluetooth: hci_qca: Collect controller memory dump during SSR") >> Reported-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx> >> Signed-off-by: Venkata Lakshmi Narayana Gubba <gubbaven@xxxxxxxxxxxxxx> >> --- >> drivers/bluetooth/hci_qca.c | 104 ++++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 90 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index eacc65b..ea956c3 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -69,7 +69,8 @@ enum qca_flags { >> QCA_IBS_ENABLED, >> QCA_DROP_VENDOR_EVENT, >> QCA_SUSPENDING, >> - QCA_MEMDUMP_COLLECTION >> + QCA_MEMDUMP_COLLECTION, >> + QCA_HW_ERROR_EVENT >> }; >> >> >> @@ -150,6 +151,7 @@ struct qca_data { >> struct completion drop_ev_comp; >> wait_queue_head_t suspend_wait_q; >> enum qca_memdump_states memdump_state; >> + spinlock_t hci_memdump_lock; > In an earlier revision of this patch, you had this lock as a mutex. > Why change it from mutex to spinlock_t? I think this has made your > change more complex since you have to unlock during the middle of an > operation more often (i.e. since it can block) I agree that we should try to keep a mutex since all event processing in Bluetooth core happens in a workqueue anyway. Regards Marcel
