| Message ID | 8eee4415-9082-488a-99ca-e016f9d80bb6@xs4all.nl |
|---|---|
| State | New |
| Headers | show |
| Series | media: siano: add loadfirmware_handler NULL check | expand |
Hi Hans On Sat, 3 Aug 2024 at 11:35, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > Add check for a NULL value of the loadfirmware_handler to fix the > following smatch error: > > drivers/media/common/siano/smscoreapi.c:1172 smscore_load_firmware_from_file() error: we previously assumed 'loadfirmware_handler' could be null (see line 1150) > > Note that it is a false positive, but frankly, this change makes the > code more robust. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/common/siano/smscoreapi.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/common/siano/smscoreapi.c b/drivers/media/common/siano/smscoreapi.c > index b6f1eb5dbbdf..329aee411562 100644 > --- a/drivers/media/common/siano/smscoreapi.c > +++ b/drivers/media/common/siano/smscoreapi.c > @@ -1166,10 +1166,15 @@ static int smscore_load_firmware_from_file(struct smscore_device_t *coredev, I cannot find any place where loadfirmware_handler is != than NULL. Maybe it is better to clean the dead code instead? diff --git a/drivers/media/common/siano/smscoreapi.c b/drivers/media/common/siano/smscoreapi.c index b6f1eb5dbbdf..3732367e0c62 100644 --- a/drivers/media/common/siano/smscoreapi.c +++ b/drivers/media/common/siano/smscoreapi.c @@ -1132,8 +1132,7 @@ static char *smscore_get_fw_filename(struct smscore_device_t *coredev, * return: 0 on success, <0 on error. */ static int smscore_load_firmware_from_file(struct smscore_device_t *coredev, - int mode, - loadfirmware_t loadfirmware_handler) + int mode) { int rc = -ENOENT; u8 *fw_buf; @@ -1147,8 +1146,7 @@ static int smscore_load_firmware_from_file(struct smscore_device_t *coredev, } pr_debug("Firmware name: %s\n", fw_filename); - if (!loadfirmware_handler && - !(coredev->device_flags & SMS_DEVICE_FAMILY2)) + if (!(coredev->device_flags & SMS_DEVICE_FAMILY2)) return -EINVAL; rc = request_firmware(&fw, fw_filename, coredev->device); @@ -1166,10 +1164,8 @@ static int smscore_load_firmware_from_file(struct smscore_device_t *coredev, memcpy(fw_buf, fw->data, fw->size); fw_buf_size = fw->size; - rc = (coredev->device_flags & SMS_DEVICE_FAMILY2) ? - smscore_load_firmware_family2(coredev, fw_buf, fw_buf_size) - : loadfirmware_handler(coredev->context, fw_buf, - fw_buf_size); + rc = smscore_load_firmware_family2(coredev, fw_buf, + fw_buf_size); } kfree(fw_buf); @@ -1353,8 +1349,7 @@ int smscore_set_device_mode(struct smscore_device_t *coredev, int mode) } if (!(coredev->modes_supported & (1 << mode))) { - rc = smscore_load_firmware_from_file(coredev, - mode, NULL); + rc = smscore_load_firmware_from_file(coredev, mode); if (rc >= 0) pr_debug("firmware download success\n"); } else { diff --git a/drivers/media/common/siano/smscoreapi.h b/drivers/media/common/siano/smscoreapi.h index 82d9f8a64d99..3c15082ce0e3 100644 --- a/drivers/media/common/siano/smscoreapi.h +++ b/drivers/media/common/siano/smscoreapi.h @@ -97,7 +97,6 @@ typedef int (*hotplug_t)(struct smscore_device_t *coredev, typedef int (*setmode_t)(void *context, int mode); typedef void (*detectmode_t)(void *context, int *mode); typedef int (*sendrequest_t)(void *context, void *buffer, size_t size); -typedef int (*loadfirmware_t)(void *context, void *buffer, size_t size); typedef int (*preload_t)(void *context); typedef int (*postload_t)(void *context); @@ -1102,9 +1101,6 @@ extern int smscore_register_device(struct smsdevice_params_t *params, extern void smscore_unregister_device(struct smscore_device_t *coredev); extern int smscore_start_device(struct smscore_device_t *coredev); -extern int smscore_load_firmware(struct smscore_device_t *coredev, - char *filename, - loadfirmware_t loadfirmware_handler); extern int smscore_set_device_mode(struct smscore_device_t *coredev, int mode); extern int smscore_get_device_mode(struct smscore_device_t *coredev); > memcpy(fw_buf, fw->data, fw->size); > fw_buf_size = fw->size; > > + /* > + * Note that loadfirmware_handler can't be NULL due to the > + * check above, but it is confusing smatch. > + */ > rc = (coredev->device_flags & SMS_DEVICE_FAMILY2) ? > - smscore_load_firmware_family2(coredev, fw_buf, fw_buf_size) > - : loadfirmware_handler(coredev->context, fw_buf, > - fw_buf_size); > + smscore_load_firmware_family2(coredev, fw_buf, fw_buf_size) : > + (loadfirmware_handler ? > + loadfirmware_handler(coredev->context, fw_buf, fw_buf_size) : > + -EINVAL); > } > > kfree(fw_buf); > -- > 2.43.0 >
On 05/08/2024 08:59, Ricardo Ribalda wrote: > Hi Hans > > On Sat, 3 Aug 2024 at 11:35, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> Add check for a NULL value of the loadfirmware_handler to fix the >> following smatch error: >> >> drivers/media/common/siano/smscoreapi.c:1172 smscore_load_firmware_from_file() error: we previously assumed 'loadfirmware_handler' could be null (see line 1150) >> >> Note that it is a false positive, but frankly, this change makes the >> code more robust. >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> drivers/media/common/siano/smscoreapi.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/common/siano/smscoreapi.c b/drivers/media/common/siano/smscoreapi.c >> index b6f1eb5dbbdf..329aee411562 100644 >> --- a/drivers/media/common/siano/smscoreapi.c >> +++ b/drivers/media/common/siano/smscoreapi.c >> @@ -1166,10 +1166,15 @@ static int smscore_load_firmware_from_file(struct smscore_device_t *coredev, > > I cannot find any place where loadfirmware_handler is != than NULL. > Maybe it is better to clean the dead code instead? Good point, I didn't check that. Can you post this as a proper patch? Then I can commit it. Regards, Hans > > diff --git a/drivers/media/common/siano/smscoreapi.c > b/drivers/media/common/siano/smscoreapi.c > index b6f1eb5dbbdf..3732367e0c62 100644 > --- a/drivers/media/common/siano/smscoreapi.c > +++ b/drivers/media/common/siano/smscoreapi.c > @@ -1132,8 +1132,7 @@ static char *smscore_get_fw_filename(struct > smscore_device_t *coredev, > * return: 0 on success, <0 on error. > */ > static int smscore_load_firmware_from_file(struct smscore_device_t *coredev, > - int mode, > - loadfirmware_t loadfirmware_handler) > + int mode) > { > int rc = -ENOENT; > u8 *fw_buf; > @@ -1147,8 +1146,7 @@ static int > smscore_load_firmware_from_file(struct smscore_device_t *coredev, > } > pr_debug("Firmware name: %s\n", fw_filename); > > - if (!loadfirmware_handler && > - !(coredev->device_flags & SMS_DEVICE_FAMILY2)) > + if (!(coredev->device_flags & SMS_DEVICE_FAMILY2)) > return -EINVAL; > > rc = request_firmware(&fw, fw_filename, coredev->device); > @@ -1166,10 +1164,8 @@ static int > smscore_load_firmware_from_file(struct smscore_device_t *coredev, > memcpy(fw_buf, fw->data, fw->size); > fw_buf_size = fw->size; > > - rc = (coredev->device_flags & SMS_DEVICE_FAMILY2) ? > - smscore_load_firmware_family2(coredev, fw_buf, > fw_buf_size) > - : loadfirmware_handler(coredev->context, fw_buf, > - fw_buf_size); > + rc = smscore_load_firmware_family2(coredev, fw_buf, > + fw_buf_size); > } > > kfree(fw_buf); > @@ -1353,8 +1349,7 @@ int smscore_set_device_mode(struct > smscore_device_t *coredev, int mode) > } > > if (!(coredev->modes_supported & (1 << mode))) { > - rc = smscore_load_firmware_from_file(coredev, > - mode, NULL); > + rc = smscore_load_firmware_from_file(coredev, mode); > if (rc >= 0) > pr_debug("firmware download success\n"); > } else { > diff --git a/drivers/media/common/siano/smscoreapi.h > b/drivers/media/common/siano/smscoreapi.h > index 82d9f8a64d99..3c15082ce0e3 100644 > --- a/drivers/media/common/siano/smscoreapi.h > +++ b/drivers/media/common/siano/smscoreapi.h > @@ -97,7 +97,6 @@ typedef int (*hotplug_t)(struct smscore_device_t *coredev, > typedef int (*setmode_t)(void *context, int mode); > typedef void (*detectmode_t)(void *context, int *mode); > typedef int (*sendrequest_t)(void *context, void *buffer, size_t size); > -typedef int (*loadfirmware_t)(void *context, void *buffer, size_t size); > typedef int (*preload_t)(void *context); > typedef int (*postload_t)(void *context); > > @@ -1102,9 +1101,6 @@ extern int smscore_register_device(struct > smsdevice_params_t *params, > extern void smscore_unregister_device(struct smscore_device_t *coredev); > > extern int smscore_start_device(struct smscore_device_t *coredev); > -extern int smscore_load_firmware(struct smscore_device_t *coredev, > - char *filename, > - loadfirmware_t loadfirmware_handler); > > extern int smscore_set_device_mode(struct smscore_device_t *coredev, int mode); > extern int smscore_get_device_mode(struct smscore_device_t *coredev); > >> memcpy(fw_buf, fw->data, fw->size); >> fw_buf_size = fw->size; >> >> + /* >> + * Note that loadfirmware_handler can't be NULL due to the >> + * check above, but it is confusing smatch. >> + */ >> rc = (coredev->device_flags & SMS_DEVICE_FAMILY2) ? >> - smscore_load_firmware_family2(coredev, fw_buf, fw_buf_size) >> - : loadfirmware_handler(coredev->context, fw_buf, >> - fw_buf_size); >> + smscore_load_firmware_family2(coredev, fw_buf, fw_buf_size) : >> + (loadfirmware_handler ? >> + loadfirmware_handler(coredev->context, fw_buf, fw_buf_size) : >> + -EINVAL); >> } >> >> kfree(fw_buf); >> -- >> 2.43.0 >> > >
diff --git a/drivers/media/common/siano/smscoreapi.c b/drivers/media/common/siano/smscoreapi.c index b6f1eb5dbbdf..329aee411562 100644 --- a/drivers/media/common/siano/smscoreapi.c +++ b/drivers/media/common/siano/smscoreapi.c @@ -1166,10 +1166,15 @@ static int smscore_load_firmware_from_file(struct smscore_device_t *coredev, memcpy(fw_buf, fw->data, fw->size); fw_buf_size = fw->size; + /* + * Note that loadfirmware_handler can't be NULL due to the + * check above, but it is confusing smatch. + */ rc = (coredev->device_flags & SMS_DEVICE_FAMILY2) ? - smscore_load_firmware_family2(coredev, fw_buf, fw_buf_size) - : loadfirmware_handler(coredev->context, fw_buf, - fw_buf_size); + smscore_load_firmware_family2(coredev, fw_buf, fw_buf_size) : + (loadfirmware_handler ? + loadfirmware_handler(coredev->context, fw_buf, fw_buf_size) : + -EINVAL); } kfree(fw_buf);
Add check for a NULL value of the loadfirmware_handler to fix the following smatch error: drivers/media/common/siano/smscoreapi.c:1172 smscore_load_firmware_from_file() error: we previously assumed 'loadfirmware_handler' could be null (see line 1150) Note that it is a false positive, but frankly, this change makes the code more robust. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/media/common/siano/smscoreapi.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)