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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d120d5000703070647t223f8858i8c495dcc3e1378a2@mail.gmail.com>
Date:	Wed, 7 Mar 2007 09:47:08 -0500
From:	"Dmitry Torokhov" <dmitry.torokhov@...il.com>
To:	"Jiri Slaby" <jirislaby@...il.com>
Cc:	"Andrew Morton" <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-input@...ey.karlin.mff.cuni.cz
Subject: Re: [PATCH 1/1] Input: add sensable phantom driver

Hi Jiri,

On 3/7/07, Jiri Slaby <jirislaby@...il.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?

> +
> +#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?
> +               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.

> +
> +       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.

-- 
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/