* [PATCH] media: davinci/vpfe_capture.c: Avoid BUG_ON for register failure @ 2019-12-06 1:00 Aditya Pakki 2019-12-06 9:20 ` Ezequiel Garcia 2019-12-07 20:40 ` kbuild test robot 0 siblings, 2 replies; 4+ messages in thread From: Aditya Pakki @ 2019-12-06 1:00 UTC (permalink / raw) To: pakki001 Cc: kjlu, Lad, Prabhakar, Mauro Carvalho Chehab, linux-media, linux-kernel 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(-) 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: davinci/vpfe_capture.c: Avoid BUG_ON for register failure 2019-12-06 1:00 [PATCH] media: davinci/vpfe_capture.c: Avoid BUG_ON for register failure Aditya Pakki @ 2019-12-06 9:20 ` Ezequiel Garcia 2019-12-08 9:44 ` Ezequiel Garcia 2019-12-07 20:40 ` kbuild test robot 1 sibling, 1 reply; 4+ messages in thread From: Ezequiel Garcia @ 2019-12-06 9:20 UTC (permalink / raw) To: Aditya Pakki Cc: kjlu, Lad, Prabhakar, Mauro Carvalho Chehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: davinci/vpfe_capture.c: Avoid BUG_ON for register failure 2019-12-06 9:20 ` Ezequiel Garcia @ 2019-12-08 9:44 ` Ezequiel Garcia 0 siblings, 0 replies; 4+ messages in thread From: Ezequiel Garcia @ 2019-12-08 9:44 UTC (permalink / raw) To: Aditya Pakki Cc: kjlu, Lad, Prabhakar, Mauro Carvalho Chehab, linux-media, linux-kernel 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: davinci/vpfe_capture.c: Avoid BUG_ON for register failure 2019-12-06 1:00 [PATCH] media: davinci/vpfe_capture.c: Avoid BUG_ON for register failure Aditya Pakki 2019-12-06 9:20 ` Ezequiel Garcia @ 2019-12-07 20:40 ` kbuild test robot 1 sibling, 0 replies; 4+ messages in thread From: kbuild test robot @ 2019-12-07 20:40 UTC (permalink / raw) To: Aditya Pakki Cc: kbuild-all, pakki001, kjlu, Lad, Prabhakar, Mauro Carvalho Chehab, linux-media, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2827 bytes --] 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 [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 52253 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-12-08 9:44 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-06 1:00 [PATCH] media: davinci/vpfe_capture.c: Avoid BUG_ON for register failure Aditya Pakki 2019-12-06 9:20 ` Ezequiel Garcia 2019-12-08 9:44 ` Ezequiel Garcia 2019-12-07 20:40 ` kbuild test robot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®