Re: [PATCH -v2 1/1] Misc: add sensable phantom driver

Jiri Slaby
2007-05-03T12:40:21Z

Andrew Morton napsal(a):
> On Wed, 2 May 2007 17:56:01 +0200 (CEST) Jiri Slaby <jirislaby@gmail.com> wrote:
>
>> add sensable phantom driver
>
> <googles>
>
> Is that one of these?
> http://www.est-kl.com/aufbau_general/index_hard_haptic.html?http://www.est-kl.com/hardware/haptic/sensable/phantom_omni.html

Omni is a small ieee1394 device. This is much bigger phantom premium pci
version:
http://www.sensable.com/haptic-phantom-premium-6dof.htm

> I don't think we've ever had a driver for a device like that before. I
> wonder how you designed the userspace interface to it.

I used the userspace libs that worked on 2.4 linux and rewrote them a little
bit. However this is the old way, obsoleted I think, but it's still used in
our human-computer interaction lab.

The other one is through supported OpenHaptics suite, which was added in
this version -- that is the original Sensable (the device manufacturer)
product, but they provide 2.4 linux drivers only with it.

>> +static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd,
>> + u_long arg)
>
> Could we please use standard-looking types in this driver? u32 and u64 and
> things?

Huh, wonders how this got there, thanks!

> However in this case, as it's an ioctl handler it should be "int" and
> "unsigned long".
>
> Does this driver require compat handling? (Seems not?)

>> +static int phantom_release(struct inode *inode, struct file *file)
>> +{
>> + struct phantom_device *dev = file->private_data;
>> +
>> + if (mutex_lock_interruptible(&dev->open_lock))
>> + return -ERESTARTSYS;
>> +
>> + dev->opened = 0;
>> + phantom_status(dev, dev->status & ~PHB_RUNNING);
>> +
>> + mutex_unlock(&dev->open_lock);
>> +
>> + return 0;
>> +}
>
> The mutex_lock_interruptible() is wrong, I think. This function is called
> when we do the final close(). If the signal _is_ taken then it's game
> over: the device can no longer be opened.

Yes, starving at sys_close, I think so.

<fast searching in drivers/*>

Wouldn't this be a problem here too:
drivers/media/dvb/cinergyT2/cinergyT2.c
drivers/usb/misc/auerswald.c
at least that the device won't be stopped or something like that?

>> +/*
>> + * Init and deinit driver
>> + */
>> +
>> +static unsigned int __devinit phantom_get_free(void)
>> +{
>> + unsigned int i;
>> +
>> + for (i = 0; i < PHANTOM_MAX_MINORS; i++)
>> + if (phantom_devices[i] == 0)
>> + break;
>> +
>> + return i;
>> +}
>
> This function assumes that the phantom_devices[] array will never have any
> holes in it. But if phantom_remove() is called out-of-order, it _will_
> have holes. Perhaps - I didn't look too hard.

I'm afraid I didn't get this. It goes through all phantom_devices and
returns first free index. If a hole is present it will return "i" which
corresponds to the hole, because phantom_devices[i] is 0. Note that it is
unsigned char array.

thanks for the clues,
--
http://www.fi.muni.cz/~xslaby/ Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8 22A0 32CC 55C3 39D4 7A7E