Re: [PATCH 3/3] can/peak_usb: CAN-FD: driver's core modifications

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hello Stephane,

I have two more questions:

On 27.11.2014 11:32, Stephane Grosjean wrote:

> @@ -322,7 +341,9 @@ static netdev_tx_t peak_usb_ndo_start_xmit(struct sk_buff *skb,
>   	}
>
>   	context->echo_index = i;
> -	context->dlc = cf->can_dlc;
> +
> +	/* Note: this works with CANFD frames too */
> +	context->data_len = cf->can_dlc;
>
>   	usb_anchor_urb(urb, &dev->tx_submitted);
>

Even with this comment the assignment looks fishy.

What about defining

	struct canfd_frame *cfd = (struct can_frame *)skb->data;

instead of

	struct can_frame *cf = (struct can_frame *)skb->data;

at the top of the function and assign

	context->data_len = cfd->len;

??


> @@ -679,19 +700,43 @@ static int peak_usb_set_mode(struct net_device *netdev, enum can_mode mode)
>   }
>
>   /*
> - * candev callback used to set device bitrate.
> + * candev callback used to set device nominal bitrate.

Is this the nominal bitrate or the arbitration bitrate ?

What about stating both:
candev callback used to set device nominal/arbitration bitrate.


> @@ -735,7 +780,7 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
>   	dev->cmd_buf = kmalloc(PCAN_USB_MAX_CMD_LEN, GFP_KERNEL);
>   	if (!dev->cmd_buf) {
>   		err = -ENOMEM;
> -		goto lbl_set_intf_data;
> +		goto lbl_free_candev;

These label changes

>   	}
>
>   	dev->udev = usb_dev;
> @@ -750,9 +795,10 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
>   	dev->can.clock = peak_usb_adapter->clock;
>   	dev->can.bittiming_const = &peak_usb_adapter->bittiming_const;
>   	dev->can.do_set_bittiming = peak_usb_set_bittiming;
> +	dev->can.data_bittiming_const = &peak_usb_adapter->data_bittiming_const;
> +	dev->can.do_set_data_bittiming = peak_usb_set_data_bittiming;
>   	dev->can.do_set_mode = peak_usb_set_mode;
> -	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
> -				      CAN_CTRLMODE_LISTENONLY;
> +	dev->can.ctrlmode_supported = peak_usb_adapter->ctrlmode_supported;
>
>   	netdev->netdev_ops = &peak_usb_netdev_ops;
>
> @@ -770,12 +816,11 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
>   	usb_set_intfdata(intf, dev);
>
>   	SET_NETDEV_DEV(netdev, &intf->dev);
> -	netdev->dev_id = ctrl_idx;
>
>   	err = register_candev(netdev);
>   	if (err) {
>   		dev_err(&intf->dev, "couldn't register CAN device: %d\n", err);
> -		goto lbl_free_cmd_buf;
> +		goto lbl_set_intf_data;

here

>   	}
>
>   	if (dev->prev_siblings)
> @@ -788,14 +833,14 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
>   	if (dev->adapter->dev_init) {
>   		err = dev->adapter->dev_init(dev);
>   		if (err)
> -			goto lbl_free_cmd_buf;
> +			goto lbl_unregister;

here

>   	}
>
>   	/* set bus off */
>   	if (dev->adapter->dev_set_bus) {
>   		err = dev->adapter->dev_set_bus(dev, 0);
>   		if (err)
> -			goto lbl_free_cmd_buf;
> +			goto lbl_unregister;

here

>   	}
>
>   	/* get device number early */
> @@ -807,11 +852,14 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
>
>   	return 0;
>
> -lbl_free_cmd_buf:
> -	kfree(dev->cmd_buf);
> +lbl_unregister:
> +	unregister_netdev(netdev);
>
>   lbl_set_intf_data:
>   	usb_set_intfdata(intf, dev->prev_siblings);
> +	kfree(dev->cmd_buf);
> +
> +lbl_free_candev:
>   	free_candev(netdev);

and here ...

Are these changes necessary for the current PCAN USB driver too?
Should they go into the stable tree with a separate patch?

If so this separate patch should be the first in the series to be picked.

There's something like this in patch 1/3 too - will send it after this mail.

Regards,
Oliver