drivers/net/enic/vnic_cq.c

drivers/net/enic/vnic_cq.c

[Andrew Morton]

i386 allmodconfig, all trees applied:

drivers/net/enic/vnic_cq.c: In function 'vnic_cq_init':
drivers/net/enic/vnic_cq.c:65: error: implicit declaration of function 'writeq'

I can't immediately find an i386 implementation of writeq.

Re: drivers/net/enic/vnic_cq.c

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Thu, 9 Oct 2008 21:12:17 -0700

> 
> i386 allmodconfig, all trees applied:
> 
> drivers/net/enic/vnic_cq.c: In function 'vnic_cq_init':
> drivers/net/enic/vnic_cq.c:65: error: implicit declaration of function 'writeq'
> 
> I can't immediately find an i386 implementation of writeq.

There isn't, because only a non-atomic implementation (two writew's)
is possible.

So what ends up happening is that every driver that wants readq and
writeq does this ifdef dance:

#ifndef readq
static u64 readq(void __iomem *reg)
{
	return (((u64)readl(reg + 0x4UL) << 32) |
		(u64)readl(reg));
}

static void writeq(u64 val, void __iomem *reg)
{
	writel(val & 0xffffffff, reg);
	writel(val >> 32, reg + 0x4UL);
}
#endif

basically stating that they explicitly understand that these are
non-atomic and that the driver can handle it.

But this is completely stupid.  Instead of putting this in every driver
we should put it in the 32-bit asm/io.h files and guard it with
some ifdef test, on a macro that the driver can define.

Re: drivers/net/enic/vnic_cq.c

[Andrew Morton]

On Thu, 09 Oct 2008 21:15:52 -0700 (PDT) David Miller <davem@davemloft.net> wrote:

> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Thu, 9 Oct 2008 21:12:17 -0700
> 
> > 
> > i386 allmodconfig, all trees applied:
> > 
> > drivers/net/enic/vnic_cq.c: In function 'vnic_cq_init':
> > drivers/net/enic/vnic_cq.c:65: error: implicit declaration of function 'writeq'
> > 
> > I can't immediately find an i386 implementation of writeq.
> 
> There isn't, because only a non-atomic implementation (two writew's)
> is possible.
> 
> So what ends up happening is that every driver that wants readq and
> writeq does this ifdef dance:
> 
> #ifndef readq
> static u64 readq(void __iomem *reg)
> {
> 	return (((u64)readl(reg + 0x4UL) << 32) |
> 		(u64)readl(reg));
> }
> 
> static void writeq(u64 val, void __iomem *reg)
> {
> 	writel(val & 0xffffffff, reg);
> 	writel(val >> 32, reg + 0x4UL);
> }
> #endif
> 
> basically stating that they explicitly understand that these are
> non-atomic and that the driver can handle it.
> 
> But this is completely stupid.  Instead of putting this in every driver
> we should put it in the 32-bit asm/io.h files and guard it with
> some ifdef test, on a macro that the driver can define.

Well OK.



We have a driver which has never been compileable on i386 which has
slipped through at least three sets of fingers only to land on the
usual guy's head.

Someone should write a book about this or something.

Re: drivers/net/enic/vnic_cq.c

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Thu, 9 Oct 2008 21:27:55 -0700

> We have a driver which has never been compileable on i386 which has
> slipped through at least three sets of fingers only to land on the
> usual guy's head.

The person who submitted the driver compiled it on his box,
I made sure to hit sparc64 to try and hit the whacky cases,
and I'm only doing these driver merges because Jeff Garzik
is out of town so give me a break :-)))

Re: drivers/net/enic/vnic_cq.c

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Thu, 09 Oct 2008 21:54:52 -0700 (PDT)

> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Thu, 9 Oct 2008 21:27:55 -0700
> 
> > We have a driver which has never been compileable on i386 which has
> > slipped through at least three sets of fingers only to land on the
> > usual guy's head.
> 
> The person who submitted the driver compiled it on his box,
> I made sure to hit sparc64 to try and hit the whacky cases,
> and I'm only doing these driver merges because Jeff Garzik
> is out of town so give me a break :-)))

Anyways, in case it isn't clear I'll get this fixed since it's
coming from my tree.

Re: drivers/net/enic/vnic_cq.c

[David Miller]

