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
