2015-11-23 11:54:41

by Sudip Mukherjee

Subject: [PATCH char-misc-next 1/4] misc: mic: remove unneeded debug message

>From the error path we are printing an error message with dev_err(). No
need to print almost same message with dev_dbg().

Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/misc/mic/host/mic_x100.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/mic/host/mic_x100.c b/drivers/misc/mic/host/mic_x100.c
index 8118ac4..cd5208d 100644
--- a/drivers/misc/mic/host/mic_x100.c
+++ b/drivers/misc/mic/host/mic_x100.c
@@ -450,21 +450,21 @@ mic_x100_load_firmware(struct mic_device *mdev, const char *buf)

rc = mic_x100_get_boot_addr(mdev);
if (rc)
- goto error;
+ goto done;
/* load OS */
rc = request_firmware(&fw, mdev->cosm_dev->firmware, &mdev->pdev->dev);
if (rc < 0) {
dev_err(&mdev->pdev->dev,
"ramdisk request_firmware failed: %d %s\n",
rc, mdev->cosm_dev->firmware);
- goto error;
+ goto done;
}
if (mdev->bootaddr > mdev->aper.len - fw->size) {
rc = -EINVAL;
dev_err(&mdev->pdev->dev, "%s %d rc %d bootaddr 0x%x\n",
__func__, __LINE__, rc, mdev->bootaddr);
release_firmware(fw);
- goto error;
+ goto done;
}
memcpy_toio(mdev->aper.va + mdev->bootaddr, fw->data, fw->size);
mdev->ops->write_spad(mdev, MIC_X100_FW_SIZE, fw->size);
@@ -475,14 +475,13 @@ mic_x100_load_firmware(struct mic_device *mdev, const char *buf)
if (rc) {
dev_err(&mdev->pdev->dev, "%s %d rc %d\n",
__func__, __LINE__, rc);
- goto error;
+ goto done;
}
release_firmware(fw);
/* load ramdisk */
if (mdev->cosm_dev->ramdisk)
rc = mic_x100_load_ramdisk(mdev);
-error:
- dev_dbg(&mdev->pdev->dev, "%s %d rc %d\n", __func__, __LINE__, rc);
+
done:
return rc;
}
--
1.9.1

2015-11-23 11:54:45

by Sudip Mukherjee

Subject: [PATCH char-misc-next 2/4] misc: mic: return error properly

If request_firmware() succeeds then rc becomes 0. After that if the test
for strcmp() fails then we were jumping to label done: and returning rc.
But rc being 0 we returned success whereas we have failed here and we
were supposed to return an error.

Signed-off-by: Sudip Mukherjee <[email protected]>
---

I am not sure if this was intentional. Another case might be that if the
string bootmode is "flash" then mic_x100_load_command_line() was
intentionally skipped and success was returned.

