Re: [patch sungem] improved locking
On Thu, 2006-12-28 at 21:05 -0800, David Miller wrote: From: Benjamin Herrenschmidt [EMAIL PROTECTED] Date: Wed, 13 Dec 2006 15:07:24 +1100 tg3 says tg3: eth0: Link is up at 1000 Mbps, full duplex. tg3: eth0: Flow control is on for TX and on for RX. but sungem says eth0: Link is up at 1000 Mbps, full-duplex. eth0: Pause is disabled Hrm... I suppose I need to dig more. No time to do that today though. I was about to try and debug this, and noticed immediately that I didn't recognize any of the code. Could you look into this, you rewrote all of this stuff and this looks like a regression added, because I know this pause stuff used to work perfectly when I wrote the original GEM driver. :-) Heh, it's very possible it's a regression I added indeed. I'll try to have a look next week. Do you know of anybody who can verify on non-mii hardware or is pause irrelevant there ? Ben. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
From: Benjamin Herrenschmidt [EMAIL PROTECTED] Date: Sat, 30 Dec 2006 08:36:07 +1100 Heh, it's very possible it's a regression I added indeed. I'll try to have a look next week. Do you know of anybody who can verify on non-mii hardware or is pause irrelevant there ? For PCS based PHY's most of the logic is taken care of internally, and the pause-present signal is just sampled from one of the PCS registers. I think I have one here using non-mii that I could check at some point :) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
From: Benjamin Herrenschmidt [EMAIL PROTECTED] Date: Wed, 13 Dec 2006 15:07:24 +1100 tg3 says tg3: eth0: Link is up at 1000 Mbps, full duplex. tg3: eth0: Flow control is on for TX and on for RX. but sungem says eth0: Link is up at 1000 Mbps, full-duplex. eth0: Pause is disabled Hrm... I suppose I need to dig more. No time to do that today though. I was about to try and debug this, and noticed immediately that I didn't recognize any of the code. Could you look into this, you rewrote all of this stuff and this looks like a regression added, because I know this pause stuff used to work perfectly when I wrote the original GEM driver. :-) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
Thanks for testing this stuff. I'll take a look at the pause-enabling issue in the sungem drive then work on integrating Eric's patch. Ok, thanks. Probably we'll need to put this in post-2.6.20 as the merge window is closed. Yup, no hurry there. Ben. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
On Tue, 2006-12-12 at 06:49 +0100, Eric Lemoine wrote: On 12/12/06, Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: On Tue, 2006-12-12 at 06:33 +0100, Eric Lemoine wrote: On 12/12/06, David Miller [EMAIL PROTECTED] wrote: [...] Anyways, Eric your changes look fine as far as I can tell, can you give them a really good testing on some SMP boxes? Unfortunately I can't, I don't have the hardware (only an old ibook here). I do however, I'll give it a beating on a dual G5 as soon as I get a chance. I'm pretty swamped at the moment and the box is used by somebody else today. Ok, thanks a lot Benjamin. Patched driver's been running fine for a couple of days nights with constant beating... just those RX MAC fifo overflows every now and then (though they cause no data corruption and no big hit on the driver perfs neither). I suppose still worth investigating when I have a bit of time, I must have done something stupid with the pause settings. In the meantime, Eric's patch is all good. Ben. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
On Wed, 2006-12-13 at 14:12 +1100, Benjamin Herrenschmidt wrote: Been hitting a raw throughput in both directions plus a few other things on a dual G5 and the driver didn't crash :-) I'm seeing a problem though but I'm not sure it's related to your patch, I'll have to test without it. Basically, if I use a slightly modified versio of tridge's socklib (raw xput test, basically, a tcp connection with one side pushing as fast as it can a known pattern and the other one just receiving and verifying the data integrity), it works fine when running only one side (either rx or tx, doesn't matter). But if I start it both ways (that is both a receiver and a sender on the GMAC box) and the other end is a tg3 (quad g5), then I'm getting a lot of eth0: RX MAC fifo overflow smac[02045822 but there are other numbers here every now and then] errors on the sungem side. David, could that be the pause stuff not working properly ? The link is gigabit and the tg3 side doesn't complain about anything. I've verified that the problem isn't related to Eric's patch and is there without it... I suppose I just never noticed it before :-) So far, the patch looks solid. I'll let it run overnight and will holler if things went wrong tomorrow. Cheers, Ben. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
From: Benjamin Herrenschmidt [EMAIL PROTECTED] Date: Wed, 13 Dec 2006 14:12:13 +1100 David, could that be the pause stuff not working properly ? It could be, but if the tg3 says flow control is up in the kernel logs things ought to be OK. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
Been hitting a raw throughput in both directions plus a few other things on a dual G5 and the driver didn't crash :-) I'm seeing a problem though but I'm not sure it's related to your patch, I'll have to test without it. Basically, if I use a slightly modified versio of tridge's socklib (raw xput test, basically, a tcp connection with one side pushing as fast as it can a known pattern and the other one just receiving and verifying the data integrity), it works fine when running only one side (either rx or tx, doesn't matter). But if I start it both ways (that is both a receiver and a sender on the GMAC box) and the other end is a tg3 (quad g5), then I'm getting a lot of eth0: RX MAC fifo overflow smac[02045822 but there are other numbers here every now and then] errors on the sungem side. David, could that be the pause stuff not working properly ? The link is gigabit and the tg3 side doesn't complain about anything. Ben. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
On Tue, 2006-12-12 at 20:03 -0800, David Miller wrote: From: Benjamin Herrenschmidt [EMAIL PROTECTED] Date: Wed, 13 Dec 2006 14:12:13 +1100 David, could that be the pause stuff not working properly ? It could be, but if the tg3 says flow control is up in the kernel logs things ought to be OK. tg3 says tg3: eth0: Link is up at 1000 Mbps, full duplex. tg3: eth0: Flow control is on for TX and on for RX. but sungem says eth0: Link is up at 1000 Mbps, full-duplex. eth0: Pause is disabled Hrm... I suppose I need to dig more. No time to do that today though. Ben. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
From: Eric Lemoine [EMAIL PROTECTED] Date: Wed, 29 Nov 2006 11:56:30 +0100 I don't understand why we'd need all this. I think the following code for gem_interrupt should do the trick: ... The important thing is: we __netif_rx_schedule even if gem_status is 0 (shared irq case) because we don't want to miss events should the following scenario occur: I see, I miseed the bit where we're reading the status register also in the -poll() loop. Reading the status register at least twice every interrupt is really expensive. I hope there is a lesson being learned, and nobody out there is making modern networking NICs that put the interrupt status in a register. It should always be in a DMA'd status block, without any exception. This way it can be polled in the cheapest manner possible. When the status block is in main memory, the CPU only takes the cost of a read when the value actually changes. Anyways, Eric your changes look fine as far as I can tell, can you give them a really good testing on some SMP boxes? Then we can think about putting it into 2.6.21 or similar, I don't want to put any more major networking changes into 2.6.20 at this time. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
On 12/12/06, David Miller [EMAIL PROTECTED] wrote: [...] Anyways, Eric your changes look fine as far as I can tell, can you give them a really good testing on some SMP boxes? Unfortunately I can't, I don't have the hardware (only an old ibook here). -- Eric - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
On Tue, 2006-12-12 at 06:33 +0100, Eric Lemoine wrote: On 12/12/06, David Miller [EMAIL PROTECTED] wrote: [...] Anyways, Eric your changes look fine as far as I can tell, can you give them a really good testing on some SMP boxes? Unfortunately I can't, I don't have the hardware (only an old ibook here). I do however, I'll give it a beating on a dual G5 as soon as I get a chance. I'm pretty swamped at the moment and the box is used by somebody else today. Ben. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
On 12/12/06, Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: On Tue, 2006-12-12 at 06:33 +0100, Eric Lemoine wrote: On 12/12/06, David Miller [EMAIL PROTECTED] wrote: [...] Anyways, Eric your changes look fine as far as I can tell, can you give them a really good testing on some SMP boxes? Unfortunately I can't, I don't have the hardware (only an old ibook here). I do however, I'll give it a beating on a dual G5 as soon as I get a chance. I'm pretty swamped at the moment and the box is used by somebody else today. Ok, thanks a lot Benjamin. -- Eric - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
On 11/29/06, David Miller [EMAIL PROTECTED] wrote: From: Benjamin Herrenschmidt [EMAIL PROTECTED] Date: Wed, 29 Nov 2006 09:57:24 +1100 This looks mostly fine. I was thinking about the lockless stuff, and I wonder if there is a clever way you can get it back down to one PIO on the GREG_STAT register. I think you'd need to have the -poll() clear gp-status, then do a smp_wb(), right before it re-enables interrupts. Then in the interrupt handler, you need to find a way to safely OR-in any unset bits in gp-status in a race-free manner. Having it atomic might work at a slightly smaller cost than a lock, though atomics don't have strong ordering requirements so you'd still have to be a bit careful. At least in theory the atomic + any necessary memory barriers would be cheaper than the extra PIO read we need otherwise. Just a remark: the lockless impl doesn't add a PIO read, it adds a PIO write (to the IACK reg). FYI, I'm currently checking whether I can get rid of this extra PIO write, based on David's suggestion... -- Eric - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
On 11/28/06, David Miller [EMAIL PROTECTED] wrote:
From: Eric Lemoine [EMAIL PROTECTED]
Date: Tue, 14 Nov 2006 22:54:40 +0100
On 11/14/06, David Miller [EMAIL PROTECTED] wrote:
From: Eric Lemoine [EMAIL PROTECTED]
Date: Tue, 14 Nov 2006 08:28:42 +0100
because it makes it explicit that only bits 0 through 6 are taken into
account when writing the IACK register.
The phrase bits 0 through 6 doesn't make any sense when bit 3 DOES
NOT EXIST in the hardware, it's reserved, it's not there, so including
it only confuses people and obfuscates the code.
Please use the explicit bit mask composed of existing macros, which
not only makes sure that the mask has meaning, but it also makes sure
that reserved and non-existing bits are never referenced.
Patch attached.
Remember, the GEM_INTERRUPT_LOCKLESS isn't there to stay. It's
currently there because I'm not sure the lockless implementation of
gem_interrupt() will work with poll_net_controller.
Thanks,
Signed-off-by: Eric Lemoine [EMAIL PROTECTED]
This looks mostly fine.
I was thinking about the lockless stuff, and I wonder if there
is a clever way you can get it back down to one PIO on the
GREG_STAT register.
I think you'd need to have the -poll() clear gp-status, then
do a smp_wb(), right before it re-enables interrupts.
Then in the interrupt handler, you need to find a way to safely
OR-in any unset bits in gp-status in a race-free manner.
I don't understand why we'd need all this.
I think the following code for gem_interrupt should do the trick:
static irqreturn_t gem_interrupt(int irq, void *dev_id)
{
struct net_device *dev = dev_id;
struct gem *gp = dev-priv;
unsigned int handled = 1;
if (!gp-running)
goto out;
if (netif_rx_schedule_prep(dev)) {
u32 gem_status = gem_read_status(gp);
gem_disable_ints();
if (unlikely(!gem_status))
handled = 0;
if (gem_irq_sync(gp)) {
netif_poll_enable(dev);
goto out;
}
gp-status = gem_status;
__netif_rx_schedule(dev);
}
out:
return IRQ_RETVAL(handled);
}
The important thing is: we __netif_rx_schedule even if gem_status is 0
(shared irq case) because we don't want to miss events should the
following scenario occur:
CPU0 CPU1
gem interrupt
prepare rx schedule
gem_interrupt
rx is
already scheduled
shared irq - (we can miss NIC events
if we don't __netif_rx_schedule before
returning)
Thanks,
--
Eric
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
From: Eric Lemoine [EMAIL PROTECTED] Date: Tue, 14 Nov 2006 22:54:40 +0100 On 11/14/06, David Miller [EMAIL PROTECTED] wrote: From: Eric Lemoine [EMAIL PROTECTED] Date: Tue, 14 Nov 2006 08:28:42 +0100 because it makes it explicit that only bits 0 through 6 are taken into account when writing the IACK register. The phrase bits 0 through 6 doesn't make any sense when bit 3 DOES NOT EXIST in the hardware, it's reserved, it's not there, so including it only confuses people and obfuscates the code. Please use the explicit bit mask composed of existing macros, which not only makes sure that the mask has meaning, but it also makes sure that reserved and non-existing bits are never referenced. Patch attached. Remember, the GEM_INTERRUPT_LOCKLESS isn't there to stay. It's currently there because I'm not sure the lockless implementation of gem_interrupt() will work with poll_net_controller. Thanks, Signed-off-by: Eric Lemoine [EMAIL PROTECTED] This looks mostly fine. I was thinking about the lockless stuff, and I wonder if there is a clever way you can get it back down to one PIO on the GREG_STAT register. I think you'd need to have the -poll() clear gp-status, then do a smp_wb(), right before it re-enables interrupts. Then in the interrupt handler, you need to find a way to safely OR-in any unset bits in gp-status in a race-free manner. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
From: Benjamin Herrenschmidt [EMAIL PROTECTED] Date: Wed, 29 Nov 2006 09:57:24 +1100 This looks mostly fine. I was thinking about the lockless stuff, and I wonder if there is a clever way you can get it back down to one PIO on the GREG_STAT register. I think you'd need to have the -poll() clear gp-status, then do a smp_wb(), right before it re-enables interrupts. Then in the interrupt handler, you need to find a way to safely OR-in any unset bits in gp-status in a race-free manner. Having it atomic might work at a slightly smaller cost than a lock, though atomics don't have strong ordering requirements so you'd still have to be a bit careful. At least in theory the atomic + any necessary memory barriers would be cheaper than the extra PIO read we need otherwise. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
On Tue, 2006-11-28 at 15:43 -0800, David Miller wrote: At least in theory the atomic + any necessary memory barriers would be cheaper than the extra PIO read we need otherwise. Yes, IO reads are generally the worst case scenarios even on machines with fairly slow locks. Ben. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
From: Eric Lemoine [EMAIL PROTECTED] Date: Mon, 13 Nov 2006 00:11:49 +0100 +#if GEM_INTERRUPT_LOCKLESS + +/* Bitmask representing the interrupt conditions that we clear using GREG_IACK. + * We clear all the top-level interrupt conditions (bits 0 through 6) that we + * handle. + */ +#define GREG_STAT_IACK (GREG_STAT_NAPI 0x3f) +#endif I'm asking for this a second time... and to be honest I'm a little bit ticked off. Please spell out the explicit bits using the existing defines to create this mask. Do not use this magic 0x3f magic number which contains bits which are undefined, so obtain this mask (as I asked the first time) like this: (GREG_STAT_TXINTME | GREG_STAT_TXALL | GREG_STAT_TXDONE | GREG_STAT_RXDONE | GREG_STAT_RXNOBUF) I will not go through the effort a third time, instead I will simply ignore your patch submissions, it's that simple. If you ignore me, I ignore you. You want me to review this patch and the locking bits, but you're never going to get to that point if you continually ignore the most simple requests I'm making of you. :-( - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
On 11/14/06, David Miller [EMAIL PROTECTED] wrote: From: Eric Lemoine [EMAIL PROTECTED] Date: Mon, 13 Nov 2006 00:11:49 +0100 +#if GEM_INTERRUPT_LOCKLESS + +/* Bitmask representing the interrupt conditions that we clear using GREG_IACK. + * We clear all the top-level interrupt conditions (bits 0 through 6) that we + * handle. + */ +#define GREG_STAT_IACK (GREG_STAT_NAPI 0x3f) +#endif I'm asking for this a second time... and to be honest I'm a little bit ticked off. Please spell out the explicit bits using the existing defines to create this mask. Do not use this magic 0x3f magic number which contains bits which are undefined, so obtain this mask (as I asked the first time) like this: (GREG_STAT_TXINTME | GREG_STAT_TXALL | GREG_STAT_TXDONE | GREG_STAT_RXDONE | GREG_STAT_RXNOBUF) I will not go through the effort a third time, instead I will simply ignore your patch submissions, it's that simple. If you ignore me, I ignore you. Dave, please believe me, I'm not ignoring you at all! I'm just trying to make things better. I had thought that: (GREG_STAT_NAPI 0x3f) was better than (GREG_STAT_TXINTME | GREG_STAT_TXALL | GREG_STAT_TXDONE | GREG_STAT_RXDONE | GREG_STAT_RXNOBUF) because it makes it explicit that only bits 0 through 6 are taken into account when writing the IACK register. In addition, GREG_STAT_IACK doesn't need to be changed if GREG_STAT_NAPI is changed. That's why I did it that way. And as opposed to what I did in the previous patch, I no longer set bits that don't exist with this new macro. If you don't like it, let's discuss, and I can change my mind. But again, please, do not think I'm ignoring your comments. Thanks, -- Eric - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
From: Eric Lemoine [EMAIL PROTECTED] Date: Tue, 14 Nov 2006 08:28:42 +0100 because it makes it explicit that only bits 0 through 6 are taken into account when writing the IACK register. The phrase bits 0 through 6 doesn't make any sense when bit 3 DOES NOT EXIST in the hardware, it's reserved, it's not there, so including it only confuses people and obfuscates the code. Please use the explicit bit mask composed of existing macros, which not only makes sure that the mask has meaning, but it also makes sure that reserved and non-existing bits are never referenced. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
On 11/10/06, David Miller [EMAIL PROTECTED] wrote:
Please use GREG_STAT_* instead of magic constants for the
interrupt mask and ACK register writes. In fact, there are
some questionable values you use, in particular this one:
+static inline void gem_ack_int(struct gem *gp)
+{
+ writel(0x3f, gp-regs + GREG_IACK);
+}
This code clears bits 0 through 6, which GREG_IACK is for.
There is no bit defined in GREG_STAT_* for 0x08, but you
set it in this magic bitmask. It is another reason not
to use magic constants like this :-)
Yes, the bit 4 isn't used, but I assumed clearing it shouldn't be prolem.
Would you prefer that:
#define GREG_STAT_IACK 0x3f
static inline void gem_ack_int(struct gem *gp)
{
writel(GREG_STAT_IACK, gp-regs + GREG_IACK);
}
or that:
#define GREG_STAT_IACK (GREG_STAT_TXINTME | GREG_STAT_TXALL | \
GREG_STAT_RXDONE | GREG_STAT_RXNOBUF)
to avoid bit 4, and bit 3 (TX_DONE) which is masked.
Personnaly, I like the first way best.
Also, if you need to use an attachment to get the tabbing
right, that's fine, but please also provide a copy inline
so that it is easy to quote the patch for review purposes.
It's a truly a pain in the rear to quote things when you use
a binary attachment.
I will.
I'd like these very simple and straightforward issues to
be worked out before I even begin to review the actual
locking change itself.
Ok.
I'll wait for you regarding gem_ack_int() and send out another patch.
Thanks,
--
Eric
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
Yes, the bit 4 isn't used, but I assumed clearing it shouldn't be prolem. Always leave reserved bits alone ... I agree it's very unlikely in this case that it could cause a problem but I've seen stranger things Ben. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
From: Eric Lemoine [EMAIL PROTECTED]
Date: Fri, 10 Nov 2006 14:28:41 +0100
On 11/10/06, David Miller [EMAIL PROTECTED] wrote:
Please use GREG_STAT_* instead of magic constants for the
interrupt mask and ACK register writes. In fact, there are
some questionable values you use, in particular this one:
+static inline void gem_ack_int(struct gem *gp)
+{
+ writel(0x3f, gp-regs + GREG_IACK);
+}
This code clears bits 0 through 6, which GREG_IACK is for.
There is no bit defined in GREG_STAT_* for 0x08, but you
set it in this magic bitmask. It is another reason not
to use magic constants like this :-)
Yes, the bit 4 isn't used, but I assumed clearing it shouldn't be prolem.
Would you prefer that:
#define GREG_STAT_IACK 0x3f
static inline void gem_ack_int(struct gem *gp)
{
writel(GREG_STAT_IACK, gp-regs + GREG_IACK);
}
or that:
#define GREG_STAT_IACK (GREG_STAT_TXINTME | GREG_STAT_TXALL | \
GREG_STAT_RXDONE | GREG_STAT_RXNOBUF)
to avoid bit 4, and bit 3 (TX_DONE) which is masked.
Personnaly, I like the first way best.
I think the second way is better because it clearly states
your intentions. The GREG_STAT_IACK 0x3f macro is still
magic because it doesn't say what the bits actually mean.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch sungem] improved locking
Please use GREG_STAT_* instead of magic constants for the
interrupt mask and ACK register writes. In fact, there are
some questionable values you use, in particular this one:
+static inline void gem_ack_int(struct gem *gp)
+{
+ writel(0x3f, gp-regs + GREG_IACK);
+}
There is no bit defined in GREG_STAT_* for 0x08, but you
set it in this magic bitmask. It is another reason not
to use magic constants like this :-)
Also, if you need to use an attachment to get the tabbing
right, that's fine, but please also provide a copy inline
so that it is easy to quote the patch for review purposes.
It's a truly a pain in the rear to quote things when you use
a binary attachment.
I'd like these very simple and straightforward issues to
be worked out before I even begin to review the actual
locking change itself.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
