From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1754127Ab3AaJz7 (ORCPT <rfc822;w@1wt.eu>);
	Thu, 31 Jan 2013 04:55:59 -0500
Received: from cantor2.suse.de ([195.135.220.15]:59021 "EHLO mx2.suse.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752432Ab3AaJzz (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Thu, 31 Jan 2013 04:55:55 -0500
Date: Thu, 31 Jan 2013 10:55:54 +0100
Message-ID: <s5hsj5hwuad.wl%tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Ming Lei <ming.lei@canonical.com>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code
In-Reply-To: <CACVXFVOu0PKy0MhHYrj9wP8Bcu-VRWRZRRVvnRwN3fxBEpUSMA@mail.gmail.com>
References: <1359542109-3043-1-git-send-email-tiwai@suse.de>
	<1359542109-3043-2-git-send-email-tiwai@suse.de>
	<CACVXFVOu0PKy0MhHYrj9wP8Bcu-VRWRZRRVvnRwN3fxBEpUSMA@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.2
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

At Thu, 31 Jan 2013 17:48:34 +0800,
Ming Lei wrote:
> 
(snip)
> > +/* called from request_firmware() and request_firmware_work_func() */
> > +static int
> > +_request_firmware(const struct firmware **firmware_p, const char *name,
> > +                 struct device *device, bool uevent, bool nowait)
> > +{
> > +       struct firmware *fw;
> > +       long timeout;
> > +       int ret;
> > +
> > +       if (!firmware_p)
> > +               return -EINVAL;
> > +
> > +       ret = _request_firmware_prepare(&fw, name, device);
> > +       if (ret <= 0) /* error or already assigned */
> > +               goto out;
> > +
> > +       ret = 0;
> > +       timeout = firmware_loading_timeout();
> 
> The above line may be moved below the line of 'if (nowait) '.

I thought of that, too, but later found that the timeout is used in
the call of fw_load_from_user_helper() below.

> > +       if (nowait) {
> > +               timeout = usermodehelper_read_lock_wait(timeout);
> > +               if (!timeout) {
> > +                       dev_dbg(device, "firmware: %s loading timed out\n",
> > +                               name);
> > +                       ret = -EBUSY;
> > +               }
> > +       } else {
> > +               ret = usermodehelper_read_trylock();
> > +               if (WARN_ON(ret)) {
> > +                       dev_err(device, "firmware: %s will not be loaded\n",
> > +                               name);
> > +               }
> > +       }
> > +
> > +       if (!ret) {
> > +               if (!fw_get_filesystem_firmware(device, fw->priv))
> > +                       ret = fw_load_from_user_helper(fw, name, device,
> > +                                                      uevent, nowait,
> > +                                                      timeout);
> > +               if (!ret)
> > +                       ret = assign_firmware_buf(fw, device);
> > +       }
> > +
> > +       usermodehelper_read_unlock();
> 
> The above should be move inside the above 'if (!ret) {...}'

Yep, but it would make the code less understandable.
I'll make rather the error path of usermodehelper lock goto's.

> 
> Except for the above two comments, looks the patch v4 is fine.
> 
> Please CC greg-kh when you send v5, and you may add my ack
> for the patchset.
> 
>             Acked-by: Ming Lei <ming.lei@canonical.com>

OK, thanks!


Takashi