On Wed, 8 Nov 2006, Hans Verkuil wrote:
> On Wednesday 08 November 2006 22:53, Robert Hardy wrote:
> > Here is an updated version of my patch using memcpy_toio.
> > I also trimmed a bunch of extra debugging that I had added.
> > This version also work fine for me here.
> >
> > http://webcon.ca/~rhardy/patches/ivtv-0.8.0_svn3507-fix_firmware_loadingV2.patch.bz2
> >
> > On Wed, 8 Nov 2006, Tyler Trafford wrote:
> > > > The third call to load_fw_direct, was passing data[2] which was
> > > > in theory seemed to be coming out of some decoder mailbox which,
> > > > given the results, can obviously have garbage in it... Hard
> > > > coding the third call too prevents this kind of problem.
> > >
> > > Ah, I was unaware of that- that is a real problem.
> >
> > Agreed :)
>
> For the record: the ivtv_api_getresult_nosleep() call sets data[2] to
> the maximum amount of data the MPEG decoder will accept. That however
> bears no relationship to the actual init-mpeg file. So this is a pure
> bug.
>
> Regarding the firmware load: from your tests it is clear that the size
> can sometimes be the size of an earlier firmware load. My question is:
> it is only the size or is it also the actual contents?
It's possible the seemingly random fw->size was a value from earlier
firmware's load but it wasn't always the immediately previous load. I've
seen no indication that it was loading anything but the correct firmware. I
would expect a lot of complaints in the log if the wrong file got loaded. I
suspect if that was happening it was being obscured by third firmware file
loading lockups.
> The reason I ask is that the two encoder and decoder firmwares are
> identical in size (you seem to be using an older encoder firmware that
> was actually padded with garbage bytes, on ivtvdriver.org the
> firmware.tar.gz contains a version that's just 256 kB).
Sure the older one had the end zero/null padded (I forget which) but ivtv
was only loading the first 256kB of it anyways... I've already switched to
the current files, it made no difference.
> Is it possible that instead of loading the decoder firmware you are
> actually loading the encoding firmware? So not only the fw->size is wrong,
> but the whole file is wrong. There is no way to actually detect that
> (except that you probably cannot find a decoder mailbox).
I've had plenty of random fw->size loads where the encoder and decoder
worked 100% in mythtv.
Since applying my patch and upgrading udev to 0.95 I haven't had any more
size/fw->size mismatches... I haven't tried removing the patch to see if it
dies but I think we both agree the third firmware load needs a proper size
argument.
> It's not clear from the discussion whether the problem is just fw->size
> or if that just tells you that the wrong firmware was loaded.
I'm fairly sure if I change the code back so it reads and writes 1MB of a
152KB file we will quickly see crashes again. Too tired tonight to get into
that.
It would be great if the driver could validate the firmware files before
loading them. It would be easy enough to check between those three specific
firmware files just from the first few bytes but not so simple to do it in a
generic manner (where you can change the firmware files.) The three firmware
files here start with: a7 03, a7 0d and ba 01 respectively.
Can you think of a generic way of doing this?
I suppose we could CRC/md5 the firmware files and check them against the table
posted in the wiki of known firmware files...
> I consider this to be quite a horrible kernel bug, BTW.
It's certainly a horrible bug. I'm suspecting it may be a udev bug which is
fixed in 0.95 as I don't think I've seen a size mismatch in dmesg since the
upgrade... Unfortunately many distributions still use antique udevs.
> Also, your patch is quite big for what basically amounts to:
>
> if (fw->size < size) {
> IVTV_ERR("invalid firmware size\n");
> return -EINVAL;
> }
> retval = size;
>
> The warning can be improved upon and some comments are needed inside
> the 'if', but otherwise that's all you need as far as I can tell.
I'm sure there are cleaner ways to implement it. Pick whatever you are
comfortable with. I've never claimed to be an expert, however that fragment
seems to have missed the important "pure bug" you agreed was present at the
start of your reply (related to the third call of load_fw_direct.) A fix for
the "pure bug" was the main point of my patch.
My code should work even if the old udev was in use, as the size for the
third load_fw_direct was hard coded to the correct size. The fragment above
would always abort unnecessarily (as it doesn't address the size=1MB for the
third firmware load_fw_direct call.)
Regards,
Rob
--
---------------------"Happiness is understanding."----------------------
Robert Hardy, B.Eng Computer Systems C.E.O. Webcon Inc.
rhardy <at> webcon <dot> ca GPG Key available (613) 276-7327
_______________________________________________
ivtv-devel mailing list
[email protected]
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
