| 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
| ||
|
Message-ID: <575AC03C.2010306@posteo.de>
Date: Fri, 10 Jun 2016 15:27:24 +0200
From: Martin Kepplinger <martink@...teo.de>
To: Oliver Neukum <oneukum@...e.com>
Cc: dmitry.torokhov@...il.com, kernel-testers@...r.kernel.org,
linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-usb@...r.kernel.org
Subject: Re: [PATCH v7] input: tablet: add Pegasus Notetaker tablet driver
Am 2016-06-09 um 17:00 schrieb Oliver Neukum:
> On Wed, 2016-06-01 at 14:55 +0200, Martin Kepplinger wrote:
>
>> It's *really* fun to use as an input tablet though! So let's support this
>> for everybody.
>
> Nice job, but a few issues are left. I'll comment in the code.
>
Hi Oliver,
Thanks for reviewing again. I'll change PM a little.
martin
>
>> +static void pegasus_close(struct input_dev *dev)
>> +{
>> + struct pegasus *pegasus = input_get_drvdata(dev);
>> + int autopm_error;
>> +
>> + autopm_error = usb_autopm_get_interface(pegasus->intf);
>
> There is no need for this.
>
>> + usb_kill_urb(pegasus->irq);
>> + cancel_work_sync(&pegasus->init);
>> +
>> + if (!autopm_error)
>> + usb_autopm_put_interface(pegasus->intf);
>> +}
>> +
>> +static int pegasus_probe(struct usb_interface *intf,
>> + const struct usb_device_id *id)
>> +{
>> + struct usb_device *dev = interface_to_usbdev(intf);
>> + struct usb_endpoint_descriptor *endpoint;
>> + struct pegasus *pegasus;
>> + struct input_dev *input_dev;
>> + int error;
>> + int pipe, maxp;
>> +
>> + /* We control interface 0 */
>> + if (intf->cur_altsetting->desc.bInterfaceNumber == 1)
>> + return -ENODEV;
>> +
>> + /* Sanity check that the device has an endpoint */
>> + if (intf->altsetting[0].desc.bNumEndpoints < 1) {
>> + dev_err(&intf->dev, "Invalid number of endpoints\n");
>> + return -EINVAL;
>> + }
>> + endpoint = &intf->cur_altsetting->endpoint[0].desc;
>> +
>> + pegasus = kzalloc(sizeof(*pegasus), GFP_KERNEL);
>> + input_dev = input_allocate_device();
>> + if (!pegasus || !input_dev) {
>> + error = -ENOMEM;
>> + goto err_free_mem;
>> + }
>> +
>> + pegasus->usbdev = dev;
>> + pegasus->dev = input_dev;
>> + pegasus->intf = intf;
>> +
>> + pegasus->data = usb_alloc_coherent(dev, endpoint->wMaxPacketSize,
>> + GFP_KERNEL, &pegasus->data_dma);
>> + if (!pegasus->data) {
>> + error = -ENOMEM;
>> + goto err_free_mem;
>> + }
>> +
>> + pegasus->irq = usb_alloc_urb(0, GFP_KERNEL);
>> + if (!pegasus->irq) {
>> + error = -ENOMEM;
>> + goto err_free_dma;
>> + }
>> +
>> + pipe = usb_rcvintpipe(dev, endpoint->bEndpointAddress);
>> + maxp = usb_maxpacket(dev, pipe, usb_pipeout(pipe));
>> + pegasus->data_len = maxp;
>> +
>> + usb_fill_int_urb(pegasus->irq, dev, pipe, pegasus->data, maxp,
>> + pegasus_irq, pegasus, endpoint->bInterval);
>> +
>> + pegasus->irq->transfer_dma = pegasus->data_dma;
>> + pegasus->irq->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>> +
>> + if (dev->manufacturer)
>> + strlcpy(pegasus->name, dev->manufacturer,
>> + sizeof(pegasus->name));
>> +
>> + if (dev->product) {
>> + if (dev->manufacturer)
>> + strlcat(pegasus->name, " ", sizeof(pegasus->name));
>> + strlcat(pegasus->name, dev->product, sizeof(pegasus->name));
>> + }
>> +
>> + if (!strlen(pegasus->name))
>> + snprintf(pegasus->name, sizeof(pegasus->name),
>> + "USB Pegasus Device %04x:%04x",
>> + le16_to_cpu(dev->descriptor.idVendor),
>> + le16_to_cpu(dev->descriptor.idProduct));
>> +
>> + usb_make_path(dev, pegasus->phys, sizeof(pegasus->phys));
>> + strlcat(pegasus->phys, "/input0", sizeof(pegasus->phys));
>> +
>> + INIT_WORK(&pegasus->init, pegasus_init);
>> +
>> + usb_set_intfdata(intf, pegasus);
>> +
>> + input_dev->name = pegasus->name;
>> + input_dev->phys = pegasus->phys;
>> + usb_to_input_id(dev, &input_dev->id);
>> + input_dev->dev.parent = &intf->dev;
>> +
>> + input_set_drvdata(input_dev, pegasus);
>> +
>> + input_dev->open = pegasus_open;
>> + input_dev->close = pegasus_close;
>> +
>> + __set_bit(EV_ABS, input_dev->evbit);
>> + __set_bit(EV_KEY, input_dev->evbit);
>> +
>> + __set_bit(ABS_X, input_dev->absbit);
>> + __set_bit(ABS_Y, input_dev->absbit);
>> +
>> + __set_bit(BTN_TOUCH, input_dev->keybit);
>> + __set_bit(BTN_RIGHT, input_dev->keybit);
>> + __set_bit(BTN_TOOL_PEN, input_dev->keybit);
>> +
>> + __set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>> + __set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>> +
>> + input_set_abs_params(input_dev, ABS_X, -1500, 1500, 8, 0);
>> + input_set_abs_params(input_dev, ABS_Y, 1600, 3000, 8, 0);
>> +
>> + pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
>
> If you need to do this here. It is also needed in reset_resume()
>
This is done in open() now.
>> + error = input_register_device(pegasus->dev);
>> + if (error)
>> + goto err_free_urb;
>> +
>> + return 0;
>> +
>> +err_free_urb:
>> + usb_free_urb(pegasus->irq);
>> +err_free_dma:
>> + usb_free_coherent(dev, pegasus->data_len,
>> + pegasus->data, pegasus->data_dma);
>> +err_free_mem:
>> + input_free_device(input_dev);
>> + kfree(pegasus);
>> + usb_set_intfdata(intf, NULL);
>> +
>> + return error;
>> +}
>> +
>> +static void pegasus_disconnect(struct usb_interface *intf)
>> +{
>> + struct pegasus *pegasus = usb_get_intfdata(intf);
>> +
>> + input_unregister_device(pegasus->dev);
>> +
>> + usb_free_urb(pegasus->irq);
>> + usb_free_coherent(interface_to_usbdev(intf),
>> + pegasus->data_len, pegasus->data,
>> + pegasus->data_dma);
>> + kfree(pegasus);
>> + usb_set_intfdata(intf, NULL);
>> +}
>> +
>> +static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
>> +{
>> + struct pegasus *pegasus = usb_get_intfdata(intf);
>> +
>> + mutex_lock(&pegasus->dev->mutex);
>> + usb_kill_urb(pegasus->irq);
>
> You must also stop the scheduled work.
thanks!
>
>> + mutex_unlock(&pegasus->dev->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static int pegasus_resume(struct usb_interface *intf)
>> +{
>> + struct pegasus *pegasus = usb_get_intfdata(intf);
>> + int retval = 0;
>> +
>> + mutex_lock(&pegasus->dev->mutex);
>> + if (pegasus->dev->users && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
>> + retval = -EIO;
>> + mutex_unlock(&pegasus->dev->mutex);
>> +
>> + return retval;
>> +}
>> +
>> +static int pegasus_reset_resume(struct usb_interface *intf)
>> +{
>
> Either the mode must be set on fresh hardware, or this is unnecessary.
>
>> + return pegasus_resume(intf);
>> +}
>
> HTH
> Oliver
>
>
Powered by blists - more mailing lists