>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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
