I have checked in CVS the fix for the cli locking on the stack. That was my mistake. I have removed the "#ifdef" compatibility stuff. I was refering to Russels comments about: 1) Scheduling with interrupts disabled. 2) Calling disable_irq before free_irq. 3) Loop without relax_cpu. 4) Calling invalidate_dcache_range improperly. 5) Checking if irq > 0 while 0 is valid. 6) Checking region before probing. "Mark D. Studebaker" wrote: > > Specifically it was sti/cli removal. > I guess some of the issues raised in the mail below were related > and some were not. > > Forgive me, I am irq/locking stupid. > Albert Cranford, who is on the sensors mailing list, > is creating and submitting the patches; > his patches are here > http://personal.atl.bellsouth.net/mia/a/c/ac9410/albert/albert.html > and his email is at the bottom of that page. > > I think the 8xx driver also needs mods; Albert can fill you in. > Any help you can give Albert directly, and (especially) > pointers to anybody else doing the work or if you can > verify that the 405/8xx drivers we have are the latest would be > most helpful. > > thanks > mds > > akuster wrote: > > > > Mark D. Studebaker wrote: > > > Armin, Tom, > > > As you know we checked in the 405 drivers you submitted to us in late > > > March > > > and they are now in our 2.6.4 release. > > > > > > We are attempting to fix up the locking in the 405 drivers for > > > submission > > > to Linus. Can you help? > > > > > > thanks > > > mds > > > > > > > > > > > > Albert Cranford wrote: > > > > > >>Linus, please hold off incorporating all 4 patches until we > > >>can get corrections. Thanks. > > >> > > >>Rusty, > > >>Couple of quick comments while I forward to I2C authors. > > >>First thanks for your effort to review our code. It is > > >>very much appreciated. Next time I'll post to kernel list > > >>before submitting to Linus. > > >> > > >>I meant to remove the compatibility stuff from 2.5. > > >>Something I forgot to check in the new drivers. > > >> > > >>The cli&sti replacement with driver_lock should be at the > > >>module level and not proc level for SMP safety. I'll > > >>change this in all three drivers. > > >> > > >>The requirement to support I2C and Sensors for releases > > >>2.2 through current is very challenging. I took your > > >>advice from the last time, but haven't completed the work > > >>yet. > > >> > > >>The important clues you provided will be reviewed and incorporated in the next patch attempt. > > >>Thanks again, > > >>Albert > > >> > > >>Russell King wrote: > > >> > > >>>The following are _some_ comments from a quick read through; they're not > > >>>a thorough review, so there's probably lots of stuff I've missed. But I > > >>>felt I couldn't carry on reading anything else on lkml... 8) > > >>> > > >>>On Wed, Aug 07, 2002 at 01:29:03AM -0400, Albert Cranford wrote: > > >>> > > >>>>+static void iic_ibmocp_waitforpin(void *data) { > > >>>>+ > > >>>>+ int timeout = 2; > > >>>>+ struct iic_ibm *priv_data = data; > > >>>>+ spinlock_t driver_lock = SPIN_LOCK_UNLOCKED; > > >>>> > > >>>What's the point of a spinlock on the stack? It doesn't gain you any > > >>>protection against other threads on a SMP system. > > >>> > > >>> > > >>>>+ if (priv_data->iic_irq > 0) { > > >>>>+ spin_lock_irq(&driver_lock); > > >>>>+ if (iic_pending == 0) { > > >>>>+ interruptible_sleep_on_timeout(&(iic_wait[priv_data->index]), timeout*HZ ); > > >>>> > > >>>You shouldn't be scheduling with interrupts disabled, spinlock locked. > > >>>Using a derivative of sleep_on(). Please look at wait_event() and > > >>>interruptible_wait_event(). Also, interruptible_sleep_on() returns a > > >>>value to tell you if it was interrupted... > > >>> > > >>> > > >>>>+ for(i=0; i<IIC_NUMS; i++) { > > >>>>+ struct iic_ibm *priv_data = (struct iic_ibm *)iic_ibmocp_data[i]->data; > > >>>>+ if (priv_data->iic_irq > 0) { > > >>>>+ disable_irq(priv_data->iic_irq); > > >>>>+ free_irq(priv_data->iic_irq, 0); > > >>>> > > >>>You shouldn't be calling disable_irq() just before free_irq(). Firstly, if > > >>>the interrupt is being shared, then you just killed the other devices using > > >>>that IRQ. Secondly, free_irq will disable the interrupt source for you, so > > >>>its redundant anyway. > > >>> > > >>> > > >>>>+ if (cpm->reloc == 0) { > > >>>>+ volatile cpm8xx_t *cp = cpm->cp; > > >>>>+ > > >>>>+ if (cpm_debug) printk("force_close()\n"); > > >>>>+ cp->cp_cpcr = > > >>>>+ mk_cr_cmd(CPM_CR_CH_I2C, CPM_CR_CLOSE_RXBD) | > > >>>>+ CPM_CR_FLG; > > >>>>+ > > >>>>+ while (cp->cp_cpcr & CPM_CR_FLG); > > >>>> > > >>>Ouch. There should be a call to cpu_relax() in this (and any other such) > > >>>while loops. (IIRC Athlons tend to self-destruct without this.) > > >>> > > >>> > > >>>>+ invalidate_dcache_range((unsigned long) buf, (unsigned long) (buf+count)); > > >>>> > > >>>This is only defined on ARM and PPC; on ARM its not intended to be called > > >>>by any driver directly (it should be using the pci_* DMA consistency stuff). > > >>>On PPC, it looks like it should be a call to dma_cache_inv() which is the > > >>>more accepted interface. You can find them in include/asm-ppc/io.h > > >>> > > >>> > > >>>>+ > > >>>>+ /* Chip bug, set enable here */ > > >>>>+ local_irq_save(flags); > > >>>>+ i2c->i2c_i2cmr = 0x13; /* Enable some interupts */ > > >>>>+ i2c->i2c_i2cer = 0xff; > > >>>>+ i2c->i2c_i2mod = 1; /* Enable */ > > >>>>+ i2c->i2c_i2com = 0x81; /* Start master */ > > >>>>+ > > >>>>+ /* Wait for IIC transfer */ > > >>>>+ interruptible_sleep_on(&iic_wait); > > >>>> > > >>>Again, sleeping with interrupts disabled... > > >>> > > >>> > > >>>>+ flush_dcache_range((unsigned long) tb, (unsigned long) (tb+1)); > > >>>>+ flush_dcache_range((unsigned long) buf, (unsigned long) (buf+count)); > > >>>> > > >>>Same comments as invalidate_dcache_range(); dma_cache_wback_inv(). > > >>> > > >>> > > >>>>+static void pcf_epp_waitforpin(void) { > > >>>>+ int timeout = 10; > > >>>>+ spinlock_t driver_lock = SPIN_LOCK_UNLOCKED; > > >>>>+ > > >>>>+ if (gpe.pe_irq > 0) { > > >>>>+ spin_lock_irq(&driver_lock); > > >>>>+ if (pcf_pending == 0) { > > >>>>+ interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ); > > >>>>+ //udelay(100); > > >>>>+ } else { > > >>>>+ pcf_pending = 0; > > >>>>+ } > > >>>>+ spin_unlock_irq(&driver_lock); > > >>>> > > >>>See above. > > >>> > > >>> > > >>>>+ if (gpe.pe_irq > 0) { > > >>>> > > >>>An IRQ number of zero is completely valid on some systems. > > >>> > > >>> > > >>>>+ if (request_irq(gpe.pe_irq, pcf_epp_handler, 0, "PCF8584", 0) < 0) { > > >>>>+ printk(KERN_NOTICE "i2c-pcf-epp.o: Request irq%d failed\n", gpe.pe_irq); > > >>>>+ gpe.pe_irq = 0; > > >>>>+ } else > > >>>>+ disable_irq(gpe.pe_irq); > > >>>>+ enable_irq(gpe.pe_irq); > > >>>> > > >>>The indentation here makes the code look broken. However, I suspect the > > >>>bug is balanced out because of the following bit of code: > > >>> > > >>> > > >>>>+static void pcf_epp_exit(void) > > >>>>+{ > > >>>>+ if (gpe.pe_irq > 0) { > > >>>>+ disable_irq(gpe.pe_irq); > > >>>>+ free_irq(gpe.pe_irq, 0); > > >>>> > > >>>See comments about disable_irq/free_irq above. > > >>> > > >>> > > >>>>+static int bit_pport_init(void) > > >>>>+{ > > >>>>+ //release_region( (base+2) ,1); > > >>>> > > >>>Eww. Please don't give people ideas about releasing someone elses > > >>>resources. > > >>> > > >>> > > >>>>+ if (check_region((base+2),1) < 0 ) { > > >>>> > > >>>You should request the region first, then probe. If the probe fails, > > >>>release the region. Using check_region is not safe on any 2.2 or later > > >>>kernel (hint: you can sleep in request_region). > > >>> > > >>>And finally, I think keeping all the ifdef crap for "I want i2c to build > > >>>across any kernel what so ever" is really bad. For an example how to > > >>>handle this, please see the jffs2/mtd code which has more or less the > > >>>same requirement, but without the pain of having to put lots of ifdefs > > >>>in the code. The code is written to support the latest kernel, and the > > >>>compatibility stuff is tucked away inside a header file. > > >>> > > >>>-- > > >>>Russell King (rmk at arm.linux.org.uk) The developer of ARM Linux > > >>> http://www.arm.linux.org.uk/personal/aboutme.html > > >>> > > >>-- > > >>Albert Cranford Deerfield Beach FL USA > > >>ac9410 at bellsouth.net > > >> > > > > Mark & others, > > > > By fix up, you mean whats described in this mail thread? or is there > > something else. > > > > Armin -- Albert Cranford Deerfield Beach FL USA ac9410 at bellsouth.net
