| Message ID | 20200415002517.4328-1-scott.branden@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 55623260bb33e2ab849af76edf2253bc04cb241f |
| Headers | show |
| Series | test_firmware: remove unnecessary test_fw_mutex in test_dev_config_show_xxx | expand |
On Tue, Apr 14, 2020 at 05:25:17PM -0700, Scott Branden wrote: > Remove unnecessary use of test_fw_mutex in test_dev_config_show_xxx > functions that show simple bool, int, and u8. I would expect at least a READ_ONCE(), yes? > > Signed-off-by: Scott Branden <scott.branden@broadcom.com> > --- > lib/test_firmware.c | 26 +++----------------------- > 1 file changed, 3 insertions(+), 23 deletions(-) > > diff --git a/lib/test_firmware.c b/lib/test_firmware.c > index 0c7fbcf07ac5..9fee2b93a8d1 100644 > --- a/lib/test_firmware.c > +++ b/lib/test_firmware.c > @@ -310,27 +310,13 @@ static int test_dev_config_update_bool(const char *buf, size_t size, > return ret; > } > > -static ssize_t > -test_dev_config_show_bool(char *buf, > - bool config) > +static ssize_t test_dev_config_show_bool(char *buf, bool val) > { > - bool val; > - > - mutex_lock(&test_fw_mutex); > - val = config; > - mutex_unlock(&test_fw_mutex); > - > return snprintf(buf, PAGE_SIZE, "%d\n", val); > } > > -static ssize_t test_dev_config_show_int(char *buf, int cfg) > +static ssize_t test_dev_config_show_int(char *buf, int val) > { > - int val; > - > - mutex_lock(&test_fw_mutex); > - val = cfg; > - mutex_unlock(&test_fw_mutex); > - > return snprintf(buf, PAGE_SIZE, "%d\n", val); > } > > @@ -354,14 +340,8 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) > return size; > } > > -static ssize_t test_dev_config_show_u8(char *buf, u8 cfg) > +static ssize_t test_dev_config_show_u8(char *buf, u8 val) > { > - u8 val; > - > - mutex_lock(&test_fw_mutex); > - val = cfg; > - mutex_unlock(&test_fw_mutex); > - > return snprintf(buf, PAGE_SIZE, "%u\n", val); > } > > -- > 2.17.1 >
Hi Kees, On 2020-04-14 8:10 p.m., Kees Cook wrote: > On Tue, Apr 14, 2020 at 05:25:17PM -0700, Scott Branden wrote: >> Remove unnecessary use of test_fw_mutex in test_dev_config_show_xxx >> functions that show simple bool, int, and u8. > I would expect at least a READ_ONCE(), yes? I don't understand why you need a READ_ONCE when removing a mutex around an assignment of a parameter passed into a function being assigned to a local variable. Could you please explain your expectations. > >> Signed-off-by: Scott Branden <scott.branden@broadcom.com> >> --- >> lib/test_firmware.c | 26 +++----------------------- >> 1 file changed, 3 insertions(+), 23 deletions(-) >> >> diff --git a/lib/test_firmware.c b/lib/test_firmware.c >> index 0c7fbcf07ac5..9fee2b93a8d1 100644 >> --- a/lib/test_firmware.c >> +++ b/lib/test_firmware.c >> @@ -310,27 +310,13 @@ static int test_dev_config_update_bool(const char *buf, size_t size, >> return ret; >> } >> >> -static ssize_t >> -test_dev_config_show_bool(char *buf, >> - bool config) >> +static ssize_t test_dev_config_show_bool(char *buf, bool val) >> { >> - bool val; >> - >> - mutex_lock(&test_fw_mutex); >> - val = config; >> - mutex_unlock(&test_fw_mutex); >> - >> return snprintf(buf, PAGE_SIZE, "%d\n", val); >> } >> >> -static ssize_t test_dev_config_show_int(char *buf, int cfg) >> +static ssize_t test_dev_config_show_int(char *buf, int val) >> { >> - int val; >> - >> - mutex_lock(&test_fw_mutex); >> - val = cfg; >> - mutex_unlock(&test_fw_mutex); >> - >> return snprintf(buf, PAGE_SIZE, "%d\n", val); >> } >> >> @@ -354,14 +340,8 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) >> return size; >> } >> >> -static ssize_t test_dev_config_show_u8(char *buf, u8 cfg) >> +static ssize_t test_dev_config_show_u8(char *buf, u8 val) >> { >> - u8 val; >> - >> - mutex_lock(&test_fw_mutex); >> - val = cfg; >> - mutex_unlock(&test_fw_mutex); >> - >> return snprintf(buf, PAGE_SIZE, "%u\n", val); >> } >> >> -- >> 2.17.1 >>
On Wed, Apr 15, 2020 at 09:28:18AM -0700, Scott Branden wrote: > Hi Kees, > > On 2020-04-14 8:10 p.m., Kees Cook wrote: > > On Tue, Apr 14, 2020 at 05:25:17PM -0700, Scott Branden wrote: > > > Remove unnecessary use of test_fw_mutex in test_dev_config_show_xxx > > > functions that show simple bool, int, and u8. > > I would expect at least a READ_ONCE(), yes? > I don't understand why you need a READ_ONCE when removing a mutex around an > assignment > of a parameter passed into a function being assigned to a local variable. > > Could you please explain your expectations. Oops, yes, you're right. I misread and was thinking this was reading from a global. This looks fine. Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > > > > > Signed-off-by: Scott Branden <scott.branden@broadcom.com> > > > --- > > > lib/test_firmware.c | 26 +++----------------------- > > > 1 file changed, 3 insertions(+), 23 deletions(-) > > > > > > diff --git a/lib/test_firmware.c b/lib/test_firmware.c > > > index 0c7fbcf07ac5..9fee2b93a8d1 100644 > > > --- a/lib/test_firmware.c > > > +++ b/lib/test_firmware.c > > > @@ -310,27 +310,13 @@ static int test_dev_config_update_bool(const char *buf, size_t size, > > > return ret; > > > } > > > -static ssize_t > > > -test_dev_config_show_bool(char *buf, > > > - bool config) > > > +static ssize_t test_dev_config_show_bool(char *buf, bool val) > > > { > > > - bool val; > > > - > > > - mutex_lock(&test_fw_mutex); > > > - val = config; > > > - mutex_unlock(&test_fw_mutex); > > > - > > > return snprintf(buf, PAGE_SIZE, "%d\n", val); > > > } > > > -static ssize_t test_dev_config_show_int(char *buf, int cfg) > > > +static ssize_t test_dev_config_show_int(char *buf, int val) > > > { > > > - int val; > > > - > > > - mutex_lock(&test_fw_mutex); > > > - val = cfg; > > > - mutex_unlock(&test_fw_mutex); > > > - > > > return snprintf(buf, PAGE_SIZE, "%d\n", val); > > > } > > > @@ -354,14 +340,8 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) > > > return size; > > > } > > > -static ssize_t test_dev_config_show_u8(char *buf, u8 cfg) > > > +static ssize_t test_dev_config_show_u8(char *buf, u8 val) > > > { > > > - u8 val; > > > - > > > - mutex_lock(&test_fw_mutex); > > > - val = cfg; > > > - mutex_unlock(&test_fw_mutex); > > > - > > > return snprintf(buf, PAGE_SIZE, "%u\n", val); > > > } > > > -- > > > 2.17.1 > > > >
On Wed, Apr 15, 2020 at 09:44:31AM -0700, Kees Cook wrote: > On Wed, Apr 15, 2020 at 09:28:18AM -0700, Scott Branden wrote: > > Hi Kees, > > > > On 2020-04-14 8:10 p.m., Kees Cook wrote: > > > On Tue, Apr 14, 2020 at 05:25:17PM -0700, Scott Branden wrote: > > > > Remove unnecessary use of test_fw_mutex in test_dev_config_show_xxx > > > > functions that show simple bool, int, and u8. > > > I would expect at least a READ_ONCE(), yes? > > I don't understand why you need a READ_ONCE when removing a mutex around an > > assignment > > of a parameter passed into a function being assigned to a local variable. > > > > Could you please explain your expectations. > > Oops, yes, you're right. I misread and was thinking this was reading > from a global. This looks fine. > > Reviewed-by: Kees Cook <keescook@chromium.org> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> Luis
diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 0c7fbcf07ac5..9fee2b93a8d1 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -310,27 +310,13 @@ static int test_dev_config_update_bool(const char *buf, size_t size, return ret; } -static ssize_t -test_dev_config_show_bool(char *buf, - bool config) +static ssize_t test_dev_config_show_bool(char *buf, bool val) { - bool val; - - mutex_lock(&test_fw_mutex); - val = config; - mutex_unlock(&test_fw_mutex); - return snprintf(buf, PAGE_SIZE, "%d\n", val); } -static ssize_t test_dev_config_show_int(char *buf, int cfg) +static ssize_t test_dev_config_show_int(char *buf, int val) { - int val; - - mutex_lock(&test_fw_mutex); - val = cfg; - mutex_unlock(&test_fw_mutex); - return snprintf(buf, PAGE_SIZE, "%d\n", val); } @@ -354,14 +340,8 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) return size; } -static ssize_t test_dev_config_show_u8(char *buf, u8 cfg) +static ssize_t test_dev_config_show_u8(char *buf, u8 val) { - u8 val; - - mutex_lock(&test_fw_mutex); - val = cfg; - mutex_unlock(&test_fw_mutex); - return snprintf(buf, PAGE_SIZE, "%u\n", val); }
Remove unnecessary use of test_fw_mutex in test_dev_config_show_xxx functions that show simple bool, int, and u8. Signed-off-by: Scott Branden <scott.branden@broadcom.com> --- lib/test_firmware.c | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-)