RE: Magic in videobuf

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>Hans Verkuil wrote:
>>>Andy Walls wrote:
>>>On Mon, 2010-03-15 at 14:26 -0300, Mauro Carvalho Chehab wrote:
>>>> Hans Verkuil wrote:
>>>> >> Hans Verkuil wrote:
>>>> >>>> Pawel Osciak wrote:
>>>
>>>> >>>>> is anyone aware of any other uses for MAGIC_CHECK()s in videobuf
>>>> code
>>>> >>>>> besides driver debugging? I intend to remove them, as we weren't
>>>> able
>>>> >>>>> to find any particular use for them when we were discussing this
>>>> at
>>>> >>>>> the memory handling meeting in Norway...
>>>> >>>> It is a sort of paranoid check to avoid the risk of mass memory
>>>> >>>> corruption
>>>> >>>> if something goes deadly wrong with the video buffers.
>>>> >>>>
>>>
>>>> >>> What on earth does this magic check have to do with possible DMA
>>>> >>> overruns/memory corruption? This assumes that somehow exactly these
>>>> >>> magic
>>>> >>> fields are overwritten and that you didn't crash because of memory
>>>> >>> corruption elsewhere much earlier.
>>>
>>>>  All it does is oops anyway, so it really doesn't 'avoid' a crash
>>>> >>> (as if you could in such scenarios). And most likely the damage has
>>>> been
>>>> >>> done already in that case.
>>>> >> It won't avoid the damage, but the error message could potentially
>>>> help
>>>> >> to track the issue. It will also likely limit the damage.
>>>> >>
>>>> >>> Please let us get rid of this. It makes no sense whatsoever.
>>>> >> I don't have a strong opinion about this subject, but if this code
>>>> might
>>>> >> help
>>>> >> to avoid propagating the damage and to track the issue, I don't see
>>>> why we
>>>> >> need to remove it, especially since it is easy to disable the entire
>>>> logic
>>>> >> by just adding a few #if's to remove this code on environments where
>>>> no
>>>> >> problem is expected.
>>>> >
>>>> > It is highly unlikely that this code ever prevented these issues.
>>>> > Especially given the places where the check is done. I think this is
>>>> just
>>>> > debug code that has been dragged along for all these years without
>>>> anyone
>>>> > bothering to remove it.
>>>
>>>> I remember I had to re-format one disk, during that time, due to a
>>>> videobuf issue.
>>>> So, those checks help people that are touching at the videobuf code,
>>>> reducing the
>>>> chances of damaging their disk partitions when trying to implement
>>>> overlay mode and
>>>> userptr on the videobuf implementations that misses those features, or
>>>> when
>>>> working on a different mmap() logic at the driver.
>>>
>>>
>>>In a previous job, working on a particularly large application, I had
>>>occasional corruption in a shared memory segment that was shared by many
>>>writer processes and 2 readers.  A simple checksum on the data header
>>>(and contents if appropriate) was enough to detect corrpution and avoid
>>>dereferencing a corrupted pointer to the next data element (when walking
>>>a data area filled with Key-Length-Value encoded data).
>>>
>>>This "forward error detection" was inelegant to me - kind of like
>>>putting armor on one's car instead of learning to drive properly.  I
>>>only resorted to using the checksum because there was almost no way to
>>>find which process was corrupting shared memory in a reasonable amount
>>>of time.  It allowed me to change a "show stopper" bug into an annoying
>>>data presentation bug, so the product could be released to a production
>>>environment.
>>>
>>>In a development environment, it would be much better to disable such
>>>defensive coding and let the kernel Oops.  You'll never find the
>>>problems if you keep hiding them from yourself.
>>
>> So, to sum up (I hope I understood you guys correctly):
>>
>> we are not seeing any particular reason (besides debugging) for having
>> the checks in videobuf-core. Checks in memory-specific handling may have
>> some
>> uses, although I am not sure how much. I am not an expert on sg drivers,
>> but as
>> the magics are in the kernel control structures, they are not really a
>> subject
>> to corruption. What may get corrupted is video data or sg lists, but the
>> magics
>> are in a separate memory areas anyway. So videobuf-core magics should be
>> removed
>> and we are leaning towards removing memory-type magics as well?
>
>That is my opinion, yes. However, there is one case where this is actually
>useful. Take for example the function videobuf_to_dma in
>videobuf-dma-sg.c. This is called by drivers and it makes sense that that
>function should double-check that the videobuf_buffer is associated with
>the dma_sg memtype.
>
>But calling this 'magic' is a poor choice of name. There is nothing magic
>about it, in this case it is just an identifier of the memtype. And there
>may be better ways to do this check anyway.
>
>I have not done any analysis, but might be enough to check whether the
>int_ops field of videobuf_queue equals the sg_ops pointer. If so, then the
>whole magic handling can go away in this case.


Well... I see this discussion is dragging on a bit.
I will not be touching magics for now then, at least not until we arrive at
a consensus sometime in the future.


Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center



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

[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux