Em Sat, 15 Sep 2012 12:22:48 -0300
Ezequiel Garcia <elezegarcia@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > Em Sat, 15 Sep 2012 10:05:40 -0300
> > Ezequiel Garcia <elezegarcia@xxxxxxxxx> escreveu:
> >
> >> On Sat, Sep 15, 2012 at 9:48 AM, Mauro Carvalho Chehab
> >> <mchehab@xxxxxxxxxx> 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@xxxxxxx> wrote:
> >> >>> On Sun, 26 Aug 2012 19:59:40 -0300
> >> >>> Ezequiel Garcia <elezegarcia@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
