[PATCH]2.5.30 i2c updates 3/4
Albert Cranford
ac9410 at attbi.com
Fri Aug 9 04:11:46 CEST 2002
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
More information about the lm-sensors
mailing list