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@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx>
>
> Thanks,
> Ezequiel
>
