* [PATCH 1/2] test_firmware: remove some dead code
@ 2019-02-21 18:37 Dan Carpenter
2019-02-21 18:38 ` [PATCH 2/2] test_firmware: silence underflow warning in test_dev_config_update_u8() Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-02-21 18:37 UTC (permalink / raw)
To: Andrew Morton, Luis R. Rodriguez
Cc: Randy Dunlap, linux-kernel, kernel-janitors, Greg Kroah-Hartman
The test_fw_config->reqs allocation succeeded so these addresses can't
be NULL. Also on the second error path, we forgot to set
"rc = -ENOMEM;".
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
lib/test_firmware.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 7cab9a9869ac..7222093ee00b 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -631,11 +631,6 @@ static ssize_t trigger_batched_requests_store(struct device *dev,
for (i = 0; i < test_fw_config->num_requests; i++) {
req = &test_fw_config->reqs[i];
- if (!req) {
- WARN_ON(1);
- rc = -ENOMEM;
- goto out_bail;
- }
req->fw = NULL;
req->idx = i;
req->name = test_fw_config->name;
@@ -737,10 +732,6 @@ ssize_t trigger_batched_requests_async_store(struct device *dev,
for (i = 0; i < test_fw_config->num_requests; i++) {
req = &test_fw_config->reqs[i];
- if (!req) {
- WARN_ON(1);
- goto out_bail;
- }
req->name = test_fw_config->name;
req->fw = NULL;
req->idx = i;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] test_firmware: silence underflow warning in test_dev_config_update_u8()
2019-02-21 18:37 [PATCH 1/2] test_firmware: remove some dead code Dan Carpenter
@ 2019-02-21 18:38 ` Dan Carpenter
2019-02-21 18:54 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-02-21 18:38 UTC (permalink / raw)
To: Andrew Morton, Luis R. Rodriguez
Cc: Randy Dunlap, linux-kernel, kernel-janitors, Greg Kroah-Hartman
We put an upper bound on "new" but we don't check for negatives. In
this case the underflow doesn't matter very much, but we may as well
make the static checker happy.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
lib/test_firmware.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 7222093ee00b..5911b0f1a715 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -326,15 +326,12 @@ static ssize_t test_dev_config_show_int(char *buf, int cfg)
static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
{
int ret;
- long new;
+ u8 new;
- ret = kstrtol(buf, 10, &new);
+ ret = kstrtou8(buf, 10, &new);
if (ret)
return ret;
- if (new > U8_MAX)
- return -EINVAL;
-
mutex_lock(&test_fw_mutex);
*(u8 *)cfg = new;
mutex_unlock(&test_fw_mutex);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] test_firmware: silence underflow warning in test_dev_config_update_u8()
2019-02-21 18:38 ` [PATCH 2/2] test_firmware: silence underflow warning in test_dev_config_update_u8() Dan Carpenter
@ 2019-02-21 18:54 ` Andrew Morton
2019-02-21 19:18 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2019-02-21 18:54 UTC (permalink / raw)
To: Dan Carpenter
Cc: Luis R. Rodriguez, Randy Dunlap, linux-kernel, kernel-janitors,
Greg Kroah-Hartman
On Thu, 21 Feb 2019 21:38:26 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:
> We put an upper bound on "new" but we don't check for negatives.
U8_MAX has unsigned type, so `if (new > U8_MAX)' does check for negative.
> In
> this case the underflow doesn't matter very much, but we may as well
> make the static checker happy.
>
> ...
>
> --- a/lib/test_firmware.c
> +++ b/lib/test_firmware.c
> @@ -326,15 +326,12 @@ static ssize_t test_dev_config_show_int(char *buf, int cfg)
> static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
> {
> int ret;
> - long new;
> + u8 new;
>
> - ret = kstrtol(buf, 10, &new);
> + ret = kstrtou8(buf, 10, &new);
> if (ret)
> return ret;
>
> - if (new > U8_MAX)
> - return -EINVAL;
> -
> mutex_lock(&test_fw_mutex);
> *(u8 *)cfg = new;
> mutex_unlock(&test_fw_mutex);
if *buf=="257",
previous behavior: -EINVAL
new behavior: *cfg = 1
yes?
The old behavior seems better.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] test_firmware: silence underflow warning in test_dev_config_update_u8()
2019-02-21 18:54 ` Andrew Morton
@ 2019-02-21 19:18 ` Dan Carpenter
2019-02-21 19:54 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-02-21 19:18 UTC (permalink / raw)
To: Andrew Morton
Cc: Luis R. Rodriguez, Randy Dunlap, linux-kernel, kernel-janitors,
Greg Kroah-Hartman
On Thu, Feb 21, 2019 at 10:54:58AM -0800, Andrew Morton wrote:
> On Thu, 21 Feb 2019 21:38:26 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> > We put an upper bound on "new" but we don't check for negatives.
>
> U8_MAX has unsigned type, so `if (new > U8_MAX)' does check for negative.
>
No, doesn't work in this case.
#define U8_MAX ((u8)~0U)
It would need to unsigned long for the type promotion to prevent
negatives, but it starts as unsigned int, then unsigned char, which is
type promoted to int.
It would be more clear to just write it as:
#define U8_MAX 0xff
> > In
> > this case the underflow doesn't matter very much, but we may as well
> > make the static checker happy.
> >
> > ...
> >
> > --- a/lib/test_firmware.c
> > +++ b/lib/test_firmware.c
> > @@ -326,15 +326,12 @@ static ssize_t test_dev_config_show_int(char *buf, int cfg)
> > static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
> > {
> > int ret;
> > - long new;
> > + u8 new;
> >
> > - ret = kstrtol(buf, 10, &new);
> > + ret = kstrtou8(buf, 10, &new);
> > if (ret)
> > return ret;
> >
> > - if (new > U8_MAX)
> > - return -EINVAL;
> > -
> > mutex_lock(&test_fw_mutex);
> > *(u8 *)cfg = new;
> > mutex_unlock(&test_fw_mutex);
>
> if *buf=="257",
>
> previous behavior: -EINVAL
> new behavior: *cfg = 1
>
> yes?
No. The kstrtou8() check the limit so it will return -ERANGE. I should
have probably spelled that out better in the commit message. I'll
resend tomorrow.
regard,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] test_firmware: silence underflow warning in test_dev_config_update_u8()
2019-02-21 19:18 ` Dan Carpenter
@ 2019-02-21 19:54 ` Andrew Morton
2019-02-22 7:29 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2019-02-21 19:54 UTC (permalink / raw)
To: Dan Carpenter
Cc: Luis R. Rodriguez, Randy Dunlap, linux-kernel, kernel-janitors,
Greg Kroah-Hartman
On Thu, 21 Feb 2019 22:18:56 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, Feb 21, 2019 at 10:54:58AM -0800, Andrew Morton wrote:
> > On Thu, 21 Feb 2019 21:38:26 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > > We put an upper bound on "new" but we don't check for negatives.
> >
> > U8_MAX has unsigned type, so `if (new > U8_MAX)' does check for negative.
> >
>
> No, doesn't work in this case.
>
> #define U8_MAX ((u8)~0U)
>
> It would need to unsigned long for the type promotion to prevent
> negatives, but it starts as unsigned int, then unsigned char, which is
> type promoted to int.
OK.
> It would be more clear to just write it as:
>
> #define U8_MAX 0xff
That doesn't work either. Tricky.
#include <stdio.h>
typedef unsigned char u8;
#define U8_MAX 0xff
int main(int argc, char *argv[])
{
long new;
new = -20;
if (new > U8_MAX)
printf("over\n");
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] test_firmware: silence underflow warning in test_dev_config_update_u8()
2019-02-21 19:54 ` Andrew Morton
@ 2019-02-22 7:29 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2019-02-22 7:29 UTC (permalink / raw)
To: Andrew Morton
Cc: Luis R. Rodriguez, Randy Dunlap, linux-kernel, kernel-janitors,
Greg Kroah-Hartman
I've looked at this some more and I don't think it's a good idea to
change the type of U8_MAX. Right now INT_MAX is int and USHRT_MAX is
unsigned short etc. That's the only intuitive thing for them to be.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-22 7:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-21 18:37 [PATCH 1/2] test_firmware: remove some dead code Dan Carpenter
2019-02-21 18:38 ` [PATCH 2/2] test_firmware: silence underflow warning in test_dev_config_update_u8() Dan Carpenter
2019-02-21 18:54 ` Andrew Morton
2019-02-21 19:18 ` Dan Carpenter
2019-02-21 19:54 ` Andrew Morton
2019-02-22 7:29 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).