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
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: 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
From: Benjamin Herrenschmidt <[EMAIL PROTECTED]> Date: Fri, 15 Dec 2006 11:59:06 +1100 > 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. 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. Probably we'll need to put this in post-2.6.20 as the merge window is closed. - 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 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
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
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
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
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 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, 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
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 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
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 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: 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
> 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. 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: 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: "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/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: 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
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
> 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
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
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
[patch sungem] improved locking
The attached patch improves locking in the sungem driver: - a single lock is used in the driver - gem_start_xmit, gem_poll, and gem_interrupt are lockless The new locking design is based on what's in tg3.c. The patch runs smoothly on my ibook (with CONFIG_SMP set), but it will need extensive testing on a multi-cpu box. The patch includes two implementations for gem_interrupt(). One is lockless while the other makes use of a spinlock. The spinlock version is there because I was not sure the lockless version would work with net_poll_controller. One of the two versions must be removed in the final patch. Patch applies to current git net-2.6. Please review, and test if possible. Thanks, Signed-ff-by: Eric Lemoine <[EMAIL PROTECTED]> -- Eric sungem-locking.patch Description: Binary data