| Message ID | 4F0B608D.6090309@essax.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On 01/09/2012 10:47 PM, Wolfgang Zarre wrote: [...] >>> OK. My concern: Can we be sure that 16bit accesses are always >> supported >>> by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around >> the >>> 8bit accesses already help? >> >> Hmmm... are there any register reads that need the >> same 'double cycle' sequence ?? >> If so you need to stop reads being interleaved (with >> themselves and writes) so requesting a 16bit access >> doesn't help. >> >> Which means you need a spinlock... >> >> David >> >> > > @David: Thank You very much for that hint. You are right and to > implement correct we need a spinlock. > > @Wolfgang: I was thinking about Your question regarding 8/16 bit and > in fact it wouldn't work at all on a clean 8 bit cards. > > Further it wouldn't work on 16 bit cards where the MSB is not equal > to base port +1 and anyway, it's depending always on how the chip is > interfaced to the ISA bus and in which mode the chip is configured. > > > And therefore I was giving David's hint a try in using a spinlock in > function cc770_isa_port_write_reg_indirect() and patched as follows: > > --------------------------------------------------------------------- > diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c > index 2d12f89..dad6707 100644 > --- a/drivers/net/can/cc770/cc770.c > +++ b/drivers/net/can/cc770/cc770.c > @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff > *skb, struct net_device *dev) > > stats->tx_bytes += dlc; > > - > - /* > - * HM: We had some cases of repeated IRQs so make sure the > - * INT is acknowledged I know it's already further up, but > - * doing again fixed the issue > - */ > - cc770_write_reg(priv, msgobj[mo].ctrl0, > - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); > - > return NETDEV_TX_OK; > } > > @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device > *dev, unsigned int o) > /* Nothing more to send, switch off interrupts */ > cc770_write_reg(priv, msgobj[mo].ctrl0, > MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); > - /* > - * We had some cases of repeated IRQ so make sure the > - * INT is acknowledged > - */ > - cc770_write_reg(priv, msgobj[mo].ctrl0, > - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); > > stats->tx_packets++; > can_get_echo_skb(dev, 0); > diff --git a/drivers/net/can/cc770/cc770_isa.c > b/drivers/net/can/cc770/cc770_isa.c > index 4be5fe2..fe39eed 100644 > --- a/drivers/net/can/cc770/cc770_isa.c > +++ b/drivers/net/can/cc770/cc770_isa.c > @@ -110,6 +110,9 @@ MODULE_PARM_DESC(bcr, "Bus configuration register > (default=0x40 [CBY])"); > #define CC770_IOSIZE 0x20 > #define CC770_IOSIZE_INDIRECT 0x02 > > +/* Spinlock for cc770_isa_port_write_reg_indirect */ > +static DEFINE_SPINLOCK( outb_lock); > + Do we need a global or a per device spin lock? If this should be a per device one, please introduce a cc770_isa_priv and put the spinlock there. Don't forget to initialize the spinlock. > static struct platform_device *cc770_isa_devs[MAXDEV]; > > static u8 cc770_isa_mem_read_reg(const struct cc770_priv *priv, int reg) > @@ -147,9 +150,12 @@ static void cc770_isa_port_write_reg_indirect(const > struct cc770_priv *priv, > int reg, u8 val) > { > unsigned long base = (unsigned long)priv->reg_base; > + unsigned long flags; please indent with tab, not space (your mailer probably converts tabs to space) > > + spin_lock_irqsave( &outb_lock, flags); ^ please remove the whitespace. > outb(reg, base); > outb(val, base + 1); > + spin_unlock_irqrestore( &outb_lock, flags); ^ ^^^ please remove this and trailing whitespace. > } > > static int __devinit cc770_isa_probe(struct platform_device *pdev) > ---------------------------------------------------------------------- cheers, Marc
On 01/10/2012 12:11 AM, Marc Kleine-Budde wrote: > On 01/09/2012 10:47 PM, Wolfgang Zarre wrote: > [...] > >>>> OK. My concern: Can we be sure that 16bit accesses are always >>> supported >>>> by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around >>> the >>>> 8bit accesses already help? >>> >>> Hmmm... are there any register reads that need the >>> same 'double cycle' sequence ?? >>> If so you need to stop reads being interleaved (with >>> themselves and writes) so requesting a 16bit access >>> doesn't help. >>> >>> Which means you need a spinlock... >>> >>> David >>> >>> >> >> @David: Thank You very much for that hint. You are right and to >> implement correct we need a spinlock. >> >> @Wolfgang: I was thinking about Your question regarding 8/16 bit and >> in fact it wouldn't work at all on a clean 8 bit cards. >> >> Further it wouldn't work on 16 bit cards where the MSB is not equal >> to base port +1 and anyway, it's depending always on how the chip is >> interfaced to the ISA bus and in which mode the chip is configured. >> >> >> And therefore I was giving David's hint a try in using a spinlock in >> function cc770_isa_port_write_reg_indirect() and patched as follows: >> >> --------------------------------------------------------------------- >> diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c >> index 2d12f89..dad6707 100644 >> --- a/drivers/net/can/cc770/cc770.c >> +++ b/drivers/net/can/cc770/cc770.c >> @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff >> *skb, struct net_device *dev) >> >> stats->tx_bytes += dlc; >> >> - >> - /* >> - * HM: We had some cases of repeated IRQs so make sure the >> - * INT is acknowledged I know it's already further up, but >> - * doing again fixed the issue >> - */ >> - cc770_write_reg(priv, msgobj[mo].ctrl0, >> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); >> - >> return NETDEV_TX_OK; >> } >> >> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device >> *dev, unsigned int o) >> /* Nothing more to send, switch off interrupts */ >> cc770_write_reg(priv, msgobj[mo].ctrl0, >> MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); >> - /* >> - * We had some cases of repeated IRQ so make sure the >> - * INT is acknowledged >> - */ >> - cc770_write_reg(priv, msgobj[mo].ctrl0, >> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); Please provide an extra patch for these unrelated changes. If we really want to remove it. >> stats->tx_packets++; >> can_get_echo_skb(dev, 0); >> diff --git a/drivers/net/can/cc770/cc770_isa.c >> b/drivers/net/can/cc770/cc770_isa.c >> index 4be5fe2..fe39eed 100644 >> --- a/drivers/net/can/cc770/cc770_isa.c >> +++ b/drivers/net/can/cc770/cc770_isa.c >> @@ -110,6 +110,9 @@ MODULE_PARM_DESC(bcr, "Bus configuration register >> (default=0x40 [CBY])"); >> #define CC770_IOSIZE 0x20 >> #define CC770_IOSIZE_INDIRECT 0x02 >> >> +/* Spinlock for cc770_isa_port_write_reg_indirect */ >> +static DEFINE_SPINLOCK( outb_lock); >> + > > Do we need a global or a per device spin lock? If this should be a per > device one, please introduce a cc770_isa_priv and put the spinlock > there. Don't forget to initialize the spinlock. Yes, that's what I was thinking as well but in the ocan driver I find: /* * we need a spinlock here, as the address register looks shared between * two PC-ECAN devices. Moreover, we need to protect WRT interrupts */ Looks like wired hardware. Anyway, a global spinlock might be safer. Wolfgang. -- 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
> cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, > int reg, u8 val) > { > unsigned long base = (unsigned long)priv->reg_base; > + unsigned long flags; > > + spin_lock_irqsave( &outb_lock, flags); > outb(reg, base); > outb(val, base + 1); > + spin_unlock_irqrestore( &outb_lock, flags); Is there a 'read_reg_indirect' function?? If so it also needs to use the same mutex. I'd double check all references to the 'reg_base' field. David -- 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
Hello guys, see comments below. On 9/01/2012 10:47 PM, Wolfgang Zarre wrote: > Further I have also investigated a bit the issue regarding the "HM" > patches. > > I checked out the can4linux and I could find this patch in file > can_i82527funcs.c as mentioned by Oliver. > @Oliver: BTW thanks for Your investigation. > > After studying the Makefile and some tests I discovered that > this file is just used for the target SBS_PC7 but funny enough not > for GENERIC_I82527. > > I was seeking the declaration of CANout used in can_i82527funcs.c > to see if it's based on the same code we had and as well as in Lincan > but unfortunately the target SBS_PC7 doesn't compile with kernel > 2.6.39-4 and moreover I got the message: > ...can4linux/trunk/can4linux/i82527funcs.c:72: > error: implicit declaration of function 'CANout' > > And additional not knowing if the board of SBS_PC7 is ISA or PCI based > I tried to find out and found after some research the following > thread: > http://old.nabble.com/-PATCH-v2--cc770-isa%3A-legacy-driver-for-CC770-i82725-on-the-ISA-bus-td24216347.html > > > According to that I assumed it's ISA based and is working with cc770_isa. > > But not finding the declaration of CANout used in can_i82527funcs.c I > assumed further that it was done without spinlock which might cause > the effect of repeated or even lost interrupts as well depending on > the hardware configuration. > > If this is the case then the HM patches would be obsolete and maybe > someone owning a SBS PC7 can test without these patches. > > @Henrik: I would like to ask You if You can confirm this when You are > back. > Thanks a lot. > > > Wolfgang The can4linux port I made was done for a specific project and a specific hardware, the SBS PC7, and only ever tested on that hardware. The i82527 code was already in the can4linux 3.x archive but not maintained for a while and has become non-functional over time. Initially we used the 2.4 Linux kernel and the can4linux version 3.1 as basis for the re-integration of the i82527. I called our modified version "3.1i" (i for Intel). This is from the changelog: 3.1i - Integration of the i82527 code into can4linux version 3.1, major refactoring and clean-up of the i82527 code, fixed interrupt lock-up issue w/ sending while receiving in heavy load, fixed issue w/ stopping and re-starting the bus, added new acceptance mask function to bring code in line with SJA1000 code base, new I/O model to support the SJA1000 CANin/CANout macros, support added to the makefile for generic i82527 cards and SBS PC7compact DINrail mounted industry PC by Henrik Maier of FOCUS Software Engineering Pty Ltd <www.focus-sw.com> As time passed the requirement for kernel 2.6 support emerged and we did the same based on can4linux 3.3.2 and called it "3.3.2i" which later was merged into the official can4linux archive and became 3.3.3. The "repeated TX IRQ" issue came up only with kernel 2.6 and was not required in the 2.4 version. The way I fixed it may not have been the best, but it solved the immediate need of finishing a project and getting the driver stable. I am not a kernel driver expert. The PC7 hardware is now obsolete anyway. From my point we can do a "clean" start and remove the old baggage. The PC7 CAN interface is ISA based and I don't have a hardware any more to test. Some background info and our "i" versions are published here: http://www.focus-sw.com/can/can4linux.html I hope that helps a bit. It has been a few years since I worked with can4linux but it is great to see that the community is further enhancing CAN support for Linux. Henrik -- 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
Hi Henrik, On 01/10/2012 12:41 PM, Henrik Maier wrote: > Hello guys, > > see comments below. > > On 9/01/2012 10:47 PM, Wolfgang Zarre wrote: >> Further I have also investigated a bit the issue regarding the "HM" >> patches. >> >> I checked out the can4linux and I could find this patch in file >> can_i82527funcs.c as mentioned by Oliver. >> @Oliver: BTW thanks for Your investigation. >> >> After studying the Makefile and some tests I discovered that >> this file is just used for the target SBS_PC7 but funny enough not >> for GENERIC_I82527. >> >> I was seeking the declaration of CANout used in can_i82527funcs.c >> to see if it's based on the same code we had and as well as in Lincan >> but unfortunately the target SBS_PC7 doesn't compile with kernel >> 2.6.39-4 and moreover I got the message: >> ...can4linux/trunk/can4linux/i82527funcs.c:72: >> error: implicit declaration of function 'CANout' >> >> And additional not knowing if the board of SBS_PC7 is ISA or PCI based >> I tried to find out and found after some research the following >> thread: >> http://old.nabble.com/-PATCH-v2--cc770-isa%3A-legacy-driver-for-CC770-i82725-on-the-ISA-bus-td24216347.html >> >> >> According to that I assumed it's ISA based and is working with cc770_isa. >> >> But not finding the declaration of CANout used in can_i82527funcs.c I >> assumed further that it was done without spinlock which might cause >> the effect of repeated or even lost interrupts as well depending on >> the hardware configuration. >> >> If this is the case then the HM patches would be obsolete and maybe >> someone owning a SBS PC7 can test without these patches. >> >> @Henrik: I would like to ask You if You can confirm this when You are >> back. >> Thanks a lot. >> >> >> Wolfgang > > The can4linux port I made was done for a specific project and a specific > hardware, the SBS PC7, and only ever tested on that hardware. The i82527 > code was already in the can4linux 3.x archive but not maintained for a > while and has become non-functional over time. > > Initially we used the 2.4 Linux kernel and the can4linux version 3.1 as > basis for the re-integration of the i82527. I called our modified > version "3.1i" (i for Intel). This is from the changelog: > > 3.1i - Integration of the i82527 code into can4linux version 3.1, > major refactoring and clean-up of the i82527 code, fixed > interrupt lock-up issue w/ sending while receiving in heavy load, > fixed issue w/ stopping and re-starting the bus, added > new acceptance mask function to bring code in line with SJA1000 code > base, new I/O model to support the SJA1000 CANin/CANout macros, > support added to the makefile for generic i82527 cards and > SBS PC7compact DINrail mounted industry PC > by Henrik Maier of FOCUS Software Engineering Pty Ltd > <www.focus-sw.com> > > As time passed the requirement for kernel 2.6 support emerged and we did > the same based on can4linux 3.3.2 and called it "3.3.2i" which later was > merged into the official can4linux archive and became 3.3.3. > > The "repeated TX IRQ" issue came up only with kernel 2.6 and was not > required in the 2.4 version. The way I fixed it may not have been the > best, but it solved the immediate need of finishing a project and > getting the driver stable. I am not a kernel driver expert. OK, then most likely it's not a hardware issue. > The PC7 hardware is now obsolete anyway. From my point we can do a > "clean" start and remove the old baggage. > > The PC7 CAN interface is ISA based and I don't have a hardware any more > to test. > > Some background info and our "i" versions are published here: > > http://www.focus-sw.com/can/can4linux.html > > I hope that helps a bit. It has been a few years since I worked with > can4linux but it is great to see that the community is further enhancing > CAN support for Linux. Yes, of course. Thanks for digging that out. As you suggest, we will remove that code and wait what happens. Wolfgang. -- 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
Hello Wolfgang, > On 01/10/2012 12:11 AM, Marc Kleine-Budde wrote: >> On 01/09/2012 10:47 PM, Wolfgang Zarre wrote: >> [...] >> >>>>> OK. My concern: Can we be sure that 16bit accesses are always >>>> supported >>>>> by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around >>>> the >>>>> 8bit accesses already help? >>>> >>>> Hmmm... are there any register reads that need the >>>> same 'double cycle' sequence ?? >>>> If so you need to stop reads being interleaved (with >>>> themselves and writes) so requesting a 16bit access >>>> doesn't help. >>>> >>>> Which means you need a spinlock... >>>> >>>> David >>>> >>>> >>> >>> @David: Thank You very much for that hint. You are right and to >>> implement correct we need a spinlock. >>> >>> @Wolfgang: I was thinking about Your question regarding 8/16 bit and >>> in fact it wouldn't work at all on a clean 8 bit cards. >>> >>> Further it wouldn't work on 16 bit cards where the MSB is not equal >>> to base port +1 and anyway, it's depending always on how the chip is >>> interfaced to the ISA bus and in which mode the chip is configured. >>> >>> >>> And therefore I was giving David's hint a try in using a spinlock in >>> function cc770_isa_port_write_reg_indirect() and patched as follows: >>> >>> --------------------------------------------------------------------- >>> diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c >>> index 2d12f89..dad6707 100644 >>> --- a/drivers/net/can/cc770/cc770.c >>> +++ b/drivers/net/can/cc770/cc770.c >>> @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff >>> *skb, struct net_device *dev) >>> >>> stats->tx_bytes += dlc; >>> >>> - >>> - /* >>> - * HM: We had some cases of repeated IRQs so make sure the >>> - * INT is acknowledged I know it's already further up, but >>> - * doing again fixed the issue >>> - */ >>> - cc770_write_reg(priv, msgobj[mo].ctrl0, >>> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); >>> - >>> return NETDEV_TX_OK; >>> } >>> >>> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device >>> *dev, unsigned int o) >>> /* Nothing more to send, switch off interrupts */ >>> cc770_write_reg(priv, msgobj[mo].ctrl0, >>> MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); >>> - /* >>> - * We had some cases of repeated IRQ so make sure the >>> - * INT is acknowledged >>> - */ >>> - cc770_write_reg(priv, msgobj[mo].ctrl0, >>> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); > > Please provide an extra patch for these unrelated changes. If we really > want to remove it. > Sure, this I can do. >>> stats->tx_packets++; >>> can_get_echo_skb(dev, 0); >>> diff --git a/drivers/net/can/cc770/cc770_isa.c >>> b/drivers/net/can/cc770/cc770_isa.c >>> index 4be5fe2..fe39eed 100644 >>> --- a/drivers/net/can/cc770/cc770_isa.c >>> +++ b/drivers/net/can/cc770/cc770_isa.c >>> @@ -110,6 +110,9 @@ MODULE_PARM_DESC(bcr, "Bus configuration register >>> (default=0x40 [CBY])"); >>> #define CC770_IOSIZE 0x20 >>> #define CC770_IOSIZE_INDIRECT 0x02 >>> >>> +/* Spinlock for cc770_isa_port_write_reg_indirect */ >>> +static DEFINE_SPINLOCK( outb_lock); >>> + >> >> Do we need a global or a per device spin lock? If this should be a per >> device one, please introduce a cc770_isa_priv and put the spinlock >> there. Don't forget to initialize the spinlock. > > Yes, that's what I was thinking as well but in the ocan driver I find: > > /* > * we need a spinlock here, as the address register looks shared between > * two PC-ECAN devices. Moreover, we need to protect WRT interrupts > */ > > Looks like wired hardware. Anyway, a global spinlock might be safer. > Hmmm, actually I thought to place the spinlock local because of having the problem just with the interrupt and not with mutex. But if global wouldn't it then better to make an array[MAX_DEV] for the lock with initialisation in _init or _start? But if PC-ECAN works with that configuration? > Wolfgang. Wolfgang -- 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
Hello David, > >> cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, >> int reg, u8 val) >> { >> unsigned long base = (unsigned long)priv->reg_base; >> + unsigned long flags; >> >> + spin_lock_irqsave(&outb_lock, flags); >> outb(reg, base); >> outb(val, base + 1); >> + spin_unlock_irqrestore(&outb_lock, flags); > > Is there a 'read_reg_indirect' function?? Yes, there is. > If so it also needs to use the same mutex. Actually, I don't think that we have a problem with mutex beside that it's using just one inb() statement but having for sure with an interrupt between both outb() statements which seems to be critical for the cc770. However, if inb() or outb() can be interrupted then it would be an issue. > I'd double check all references to the 'reg_base' field. > > David > > Wolfgang -- 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
Hello guys, > Hi Henrik, > > On 01/10/2012 12:41 PM, Henrik Maier wrote: >> Hello guys, >> >> see comments below. >> >> On 9/01/2012 10:47 PM, Wolfgang Zarre wrote: >>> Further I have also investigated a bit the issue regarding the "HM" >>> patches. >>> >>> I checked out the can4linux and I could find this patch in file >>> can_i82527funcs.c as mentioned by Oliver. >>> @Oliver: BTW thanks for Your investigation. >>> >>> After studying the Makefile and some tests I discovered that >>> this file is just used for the target SBS_PC7 but funny enough not >>> for GENERIC_I82527. >>> >>> I was seeking the declaration of CANout used in can_i82527funcs.c >>> to see if it's based on the same code we had and as well as in Lincan >>> but unfortunately the target SBS_PC7 doesn't compile with kernel >>> 2.6.39-4 and moreover I got the message: >>> ...can4linux/trunk/can4linux/i82527funcs.c:72: >>> error: implicit declaration of function 'CANout' >>> >>> And additional not knowing if the board of SBS_PC7 is ISA or PCI based >>> I tried to find out and found after some research the following >>> thread: >>> http://old.nabble.com/-PATCH-v2--cc770-isa%3A-legacy-driver-for-CC770-i82725-on-the-ISA-bus-td24216347.html >>> >>> >>> According to that I assumed it's ISA based and is working with cc770_isa. >>> >>> But not finding the declaration of CANout used in can_i82527funcs.c I >>> assumed further that it was done without spinlock which might cause >>> the effect of repeated or even lost interrupts as well depending on >>> the hardware configuration. >>> >>> If this is the case then the HM patches would be obsolete and maybe >>> someone owning a SBS PC7 can test without these patches. >>> >>> @Henrik: I would like to ask You if You can confirm this when You are >>> back. >>> Thanks a lot. >>> >>> >>> Wolfgang >> >> The can4linux port I made was done for a specific project and a specific >> hardware, the SBS PC7, and only ever tested on that hardware. The i82527 >> code was already in the can4linux 3.x archive but not maintained for a >> while and has become non-functional over time. >> >> Initially we used the 2.4 Linux kernel and the can4linux version 3.1 as >> basis for the re-integration of the i82527. I called our modified >> version "3.1i" (i for Intel). This is from the changelog: >> >> 3.1i - Integration of the i82527 code into can4linux version 3.1, >> major refactoring and clean-up of the i82527 code, fixed >> interrupt lock-up issue w/ sending while receiving in heavy load, >> fixed issue w/ stopping and re-starting the bus, added >> new acceptance mask function to bring code in line with SJA1000 code >> base, new I/O model to support the SJA1000 CANin/CANout macros, >> support added to the makefile for generic i82527 cards and >> SBS PC7compact DINrail mounted industry PC >> by Henrik Maier of FOCUS Software Engineering Pty Ltd >> <www.focus-sw.com> >> >> As time passed the requirement for kernel 2.6 support emerged and we did >> the same based on can4linux 3.3.2 and called it "3.3.2i" which later was >> merged into the official can4linux archive and became 3.3.3. >> >> The "repeated TX IRQ" issue came up only with kernel 2.6 and was not >> required in the 2.4 version. The way I fixed it may not have been the >> best, but it solved the immediate need of finishing a project and >> getting the driver stable. I am not a kernel driver expert. > > OK, then most likely it's not a hardware issue. > >> The PC7 hardware is now obsolete anyway. From my point we can do a >> "clean" start and remove the old baggage. >> >> The PC7 CAN interface is ISA based and I don't have a hardware any more >> to test. >> >> Some background info and our "i" versions are published here: >> >> http://www.focus-sw.com/can/can4linux.html >> >> I hope that helps a bit. It has been a few years since I worked with >> can4linux but it is great to see that the community is further enhancing >> CAN support for Linux. > > Yes, of course. Thanks for digging that out. As you suggest, we will > remove that code and wait what happens. > Ok, I'll prepare this patch separately as said. > Wolfgang. > Wolfgang -- 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
On 01/10/2012 01:30 PM, Wolfgang Zarre wrote: > Hello Wolfgang, > >> On 01/10/2012 12:11 AM, Marc Kleine-Budde wrote: >>> On 01/09/2012 10:47 PM, Wolfgang Zarre wrote: >>> [...] >>> >>>>>> OK. My concern: Can we be sure that 16bit accesses are always >>>>> supported >>>>>> by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around >>>>> the >>>>>> 8bit accesses already help? >>>>> >>>>> Hmmm... are there any register reads that need the >>>>> same 'double cycle' sequence ?? >>>>> If so you need to stop reads being interleaved (with >>>>> themselves and writes) so requesting a 16bit access >>>>> doesn't help. >>>>> >>>>> Which means you need a spinlock... >>>>> >>>>> David >>>>> >>>>> >>>> >>>> @David: Thank You very much for that hint. You are right and to >>>> implement correct we need a spinlock. >>>> >>>> @Wolfgang: I was thinking about Your question regarding 8/16 bit and >>>> in fact it wouldn't work at all on a clean 8 bit cards. >>>> >>>> Further it wouldn't work on 16 bit cards where the MSB is not equal >>>> to base port +1 and anyway, it's depending always on how the chip is >>>> interfaced to the ISA bus and in which mode the chip is configured. >>>> >>>> >>>> And therefore I was giving David's hint a try in using a spinlock in >>>> function cc770_isa_port_write_reg_indirect() and patched as follows: >>>> >>>> --------------------------------------------------------------------- >>>> diff --git a/drivers/net/can/cc770/cc770.c >>>> b/drivers/net/can/cc770/cc770.c >>>> index 2d12f89..dad6707 100644 >>>> --- a/drivers/net/can/cc770/cc770.c >>>> +++ b/drivers/net/can/cc770/cc770.c >>>> @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff >>>> *skb, struct net_device *dev) >>>> >>>> stats->tx_bytes += dlc; >>>> >>>> - >>>> - /* >>>> - * HM: We had some cases of repeated IRQs so make sure the >>>> - * INT is acknowledged I know it's already further up, but >>>> - * doing again fixed the issue >>>> - */ >>>> - cc770_write_reg(priv, msgobj[mo].ctrl0, >>>> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); >>>> - >>>> return NETDEV_TX_OK; >>>> } >>>> >>>> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device >>>> *dev, unsigned int o) >>>> /* Nothing more to send, switch off interrupts */ >>>> cc770_write_reg(priv, msgobj[mo].ctrl0, >>>> MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); >>>> - /* >>>> - * We had some cases of repeated IRQ so make sure the >>>> - * INT is acknowledged >>>> - */ >>>> - cc770_write_reg(priv, msgobj[mo].ctrl0, >>>> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); >> >> Please provide an extra patch for these unrelated changes. If we really >> want to remove it. >> > > Sure, this I can do. > >>>> stats->tx_packets++; >>>> can_get_echo_skb(dev, 0); >>>> diff --git a/drivers/net/can/cc770/cc770_isa.c >>>> b/drivers/net/can/cc770/cc770_isa.c >>>> index 4be5fe2..fe39eed 100644 >>>> --- a/drivers/net/can/cc770/cc770_isa.c >>>> +++ b/drivers/net/can/cc770/cc770_isa.c >>>> @@ -110,6 +110,9 @@ MODULE_PARM_DESC(bcr, "Bus configuration register >>>> (default=0x40 [CBY])"); >>>> #define CC770_IOSIZE 0x20 >>>> #define CC770_IOSIZE_INDIRECT 0x02 >>>> >>>> +/* Spinlock for cc770_isa_port_write_reg_indirect */ >>>> +static DEFINE_SPINLOCK( outb_lock); Please use a more specific name, e.g.: cc770_isa_port_lock >>>> + >>> >>> Do we need a global or a per device spin lock? If this should be a per >>> device one, please introduce a cc770_isa_priv and put the spinlock >>> there. Don't forget to initialize the spinlock. >> >> Yes, that's what I was thinking as well but in the ocan driver I find: >> >> /* >> * we need a spinlock here, as the address register looks shared between >> * two PC-ECAN devices. Moreover, we need to protect WRT interrupts >> */ >> >> Looks like wired hardware. Anyway, a global spinlock might be safer. >> > > Hmmm, actually I thought to place the spinlock local because of having > the problem just with the interrupt and not with mutex. > > But if global wouldn't it then better to make an array[MAX_DEV] for the > lock with initialisation in _init or _start? Global means *one* spin-irq-lock for the indirect register access of all devices. That might be the most efficient solution but we are sure that it works, also with wired i82527 hardware, which seem to exist. That's also what the related lincan and ocan drivers used. Wolfgang. -- 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
On 01/10/2012 03:20 PM, Wolfgang Grandegger wrote: > On 01/10/2012 01:30 PM, Wolfgang Zarre wrote: >> Hello Wolfgang, >> >>> On 01/10/2012 12:11 AM, Marc Kleine-Budde wrote: >>>> On 01/09/2012 10:47 PM, Wolfgang Zarre wrote: >>>> [...] >>>> >>>>>>> OK. My concern: Can we be sure that 16bit accesses are always >>>>>> supported >>>>>>> by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around >>>>>> the >>>>>>> 8bit accesses already help? >>>>>> >>>>>> Hmmm... are there any register reads that need the >>>>>> same 'double cycle' sequence ?? >>>>>> If so you need to stop reads being interleaved (with >>>>>> themselves and writes) so requesting a 16bit access >>>>>> doesn't help. >>>>>> >>>>>> Which means you need a spinlock... >>>>>> >>>>>> David >>>>>> >>>>>> >>>>> >>>>> @David: Thank You very much for that hint. You are right and to >>>>> implement correct we need a spinlock. >>>>> >>>>> @Wolfgang: I was thinking about Your question regarding 8/16 bit and >>>>> in fact it wouldn't work at all on a clean 8 bit cards. >>>>> >>>>> Further it wouldn't work on 16 bit cards where the MSB is not equal >>>>> to base port +1 and anyway, it's depending always on how the chip is >>>>> interfaced to the ISA bus and in which mode the chip is configured. >>>>> >>>>> >>>>> And therefore I was giving David's hint a try in using a spinlock in >>>>> function cc770_isa_port_write_reg_indirect() and patched as follows: >>>>> >>>>> --------------------------------------------------------------------- >>>>> diff --git a/drivers/net/can/cc770/cc770.c >>>>> b/drivers/net/can/cc770/cc770.c >>>>> index 2d12f89..dad6707 100644 >>>>> --- a/drivers/net/can/cc770/cc770.c >>>>> +++ b/drivers/net/can/cc770/cc770.c >>>>> @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff >>>>> *skb, struct net_device *dev) >>>>> >>>>> stats->tx_bytes += dlc; >>>>> >>>>> - >>>>> - /* >>>>> - * HM: We had some cases of repeated IRQs so make sure the >>>>> - * INT is acknowledged I know it's already further up, but >>>>> - * doing again fixed the issue >>>>> - */ >>>>> - cc770_write_reg(priv, msgobj[mo].ctrl0, >>>>> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); >>>>> - >>>>> return NETDEV_TX_OK; >>>>> } >>>>> >>>>> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device >>>>> *dev, unsigned int o) >>>>> /* Nothing more to send, switch off interrupts */ >>>>> cc770_write_reg(priv, msgobj[mo].ctrl0, >>>>> MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); >>>>> - /* >>>>> - * We had some cases of repeated IRQ so make sure the >>>>> - * INT is acknowledged >>>>> - */ >>>>> - cc770_write_reg(priv, msgobj[mo].ctrl0, >>>>> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); >>> >>> Please provide an extra patch for these unrelated changes. If we really >>> want to remove it. >>> >> >> Sure, this I can do. >> >>>>> stats->tx_packets++; >>>>> can_get_echo_skb(dev, 0); >>>>> diff --git a/drivers/net/can/cc770/cc770_isa.c >>>>> b/drivers/net/can/cc770/cc770_isa.c >>>>> index 4be5fe2..fe39eed 100644 >>>>> --- a/drivers/net/can/cc770/cc770_isa.c >>>>> +++ b/drivers/net/can/cc770/cc770_isa.c >>>>> @@ -110,6 +110,9 @@ MODULE_PARM_DESC(bcr, "Bus configuration register >>>>> (default=0x40 [CBY])"); >>>>> #define CC770_IOSIZE 0x20 >>>>> #define CC770_IOSIZE_INDIRECT 0x02 >>>>> >>>>> +/* Spinlock for cc770_isa_port_write_reg_indirect */ >>>>> +static DEFINE_SPINLOCK( outb_lock); > > Please use a more specific name, e.g.: cc770_isa_port_lock > >>>>> + >>>> >>>> Do we need a global or a per device spin lock? If this should be a per >>>> device one, please introduce a cc770_isa_priv and put the spinlock >>>> there. Don't forget to initialize the spinlock. >>> >>> Yes, that's what I was thinking as well but in the ocan driver I find: >>> >>> /* >>> * we need a spinlock here, as the address register looks shared between >>> * two PC-ECAN devices. Moreover, we need to protect WRT interrupts >>> */ >>> >>> Looks like wired hardware. Anyway, a global spinlock might be safer. >>> >> >> Hmmm, actually I thought to place the spinlock local because of having >> the problem just with the interrupt and not with mutex. >> >> But if global wouldn't it then better to make an array[MAX_DEV] for the >> lock with initialisation in _init or _start? > > Global means *one* spin-irq-lock for the indirect register access of all > devices. That might be the most efficient solution but we are sure that s/might/might not/ > it works, also with wired i82527 hardware, which seem to exist. That's > also what the related lincan and ocan drivers used. Wolfgang. -- 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
On 01/10/2012 01:41 PM, Wolfgang Zarre wrote: > Hello David, >> >>> cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, >>> int reg, u8 val) >>> { >>> unsigned long base = (unsigned long)priv->reg_base; >>> + unsigned long flags; >>> >>> + spin_lock_irqsave(&outb_lock, flags); >>> outb(reg, base); >>> outb(val, base + 1); >>> + spin_unlock_irqrestore(&outb_lock, flags); >> >> Is there a 'read_reg_indirect' function?? > > Yes, there is. > >> If so it also needs to use the same mutex. > > Actually, I don't think that we have a problem with mutex > beside that it's using just one inb() statement but having > for sure with an interrupt between both outb() statements which > seems to be critical for the cc770. But the indirect read function also sets the address register before reading the data using inb(). This sequence should also not be interrupted and therefore we need to synchronize. For the indirect access of the SJA1000 we also need to add spinlocks. Wonder why nobody complained so far. Wolfgang. -- 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
On 10.01.2012 15:43, Wolfgang Grandegger wrote: > On 01/10/2012 01:41 PM, Wolfgang Zarre wrote: >> Hello David, >>> >>>> cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, >>>> int reg, u8 val) >>>> { >>>> unsigned long base = (unsigned long)priv->reg_base; >>>> + unsigned long flags; >>>> >>>> + spin_lock_irqsave(&outb_lock, flags); >>>> outb(reg, base); >>>> outb(val, base + 1); >>>> + spin_unlock_irqrestore(&outb_lock, flags); >>> >>> Is there a 'read_reg_indirect' function?? >> >> Yes, there is. >> >>> If so it also needs to use the same mutex. >> >> Actually, I don't think that we have a problem with mutex >> beside that it's using just one inb() statement but having >> for sure with an interrupt between both outb() statements which >> seems to be critical for the cc770. > > But the indirect read function also sets the address register before > reading the data using inb(). This sequence should also not be > interrupted and therefore we need to synchronize. For the indirect > access of the SJA1000 we also need to add spinlocks. Wonder why nobody > complained so far. Maybe due to old single core hardware that has SJA1000 hw with indirect addressing? Or this old hardware has not been 'kernel-upgraded' so far ... In any case you are right with the missing spinlocks. Regards, Oliver -- 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
Hello Wolfgang, > On 01/10/2012 01:41 PM, Wolfgang Zarre wrote: >> Hello David, >>> >>>> cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, >>>> int reg, u8 val) >>>> { >>>> unsigned long base = (unsigned long)priv->reg_base; >>>> + unsigned long flags; >>>> >>>> + spin_lock_irqsave(&outb_lock, flags); >>>> outb(reg, base); >>>> outb(val, base + 1); >>>> + spin_unlock_irqrestore(&outb_lock, flags); >>> >>> Is there a 'read_reg_indirect' function?? >> >> Yes, there is. >> >>> If so it also needs to use the same mutex. >> >> Actually, I don't think that we have a problem with mutex >> beside that it's using just one inb() statement but having >> for sure with an interrupt between both outb() statements which >> seems to be critical for the cc770. > > But the indirect read function also sets the address register before > reading the data using inb(). This sequence should also not be > interrupted and therefore we need to synchronize. For the indirect > access of the SJA1000 we also need to add spinlocks. Wonder why nobody > complained so far. So, if I understand correct that means that inb() can be interrupted between setting the address and reading. If this is the case then yes, we need spinlock if this is not the case then IMHO we wouldn't need or am I wrong? > > Wolfgang. > Wolfgang -- 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
On 01/10/2012 05:13 PM, Wolfgang Zarre wrote: > Hello Wolfgang, > >> On 01/10/2012 01:41 PM, Wolfgang Zarre wrote: >>> Hello David, >>>> >>>>> cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, >>>>> int reg, u8 val) >>>>> { >>>>> unsigned long base = (unsigned long)priv->reg_base; >>>>> + unsigned long flags; >>>>> >>>>> + spin_lock_irqsave(&outb_lock, flags); >>>>> outb(reg, base); >>>>> outb(val, base + 1); >>>>> + spin_unlock_irqrestore(&outb_lock, flags); >>>> >>>> Is there a 'read_reg_indirect' function?? >>> >>> Yes, there is. >>> >>>> If so it also needs to use the same mutex. >>> >>> Actually, I don't think that we have a problem with mutex >>> beside that it's using just one inb() statement but having >>> for sure with an interrupt between both outb() statements which >>> seems to be critical for the cc770. >> >> But the indirect read function also sets the address register before >> reading the data using inb(). This sequence should also not be >> interrupted and therefore we need to synchronize. For the indirect >> access of the SJA1000 we also need to add spinlocks. Wonder why nobody >> complained so far. > > So, if I understand correct that means that inb() can be interrupted > between > setting the address and reading. If this is the case then yes, we need > spinlock if this is not the case then IMHO we wouldn't need or am I wrong? If I understand correct, this function is the problem: > static u8 cc770_isa_port_read_reg_indirect(const struct cc770_priv *priv, > int reg) > { > unsigned long base = (unsigned long)priv->reg_base; > > outb(reg, base); an interrupt can happen here, which does another indirect read or write access, leaving "reg" pointing to the wrong register. > return inb(base + 1); > } Mar c
Hi Wolfgang, On 01/10/2012 05:13 PM, Wolfgang Zarre wrote: > Hello Wolfgang, > >> On 01/10/2012 01:41 PM, Wolfgang Zarre wrote: >>> Hello David, >>>> >>>>> cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, >>>>> int reg, u8 val) >>>>> { >>>>> unsigned long base = (unsigned long)priv->reg_base; >>>>> + unsigned long flags; >>>>> >>>>> + spin_lock_irqsave(&outb_lock, flags); >>>>> outb(reg, base); >>>>> outb(val, base + 1); >>>>> + spin_unlock_irqrestore(&outb_lock, flags); >>>> >>>> Is there a 'read_reg_indirect' function?? >>> >>> Yes, there is. >>> >>>> If so it also needs to use the same mutex. >>> >>> Actually, I don't think that we have a problem with mutex >>> beside that it's using just one inb() statement but having >>> for sure with an interrupt between both outb() statements which >>> seems to be critical for the cc770. >> >> But the indirect read function also sets the address register before >> reading the data using inb(). This sequence should also not be >> interrupted and therefore we need to synchronize. For the indirect >> access of the SJA1000 we also need to add spinlocks. Wonder why nobody >> complained so far. > > So, if I understand correct that means that inb() can be interrupted > between > setting the address and reading. If this is the case then yes, we need > spinlock if this is not the case then IMHO we wouldn't need or am I wrong? I think we speak about different things. inb() cannot be interrupted but outb() followed by inb(). For indirect accesses we need something like: /* Spinlock for cc770_isa_port_write_reg_indirect */ static DEFINE_SPINLOCK(cc770_isa_port_lock); static u8 cc770_isa_port_read_reg_indirect(const struct cc770_priv *priv, int reg) { unsigned long base = (unsigned long)priv->reg_base; unsigned long flags; u8 val; spin_lock_irqsave(&cc770_isa_port_lock, flags); outb(reg, base); val = inb(base + 1); spin_unlock_irqrestore(&cc770_isa_port_lock, flags); return val; } static void cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, int reg, u8 val) { unsigned long base = (unsigned long)priv->reg_base; unsigned long flags; spin_lock_irqsave(&cc770_isa_port_lock, flags); outb(reg, base); outb(val, base + 1); spin_unlock_irqrestore(&cc770_isa_port_lock, flags); } Hope we are in synch now. Wolfgang. -- 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
Hello Wolfgang, -------- Original Message -------- Subject: Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527 From: Wolfgang Grandegger <wg@grandegger.com> To: info@essax.com Cc: David Laight <David.Laight@ACULAB.COM>, Oliver Hartkopp <socketcan@hartkopp.net>, henrik@proconx.com, netdev@vger.kernel.org, linux-can@vger.kernel.org, socketcan-users@lists.berlios.de, IreneV <boir1@yandex.ru>, Stanislav Yelenskiy <stanislavelensky@yahoo.com>, oe@port.de, henrik@focus-sw.com Date: Tue Jan 10 2012 17:23:59 GMT+0100 (CET) > Hi Wolfgang, > > On 01/10/2012 05:13 PM, Wolfgang Zarre wrote: >> Hello Wolfgang, >> >>> On 01/10/2012 01:41 PM, Wolfgang Zarre wrote: >>>> Hello David, >>>>> >>>>>> cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, >>>>>> int reg, u8 val) >>>>>> { >>>>>> unsigned long base = (unsigned long)priv->reg_base; >>>>>> + unsigned long flags; >>>>>> >>>>>> + spin_lock_irqsave(&outb_lock, flags); >>>>>> outb(reg, base); >>>>>> outb(val, base + 1); >>>>>> + spin_unlock_irqrestore(&outb_lock, flags); >>>>> >>>>> Is there a 'read_reg_indirect' function?? >>>> >>>> Yes, there is. >>>> >>>>> If so it also needs to use the same mutex. >>>> >>>> Actually, I don't think that we have a problem with mutex >>>> beside that it's using just one inb() statement but having >>>> for sure with an interrupt between both outb() statements which >>>> seems to be critical for the cc770. >>> >>> But the indirect read function also sets the address register before >>> reading the data using inb(). This sequence should also not be >>> interrupted and therefore we need to synchronize. For the indirect >>> access of the SJA1000 we also need to add spinlocks. Wonder why nobody >>> complained so far. >> >> So, if I understand correct that means that inb() can be interrupted >> between >> setting the address and reading. If this is the case then yes, we need >> spinlock if this is not the case then IMHO we wouldn't need or am I wrong? > > I think we speak about different things. inb() cannot be interrupted but > outb() followed by inb(). For indirect accesses we need something like: > > /* Spinlock for cc770_isa_port_write_reg_indirect */ > static DEFINE_SPINLOCK(cc770_isa_port_lock); > > static u8 cc770_isa_port_read_reg_indirect(const struct cc770_priv *priv, > int reg) > { > unsigned long base = (unsigned long)priv->reg_base; > unsigned long flags; > u8 val; > > spin_lock_irqsave(&cc770_isa_port_lock, flags); > outb(reg, base); > val = inb(base + 1); > spin_unlock_irqrestore(&cc770_isa_port_lock, flags); > > return val; > } > > static void cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, > int reg, u8 val) > { > unsigned long base = (unsigned long)priv->reg_base; > unsigned long flags; > > spin_lock_irqsave(&cc770_isa_port_lock, flags); > outb(reg, base); > outb(val, base + 1); > spin_unlock_irqrestore(&cc770_isa_port_lock, flags); > } > > Hope we are in synch now. > Thanks a lot. Yes, now phase locked and in synch and sorry, by mistake I looked at the wrong function (cc770_isa_port_read_reg) in the heat of the moment. Absolutely clear, there we need a spinlock definitely. I'll start another test run just to confirm. > Wolfgang. > Wolfgang -- 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
diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c index 2d12f89..dad6707 100644 --- a/drivers/net/can/cc770/cc770.c +++ b/drivers/net/can/cc770/cc770.c @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff *skb, struct net_device *dev) stats->tx_bytes += dlc; - - /* - * HM: We had some cases of repeated IRQs so make sure the - * INT is acknowledged I know it's already further up, but - * doing again fixed the issue - */ - cc770_write_reg(priv, msgobj[mo].ctrl0, - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); - return NETDEV_TX_OK; } @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device *dev, unsigned int o) /* Nothing more to send, switch off interrupts */ cc770_write_reg(priv, msgobj[mo].ctrl0, MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); - /* - * We had some cases of repeated IRQ so make sure the - * INT is acknowledged - */ - cc770_write_reg(priv, msgobj[mo].ctrl0, - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); stats->tx_packets++; can_get_echo_skb(dev, 0); diff --git a/drivers/net/can/cc770/cc770_isa.c b/drivers/net/can/cc770/cc770_isa.c index 4be5fe2..fe39eed 100644 --- a/drivers/net/can/cc770/cc770_isa.c +++ b/drivers/net/can/cc770/cc770_isa.c @@ -110,6 +110,9 @@ MODULE_PARM_DESC(bcr, "Bus configuration register (default=0x40 [CBY])"); #define CC770_IOSIZE 0x20 #define CC770_IOSIZE_INDIRECT 0x02 +/* Spinlock for cc770_isa_port_write_reg_indirect */ +static DEFINE_SPINLOCK( outb_lock); + static struct platform_device *cc770_isa_devs[MAXDEV]; static u8 cc770_isa_mem_read_reg(const struct cc770_priv *priv, int reg) @@ -147,9 +150,12 @@ static void cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, int reg, u8 val) { unsigned long base = (unsigned long)priv->reg_base; + unsigned long flags; + spin_lock_irqsave( &outb_lock, flags); outb(reg, base); outb(val, base + 1); + spin_unlock_irqrestore( &outb_lock, flags); } static int __devinit cc770_isa_probe(struct platform_device *pdev)