[PATCH] firewire:queue the right number of data - Kernel

This is a discussion on [PATCH] firewire:queue the right number of data - Kernel ; Hi, There will be 4 padding bytes in struct fw_cdev_event_response on some platforms The member:__u32 data will point to these padding bytes. While queue the response and data in complete_transaction in fw-cdev.c, it will queue like this: |response(excluding padding bytes)|4 ...

+ Reply to Thread
Results 1 to 3 of 3

Thread: [PATCH] firewire:queue the right number of data

  1. [PATCH] firewire:queue the right number of data

    Hi,

    There will be 4 padding bytes in struct fw_cdev_event_response on some platforms
    The member:__u32 data will point to these padding bytes. While queue the
    response and data in complete_transaction in fw-cdev.c, it will queue like this:
    |response(excluding padding bytes)|4 padding bytes|4 padding bytes|data.
    It queue 4 extra bytes. That is to say it use "&response + sizeof(response)"
    while other place of kernel and userspace library use "&response + offsetof
    (typeof(response), data)". So it will lost the last 4 bytes of data.This patch
    can fix it while not changing the struct definition.

    Sorry for open a new ticket.

    Signed-off-by: JiSheng Zhang

    --- old/drivers/firewire/fw-cdev.c
    +++ new/drivers/firewire/fw-cdev.c
    @@ -382,9 +382,9 @@

    response->response.type = FW_CDEV_EVENT_RESPONSE;
    response->response.rcode = rcode;
    - queue_event(client, &response->event,
    - &response->response, sizeof(response->response),
    - response->response.data, response->response.length);
    + queue_event(client, &response->event, &response->response,
    + sizeof(response->response) + response->response.length,
    + NULL, 0);
    }

    static int ioctl_send_request(struct client *client, void *buffer)
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] firewire:queue the right number of data

    JiSheng Zhang wrote at LKML:
    > Hi,
    >
    > There will be 4 padding bytes in struct fw_cdev_event_response on some platforms
    > The member:__u32 data will point to these padding bytes. While queue the
    > response and data in complete_transaction in fw-cdev.c, it will queue like this:
    > |response(excluding padding bytes)|4 padding bytes|4 padding bytes|data.
    > It queue 4 extra bytes. That is to say it use "&response + sizeof(response)"
    > while other place of kernel and userspace library use "&response + offsetof
    > (typeof(response), data)". So it will lost the last 4 bytes of data.This patch
    > can fix it while not changing the struct definition.
    >
    > Sorry for open a new ticket.
    >
    > Signed-off-by: JiSheng Zhang
    >
    > --- old/drivers/firewire/fw-cdev.c
    > +++ new/drivers/firewire/fw-cdev.c
    > @@ -382,9 +382,9 @@
    >
    > response->response.type = FW_CDEV_EVENT_RESPONSE;
    > response->response.rcode = rcode;
    > - queue_event(client, &response->event,
    > - &response->response, sizeof(response->response),
    > - response->response.data, response->response.length);
    > + queue_event(client, &response->event, &response->response,
    > + sizeof(response->response) + response->response.length,
    > + NULL, 0);
    > }
    >
    > static int ioctl_send_request(struct client *client, void *buffer)


    I tested it now on i686, x86-64, and x86-64 with i686 userland, using
    firecontrol and gscanbus. As discussed, they got corrupted block read
    responses on x86-64 and on x86-64 with i686 userland. The patch fixes this.

    I committed it to linux1394-2.6.git#fixes and intend to send it upstream
    at the end of the week or so. Thanks for spotting this bug.

    One point about which I am not sure about yet is what happens if there
    are multiple events queued up before the client can read() them. The
    tests which I did so far involved only a single event queued and
    dequeued at a time.


    PS:
    I removed a rule from linux1394-devel's header filters which matched
    your previous posts. (Message has priority, but no X-Mailer/User-Agent)
    --
    Stefan Richter
    -=====-==--- -=== =-=--
    http://arcgraph.de/sr/
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] firewire:queue the right number of data

    on Sun, 20 Jul 2008 16:00:32 +0200
    Stefan Richter wrote:

    > JiSheng Zhang wrote at LKML:
    > > Hi,
    > >
    > > There will be 4 padding bytes in struct fw_cdev_event_response on some platforms
    > > The member:__u32 data will point to these padding bytes. While queue the
    > > response and data in complete_transaction in fw-cdev.c, it will queue like this:
    > > |response(excluding padding bytes)|4 padding bytes|4 padding bytes|data.
    > > It queue 4 extra bytes. That is to say it use "&response + sizeof(response)"
    > > while other place of kernel and userspace library use "&response + offsetof
    > > (typeof(response), data)". So it will lost the last 4 bytes of data.This patch
    > > can fix it while not changing the struct definition.
    > >
    > > Sorry for open a new ticket.
    > >
    > > Signed-off-by: JiSheng Zhang
    > >
    > > --- old/drivers/firewire/fw-cdev.c
    > > +++ new/drivers/firewire/fw-cdev.c
    > > @@ -382,9 +382,9 @@
    > >
    > > response->response.type = FW_CDEV_EVENT_RESPONSE;
    > > response->response.rcode = rcode;
    > > - queue_event(client, &response->event,
    > > - &response->response, sizeof(response->response),
    > > - response->response.data, response->response.length);
    > > + queue_event(client, &response->event, &response->response,
    > > + sizeof(response->response) + response->response.length,
    > > + NULL, 0);
    > > }
    > >
    > > static int ioctl_send_request(struct client *client, void *buffer)

    >
    > I tested it now on i686, x86-64, and x86-64 with i686 userland, using
    > firecontrol and gscanbus. As discussed, they got corrupted block read
    > responses on x86-64 and on x86-64 with i686 userland. The patch fixes this.
    >
    > I committed it to linux1394-2.6.git#fixes and intend to send it upstream
    > at the end of the week or so. Thanks for spotting this bug.

    Thanks for committing.
    >
    > One point about which I am not sure about yet is what happens if there
    > are multiple events queued up before the client can read() them. The
    > tests which I did so far involved only a single event queued and
    > dequeued at a time.

    IMHO, even there are multiple events queued, it should work OK because
    events are put into event_list rather than a ring buffer, there is no
    padding bytes problem between events.
    >
    >
    > PS:
    > I removed a rule from linux1394-devel's header filters which matched
    > your previous posts. (Message has priority, but no X-Mailer/User-Agent)

    Thanks very much

    Regards,
    JiSheng
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread