[RFT,3/7] firmware: make fw_add_devm_name() return 0 if cache present
diff mbox

Message ID 20180227232101.20786-4-mcgrof@kernel.org
State New
Headers show

Commit Message

Luis Chamberlain Feb. 27, 2018, 11:20 p.m. UTC
Currently fw_add_devm_name() returns 1 if the firmware cache
was already set. This makes it complicated for us to check for
correctness. It is actually non-fatal if the firmware cache
is already setup, so just return 0, and simplify the checkers.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_loader.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Kees Cook Feb. 27, 2018, 11:31 p.m. UTC | #1
On Tue, Feb 27, 2018 at 3:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> Currently fw_add_devm_name() returns 1 if the firmware cache
> was already set. This makes it complicated for us to check for
> correctness. It is actually non-fatal if the firmware cache
> is already setup, so just return 0, and simplify the checkers.
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

That'll teach me to read all the patches first. ;)

Honestly, I'd just fold this into the prior patch: there's only one
caller and it's exactly about checking the return value.

-Kees

> ---
>  drivers/base/firmware_loader.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
> index 48932581c70c..a385622bf3e1 100644
> --- a/drivers/base/firmware_loader.c
> +++ b/drivers/base/firmware_loader.c
> @@ -403,7 +403,7 @@ static int fw_add_devm_name(struct device *dev, const char *name)
>
>         fwn = fw_find_devm_name(dev, name);
>         if (fwn)
> -               return 1;
> +               return 0;
>
>         fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
>                            GFP_KERNEL);
> @@ -450,7 +450,7 @@ int assign_fw(struct firmware *fw, struct device *device,
>         if (device && (opt_flags & FW_OPT_UEVENT) &&
>             !(opt_flags & FW_OPT_NOCACHE)) {
>                 ret = fw_add_devm_name(device, fw_priv->fw_name);
> -               if (ret && ret != 1) {
> +               if (ret) {
>                         mutex_unlock(&fw_lock);
>                         return ret;
>                 }
> --
> 2.16.2
>
Luis Chamberlain Feb. 28, 2018, 1:20 a.m. UTC | #2
On Tue, Feb 27, 2018 at 03:31:11PM -0800, Kees Cook wrote:
> On Tue, Feb 27, 2018 at 3:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > Currently fw_add_devm_name() returns 1 if the firmware cache
> > was already set. This makes it complicated for us to check for
> > correctness. It is actually non-fatal if the firmware cache
> > is already setup, so just return 0, and simplify the checkers.
> >
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> 
> That'll teach me to read all the patches first. ;)
> 
> Honestly, I'd just fold this into the prior patch: there's only one
> caller and it's exactly about checking the return value.

Sounds good.

  Luis

Patch
diff mbox

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 48932581c70c..a385622bf3e1 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -403,7 +403,7 @@  static int fw_add_devm_name(struct device *dev, const char *name)
 
 	fwn = fw_find_devm_name(dev, name);
 	if (fwn)
-		return 1;
+		return 0;
 
 	fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
 			   GFP_KERNEL);
@@ -450,7 +450,7 @@  int assign_fw(struct firmware *fw, struct device *device,
 	if (device && (opt_flags & FW_OPT_UEVENT) &&
 	    !(opt_flags & FW_OPT_NOCACHE)) {
 		ret = fw_add_devm_name(device, fw_priv->fw_name);
-		if (ret && ret != 1) {
+		if (ret) {
 			mutex_unlock(&fw_lock);
 			return ret;
 		}