Re: [PATCH v2] usb_8dev: Add support for USB2CAN interface from 8 devices

["krumboeck@universalnet.at" <krumboeck@universalnet.at>]

Hi Marc!


>> +/* Send open command to device */
>> +static int usb_8dev_cmd_open(struct usb_8dev *dev)
>> +{
>> +    struct can_bittiming *bt = &dev->can.bittiming;
>> +    struct usb_8dev_cmd_msg outmsg;
>> +    struct usb_8dev_cmd_msg inmsg;
>> +    u32 flags = 0;
>> +    u32 beflags;
>> +    u16 bebrp;
>> +    u32 ctrlmode = dev->can.ctrlmode;
>> +
>> +    if (ctrlmode & CAN_CTRLMODE_LOOPBACK)
>> +        flags |= USB_8DEV_LOOPBACK;
>> +    if (ctrlmode & CAN_CTRLMODE_LISTENONLY)
>> +        flags |= USB_8DEV_SILENT;
>> +    if (ctrlmode & CAN_CTRLMODE_ONE_SHOT)
>> +        flags |= USB_8DEV_DISABLE_AUTO_RESTRANS;
>> +
>> +    flags |= USB_8DEV_STATUS_FRAME;
>> +
>> +    memset(&outmsg, 0, sizeof(struct usb_8dev_cmd_msg));
>> +    outmsg.command = USB_8DEV_OPEN;
>> +    outmsg.opt1 = USB_8DEV_BAUD_MANUAL;
>> +    outmsg.data[0] = (bt->prop_seg + bt->phase_seg1);
>> +    outmsg.data[1] = bt->phase_seg2;
>> +    outmsg.data[2] = bt->sjw;
>
> But you should not use usb_bulk_msg() to send data which is on the
> stack, please use you already allocated memory in priv or kmalloc
> something here.

The function usb_8dev_send_cmd copies the data to dev->cmd_msg_buffer (allocated with kzalloc, GFP_KERNEL).


>
>> +
>> +    /* BRP */
>> +    bebrp = cpu_to_be16((u16) bt->brp);
>> +    memcpy(&outmsg.data[3], &bebrp, sizeof(bebrp));
>
> Are you sure about the endianess? Some data types are BE some are LE?

Where is LE used?

>
>> +
>> +    /* flags */
>> +    beflags = cpu_to_be32(flags);
>> +    memcpy(&outmsg.data[5], &beflags, sizeof(beflags));
>> +
>> +    return usb_8dev_send_cmd(dev, &outmsg, &inmsg);
>> +}
>> +
>> +/* Send close command to device */
>> +static int usb_8dev_cmd_close(struct usb_8dev *dev)
>> +{
>> +    struct usb_8dev_cmd_msg    outmsg;
>> +    struct usb_8dev_cmd_msg    inmsg;
>> +
>> +    memset(&outmsg, 0, sizeof(struct usb_8dev_cmd_msg));
>> +    outmsg.command = USB_8DEV_CLOSE;
>
> Same here, don't use stack allocated mem for usb_bulk_msg

All command functions use usb_8dev_send_cmd.

>
> Are you sure?....Do you really want to wake the queue?
>

No, I've removed.

>
> needed?
>

I don't know. -> removed.


Thanks for your comments.

best regards,
Bernd