Re: [PATCH 1/1] Input: add sensable phantom driver

[Jiri Slaby <jirislaby@gmail.com>]

Dmitry Torokhov napsal(a):
> Hi Jiri,

Hi.

> On 3/7/07, Jiri Slaby <jirislaby@gmail.com> wrote:
>> add sensable phantom driver
> ...
> 
> General question - can this driver use force-feedback mecahnisms
> already present in kernel instead of exporting raw datastream to
> userspace. What are shortcomings of kernels force-feedback
> implemenattion that make it insufficient for phantom?

I didn't even think about it, sorry. Does any problem with rate about 2kHz come 
on your mind before I'll try to rewrite it?

>> +#define phantom_remap(io, vma, addr)   ({                              \
>> +       vma->vm_pgoff = (addr) >> PAGE_SHIFT;                           \
>> +       io ## remap_pfn_range(vma, (vma)->vm_start, (vma)->vm_pgoff,    \
>> +               (vma)->vm_end - (vma)->vm_start, (vma)->vm_page_prot);  \
>> +})
>> +
>> +static int phantom_mmap(struct file *file, struct vm_area_struct *vma)
>> +{
>> +       struct phantom_device *dev = file->private_data;
>> +       int retval;
>> +
>> +       switch (vma->vm_pgoff) {
>> +       case 0:
>> +               retval = phantom_remap(, vma, virt_to_phys(dev->stat));
> 
> This really hurts my eyes. Is there any reason for using the macro here?

Not necessarilly. (It remained here from times, when this driver was not 
intended to be merged, since rtc workaround. Now we have a source of the 2.4 
driver.). Anyway, if the ff layer was used, I'll get the whole mmap away.

>> +               break;
>> +       case 1:
>> +               retval = phantom_remap(io_, vma, dev->ibase);
>> +               break;
>> +       default:
>> +               retval = phantom_remap(io_, vma, dev->obase);
>> +       }
>> +
>> +       return retval ? -EINVAL : 0;
>> +}
>> +
>> +
>> +static int __devinit phantom_probe(struct pci_dev *pdev,
>> +       const struct pci_device_id *pci_id)
>> +{
>> +       struct phantom_device *pht;
>> +       unsigned int minor;
>> +       int retval;
>> +
>> +       retval = pci_enable_device(pdev);
>> +       if (retval)
>> +               goto err;
>> +
>> +       minor = phantom_get_free();
>> +       if (minor == PHANTOM_MAX_MINORS) {
>> +               dev_err(&pdev->dev, "too many devices found!\n");
>> +               retval = -EIO;
>> +               goto err;
>> +       }
>> +
>> +       phantom_devices[minor] = 1;
> 
> Locking? In face of multithreaded PCI probes it might be needed.

I think, this is the same issue?

<cite from="akpm" date="12/19/06">
On Sat, 16 Dec 2006 02:09:48 +0100 (CET)
Jiri Slaby <jirislaby@gmail.com> wrote:

 > isicom, fix probe race
 >
 > Fix two race conditions in the probe function with mutex.
 >
 > ...
 >
 >  static int __devinit isicom_probe(struct pci_dev *pdev,
 >       const struct pci_device_id *ent)
 >  {
 > +     static DEFINE_MUTEX(probe_lock);

hm.  How can isicom_probe() race with itself?  Even with the dreaded
multithreaded-pci-probing?  It's only called once, by a single thread.

Confused.
</cite>

What do you think? Greg?

>> +
>> +       retval = pci_request_regions(pdev, "phantom");
>> +       if (retval)
>> +               goto err_null;
>> +
>> +       retval = -ENOMEM;
>> +       pht = kzalloc(sizeof(*pht), GFP_KERNEL);
>> +       if (pht == NULL) {
>> +               dev_err(&pdev->dev, "unable to allocate device\n");
>> +               goto err_reg;
>> +       }
>> +
>> +       pht->stat = (void *)__get_free_page(GFP_KERNEL | __GFP_WAIT);
>> +       if (pht->stat == NULL)
>> +               goto err_fr;
>> +
>> +       pht->caddr = pci_iomap(pdev, 0, 0);
>> +       if (pht->caddr == NULL) {
>> +               dev_err(&pdev->dev, "can't remap conf space\n");
>> +               goto err_frp;
>> +       }
>> +       pht->ibase = pci_resource_start(pdev, 2);
>> +       pht->obase = pci_resource_start(pdev, 3);
>> +
>> +       mutex_init(&pht->open_lock);
>> +       init_waitqueue_head(&pht->wait);
>> +
>> +       phantom_write_cfgl(pht, 0, PHN_IRQCTL);
>> +       retval = request_irq(pdev->irq, phantom_isr,
>> +                       IRQF_SHARED | IRQF_DISABLED, "phantom", pht);
>> +       if (retval) {
>> +               dev_err(&pdev->dev, "can't establish ISR\n");
>> +               goto err_unm;
>> +       }
>> +
>> +       cdev_init(&pht->cdev, &phantom_file_ops);
>> +       pht->cdev.owner = THIS_MODULE;
>> +       retval = cdev_add(&pht->cdev, MKDEV(phantom_major, minor), 1);
>> +       if (retval) {
>> +               dev_err(&pdev->dev, "chardev registration failed\n");
>> +               goto err_irq;
>> +       }
>> +
>> +       device_create(phantom_class, &pdev->dev, MKDEV(phantom_major, 
>> minor),
>> +                       "phantom%u", minor);
>> +
> 
> Error handling is needed.

Creating of sysfs node is not requisite, isn't it? (But yes, I'll at least spit 
some warning out.)

thanks for comments,
-- 
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