[PATCH]2.5.30 i2c updates 3/4

[ac9410@attbi.com (Albert Cranford)]

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@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@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@bellsouth.net