Re: [PATCH 2.6] natsemi.c NAPI

[resend, went to wrong list address]

Manfred, thanks for your comments.

[explicit Cc to jamal becuase of NAPI-related stuff below]

On Sat, Sep 25, 2004 at 10:39:29PM +0200, Manfred Spraul wrote:

quoted

+config NATSEMI_NAPI
+	bool "Use Rx Polling (NAPI) (EXPERIMENTAL)"

I'm not a big fan on config options for drivers. I'd prefer a runtime 
option.

I see.

quoted

TODO:
* big endian support with CFG:BEM instead of cpu_to_le32
* support for an external PHY
-	* NAPI

Hmm. Actually all TODO points are done: CFG:BEM is unusable because it 
swaps data words.
External PHYs are supported. You are closing the last point.

Ok, I'll remove all three with my next patch

Additional registers. You didn't mention that in the changelog. 
Changelogs should mention every change, please do not piggypack 
unrelated changes.

ack.

quoted

+	long ioaddr = dev->base_addr;
+	int boguscnt = max_interrupt_work;
+

This doesn't compile: you've removed max_interrupt_work.

mh, apparently I've sent an outdated patch then.  I'll repost an updated
version once I know what the solution for IntrStatus is

quoted

+	/* We cannot read IntrStatus since this would acknowledge
+	 * all interrupt sources. Thus we just blindly assume that
+	 * the interrupt really was for us -HW! */

Huge problem - this is not acceptable. It means the driver is unusable 
for shared interrupts. We must find a solution.

yes, I know :(  Luckily on my embedded boards, there are no shared
interrupts - but this is obviously a special case.

I think this overall problem can be solved if there was some per-device
variable that saves the IntrStatus until the NAPI callback gets
scheduled. What do you think?  This wouldn't even need some locking,
since interrupts would be disabled before the field is updated, and not
re-enabled before the field is read by the NAPI callback?

I was surprised that this solution is not suggested in the NAPI-HOWTO.txt, so I though there must be an error in my proposal...

By using such a scheme, isn't it also possible to only offload RX into
the NAPI callback with clear-on-read devices?

IRQ_NONE would be definitively wrong: The kernel disables interrupt 
sources if it can't find a handler. If our handler returns IRQ_NONE and 
the irq is not shared, then the kernel will disable the irq after a 
short while.

ok.

Also missing in the changelog.

is related to the register definitions above.

   Manfred

-- 
- Harald Welte [off-list ref]               http://www.gnumonks.org/
============================================================================
Programming is like sex: One mistake and you have to support it your lifetime

 > I think this overall problem can be solved if there was some per-device
 > variable that saves the IntrStatus until the NAPI callback gets
 > scheduled. What do you think?  This wouldn't even need some locking,
 > since interrupts would be disabled before the field is updated, and not
 > re-enabled before the field is read by the NAPI callback?
 > 
 > I was surprised that this solution is not suggested in the NAPI-HOWTO.txt, so I though there must be an error in my proposal...
 > 
 > By using such a scheme, isn't it also possible to only offload RX into
 > the NAPI callback with clear-on-read devices?
 > 

 e1000 used such technique before.If a remember correctly IntrStatus was
 saved in device priv struct.

 Cheers.
						--ro

 > I think this overall problem can be solved if there was some per-device
 > variable that saves the IntrStatus until the NAPI callback gets
 > scheduled. What do you think?  This wouldn't even need some locking,
 > since interrupts would be disabled before the field is updated, and not
 > re-enabled before the field is read by the NAPI callback?
 >
 > I was surprised that this solution is not suggested in the NAPI-HOWTO.txt, so I though there must be an error in my proposal...
 >
 > By using such a scheme, isn't it also possible to only offload RX into
 > the NAPI callback with clear-on-read devices?
 >

 e1000 used such technique before.If a remember correctly IntrStatus was
 saved in device priv struct.

That's also how it is done in current sungem.

-- 
Eric