I just pushed the following into net-next-2.6:

enic: Attempt to fix build in 32-bit such as i386.

Such platforms lack readq/writeq but this driver want to call them.

Noticed by Andrew Morton.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 drivers/net/enic/vnic_dev.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/net/enic/vnic_dev.h b/drivers/net/enic/vnic_dev.h
index 2dcffd3..b9dc182 100644
--- a/drivers/net/enic/vnic_dev.h
+++ b/drivers/net/enic/vnic_dev.h
@@ -27,6 +27,20 @@
 #define VNIC_PADDR_TARGET	0x0000000000000000ULL
 #endif
 
+#ifndef readq
+static inline u64 readq(void __iomem *reg)
+{
+	return (((u64)readl(reg + 0x4UL) << 32) |
+		(u64)readl(reg));
+}
+
+static inline void writeq(u64 val, void __iomem *reg)
+{
+	writel(val & 0xffffffff, reg);
+	writel(val >> 32, reg + 0x4UL);
+}
+#endif
+
 enum vnic_dev_intr_mode {
 	VNIC_DEV_INTR_MODE_UNKNOWN,
 	VNIC_DEV_INTR_MODE_INTX,
-- 
1.5.6.5

Re: drivers/net/enic/vnic_cq.c

[Roland Dreier]

cc'ing Scott so we can make sure that this actually is atomic enough to
work with the enic hardware... (Scott, the context is that enic won't
build on any architecture that doesn't define writeq and readq, such as
32-bit x86; however the definitions below make it possible that multiple
32-bit writes will be interleaved, eg if an interrupt occurs between the
first writel and the second writel)

 > I just pushed the following into net-next-2.6:
 > 
 > enic: Attempt to fix build in 32-bit such as i386.
 > 
 > Such platforms lack readq/writeq but this driver want to call them.
 > 
 > Noticed by Andrew Morton.
 > 
 > Signed-off-by: David S. Miller <davem@davemloft.net>
 > ---
 >  drivers/net/enic/vnic_dev.h |   14 ++++++++++++++
 >  1 files changed, 14 insertions(+), 0 deletions(-)
 > 
 > diff --git a/drivers/net/enic/vnic_dev.h b/drivers/net/enic/vnic_dev.h
 > index 2dcffd3..b9dc182 100644
 > --- a/drivers/net/enic/vnic_dev.h
 > +++ b/drivers/net/enic/vnic_dev.h
 > @@ -27,6 +27,20 @@
 >  #define VNIC_PADDR_TARGET	0x0000000000000000ULL
 >  #endif
 >  
 > +#ifndef readq
 > +static inline u64 readq(void __iomem *reg)
 > +{
 > +	return (((u64)readl(reg + 0x4UL) << 32) |
 > +		(u64)readl(reg));
 > +}
 > +
 > +static inline void writeq(u64 val, void __iomem *reg)
 > +{
 > +	writel(val & 0xffffffff, reg);
 > +	writel(val >> 32, reg + 0x4UL);
 > +}
 > +#endif
 > +
 >  enum vnic_dev_intr_mode {
 >  	VNIC_DEV_INTR_MODE_UNKNOWN,
 >  	VNIC_DEV_INTR_MODE_INTX,
 > -- 
 > 1.5.6.5
 > 
 > --
 > To unsubscribe from this list: send the line "unsubscribe netdev" in
 > the body of a message to majordomo@vger.kernel.org
 > More majordomo info at  http://vger.kernel.org/majordomo-info.html
 > 

Re: drivers/net/enic/vnic_cq.c

[Scott Feldman]

On 10/10/08 10:10 AM, "Roland Dreier" <rdreier@cisco.com> wrote:

> cc'ing Scott so we can make sure that this actually is atomic enough to
> work with the enic hardware... (Scott, the context is that enic won't
> build on any architecture that doesn't define writeq and readq, such as
> 32-bit x86; however the definitions below make it possible that multiple
> 32-bit writes will be interleaved, eg if an interrupt occurs between the
> first writel and the second writel)

Yes, enic hw provides atomic read/write for 64-bit regs even if register is
accessed with 32-bit read/writes.

-scott

Re: drivers/net/enic/vnic_cq.c

[David Miller]

From: Scott Feldman <sfeldma@nuovasystems.com>
Date: Fri, 10 Oct 2008 11:29:23 -0700

> On 10/10/08 10:10 AM, "Roland Dreier" <rdreier@cisco.com> wrote:
> 
> > cc'ing Scott so we can make sure that this actually is atomic enough to
> > work with the enic hardware... (Scott, the context is that enic won't
> > build on any architecture that doesn't define writeq and readq, such as
> > 32-bit x86; however the definitions below make it possible that multiple
> > 32-bit writes will be interleaved, eg if an interrupt occurs between the
> > first writel and the second writel)
> 
> Yes, enic hw provides atomic read/write for 64-bit regs even if register is
> accessed with 32-bit read/writes.

Sure, and this is why 32-bit arch's don't provide readq/writeq implementations,
things are much more subtle here.

The hardware may allow 2 32-bit writes to a 64-bit register, but...

Are there potential problems when the register is half-way updated?

For example, consider a ring index where the upper and lower 32-bits
are actually significant.  If you change the top part and then the
bottom part, in the intermediate step there is an invalid state and
the card might try to access an invalid ring index.

Then also, of course, the driver itself has to make sure it does
enough locking to make sure a partial 64-bit update isn't interrupted
by a parallel one on another cpu to the same register but I'll assume
the driver takes care of that here :-)

Re: drivers/net/enic/vnic_cq.c

[Scott Feldman]

On 10/10/08 11:58 AM, "David Miller" <davem@davemloft.net> wrote:

> From: Scott Feldman <sfeldma@nuovasystems.com>
> Date: Fri, 10 Oct 2008 11:29:23 -0700
>> Yes, enic hw provides atomic read/write for 64-bit regs even if register is
>> accessed with 32-bit read/writes.
> 
> Sure, and this is why 32-bit arch's don't provide readq/writeq
> implementations,
> things are much more subtle here.
> 
> The hardware may allow 2 32-bit writes to a 64-bit register, but...
> 
> Are there potential problems when the register is half-way updated?
> 
> For example, consider a ring index where the upper and lower 32-bits
> are actually significant.  If you change the top part and then the
> bottom part, in the intermediate step there is an invalid state and
> the card might try to access an invalid ring index.

Yes, I'd like to retract my original remark.  :(  There is a mechanism for
atomic access to a wide register, but it's not enabled for the register
cases in question, and even if it was, an intermediate access to another
address would invalidate the mechanism, as you point out.  So...
 
> Then also, of course, the driver itself has to make sure it does
> enough locking to make sure a partial 64-bit update isn't interrupted
> by a parallel one on another cpu to the same register but I'll assume
> the driver takes care of that here :-)

