lkml.org 
[lkml]   [2008]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv2 04/28] cx25840: treat firmware data as const
    On Sun, 25 May 2008, Hans Verkuil wrote:

    > Hi David,
    >
    > There is no need to do the first two bytes first. As far as I can tell
    > it was done to avoid having to allocate a local buffer. The initial
    > version of this code was writing much larger blocks at a time to the
    > i2c bus, so having to setup a large buffer on the stack was
    > undesirable. The pvrusb driver requires a much smaller buffer (FWSEND),
    > but the code was never changed. Now, however, the initial four-byte
    > write is just in the way and can be removed in favor of always writing
    > FWSEND-sized blocks of data.
    >
    > I've CC-ed Mike Isely, the pvrusb2 maintainer, just in case there is an
    > outlandish restriction I'm not aware of on that USB device requiring
    > the firmware load to be done this way.
    >

    The pvrusb2 driver's FWSEND limit is an artifact of the hardware and
    cannot be changed. HOWEVER, in the standalone driver (not in the
    kernel) I have a hack implemented that can monitor and intercept such
    I2C operations and break them into smaller I2C transactions. I
    implemented this to allow the standalone driver to function correctly on
    some older kernels where FWSEND is too large. I can enable that hack
    for the kernel driver and thus lift the limit. But without it, FWSEND
    absolutely has to be small enough that the entire transaction (including
    a few bytes of transport overhead) can fit within a 64 byte URB.

    I see that this change allocates a local buffer on the stack (sized by
    FWSEND). Though the buffer is small (48 bytes, see above paragraph), it
    is controlled by a macro and thus leaves open the possibility that it
    could be accidentally increased to a point where the stack gets
    overflowed. Seems like not a good idea, but either way it should not
    affect the pvrusb2 driver.

    Acked-By: Mike Isely <isely@pobox.com>

    -Mike


    > Regards,
    >
    > Hans
    >
    > On Sunday 25 May 2008 12:23:33 David Woodhouse wrote:
    > > Signed-off-by: David Woodhouse <dwmw2@infradead.org>
    > > ---
    > > drivers/media/video/cx25840/cx25840-firmware.c | 21
    > > +++++++++++++-------- 1 files changed, 13 insertions(+), 8
    > > deletions(-)
    > >
    > > diff --git a/drivers/media/video/cx25840/cx25840-firmware.c
    > > b/drivers/media/video/cx25840/cx25840-firmware.c index
    > > 620d295..5c08ede 100644
    > > --- a/drivers/media/video/cx25840/cx25840-firmware.c
    > > +++ b/drivers/media/video/cx25840/cx25840-firmware.c
    > > @@ -79,7 +79,7 @@ static int check_fw_load(struct i2c_client *client,
    > > int size) return 0;
    > > }
    > >
    > > -static int fw_write(struct i2c_client *client, u8 *data, int size)
    > > +static int fw_write(struct i2c_client *client, const u8 *data, int
    > > size) {
    > > if (i2c_master_send(client, data, size) < size) {
    > > v4l_err(client, "firmware load i2c failure\n");
    > > @@ -93,7 +93,8 @@ int cx25840_loadfw(struct i2c_client *client)
    > > {
    > > struct cx25840_state *state = i2c_get_clientdata(client);
    > > const struct firmware *fw = NULL;
    > > - u8 buffer[4], *ptr;
    > > + u8 buffer[FWSEND];
    > > + const u8 *ptr;
    > > int size, retval;
    > >
    > > if (state->is_cx23885)
    > > @@ -108,6 +109,8 @@ int cx25840_loadfw(struct i2c_client *client)
    > >
    > > buffer[0] = 0x08;
    > > buffer[1] = 0x02;
    > > +
    > > + /* Do we really need to do the first two bytes first? */
    > > buffer[2] = fw->data[0];
    > > buffer[3] = fw->data[1];
    > > retval = fw_write(client, buffer, 4);
    > > @@ -118,19 +121,21 @@ int cx25840_loadfw(struct i2c_client *client)
    > > }
    > >
    > > size = fw->size - 2;
    > > - ptr = fw->data;
    > > + ptr = fw->data + 2;
    > > while (size > 0) {
    > > - ptr[0] = 0x08;
    > > - ptr[1] = 0x02;
    > > - retval = fw_write(client, ptr, min(FWSEND, size + 2));
    > > + int len = min(FWSEND - 2, size);
    > > +
    > > + memcpy(buffer + 2, ptr, len);
    > > +
    > > + retval = fw_write(client, ptr, len + 2);
    > >
    > > if (retval < 0) {
    > > release_firmware(fw);
    > > return retval;
    > > }
    > >
    > > - size -= FWSEND - 2;
    > > - ptr += FWSEND - 2;
    > > + size -= len;
    > > + ptr += len;
    > > }
    > >
    > > end_fw_load(client);
    >
    >
    >

    --

    Mike Isely
    isely @ pobox (dot) com
    PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8


    \
     
     \ /
      Last update: 2008-05-25 17:23    [W:0.028 / U:0.548 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site