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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
