RE: Magic in videobuf

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

 



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


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