| Message ID | 20200903145038.20076-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: em28xx: fix function pointer check | expand |
On Thu, Sep 03, 2020 at 07:50:38AM -0700, trix@redhat.com wrote: > From: Tom Rix <trix@redhat.com> > > clang static analyzer reports this problem > > em28xx-core.c:1162:4: warning: Called function pointer > is null (null dereference) > ops->suspend(dev->dev_next); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This is the problem block > > if (ops->suspend) > ops->suspend(dev); > if (dev->dev_next) > ops->suspend(dev->dev_next); > > The check for ops->suspend only covers one statement. > So fix the check consistent with other similar in > the file. > > Change a similar check in em28xx_resume_extension() > to use consistent logic as its siblings. > > Fixes: be7fd3c3a8c5 ("media: em28xx: Hauppauge DualHD second tuner functionality") > Signed-off-by: Tom Rix <trix@redhat.com> Seems reasonable. Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > --- > drivers/media/usb/em28xx/em28xx-core.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c > index e6088b5d1b80..d60f4c2a661d 100644 > --- a/drivers/media/usb/em28xx/em28xx-core.c > +++ b/drivers/media/usb/em28xx/em28xx-core.c > @@ -1156,10 +1156,11 @@ int em28xx_suspend_extension(struct em28xx *dev) > dev_info(&dev->intf->dev, "Suspending extensions\n"); > mutex_lock(&em28xx_devlist_mutex); > list_for_each_entry(ops, &em28xx_extension_devlist, next) { > - if (ops->suspend) > + if (ops->suspend) { > ops->suspend(dev); > - if (dev->dev_next) > - ops->suspend(dev->dev_next); > + if (dev->dev_next) > + ops->suspend(dev->dev_next); > + } > } > mutex_unlock(&em28xx_devlist_mutex); > return 0; > @@ -1172,11 +1173,11 @@ int em28xx_resume_extension(struct em28xx *dev) > dev_info(&dev->intf->dev, "Resuming extensions\n"); > mutex_lock(&em28xx_devlist_mutex); > list_for_each_entry(ops, &em28xx_extension_devlist, next) { > - if (!ops->resume) > - continue; > - ops->resume(dev); > - if (dev->dev_next) > - ops->resume(dev->dev_next); > + if (ops->resume) { > + ops->resume(dev); > + if (dev->dev_next) > + ops->resume(dev->dev_next); > + } > } > mutex_unlock(&em28xx_devlist_mutex); > return 0; > -- > 2.18.1 >
On 03/09/2020 16:50, trix@redhat.com wrote: > From: Tom Rix <trix@redhat.com> > > clang static analyzer reports this problem > > em28xx-core.c:1162:4: warning: Called function pointer > is null (null dereference) > ops->suspend(dev->dev_next); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This is the problem block > > if (ops->suspend) > ops->suspend(dev); > if (dev->dev_next) > ops->suspend(dev->dev_next); > > The check for ops->suspend only covers one statement. > So fix the check consistent with other similar in > the file. > > Change a similar check in em28xx_resume_extension() > to use consistent logic as its siblings. > > Fixes: be7fd3c3a8c5 ("media: em28xx: Hauppauge DualHD second tuner functionality") > Signed-off-by: Tom Rix <trix@redhat.com> > --- > drivers/media/usb/em28xx/em28xx-core.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c > index e6088b5d1b80..d60f4c2a661d 100644 > --- a/drivers/media/usb/em28xx/em28xx-core.c > +++ b/drivers/media/usb/em28xx/em28xx-core.c > @@ -1156,10 +1156,11 @@ int em28xx_suspend_extension(struct em28xx *dev) > dev_info(&dev->intf->dev, "Suspending extensions\n"); > mutex_lock(&em28xx_devlist_mutex); > list_for_each_entry(ops, &em28xx_extension_devlist, next) { > - if (ops->suspend) > + if (ops->suspend) { > ops->suspend(dev); > - if (dev->dev_next) > - ops->suspend(dev->dev_next); > + if (dev->dev_next) > + ops->suspend(dev->dev_next); > + } > } > mutex_unlock(&em28xx_devlist_mutex); > return 0; > @@ -1172,11 +1173,11 @@ int em28xx_resume_extension(struct em28xx *dev) > dev_info(&dev->intf->dev, "Resuming extensions\n"); > mutex_lock(&em28xx_devlist_mutex); > list_for_each_entry(ops, &em28xx_extension_devlist, next) { > - if (!ops->resume) > - continue; Actually, this code is fine: if !ops->resume, then just continue. So there is no need to change this resume code. And in fact, I think it would be best if the same approach was used in em28xx_suspend_extension. Regards, Hans > - ops->resume(dev); > - if (dev->dev_next) > - ops->resume(dev->dev_next); > + if (ops->resume) { > + ops->resume(dev); > + if (dev->dev_next) > + ops->resume(dev->dev_next); > + } > } > mutex_unlock(&em28xx_devlist_mutex); > return 0; >
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c index e6088b5d1b80..d60f4c2a661d 100644 --- a/drivers/media/usb/em28xx/em28xx-core.c +++ b/drivers/media/usb/em28xx/em28xx-core.c @@ -1156,10 +1156,11 @@ int em28xx_suspend_extension(struct em28xx *dev) dev_info(&dev->intf->dev, "Suspending extensions\n"); mutex_lock(&em28xx_devlist_mutex); list_for_each_entry(ops, &em28xx_extension_devlist, next) { - if (ops->suspend) + if (ops->suspend) { ops->suspend(dev); - if (dev->dev_next) - ops->suspend(dev->dev_next); + if (dev->dev_next) + ops->suspend(dev->dev_next); + } } mutex_unlock(&em28xx_devlist_mutex); return 0; @@ -1172,11 +1173,11 @@ int em28xx_resume_extension(struct em28xx *dev) dev_info(&dev->intf->dev, "Resuming extensions\n"); mutex_lock(&em28xx_devlist_mutex); list_for_each_entry(ops, &em28xx_extension_devlist, next) { - if (!ops->resume) - continue; - ops->resume(dev); - if (dev->dev_next) - ops->resume(dev->dev_next); + if (ops->resume) { + ops->resume(dev); + if (dev->dev_next) + ops->resume(dev->dev_next); + } } mutex_unlock(&em28xx_devlist_mutex); return 0;