Hi Ben, > On Mar 21, 2018, at 10:58 AM, Ben Hutchings <ben.hutchings@xxxxxxxxxxxxxxx> wrote: > > On Wed, 2018-03-21 at 17:55 +0000, Madhani, Himanshu wrote: >> 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 > > Thanks. > Just to update, we are still going thru review and testing and will take some extra cycle to conclude. I will update as soon as we are able to reach conclusion. > Ben. > > -- > Ben Hutchings > Software Developer, Codethink Ltd. Thanks, - Himanshu
