| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4808E374.4050007@joow.be>
Date: Fri, 18 Apr 2008 20:07:48 +0200
From: Pieter Palmers <pieterp@...w.be>
To: Stefan Richter <stefanr@...6.in-berlin.de>
CC: linux1394-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH upate] iso transmit cycle skip patch
I've double-checked it, and it seems to be good. Hence:
Signed-off-by: Pieter Palmers <pieterp@...w.be>
Greets,
Pieter
Stefan Richter wrote:
> Date: Wed, 19 Mar 2008 22:10:59 +0100
> From: Pieter Palmers <pieterp@...w.be>
> Subject: ieee1394: rawiso: requeue packet for transmission after skipped cycle
>
> As it seems, some host controllers have issues that can cause them to
> skip cycles now and then when using large packets. I suspect that this
> is due to DMA not succeeding in time. If the transmit fifo can't contain
> more than one packet (big packets), the DMA should provide a new packet
> each cycle (125us). I am under the impression that my current PCI
> express test system can't guarantee this.
>
> In any case, the patch tries to provide a workaround as follows:
> The DMA program descriptors are modified such that when an error occurs,
> the DMA engine retries the descriptor the next cycle instead of
> stalling. This way no data is lost. The side effect of this is that
> packets are sent with one cycle delay. This however might not be that
> much of a problem for certain protocols (e.g. AM824). If they use
> padding packets for e.g. rate matching they can drop one of those to
> resync the streams.
>
> The amount of skips between two userspace wakeups is counted. This
> number is then propagated to userspace through the upper 16 bits of the
> 'dropped' parameter. This allows unmodified userspace applications due
> to the following:
> 1) libraw simply passes this dropped parameter to the user application
> 2) the meaning of the dropped parameter is: if it's nonzero, something
> bad has happened. The actual value of the parameter at this moment does
> not have a specific meaning.
>
> A libraw client can then retrieve the number of skipped cycles and
> account for them if needed.
> ---
>
> Update by Stefan R: Changed patch title (is it OK?) and whitespace.
>
> Pieter, please reply with a Signed-off-by line if the update is OK for
> commit. See linux/Documentation/SubmittingPatches for implications of
> the sign-off.
>
> drivers/ieee1394/iso.h | 2 ++
> drivers/ieee1394/ohci1394.c | 34 ++++++++++++++++++++++++++++++++++
> drivers/ieee1394/raw1394.c | 7 ++++++-
> 3 files changed, 42 insertions(+), 1 deletion(-)
>
> Index: linux/drivers/ieee1394/iso.h
> ===================================================================
> --- linux.orig/drivers/ieee1394/iso.h
> +++ linux/drivers/ieee1394/iso.h
> @@ -123,6 +123,8 @@ struct hpsb_iso {
>
> /* how many times the buffer has overflowed or underflowed */
> atomic_t overflows;
> + /* how many cycles were skipped for a given context */
> + atomic_t skips;
>
> /* Current number of bytes lost in discarded packets */
> int bytes_discarded;
> Index: linux/drivers/ieee1394/ohci1394.c
> ===================================================================
> --- linux.orig/drivers/ieee1394/ohci1394.c
> +++ linux/drivers/ieee1394/ohci1394.c
> @@ -1723,6 +1723,8 @@ struct ohci_iso_xmit {
> struct dma_prog_region prog;
> struct ohci1394_iso_tasklet task;
> int task_active;
> + int last_cycle;
> + atomic_t skips;
>
> u32 ContextControlSet;
> u32 ContextControlClear;
> @@ -1759,6 +1761,8 @@ static int ohci_iso_xmit_init(struct hps
> iso->hostdata = xmit;
> xmit->ohci = iso->host->hostdata;
> xmit->task_active = 0;
> + xmit->last_cycle = -1;
> + atomic_set(&iso->skips, 0);
>
> dma_prog_region_init(&xmit->prog);
>
> @@ -1856,6 +1860,26 @@ static void ohci_iso_xmit_task(unsigned
> /* parse cycle */
> cycle = le32_to_cpu(cmd->output_last.status) & 0x1FFF;
>
> + if (xmit->last_cycle > -1) {
> + int cycle_diff = cycle - xmit->last_cycle;
> + int skip;
> +
> + /* unwrap */
> + if (cycle_diff < 0) {
> + cycle_diff += 8000;
> + if (cycle_diff < 0)
> + PRINT(KERN_ERR, "bogus cycle diff %d\n",
> + cycle_diff);
> + }
> +
> + skip = cycle_diff - 1;
> + if (skip > 0) {
> + DBGMSG("skipped %d cycles without packet loss", skip);
> + atomic_add(skip, &iso->skips);
> + }
> + }
> + xmit->last_cycle = cycle;
> +
> /* tell the subsystem the packet has gone out */
> hpsb_iso_packet_sent(iso, cycle, event != 0x11);
>
> @@ -1943,6 +1967,16 @@ static int ohci_iso_xmit_queue(struct hp
> prev->output_last.branchAddress = cpu_to_le32(
> dma_prog_region_offset_to_bus(&xmit->prog, sizeof(struct iso_xmit_cmd) * next_i) | 3);
>
> + /*
> + * Link the skip address to this descriptor itself. This causes a
> + * context to skip a cycle whenever lost cycles or FIFO overruns occur,
> + * without dropping the data at that point the application should then
> + * decide whether this is an error condition or not. Some protocols
> + * can deal with this by dropping some rate-matching padding packets.
> + */
> + next->output_more_immediate.branchAddress =
> + prev->output_last.branchAddress;
> +
> /* disable interrupt, unless required by the IRQ interval */
> if (prev_i % iso->irq_interval) {
> prev->output_last.control &= cpu_to_le32(~(3 << 20)); /* no interrupt */
> Index: linux/drivers/ieee1394/raw1394.c
> ===================================================================
> --- linux.orig/drivers/ieee1394/raw1394.c
> +++ linux/drivers/ieee1394/raw1394.c
> @@ -2356,13 +2356,16 @@ static void rawiso_activity_cb(struct hp
> static void raw1394_iso_fill_status(struct hpsb_iso *iso,
> struct raw1394_iso_status *stat)
> {
> + int overflows = atomic_read(&iso->overflows);
> + int skips = atomic_read(&iso->skips);
> +
> stat->config.data_buf_size = iso->buf_size;
> stat->config.buf_packets = iso->buf_packets;
> stat->config.channel = iso->channel;
> stat->config.speed = iso->speed;
> stat->config.irq_interval = iso->irq_interval;
> stat->n_packets = hpsb_iso_n_ready(iso);
> - stat->overflows = atomic_read(&iso->overflows);
> + stat->overflows = ((skips & 0xFFFF) << 16) | ((overflows & 0xFFFF));
> stat->xmit_cycle = iso->xmit_cycle;
> }
>
> @@ -2437,6 +2440,8 @@ static int raw1394_iso_get_status(struct
>
> /* reset overflow counter */
> atomic_set(&iso->overflows, 0);
> + /* reset skip counter */
> + atomic_set(&iso->skips, 0);
>
> return 0;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists