>>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.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
--
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
