| Message ID | 20191206010029.14265-1-pakki001@umn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: davinci/vpfe_capture.c: Avoid BUG_ON for register failure | expand |
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 <pakki001@umn.edu> > --- > 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 <ezequiel@collabora.com> 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 <lkp@intel.com>
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/kbuild-all@lists.01.org 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 <pakki001@umn.edu> > > --- > > 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 <ezequiel@collabora.com> > > Thanks, > Ezequiel >
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);
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 <pakki001@umn.edu> --- drivers/media/platform/davinci/vpfe_capture.c | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-)