...There is enough locking.

-scott

Re: drivers/net/enic/vnic_cq.c

[Andrew Morton]

On Thu, 09 Oct 2008 21:54:52 -0700 (PDT) David Miller <davem@davemloft.net> wrote:

> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Thu, 9 Oct 2008 21:27:55 -0700
> 
> > We have a driver which has never been compileable on i386 which has
> > slipped through at least three sets of fingers only to land on the
> > usual guy's head.
> 
> The person who submitted the driver compiled it on his box,
> I made sure to hit sparc64 to try and hit the whacky cases,
> and I'm only doing these driver merges because Jeff Garzik
> is out of town so give me a break :-)))

yup.

Thing is, since 10AM this morning I've looked at no mailing list
traffic, reviewed no patches, merged no patches.  I've spent half the
day getting all the trees merged (and I last did it 24 hours ago) and
the other half weeding out compilation errors.  I have this starry-eyed
theory that I shouldn't need to do any of this.

The stephen/me function is a single point of failure and right now,
it has failed.  I have to push back.

Re: drivers/net/enic/vnic_cq.c

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Thu, 9 Oct 2008 22:16:47 -0700

> The stephen/me function is a single point of failure and right now,
> it has failed.  I have to push back.

It seems to go perfectly smooth when I interact with Stephen on these
kinds of situations.

He just says "this broke/conflicts, please fix it", I do exactly that
and we move on.  Rinse, repeat.

And I don't think there is any kind of problem with that.  We have
multiple levels of checks, and we end up catching it %99.999 of
the time before it hits Linus's tree and we fixup whatever slips
through.

Otherwise we could skip the whole -rc thing :-)