In vpfe_register_ccdc_device(), failure to allocate dev->hw_ops
invokes calls to BUG_ON(). This patch returns the error to callers
instead of crashing.
Signed-off-by: Aditya Pakki <[email protected]>
---
drivers/media/platform/davinci/vpfe_capture.c | 21 ++++++-------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/drivers/media/platform/davinci/vpfe_capture.c b/drivers/media/platform/davinci/vpfe_capture.c
index 916ed743d716..6d394a006977 100644
--- a/drivers/media/platform/davinci/vpfe_capture.c
+++ b/drivers/media/platform/davinci/vpfe_capture.c
@@ -168,21 +168,11 @@ int vpfe_register_ccdc_device(const struct ccdc_hw_device *dev)
int ret = 0;
printk(KERN_NOTICE "vpfe_register_ccdc_device: %s\n", dev->name);
- BUG_ON(!dev->hw_ops.open);
- BUG_ON(!dev->hw_ops.enable);
- BUG_ON(!dev->hw_ops.set_hw_if_params);
- BUG_ON(!dev->hw_ops.configure);
- BUG_ON(!dev->hw_ops.set_buftype);
- BUG_ON(!dev->hw_ops.get_buftype);
- BUG_ON(!dev->hw_ops.enum_pix);
- BUG_ON(!dev->hw_ops.set_frame_format);
- BUG_ON(!dev->hw_ops.get_frame_format);
- BUG_ON(!dev->hw_ops.get_pixel_format);
- BUG_ON(!dev->hw_ops.set_pixel_format);
- BUG_ON(!dev->hw_ops.set_image_window);
- BUG_ON(!dev->hw_ops.get_image_window);
- BUG_ON(!dev->hw_ops.get_line_length);
- BUG_ON(!dev->hw_ops.getfid);
+ if (!dev->hw_ops) {
+ printk(KERN_ERR "could not allocate hw_ops\n");
+ ret = -EINVAL;
+ goto rvalue;
+ }
mutex_lock(&ccdc_lock);
if (!ccdc_cfg) {
@@ -211,6 +201,7 @@ int vpfe_register_ccdc_device(const struct ccdc_hw_device *dev)
ccdc_dev = dev;
unlock:
mutex_unlock(&ccdc_lock);
+rvalue:
return ret;
}
EXPORT_SYMBOL(vpfe_register_ccdc_device);
--
2.17.1
Hello Aditya,
Thanks for the patch.
On Thu, 2019-12-05 at 19:00 -0600, Aditya Pakki wrote:
> In vpfe_register_ccdc_device(), failure to allocate dev->hw_ops
> invokes calls to BUG_ON(). This patch returns the error to callers
> instead of crashing.
>
I'm curious, are you actually getting this type of crash?
> Signed-off-by: Aditya Pakki <[email protected]>
> ---
> drivers/media/platform/davinci/vpfe_capture.c | 21 ++++++-------------
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/platform/davinci/vpfe_capture.c b/drivers/media/platform/davinci/vpfe_capture.c
> index 916ed743d716..6d394a006977 100644
> --- a/drivers/media/platform/davinci/vpfe_capture.c
> +++ b/drivers/media/platform/davinci/vpfe_capture.c
> @@ -168,21 +168,11 @@ int vpfe_register_ccdc_device(const struct ccdc_hw_device *dev)
> int ret = 0;
> printk(KERN_NOTICE "vpfe_register_ccdc_device: %s\n", dev->name);
>
> - BUG_ON(!dev->hw_ops.open);
> - BUG_ON(!dev->hw_ops.enable);
> - BUG_ON(!dev->hw_ops.set_hw_if_params);
> - BUG_ON(!dev->hw_ops.configure);
> - BUG_ON(!dev->hw_ops.set_buftype);
> - BUG_ON(!dev->hw_ops.get_buftype);
> - BUG_ON(!dev->hw_ops.enum_pix);
> - BUG_ON(!dev->hw_ops.set_frame_format);
> - BUG_ON(!dev->hw_ops.get_frame_format);
> - BUG_ON(!dev->hw_ops.get_pixel_format);
> - BUG_ON(!dev->hw_ops.set_pixel_format);
> - BUG_ON(!dev->hw_ops.set_image_window);
> - BUG_ON(!dev->hw_ops.get_image_window);
> - BUG_ON(!dev->hw_ops.get_line_length);
> - BUG_ON(!dev->hw_ops.getfid);
> + if (!dev->hw_ops) {
> + printk(KERN_ERR "could not allocate hw_ops\n");
I'd drop this error message, as hw_ops is not really allocated here.
> + ret = -EINVAL;
> + goto rvalue;
Instead of a goto to a return, just return -EINVAL here.
> + }
>
> mutex_lock(&ccdc_lock);
> if (!ccdc_cfg) {
> @@ -211,6 +201,7 @@ int vpfe_register_ccdc_device(const struct ccdc_hw_device *dev)
> ccdc_dev = dev;
> unlock:
> mutex_unlock(&ccdc_lock);
> +rvalue:
> return ret;
> }
> EXPORT_SYMBOL(vpfe_register_ccdc_device);
With those changes, the patch looks good.
Reviewed-by: Ezequiel Garcia <[email protected]>
Thanks,
Ezequiel
Hi Aditya,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.4 next-20191207]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Aditya-Pakki/media-davinci-vpfe_capture-c-Avoid-BUG_ON-for-register-failure/20191207-184817
base: git://linuxtv.org/media_tree.git master
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/media//platform/davinci/vpfe_capture.c: In function 'vpfe_register_ccdc_device':
>> drivers/media//platform/davinci/vpfe_capture.c:171:6: error: wrong type argument to unary exclamation mark
if (!dev->hw_ops) {
^
vim +171 drivers/media//platform/davinci/vpfe_capture.c
161
162 /*
163 * vpfe_register_ccdc_device. CCDC module calls this to
164 * register with vpfe capture
165 */
166 int vpfe_register_ccdc_device(const struct ccdc_hw_device *dev)
167 {
168 int ret = 0;
169 printk(KERN_NOTICE "vpfe_register_ccdc_device: %s\n", dev->name);
170
> 171 if (!dev->hw_ops) {
172 printk(KERN_ERR "could not allocate hw_ops\n");
173 ret = -EINVAL;
174 goto rvalue;
175 }
176
177 mutex_lock(&ccdc_lock);
178 if (!ccdc_cfg) {
179 /*
180 * TODO. Will this ever happen? if so, we need to fix it.
181 * Proabably we need to add the request to a linked list and
182 * walk through it during vpfe probe
183 */
184 printk(KERN_ERR "vpfe capture not initialized\n");
185 ret = -EFAULT;
186 goto unlock;
187 }
188
189 if (strcmp(dev->name, ccdc_cfg->name)) {
190 /* ignore this ccdc */
191 ret = -EINVAL;
192 goto unlock;
193 }
194
195 if (ccdc_dev) {
196 printk(KERN_ERR "ccdc already registered\n");
197 ret = -EINVAL;
198 goto unlock;
199 }
200
201 ccdc_dev = dev;
202 unlock:
203 mutex_unlock(&ccdc_lock);
204 rvalue:
205 return ret;
206 }
207 EXPORT_SYMBOL(vpfe_register_ccdc_device);
208
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Hello Aditya,
On Fri, 2019-12-06 at 06:20 -0300, Ezequiel Garcia wrote:
> Hello Aditya,
>
> Thanks for the patch.
>
> On Thu, 2019-12-05 at 19:00 -0600, Aditya Pakki wrote:
> > In vpfe_register_ccdc_device(), failure to allocate dev->hw_ops
> > invokes calls to BUG_ON(). This patch returns the error to callers
> > instead of crashing.
> >
>
> I'm curious, are you actually getting this type of crash?
>
Oops, it seems I was too fast to say this looked good.
The driver is not dynamically allocating dev->hw_ops;
as you can see hw_ops is not a pointer.
struct ccdc_hw_device {
/* ccdc device name */
char name[32];
/* module owner */
struct module *owner;
/* hw ops */
struct ccdc_hw_ops hw_ops;
};
Please, don't submit patches without testing, at the very
least compile testing.
Thanks,
Ezequiel
> > Signed-off-by: Aditya Pakki <[email protected]>
> > ---
> > drivers/media/platform/davinci/vpfe_capture.c | 21 ++++++-------------
> > 1 file changed, 6 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/media/platform/davinci/vpfe_capture.c b/drivers/media/platform/davinci/vpfe_capture.c
> > index 916ed743d716..6d394a006977 100644
> > --- a/drivers/media/platform/davinci/vpfe_capture.c
> > +++ b/drivers/media/platform/davinci/vpfe_capture.c
> > @@ -168,21 +168,11 @@ int vpfe_register_ccdc_device(const struct ccdc_hw_device *dev)
> > int ret = 0;
> > printk(KERN_NOTICE "vpfe_register_ccdc_device: %s\n", dev->name);
> >
> > - BUG_ON(!dev->hw_ops.open);
> > - BUG_ON(!dev->hw_ops.enable);
> > - BUG_ON(!dev->hw_ops.set_hw_if_params);
> > - BUG_ON(!dev->hw_ops.configure);
> > - BUG_ON(!dev->hw_ops.set_buftype);
> > - BUG_ON(!dev->hw_ops.get_buftype);
> > - BUG_ON(!dev->hw_ops.enum_pix);
> > - BUG_ON(!dev->hw_ops.set_frame_format);
> > - BUG_ON(!dev->hw_ops.get_frame_format);
> > - BUG_ON(!dev->hw_ops.get_pixel_format);
> > - BUG_ON(!dev->hw_ops.set_pixel_format);
> > - BUG_ON(!dev->hw_ops.set_image_window);
> > - BUG_ON(!dev->hw_ops.get_image_window);
> > - BUG_ON(!dev->hw_ops.get_line_length);
> > - BUG_ON(!dev->hw_ops.getfid);
> > + if (!dev->hw_ops) {
> > + printk(KERN_ERR "could not allocate hw_ops\n");
>
> I'd drop this error message, as hw_ops is not really allocated here.
>
> > + ret = -EINVAL;
> > + goto rvalue;
>
> Instead of a goto to a return, just return -EINVAL here.
>
> > + }
> >
> > mutex_lock(&ccdc_lock);
> > if (!ccdc_cfg) {
> > @@ -211,6 +201,7 @@ int vpfe_register_ccdc_device(const struct ccdc_hw_device *dev)
> > ccdc_dev = dev;
> > unlock:
> > mutex_unlock(&ccdc_lock);
> > +rvalue:
> > return ret;
> > }
> > EXPORT_SYMBOL(vpfe_register_ccdc_device);
>
> With those changes, the patch looks good.
>
> Reviewed-by: Ezequiel Garcia <[email protected]>
>
> Thanks,
> Ezequiel
>