Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em Sat, 15 Sep 2012 12:22:48 -0300
Ezequiel Garcia <elezegarcia@gmail.com> escreveu:

> Mauro,
> 
> (Cc got messed up, so I'm sending this to you and media list)
> 
> On Sat, Sep 15, 2012 at 10:37 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
> > Em Sat, 15 Sep 2012 10:05:40 -0300
> > Ezequiel Garcia <elezegarcia@gmail.com> escreveu:
> >
> >> On Sat, Sep 15, 2012 at 9:48 AM, Mauro Carvalho Chehab
> >> <mchehab@redhat.com> wrote:
> >> > Em 28-08-2012 14:23, Ezequiel Garcia escreveu:
> >> >> Hi Jon,
> >> >>
> >> >> Thanks for your answers, I really appreciate it.
> >> >>
> >> >> On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote:
> >> >>> On Sun, 26 Aug 2012 19:59:40 -0300
> >> >>> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> >> >>>
> >> >>>> 1.
> >> >>>> Why do we need to check for all these conditions in the first place?
> >> >>>> There are many other functions relying on "struct vb2_queue *q"
> >> >>>> not being null (almost all of them) and we don't check for it.
> >> >>>> What makes vb2_queue_init() so special that we need to check for it?
> >> >>>
> >> >>> There are plenty of developers who would argue for the removal of the
> >> >>> BUG_ON(!q) line regardless, since the kernel will quickly crash shortly
> >> >>> thereafter.  I'm a bit less convinced; there are attackers who are very
> >> >>> good at exploiting null pointer dereferences, and some systems still allow
> >> >>> the low part of the address space to be mapped.
> >> >>>
> >> >>> In general, IMO, checks for consistency make sense; it's nice if the
> >> >>> kernel can *tell* you that something is wrong.
> >> >>>
> >> >>> What's a mistake is the BUG_ON; that should really only be used in places
> >> >>> where things simply cannot continue.  In this case, the initialization can
> >> >>> be failed, the V4L2 device will likely be unavailable, but everything else
> >> >>> can continue as normal.  -EINVAL is the right response here.
> >> >>>
> >> >>
> >> >> I see your point.
> >> >>
> >> >> What I really can't seem to understand is why we should have a check
> >> >> at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one.
> >> >
> >> > Those BUG_ON() checks are there since likely the first version of VB1.
> >> > VB2 just inherited it.
> >> >
> >> > The think is that letting the VB code to run without checking for some
> >> > conditions is evil, as it could cause mass memory corruption, as the
> >> > videobuf code writes on a large amount of memory (typically, something
> >> > like 512MB written on every 1/30s). So, the code has protections, in order
> >> > to try avoiding it. Even so, with VB1, when the output buffer is at the
> >> > video adapter memory region (what is called PCI2PCI memory transfers),
> >> > there are known bugs with some chipsets that will cause mass data corruption
> >> > at the hard disks (as the PCI2PCI transfers interfere at the data transfers
> >> > from/to the disk, due to hardware bugs).
> >> >
> >> > Calling WARN_ON_ONCE() and returning some error code works, provided that
> >> > we enforce that the error code will be handled at the drivers that call
> >> > vb2_queue_init(), using something like __attribute__((warn_unused_result, nonnull))
> >> > and double_checking the code at VB2 callers.
> >> >
> >>
> >> So, you want me to resend?
> >
> > Yes, please.
> >
> 
> I can't decide on coding style. So, how about this?:
> 
> (copy pasted on gmail, sorry if spaces are mangled):
> 
>   */
>  int vb2_queue_init(struct vb2_queue *q)
>  {
> -       BUG_ON(!q);
> -       BUG_ON(!q->ops);
> -       BUG_ON(!q->mem_ops);
> -       BUG_ON(!q->type);
> -       BUG_ON(!q->io_modes);
> -
> -       BUG_ON(!q->ops->queue_setup);
> -       BUG_ON(!q->ops->buf_queue);
> +       /*
> +        * Sanity check
> +        */
> +       if (WARN_ON_ONCE(!q)            ||
> +           WARN_ON_ONCE(!q->ops)       ||
> +           WARN_ON_ONCE(!q->mem_ops)   ||
> +           WARN_ON_ONCE(!q->type)      ||
> +           WARN_ON_ONCE(!q->io_modes)  ||
> +           WARN_ON_ONCE(!q->ops->queue_setup) ||
> +           WARN_ON_ONCE(!q->ops->buf_queue))
> +               return -EINVAL;
> 
>         INIT_LIST_HEAD(&q->queued_list);
>         INIT_LIST_HEAD(&q->done_list);

It seems OK on my eyes.

-- 
Regards,
Mauro