Re: linux driver for CM109 voip chipset

Hi Oliver

an updated version of the CM109 driver is available:
(this time as a complete patch)

http://aeh.db.org/patch/cm109-20070227.patch

see comments inline..


Oliver Neukum wrote:
> Am Montag, 26. Februar 2007 23:58 schrieb Alfred E. Heggestad:
>  
>>> static DECLARE_RWSEM(sysfs_rwsema);
>>>
>>> A mutex is enough. You never do anything but down_write().
>>>
>> ok that can be fixed. but I am not sure yet how to use the sysfs
>> interface. at the moment the external buzzer is activated via
>> doing "echo 1 > /sys/.../buzzer" so that is write only as you
>> say. But in the future we might want to read/write the EEPROM
>> via the sysfs interface.
> 
> Nevertheless a simple mutex will do. There's no need to worry
> about contention here.
> 

OK, code is now using mutex.


>> ideally I would like to use a more standard user interface
>> for the buzzer, like SND_BELL/SND_TONE. I added some code
>> in input_ev() for triggering the buzzer, but unfortunately
>> it crashes the whole box .. also I am not sure how to
> 
> How does it crash? Any oops report?
> 

turning buzzer on/off from sysfs interface works fine. But when
the same function is called from the input_ev() event handler,
it will start the buzzer, and hang the whole machine with two
LEDs flashing, nothing in the logs..

mind you, the function input_ev() is actually called 3 times,
so there might be some restrictions on what can be done in
an input event handler. I tried various locks but nothing
seem to help..


>> actually turn *off* the buzzer if using the input event API.
>> any ideas?
> 
> You might ask on the input list.
>  

ok, we can probably disable buzzer when the keypad is pressed.


>>> static void input_close(struct input_dev *dev)
>>> {
>>> 	struct cm109_dev *cmd = dev->private;
>>>
>>> 	usb_kill_urb(cmd->urb_ctl);
>>> 	usb_kill_urb(cmd->urb_irq);
>>> }
>>>
>>> This is a race condition. Due to your special requirement of submitting
>>> the URBs from each other's completion handler the guarantees of usb_kill_urb()
>>> are not strong enough for you. You need to use a flag under a spin_lock.
>>>
>> I added "struct mutex io_mutex" to my device object. Should this be
>> locked in the USB irq/ctl handlers ?
> 
> No. You cannot use a mutex in interrupt. In fact that mutex currently has no
> function in your code.
> 
> The problem is as follows:
> 
> CPU A						CPU B
> urb_irq_callback() called
> 							usb_kill_urb(urb_irq) //will wait for the callback to finish
> usb_submit_urb(urb_ctl) //still allowed
> urb_ctl_callback()
> usb_submit_urb(urb_irq)
> 							usb_kill_urb(urb_ctl)
> 
> Your URBs are still running.
> You need to make conditional submitting the URBs on a flag and
> the whole thing protected by a spinlock, eg in urb_ctl_callback:
> 
> 	spin_lock(&dev->submit_lock);
> 	/* ask for a response */
> 	if (!dev->disconnecting)
> 		ret = usb_submit_urb(dev->urb_irq, GFP_KERNEL);
> 	spin_unlock(&dev->submit_lock);
> 
> And in usb_disconnect:
> 
> 	spin_lock_irq(&dev->submit_lock);
> 	dev->disconnecting = 1;
> 	spin_unlock_irq(&dev->submit_lock);
> and now cleanup

ok that should be fixed now. thanks for the help.

should the same be done in urb_irq_callback() ?


/alfred

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel