On Thu, 1 Feb 2018, Yoshida, Shigeru wrote: > Hi Alan, > > Thank you for your commenting. > > On Wed, 31 Jan 2018 11:02:47 -0500, Alan Stern wrote: > >> To address above scenario, this patch introduces timer_running flag to > >> ohci_hcd structure. Setting true to ohci->timer_running indicates > >> io_watchdog_func() is scheduled or is running. ohci_urb_enqueue() > >> checks the flag when it schedules the watchdog (step 4 and 12 above), > >> so ohci->prev_frame_no is not overwritten while io_watchdog_func() is > >> running. > > > > Instead of adding an extra flag variable, which has to be kept in sync > > with the timer routine, how about defining a special sentinel value for > > prev_frame_no? For example: > > > > #define IO_WATCHDOG_OFF 0xffffff00 > > > > Then whenever the timer isn't scheduled or running, set > > ohci->prev_frame_no to IO_WATCHDOG_OFF. And instead of testing > > timer_pending(), compare prev_frame_no to this special value. > > > > I think that approach will be slightly more robust. > > It's reasonable since ohci->prev_frame_no is not used while the > watchdog timer is stopped. > > I think we must choose an invalid frame number for the special > sentinel value, but I'm not sure which value is adequate for it. > Is 0xffffff00 an invalid frame number, otherwise how about simply > -1(0xffffffff)?
Well, the frame_no register is 32 bits wide, but only the 16 low-order bits are meaningful. ohci_frame_no() strips off the high-order 16 bits, so any value with one of those bits set would be acceptable. (Besides, valid frame numbers only go up to 2047.) I chose 0xffffff00 because PCI reads from a non-working device generally get a value with all the bits set. But since the upper 16 bits are masked away anyhow, it doesn't matter. -1u would be fine. Alan Stern
