Date:	Thu, 22 Aug 2013 03:09:11 +0200
From:	Yann Cantin <yann.cantin@...oste.net>
To:	Oliver Neukum <oliver@...kum.org>
CC:	linux-input@...r.kernel.org, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	dmitry.torokhov@...il.com, jkosina@...e.cz, rob@...dley.net,
	gregkh@...uxfoundation.org
Subject: Re: [PATCH 2/2] input: misc: New USB eBeam input driver

Hi,

All issues addressed. Just a point :

>> +
>> +	/* input final setup */
>> +	ebeam_setup_input(ebeam, input_dev);
>> +
>> +	err = input_register_device(ebeam->input);
>> +	if (err) {
>> +		dev_dbg(&intf->dev,
>> +			"%s - input_register_device failed, err: %d\n",
>> +			__func__, err);
>> +		goto out_free_urb;
>> +	}
>> +
>> +	/* usb final setup */
>> +	usb_set_intfdata(intf, ebeam);
>> +
>> +	/* sysfs setup */
>> +	err = sysfs_create_group(&intf->dev.kobj, &ebeam_attr_group);
>> +	if (err) {
>> +		dev_dbg(&intf->dev,
>> +			"%s - cannot create sysfs group, err: %d\n",
>> +			__func__, err);
>> +		goto out_unregister_input;
>> +	}
>
> This is not nice. User space may react to a new input device of this
> type by setting up the calibration. But the sysfs files may be created
> after that. You should invert the order.

Switch done. Could you please double check the goto out-* cleanup, i've spotted
a missing usb_set_intfdata(intf, NULL), perhaps there's others missing steps :

static int ebeam_probe(struct usb_interface *intf,
		       const struct usb_device_id *id)
{
	struct ebeam_device *ebeam;
	struct input_dev *input_dev;
	struct usb_endpoint_descriptor *endpoint;
	struct usb_device *udev = interface_to_usbdev(intf);
	int err = -ENOMEM;

	endpoint = ebeam_get_input_endpoint(intf->cur_altsetting);
	if (!endpoint)
		return -ENXIO;

	ebeam = kzalloc(sizeof(struct ebeam_device), GFP_KERNEL);
	input_dev = input_allocate_device();
	if (!ebeam || !input_dev)
		goto out_free;

	ebeam_init_settings(ebeam);

	ebeam->data = usb_alloc_coherent(udev, REPT_SIZE,
					 GFP_KERNEL, &ebeam->data_dma);
	if (!ebeam->data)
		goto out_free;

	ebeam->irq = usb_alloc_urb(0, GFP_KERNEL);
	if (!ebeam->irq) {
		dev_dbg(&intf->dev,
			"%s - usb_alloc_urb failed: ebeam->irq\n", __func__);
		goto out_free_buffers;
	}

	ebeam->interface = intf;
	ebeam->input = input_dev;

	/* setup name */
	snprintf(ebeam->name, sizeof(ebeam->name),
		 "USB eBeam %04x:%04x",
		 le16_to_cpu(udev->descriptor.idVendor),
		 le16_to_cpu(udev->descriptor.idProduct));

	if (udev->manufacturer || udev->product) {
		strlcat(ebeam->name,
			" (",
			sizeof(ebeam->name));

		if (udev->manufacturer)
			strlcat(ebeam->name,
				udev->manufacturer,
				sizeof(ebeam->name));

		if (udev->product) {
			if (udev->manufacturer)
				strlcat(ebeam->name,
					" ",
					sizeof(ebeam->name));
			strlcat(ebeam->name,
				udev->product,
				sizeof(ebeam->name));
		}

		if (strlcat(ebeam->name, ")", sizeof(ebeam->name))
			>= sizeof(ebeam->name)) {
			/* overflowed, closing ) anyway */
			ebeam->name[sizeof(ebeam->name)-2] = ')';
		}
	}

	/* usb tree */
	usb_make_path(udev, ebeam->phys, sizeof(ebeam->phys));
	strlcat(ebeam->phys, "/input0", sizeof(ebeam->phys));

	/* input setup */
	input_dev->name = ebeam->name;
	input_dev->phys = ebeam->phys;
	usb_to_input_id(udev, &input_dev->id);
	input_dev->dev.parent = &intf->dev;

	input_set_drvdata(input_dev, ebeam);

	input_dev->open = ebeam_open;
	input_dev->close = ebeam_close;

	/* usb urb setup */
	if (usb_endpoint_type(endpoint) == USB_ENDPOINT_XFER_INT)
		usb_fill_int_urb(ebeam->irq, udev,
			usb_rcvintpipe(udev, endpoint->bEndpointAddress),
			ebeam->data, REPT_SIZE,
			ebeam_irq, ebeam, endpoint->bInterval);
	else
		usb_fill_bulk_urb(ebeam->irq, udev,
			usb_rcvbulkpipe(udev, endpoint->bEndpointAddress),
			ebeam->data, REPT_SIZE,
			ebeam_irq, ebeam);

	ebeam->irq->dev = udev;
	ebeam->irq->transfer_dma = ebeam->data_dma;
	ebeam->irq->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;

	/* usb final setup */
	usb_set_intfdata(intf, ebeam);

	/* sysfs setup */
	err = sysfs_create_group(&intf->dev.kobj, &ebeam_attr_group);
	if (err) {
		dev_dbg(&intf->dev,
			"%s - cannot create sysfs group, err: %d\n",
			__func__, err);
		goto out_free_usb;
	}

	/* input final setup */
	ebeam_setup_input(ebeam, input_dev);

	err = input_register_device(ebeam->input);
	if (err) {
		dev_dbg(&intf->dev,
			"%s - input_register_device failed, err: %d\n",
			__func__, err);
		goto out_remove_sysfs;
	}

	return 0;

out_remove_sysfs:
	sysfs_remove_group(&intf->dev.kobj, &ebeam_attr_group);
out_free_usb:
	usb_set_intfdata(intf, NULL);
	usb_free_urb(ebeam->irq);
out_free_buffers:
	ebeam_free_buffers(udev, ebeam);
out_free:
	input_free_device(input_dev);
	kfree(ebeam);
	return err;
}


Thanks,

--
Yann Cantin
A4FEB47F
--
--
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/

Powered by blists - more mailing lists