Hi Ben, > On Mar 21, 2018, at 10:45 AM, Ben Hutchings <ben.hutchings@xxxxxxxxxxxxxxx> wrote: > > On Wed, 2018-03-21 at 17:17 +0000, Madhani, Himanshu wrote: >> Hi Ben, >> >>> On Mar 20, 2018, at 2:36 PM, Ben Hutchings <ben.hutchings@xxxxxxxxxxxxxxx> wrote: >>> >>> qla2x00_init_timer() calls add_timer() on the iocb timeout timer, >>> which means the timeout function pointer and any data that the >>> function depends on must be initialised beforehand. >>> >>> Move this initialisation before each call to qla2x00_init_timer(). In >>> some cases qla2x00_init_timer() initialises a completion structure >>> needed by the timeout function, so move the call to add_timer() after >>> that. > [...] >> What motivated this patch? are you debugging any crash which was >> helped by moving code around? > > I saw the recent fix that added a call to del_timer(), noticed the lack > of synchronisation and then kept looking further. > >> What I see from this patch is that its moving iocb.cmd.timeout field >> before qla2x00_init_timer(), >> which are completely different from each other. > > How are they "completely different"? qla2x00_init_timer() starts a > timer that will call qla2x00_sp_timeout(), which in turn uses the > iocb_cmd.timeout function pointer. We should not assume that any > initialisation code after the call to add_timer() will run before the > timer expires, since the scheduler might run other tasks for the whole > time. It's unlikely but not impossible. > I see. I used “completely different” due to the lack of better wording. I wanted to get context and understand if there was any issue that would have helped with these changes. your explanation does help and I agree that there is window where timer() will pop before timeout is initialized. Let me put this patch in our test cycle for couple days and will respond on this one > Ben. > > -- > Ben Hutchings > Software Developer, Codethink Ltd. > Thanks, - Himanshu
