From: Jian Dong dongjian@yulong.com
fixes coccicheck Error:
drivers/staging/greybus/bootrom.c:301:41-45: ERROR: fw is NULL but dereferenced.
if procedure goto label directly, ret will be nefative, so the fw is NULL and the if(condition) end with dereferenced fw. let's fix it.
Signed-off-by: Jian Dong dongjian@yulong.com --- drivers/staging/greybus/bootrom.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c index a8efb86..0439efa 100644 --- a/drivers/staging/greybus/bootrom.c +++ b/drivers/staging/greybus/bootrom.c @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) struct gb_bootrom_get_firmware_response *firmware_response; struct device *dev = &op->connection->bundle->dev; unsigned int offset, size; - enum next_request_type next_request; + enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE; int ret = 0;
/* Disable timeouts */ @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
queue_work: /* Refresh timeout */ - if (!ret && (offset + size == fw->size)) - next_request = NEXT_REQ_READY_TO_BOOT; - else + if (!!ret) next_request = NEXT_REQ_GET_FIRMWARE; + else if (offset + size == fw->size) + next_request = NEXT_REQ_READY_TO_BOOT;
gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
On Thu, Mar 25, 2021 at 06:19:26PM +0800, Jian Dong wrote:
From: Jian Dong dongjian@yulong.com
fixes coccicheck Error:
drivers/staging/greybus/bootrom.c:301:41-45: ERROR: fw is NULL but dereferenced.
if procedure goto label directly, ret will be nefative, so the fw is NULL and the if(condition) end with dereferenced fw. let's fix it.
Why is this all indented a space?
Signed-off-by: Jian Dong dongjian@yulong.com
drivers/staging/greybus/bootrom.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c index a8efb86..0439efa 100644 --- a/drivers/staging/greybus/bootrom.c +++ b/drivers/staging/greybus/bootrom.c @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) struct gb_bootrom_get_firmware_response *firmware_response; struct device *dev = &op->connection->bundle->dev; unsigned int offset, size;
- enum next_request_type next_request;
- enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE; int ret = 0;
/* Disable timeouts */ @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) queue_work: /* Refresh timeout */
- if (!ret && (offset + size == fw->size))
next_request = NEXT_REQ_READY_TO_BOOT;- else
- if (!!ret)
That is hard to understand, please make this more obvious.
thanks,
greg k-h
The commit description is not clear but this patch doesn't change how the code works, it just silences a static checker false positive.
Just ignore the false positive. Always just ignore static checkers when they are wrong.
regards, dan carpenter
On 25-03-21, 18:19, Jian Dong wrote:
From: Jian Dong dongjian@yulong.com
fixes coccicheck Error:
drivers/staging/greybus/bootrom.c:301:41-45: ERROR: fw is NULL but dereferenced.
if procedure goto label directly, ret will be nefative, so the fw is NULL and the if(condition) end with dereferenced fw. let's fix it.
No, fw is accessed only for !ret case.
Signed-off-by: Jian Dong dongjian@yulong.com
drivers/staging/greybus/bootrom.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c index a8efb86..0439efa 100644 --- a/drivers/staging/greybus/bootrom.c +++ b/drivers/staging/greybus/bootrom.c @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) struct gb_bootrom_get_firmware_response *firmware_response; struct device *dev = &op->connection->bundle->dev; unsigned int offset, size;
- enum next_request_type next_request;
- enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE; int ret = 0;
/* Disable timeouts */ @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) queue_work: /* Refresh timeout */
- if (!ret && (offset + size == fw->size))
next_request = NEXT_REQ_READY_TO_BOOT;- else
- if (!!ret) next_request = NEXT_REQ_GET_FIRMWARE;
- else if (offset + size == fw->size)
next_request = NEXT_REQ_READY_TO_BOOT;gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
The code is fine AFAICT, the coccicheck is buggy as it is detecting a bug here.
On Thu, 25 Mar 2021 11:29:06 +0100 Greg KH gregkh@linuxfoundation.org wrote:
On Thu, Mar 25, 2021 at 06:19:26PM +0800, Jian Dong wrote:
From: Jian Dong dongjian@yulong.com
fixes coccicheck Error:
drivers/staging/greybus/bootrom.c:301:41-45: ERROR: fw is NULL but dereferenced.
if procedure goto label directly, ret will be nefative, so the fw is NULL and the if(condition) end with dereferenced fw. let's fix it.
Why is this all indented a space?
this maybe caused by my terminal, I will take notice next time.
Signed-off-by: Jian Dong dongjian@yulong.com
drivers/staging/greybus/bootrom.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c index a8efb86..0439efa 100644 --- a/drivers/staging/greybus/bootrom.c +++ b/drivers/staging/greybus/bootrom.c @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) struct gb_bootrom_get_firmware_response *firmware_response; struct device *dev = &op->connection->bundle->dev; unsigned int offset, size;
- enum next_request_type next_request;
- enum next_request_type next_request =
NEXT_REQ_GET_FIRMWARE; int ret = 0; /* Disable timeouts */ @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) queue_work: /* Refresh timeout */
- if (!ret && (offset + size == fw->size))
next_request = NEXT_REQ_READY_TO_BOOT;- else
- if (!!ret)
That is hard to understand, please make this more obvious.
if (A && B) else (!A || !B)
So, when ret is NON-ZERO, set next_request as GET_FIRMWARE, else set READ_TO_BOOT. but if second express is flase, next_request still need be set as GET_FIRMWARE, So, I initialze it as GET_FIRMWARE.
this is will keep consistent with the origin conditional express: both ret is ZERO and second express TRUE, then set as READ_TO_BOOT, else set as GET_FIRMWARE.
thanks,
greg k-h
On Thu, Mar 25, 2021 at 07:03:39PM +0800, Jian Dong wrote:
On Thu, 25 Mar 2021 11:29:06 +0100 Greg KH gregkh@linuxfoundation.org wrote:
On Thu, Mar 25, 2021 at 06:19:26PM +0800, Jian Dong wrote:
From: Jian Dong dongjian@yulong.com
fixes coccicheck Error:
drivers/staging/greybus/bootrom.c:301:41-45: ERROR: fw is NULL but dereferenced.
if procedure goto label directly, ret will be nefative, so the fw is NULL and the if(condition) end with dereferenced fw. let's fix it.
Why is this all indented a space?
this maybe caused by my terminal, I will take notice next time.
Signed-off-by: Jian Dong dongjian@yulong.com
drivers/staging/greybus/bootrom.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c index a8efb86..0439efa 100644 --- a/drivers/staging/greybus/bootrom.c +++ b/drivers/staging/greybus/bootrom.c @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) struct gb_bootrom_get_firmware_response *firmware_response; struct device *dev = &op->connection->bundle->dev; unsigned int offset, size;
- enum next_request_type next_request;
- enum next_request_type next_request =
NEXT_REQ_GET_FIRMWARE; int ret = 0; /* Disable timeouts */ @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) queue_work: /* Refresh timeout */
- if (!ret && (offset + size == fw->size))
next_request = NEXT_REQ_READY_TO_BOOT;- else
- if (!!ret)
That is hard to understand, please make this more obvious.
if (A && B) else (!A || !B)
So, when ret is NON-ZERO, set next_request as GET_FIRMWARE, else set READ_TO_BOOT. but if second express is flase, next_request still need be set as GET_FIRMWARE, So, I initialze it as GET_FIRMWARE.
My point is: if (!!ret) is odd, and is the same thing as: if (ret) correct?
And the latter is the common kernel style, no need to be complex when you do not need to.
Anyway, others have pointed out why this is incorrect, no need for further discussion.
thanks,
greg k-h
-
Dan Carpenter -
Greg KH -
Jian Dong -
Viresh Kumar