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);
--
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