drivers/misc/mic/host/mic_x100.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/mic/host/mic_x100.c b/drivers/misc/mic/host/mic_x100.c
index cd5208d..317e25f 100644
--- a/drivers/misc/mic/host/mic_x100.c
+++ b/drivers/misc/mic/host/mic_x100.c
@@ -468,8 +468,13 @@ mic_x100_load_firmware(struct mic_device *mdev, const char *buf)
}
memcpy_toio(mdev->aper.va + mdev->bootaddr, fw->data, fw->size);
mdev->ops->write_spad(mdev, MIC_X100_FW_SIZE, fw->size);
- if (!strcmp(mdev->cosm_dev->bootmode, "flash"))
+ if (!strcmp(mdev->cosm_dev->bootmode, "flash")) {
+ rc = -EINVAL;
+ dev_err(&mdev->pdev->dev, "%s %d rc %d\n",
+ __func__, __LINE__, rc);
+ release_firmware(fw);
goto done;
+ }
/* load command line */
rc = mic_x100_load_command_line(mdev, fw);
if (rc) {
--
1.9.1

2015-11-23 11:54:47

by Sudip Mukherjee

Subject: [PATCH char-misc-next 3/4] misc: mic: return error directly

Instead of jumping to a label and then returning from there lets return
directly.

Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/misc/mic/host/mic_x100.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/mic/host/mic_x100.c b/drivers/misc/mic/host/mic_x100.c
index 317e25f..37fa898 100644
--- a/drivers/misc/mic/host/mic_x100.c
+++ b/drivers/misc/mic/host/mic_x100.c
@@ -450,14 +450,14 @@ mic_x100_load_firmware(struct mic_device *mdev, const char *buf)

rc = mic_x100_get_boot_addr(mdev);
if (rc)
- goto done;
+ return rc;
/* load OS */
rc = request_firmware(&fw, mdev->cosm_dev->firmware, &mdev->pdev->dev);
if (rc < 0) {
dev_err(&mdev->pdev->dev,
"ramdisk request_firmware failed: %d %s\n",
rc, mdev->cosm_dev->firmware);
- goto done;
+ return rc;
}
if (mdev->bootaddr > mdev->aper.len - fw->size) {
rc = -EINVAL;
--
1.9.1

2015-11-23 11:54:51

by Sudip Mukherjee

Subject: [PATCH char-misc-next 4/4] misc: mic: use common error path

Instead of calling release_firmware() on every error and then jumping
lets have a common release_firmware() in the error path.
This patch also fixes a memory leak where we missed release_firmware()
if mic_x100_load_command_line() fails.

Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/misc/mic/host/mic_x100.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/mic/host/mic_x100.c b/drivers/misc/mic/host/mic_x100.c
index 37fa898..82a973c 100644
--- a/drivers/misc/mic/host/mic_x100.c
+++ b/drivers/misc/mic/host/mic_x100.c
@@ -463,8 +463,7 @@ mic_x100_load_firmware(struct mic_device *mdev, const char *buf)
rc = -EINVAL;
dev_err(&mdev->pdev->dev, "%s %d rc %d bootaddr 0x%x\n",
__func__, __LINE__, rc, mdev->bootaddr);
- release_firmware(fw);
- goto done;
+ goto error;
}
memcpy_toio(mdev->aper.va + mdev->bootaddr, fw->data, fw->size);
mdev->ops->write_spad(mdev, MIC_X100_FW_SIZE, fw->size);
@@ -472,22 +471,24 @@ mic_x100_load_firmware(struct mic_device *mdev, const char *buf)
rc = -EINVAL;
dev_err(&mdev->pdev->dev, "%s %d rc %d\n",
__func__, __LINE__, rc);
- release_firmware(fw);
- goto done;
+ goto error;
}
/* load command line */
rc = mic_x100_load_command_line(mdev, fw);
if (rc) {
dev_err(&mdev->pdev->dev, "%s %d rc %d\n",
__func__, __LINE__, rc);
- goto done;
+ goto error;
}
release_firmware(fw);
/* load ramdisk */
if (mdev->cosm_dev->ramdisk)
rc = mic_x100_load_ramdisk(mdev);

-done:
+ return rc;
+
+error:
+ release_firmware(fw);
return rc;
}

--
1.9.1

2015-12-12 02:49:46

by Dutt, Sudeep

Subject: Re: [PATCH char-misc-next 4/4] misc: mic: use common error path

On Mon, 2015-11-23 at 17:24 +0530, Sudip Mukherjee wrote:
> Instead of calling release_firmware() on every error and then jumping
> lets have a common release_firmware() in the error path.
> This patch also fixes a memory leak where we missed release_firmware()
> if mic_x100_load_command_line() fails.
>

Thanks for this patch series Sudip. All 4 patches look good.

Reviewed-by: Sudeep Dutt <[email protected]>


> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/misc/mic/host/mic_x100.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/misc/mic/host/mic_x100.c b/drivers/misc/mic/host/mic_x100.c
> index 37fa898..82a973c 100644
> --- a/drivers/misc/mic/host/mic_x100.c
> +++ b/drivers/misc/mic/host/mic_x100.c
> @@ -463,8 +463,7 @@ mic_x100_load_firmware(struct mic_device *mdev, const char *buf)
> rc = -EINVAL;
> dev_err(&mdev->pdev->dev, "%s %d rc %d bootaddr 0x%x\n",
> __func__, __LINE__, rc, mdev->bootaddr);
> - release_firmware(fw);
> - goto done;
> + goto error;
> }
> memcpy_toio(mdev->aper.va + mdev->bootaddr, fw->data, fw->size);
> mdev->ops->write_spad(mdev, MIC_X100_FW_SIZE, fw->size);
> @@ -472,22 +471,24 @@ mic_x100_load_firmware(struct mic_device *mdev, const char *buf)
> rc = -EINVAL;
> dev_err(&mdev->pdev->dev, "%s %d rc %d\n",
> __func__, __LINE__, rc);
> - release_firmware(fw);
> - goto done;
> + goto error;
> }
> /* load command line */
> rc = mic_x100_load_command_line(mdev, fw);
> if (rc) {
> dev_err(&mdev->pdev->dev, "%s %d rc %d\n",
> __func__, __LINE__, rc);
> - goto done;
> + goto error;
> }
> release_firmware(fw);
> /* load ramdisk */
> if (mdev->cosm_dev->ramdisk)
> rc = mic_x100_load_ramdisk(mdev);
>
> -done:
> + return rc;
> +
> +error:
> + release_firmware(fw);
> return rc;
> }
>