| Message ID | 20200220004825.23372-4-scott.branden@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: add partial read support in request_firmware_into_buf | expand |
On Wed, Feb 19, 2020 at 04:48:21PM -0800, Scott Branden wrote: > +static int test_dev_config_update_size_t(const char *buf, > + size_t size, > + size_t *cfg) > +{ > + int ret; > + long new; > + > + ret = kstrtol(buf, 10, &new); > + if (ret) > + return ret; > + > + if (new > SIZE_MAX) This "new" variable is long and SIZE_MAX is ULONG_MAX so the condition can't be true. > + return -EINVAL; > + > + mutex_lock(&test_fw_mutex); > + *(size_t *)cfg = new; > + mutex_unlock(&test_fw_mutex); > + > + /* Always return full write size even if we didn't consume all */ > + return size; > +} > + > +static ssize_t test_dev_config_show_size_t(char *buf, int cfg) > +{ > + size_t val; > + > + mutex_lock(&test_fw_mutex); > + val = cfg; > + mutex_unlock(&test_fw_mutex); Both val and cfg are stack variables so there is no need for locking. Probably you meant to pass a pointer to cfg? > + > + return snprintf(buf, PAGE_SIZE, "%zu\n", val); > +} > + > static ssize_t test_dev_config_show_int(char *buf, int cfg) > { > int val; regards, dan carpenter
Hi Dan, Thanks for your review and valuable comments. Will have to investigate fully and correct anything wrong. On 2020-02-20 12:42 a.m., Dan Carpenter wrote: > On Wed, Feb 19, 2020 at 04:48:21PM -0800, Scott Branden wrote: >> +static int test_dev_config_update_size_t(const char *buf, >> + size_t size, >> + size_t *cfg) >> +{ >> + int ret; >> + long new; >> + >> + ret = kstrtol(buf, 10, &new); >> + if (ret) >> + return ret; >> + >> + if (new > SIZE_MAX) > This "new" variable is long and SIZE_MAX is ULONG_MAX so the condition > can't be true. > >> + return -EINVAL; >> + >> + mutex_lock(&test_fw_mutex); >> + *(size_t *)cfg = new; >> + mutex_unlock(&test_fw_mutex); >> + >> + /* Always return full write size even if we didn't consume all */ >> + return size; >> +} >> + >> +static ssize_t test_dev_config_show_size_t(char *buf, int cfg) >> +{ >> + size_t val; >> + >> + mutex_lock(&test_fw_mutex); >> + val = cfg; >> + mutex_unlock(&test_fw_mutex); > Both val and cfg are stack variables so there is no need for locking. > Probably you meant to pass a pointer to cfg? > >> + >> + return snprintf(buf, PAGE_SIZE, "%zu\n", val); >> +} >> + >> static ssize_t test_dev_config_show_int(char *buf, int cfg) >> { >> int val; > regards, > dan carpenter > >
Reponses inline. Luis - please have a look as well. On 2020-02-21 10:30 a.m., Scott Branden wrote: > Hi Dan, > > Thanks for your review and valuable comments. > Will have to investigate fully and correct anything wrong. > > On 2020-02-20 12:42 a.m., Dan Carpenter wrote: >> On Wed, Feb 19, 2020 at 04:48:21PM -0800, Scott Branden wrote: >>> +static int test_dev_config_update_size_t(const char *buf, >>> + size_t size, >>> + size_t *cfg) >>> +{ >>> + int ret; >>> + long new; >>> + >>> + ret = kstrtol(buf, 10, &new); >>> + if (ret) >>> + return ret; >>> + >>> + if (new > SIZE_MAX) >> This "new" variable is long and SIZE_MAX is ULONG_MAX so the condition >> can't be true. Removed the check. >> >>> + return -EINVAL; >>> + >>> + mutex_lock(&test_fw_mutex); >>> + *(size_t *)cfg = new; >>> + mutex_unlock(&test_fw_mutex); >>> + >>> + /* Always return full write size even if we didn't consume all */ >>> + return size; >>> +} >>> + >>> +static ssize_t test_dev_config_show_size_t(char *buf, int cfg) >>> +{ >>> + size_t val; >>> + >>> + mutex_lock(&test_fw_mutex); >>> + val = cfg; >>> + mutex_unlock(&test_fw_mutex); >> Both val and cfg are stack variables so there is no need for locking. >> Probably you meant to pass a pointer to cfg? I am following the existing code as was done for test_dev_config_show_bool(), test_dev_config_show_int(), test_dev_config_show_u8() Mutex probably not needed but I don't think I need to deviate from the rest of the test code. Luis, could you please explain what the rest of your code is doing? >> >>> + >>> + return snprintf(buf, PAGE_SIZE, "%zu\n", val); >>> +} >>> + >>> static ssize_t test_dev_config_show_int(char *buf, int cfg) >>> { >>> int val; >> regards, >> dan carpenter >> >> >
On Fri, Feb 21, 2020 at 05:13:08PM -0800, Scott Branden wrote: > > > > +static ssize_t test_dev_config_show_size_t(char *buf, int cfg) > > > > +{ > > > > + size_t val; > > > > + > > > > + mutex_lock(&test_fw_mutex); > > > > + val = cfg; > > > > + mutex_unlock(&test_fw_mutex); > > > Both val and cfg are stack variables so there is no need for locking. > > > Probably you meant to pass a pointer to cfg? > I am following the existing code as was done for > test_dev_config_show_bool(), > test_dev_config_show_int(), > test_dev_config_show_u8() Heh. Yes. Those are buggy as well. Good eyes. Could you send a patch to fix them? regards, dan caprnter
On Fri, Feb 21, 2020 at 05:13:08PM -0800, Scott Branden wrote: > > > > +static ssize_t test_dev_config_show_size_t(char *buf, int cfg) > > > > +{ > > > > + size_t val; > > > > + > > > > + mutex_lock(&test_fw_mutex); > > > > + val = cfg; > > > > + mutex_unlock(&test_fw_mutex); > > > Both val and cfg are stack variables so there is no need for locking. > > > Probably you meant to pass a pointer to cfg? > I am following the existing code as was done for > test_dev_config_show_bool(), > test_dev_config_show_int(), > test_dev_config_show_u8() > > Mutex probably not needed but I don't think I need to deviate from the rest > of the test code. > > Luis, could you please explain what the rest of your code is doing? The lock is indeed not needed in the functions you mentioned, so you can also remove the other locks as a precursor patch. It would be a seperate patch. Luis
diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 7d1d97fa9a23..6050d3113f92 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -49,6 +49,9 @@ struct test_batched_req { * @name: the name of the firmware file to look for * @into_buf: when the into_buf is used if this is true * request_firmware_into_buf() will be used instead. + * @buf_size: size of buf to allocate when into_buf is true + * @file_offset: file offset to request when calling request_firmware_into_buf + * @partial: partial read flag value when calling request_firmware_into_buf * @sync_direct: when the sync trigger is used if this is true * request_firmware_direct() will be used instead. * @send_uevent: whether or not to send a uevent for async requests @@ -88,6 +91,9 @@ struct test_batched_req { struct test_config { char *name; bool into_buf; + size_t buf_size; + size_t file_offset; + bool partial; bool sync_direct; bool send_uevent; u8 num_requests; @@ -182,6 +188,9 @@ static int __test_firmware_config_init(void) test_fw_config->num_requests = TEST_FIRMWARE_NUM_REQS; test_fw_config->send_uevent = true; test_fw_config->into_buf = false; + test_fw_config->buf_size = TEST_FIRMWARE_BUF_SIZE; + test_fw_config->file_offset = 0; + test_fw_config->partial = false; test_fw_config->sync_direct = false; test_fw_config->req_firmware = request_firmware; test_fw_config->test_result = 0; @@ -253,6 +262,13 @@ static ssize_t config_show(struct device *dev, len += scnprintf(buf+len, PAGE_SIZE - len, "into_buf:\t\t%s\n", test_fw_config->into_buf ? "true" : "false"); + len += scnprintf(buf+len, PAGE_SIZE - len, + "buf_size:\t%zu\n", test_fw_config->buf_size); + len += scnprintf(buf+len, PAGE_SIZE - len, + "file_offset:\t%zu\n", test_fw_config->file_offset); + len += scnprintf(buf+len, PAGE_SIZE - len, + "partial:\t\t%s\n", + test_fw_config->partial ? "true" : "false"); len += scnprintf(buf+len, PAGE_SIZE - len, "sync_direct:\t\t%s\n", test_fw_config->sync_direct ? "true" : "false"); @@ -322,6 +338,39 @@ test_dev_config_show_bool(char *buf, return snprintf(buf, PAGE_SIZE, "%d\n", val); } +static int test_dev_config_update_size_t(const char *buf, + size_t size, + size_t *cfg) +{ + int ret; + long new; + + ret = kstrtol(buf, 10, &new); + if (ret) + return ret; + + if (new > SIZE_MAX) + return -EINVAL; + + mutex_lock(&test_fw_mutex); + *(size_t *)cfg = new; + mutex_unlock(&test_fw_mutex); + + /* Always return full write size even if we didn't consume all */ + return size; +} + +static ssize_t test_dev_config_show_size_t(char *buf, int cfg) +{ + size_t val; + + mutex_lock(&test_fw_mutex); + val = cfg; + mutex_unlock(&test_fw_mutex); + + return snprintf(buf, PAGE_SIZE, "%zu\n", val); +} + static ssize_t test_dev_config_show_int(char *buf, int cfg) { int val; @@ -419,6 +468,83 @@ static ssize_t config_into_buf_show(struct device *dev, } static DEVICE_ATTR_RW(config_into_buf); +static ssize_t config_buf_size_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int rc; + + mutex_lock(&test_fw_mutex); + if (test_fw_config->reqs) { + pr_err("Must call release_all_firmware prior to changing config\n"); + rc = -EINVAL; + mutex_unlock(&test_fw_mutex); + goto out; + } + mutex_unlock(&test_fw_mutex); + + rc = test_dev_config_update_size_t(buf, count, + &test_fw_config->buf_size); + +out: + return rc; +} + +static ssize_t config_buf_size_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return test_dev_config_show_size_t(buf, test_fw_config->buf_size); +} +static DEVICE_ATTR_RW(config_buf_size); + +static ssize_t config_file_offset_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int rc; + + mutex_lock(&test_fw_mutex); + if (test_fw_config->reqs) { + pr_err("Must call release_all_firmware prior to changing config\n"); + rc = -EINVAL; + mutex_unlock(&test_fw_mutex); + goto out; + } + mutex_unlock(&test_fw_mutex); + + rc = test_dev_config_update_size_t(buf, count, + &test_fw_config->file_offset); + +out: + return rc; +} + +static ssize_t config_file_offset_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return test_dev_config_show_size_t(buf, test_fw_config->file_offset); +} +static DEVICE_ATTR_RW(config_file_offset); + +static ssize_t config_partial_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + return test_dev_config_update_bool(buf, + count, + &test_fw_config->partial); +} + +static ssize_t config_partial_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return test_dev_config_show_bool(buf, test_fw_config->partial); +} +static DEVICE_ATTR_RW(config_partial); + static ssize_t config_sync_direct_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) @@ -613,18 +739,24 @@ static int test_fw_run_batch_request(void *data) if (test_fw_config->into_buf) { void *test_buf; + unsigned int pread_flags; test_buf = kzalloc(TEST_FIRMWARE_BUF_SIZE, GFP_KERNEL); if (!test_buf) return -ENOSPC; + if (test_fw_config->partial) + pread_flags = KERNEL_PREAD_FLAG_PART; + else + pread_flags = KERNEL_PREAD_FLAG_WHOLE; + req->rc = request_firmware_into_buf(&req->fw, req->name, req->dev, test_buf, - TEST_FIRMWARE_BUF_SIZE, - 0, - KERNEL_PREAD_FLAG_WHOLE); + test_fw_config->buf_size, + test_fw_config->file_offset, + pread_flags); if (!req->fw) kfree(test_buf); } else { @@ -897,6 +1029,9 @@ static struct attribute *test_dev_attrs[] = { TEST_FW_DEV_ATTR(config_name), TEST_FW_DEV_ATTR(config_num_requests), TEST_FW_DEV_ATTR(config_into_buf), + TEST_FW_DEV_ATTR(config_buf_size), + TEST_FW_DEV_ATTR(config_file_offset), + TEST_FW_DEV_ATTR(config_partial), TEST_FW_DEV_ATTR(config_sync_direct), TEST_FW_DEV_ATTR(config_send_uevent), TEST_FW_DEV_ATTR(config_read_fw_idx),
Add additional hooks to test_firmware to pass in support for partial file read using request_firmware_into_buf. buf_size: size of buffer to request firmware into partial: indicates that a partial file request is being made file_offset: to indicate offset into file to request Signed-off-by: Scott Branden <scott.branden@broadcom.com> --- lib/test_firmware.c | 141 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 138 insertions(+), 3 deletions(-)