* [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).