Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   "Yoshida, Shigeru" <Shigeru.Yoshida@windriver.com>
To:     "stern@rowland.harvard.edu" <stern@rowland.harvard.edu>
CC:     "Bai, Haiqing" <Haiqing.Bai@windriver.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ohci-hcd: Fix race condition caused by
 ohci_urb_enqueue() and io_watchdog_func()
Thread-Topic: [PATCH] ohci-hcd: Fix race condition caused by
 ohci_urb_enqueue() and io_watchdog_func()
Thread-Index: AQHTmncRqxjtZV1slkO7r5QpQNTFa6OOqw2AgADm5YA=
Date:   Thu, 1 Feb 2018 05:48:13 +0000
Message-ID: <20180201.144911.2171407625301691891.shigeru.yoshida@windriver.com>
References: <1517390197-32323-1-git-send-email-Haiqing.Bai@windriver.com>
        <Pine.LNX.4.44L0.1801311053240.1370-100000@iolanthe.rowland.org>
In-Reply-To: <Pine.LNX.4.44L0.1801311053240.1370-100000@iolanthe.rowland.org>
Accept-Language: ja-JP, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C18C3AF19F156F43B851A91BDEE2308C@local>
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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)?

Thanks,
Shigeru