Re: [PATCH] test_firmware: remove unnecessary test_fw_mutex in test_dev_config_show_xxx

[Scott Branden]

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@xxxxxxxxxxxx>
> > ---
> >   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