linux-kernel - Re: [PATCH 2/2] DMAENGINE: driver for the ARM PL080/PL081 PrimeCells

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 6 Aug 2010 10:54:50 -0700
From:	Dan Williams <dan.j.williams@...el.com>
To:	Linus Walleij <linus.ml.walleij@...il.com>
Cc:	Alessandro Rubini <rubini@...pv.it>, peter.pearse@....com,
	viresh.kumar@...com, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, yuanyabin1978@...a.com,
	ben-linux@...ff.org, kgene.kim@...sung.com, rmk@....linux.org.uk
Subject: Re: [PATCH 2/2] DMAENGINE: driver for the ARM PL080/PL081 PrimeCells

On Thu, Aug 5, 2010 at 12:30 AM, Linus Walleij
<linus.ml.walleij@...il.com> wrote:
> 2010/8/5 Alessandro Rubini <rubini@...pv.it>:
>
>>> Peter, Alessandro and Viresh especially: you will hopefully use this
>>> driver for ARM refboards, Nomadik and SPEAr, can you provide
>>> an Acked-by:?
>>
>> I'm sorry I can't test it these times, I must delay stuff to
>> the last week of August.
>
> That's be a Tested-by: tag, Acked-by: was more about whether you
> think it looks sane I believe, see Documentation/SubmittingPatches.

We have the Acked-by from Viresh, Alessandro looks like he is
interested in trying it out, the driver is self contained, marked
experimental, and you have a track-record of fixing things up, so I'll
include it with the idea it will be easier to fixup in-tree.  One
thing that should be on the todo list is to review the usage of memory
barriers.  I don't think they are serving the purpose that you think
they are.  For example in  pl08x_irq() there is:

+       /*
+        * Clear only the terminal interrupts on channels we processed
+        */
+       writel(mask, pl08x->base + PL080_TC_CLEAR);
+       mb();
+
+       return mask ? IRQ_HANDLED : IRQ_NONE;

Which seems to imply you want to ensure the interrupt bits are cleared
before the handler returns.  But the only way you can guarantee that
the write has actually taken effect is to read back the register.
Does the amba bus interface make this guarantee with a barrier?  If so
then we need a comment about why we need a full barrier versus just a
wmb()?

+static void pl08x_pause_phy_chan(struct pl08x_phy_chan *ch)
+{
+       u32 val;
+
+       /* Set the HALT bit and wait for the FIFO to drain */
+       val = readl(ch->base + PL080_CH_CONFIG);
+       val |= PL080_CONFIG_HALT;
+       writel(val, ch->base + PL080_CH_CONFIG);
+
+       /* Wait for channel inactive */
+       while (pl08x_phy_channel_busy(ch))
+               ;
+       mb();
+}

Doesn't the while loop guarantee that the write has taken effect?

+static void pl08x_ensure_on(struct pl08x_driver_data *pl08x)
+{
+       u32 val;
+
+       val = readl(pl08x->base + PL080_CONFIG);
+       val &= ~(PL080_CONFIG_M2_BE | PL080_CONFIG_M1_BE | PL080_CONFIG_ENABLE);
+       /* We implictly clear bit 1 and that means little-endian mode */
+       val |= PL080_CONFIG_ENABLE;
+       mb();
+       writel(val, pl08x->base + PL080_CONFIG);
+       mb();
+}

This almost looks like you are using mb() as a compiler barrier, but
that is taken care of by readl / writel.  Speaking of which is amba
always little-endian, should we instead be using __raw_{read|write}l
in this driver?

--
Dan
--
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/