>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
