Re: [PATCH V2 2/2] ems_usb: Added support for EMS CPC-USB/ARM7 CAN/USB interface

Sebastian Haas wrote:

This patch adds support for one channel CAN/USB interace CPC-USB/ARM7 from
EMS Dr. Thomas Wuensche (http://www.ems-wuensche.com).

Signed-off-by: Sebastian Haas <redacted>

The cleanup in the probe function is still not OK :-(.

[snip]

+/*
+ * probe function for new CPC-USB devices
+ */
+static int ems_usb_probe(struct usb_interface *intf,
+			 const struct usb_device_id *id)
+{
+	struct net_device *netdev;
+	struct ems_usb *dev;
+	int i, err;
+	int retval = -ENOMEM;

Why do you need an extra variable here.

	int err = -ENOMEM;

Should work fine!?

+
+	netdev = alloc_candev(sizeof(struct ems_usb));
+	if (!netdev) {
+		dev_err(netdev->dev.parent, "Couldn't alloc candev\n");
+		return -ENOMEM;
+	}
+
+	dev = netdev_priv(netdev);
+
+	dev->udev = interface_to_usbdev(intf);
+	dev->netdev = netdev;
+
+	dev->can.state = CAN_STATE_STOPPED;
+	dev->can.clock.freq = EMS_USB_ARM7_CLOCK;
+	dev->can.bittiming_const = &ems_usb_bittiming_const;
+	dev->can.do_set_bittiming = ems_usb_set_bittiming;
+	dev->can.do_set_mode = ems_usb_set_mode;
+
+	netdev->flags |= IFF_ECHO; /* we support local echo */
+
+	/*
+	 * The device actually uses a 16MHz clock to generate the CAN clock
+	 * but it expects SJA1000 bit settings based on 8MHz (is internally
+	 * converted).
+	 */
+
+	netdev->netdev_ops = &ems_usb_netdev_ops;
+
+	netdev->flags |= IFF_ECHO; /* we support local echo */
+
+	init_usb_anchor(&dev->rx_submitted);
+
+	init_usb_anchor(&dev->tx_submitted);
+	atomic_set(&dev->active_tx_urbs, 0);
+
+	for (i = 0; i < MAX_TX_URBS; i++)
+		dev->tx_contexts[i].echo_index = MAX_TX_URBS;
+
+	dev->intr_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!dev->intr_urb) {
+		dev_err(netdev->dev.parent, "Couldn't alloc intr URB\n");
+		goto cleanup_candev;
+	}
+
+	dev->intr_in_buffer = kzalloc(INTR_IN_BUFFER_SIZE, GFP_KERNEL);
+	if (!dev->intr_in_buffer) {
+		dev_err(netdev->dev.parent, "Couldn't alloc Intr buffer\n");
+		goto cleanup_intr_urb;
+	}
+
+	dev->tx_msg_buffer = kzalloc(CPC_HEADER_SIZE +
+				     sizeof(struct ems_cpc_msg), GFP_KERNEL);
+	if (!dev->tx_msg_buffer) {
+		dev_err(netdev->dev.parent, "Couldn't alloc Tx buffer\n");
+		goto cleanup_intr_in_buffer;
+	}
+
+	usb_set_intfdata(intf, dev);
+
+	SET_NETDEV_DEV(netdev, &intf->dev);
+
+	init_params_sja1000(&dev->active_params);
+
+	err = ems_usb_command_msg(dev, &dev->active_params);
+	if (err) {
+		dev_err(netdev->dev.parent,
+			"couldn't initialize controller: %d\n", err);
+		retval = err;

Could be removed then.

+		goto cleanup_tx_msg_buffer;
+	}
+
+	return register_candev(netdev);

Cleanup is missing if register_candev() fails!

+
+cleanup_tx_msg_buffer:
+	kfree(dev->tx_msg_buffer);
+
+cleanup_intr_in_buffer:
+	kfree(dev->intr_in_buffer);
+
+cleanup_intr_urb:
+	usb_free_urb(dev->intr_urb);
+
+cleanup_candev:
+	free_candev(netdev);
+
+	return retval;
+}

Wolfgang.