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

[Stephane Grosjean <s.grosjean@peak-system.com>]

Hi Oliver,

Le 27/11/2014 12:43, Oliver Hartkopp a écrit :
> 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;
>
> ??
>

Simply because I always think in terms of being compatible with (very) 
older versions of the Kernel, for example <= 3.5, which is obviously not 
the right (TM) way of thinking when pushing code into the mainline 
Kernel ;-) !
So I agree with you and will do the changes accordingly.

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

Okay!

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

Ok! I will prepare a single patch for these changes.

>
> Regards,
> Oliver

Regards,

Stéphane

> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--