lkml.org 
[lkml]   [2012]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH] firmware loader: Fix the race FW_STATUS_DONE is followed by class_timeout
Date


> -----Original Message-----
> From: Ming Lei [mailto:ming.lei@canonical.com]
> Sent: Wednesday, November 07, 2012 6:40 PM
> To: Liu, Chuansheng
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] firmware loader: Fix the race FW_STATUS_DONE is
> followed by class_timeout
>
> On Thu, Nov 8, 2012 at 12:45 AM, Chuansheng Liu
> <chuansheng.liu@intel.com> wrote:
> >
> > There is a race condition as below when calling request_firmware():
> >
> > CPU1 CPU2
> > write 0 > loading
> > mutex_lock(&fw_lock);
> > ...
> > set_bit FW_STATUS_DONE class_timeout is coming
> > set_bit
> FW_STATUS_ABORT
> > complete_all &completion
> > ...
> > mutex_unlock(&fw_lock)
>
> Good catch, but your fix isn't correct.
>
> >
> > In this time, the bit FW_STATUS_DONE and FW_STATUS_ABORT are set,
> > and request_firmware() will return failure due to condition in
> > _request_firmware_load():
> >
> > if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
> > retval = -ENOENT;
> >
> > But from the above scenerio, it should be a succcessful requesting.
> > So we need judge if the FW_STATUS_DONE is set before calling abort
> > in timeout function firmware_class_timeout().
> >
> > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> > ---
> > drivers/base/firmware_class.c | 7 +++++++
> > 1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 8945f4e..35fffd8 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -671,6 +671,13 @@ static void firmware_class_timeout(u_long data)
> > {
> > struct firmware_priv *fw_priv = (struct firmware_priv *) data;
> >
> > + mutex_lock(&fw_lock);
>
> mutex can't be used in interrupt context, so one candidate fix is
> to convert the timeout timer into delayed work, and hold the
> 'fw_lock' during the work function.
>
> > + if (test_bit(FW_STATUS_DONE, &(fw_priv->buf->status))) {
> > + mutex_unlock(&fw_lock);
> > + return;
> > + }
> > + mutex_unlock(&fw_lock);
> > +
> > fw_load_abort(fw_priv);
> > }
>
> Also the lock of 'fw_lock' should be held when setting 'FW_STATUS_DONE'
> in _request_firmware_load().
>
> Could you send out a new patch to fix the race?
Thanks your review and proposal, has sent the patch V2 to be reviewed.
>
> Thanks,
> --
> Ming Lei


\
 
 \ /
  Last update: 2012-11-08 03:05    [W:0.387 / U:0.852 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site