Re: "[PATCH] Add support for PEAK-System GmbH CAN USB adapters (v2) "

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 9243 bytes --]


On 12/20/2011 01:10 PM, Grosjean Stephane wrote:
> I think so. :-)
>> + * You should have received a copy of the GNU General Public License
>> along
>> + * with this program; if not, write to the Free Software Foundation,
>> Inc.,
>> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>> please remove the address
> 
> So I changed the above 3 lines into the two below:
> 
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc..
> 
> Is it ok?

Just remove completely.

>> +#define PCAN_USB_EP_CMDIN    (PCAN_USB_EP_CMDOUT|USB_DIR_IN)
>> +#define PCAN_USB_EP_MSGOUT   2
>> +#define PCAN_USB_EP_MSGIN    (PCAN_USB_EP_MSGOUT|USB_DIR_IN)
>>                                                    ^^^
>> please add spaces around the |
> Ok.
> 
>> +#define PCAN_USB_PARAMETER_LEN   14
>> +struct __packed pcan_usb_parameter {
>> +    u8 function;
>> +    u8    number;
>>           ^^^
>> please use one space
> Ok.
>> +#define PCAN_USB_TS_DIV_SHIFTER        20
>> +#define PCAN_USB_TS_US_PER_TICK        44739243
>> If you want to align the #defines please use tab between symbol and value
> So I put ONE tab in between the symbol and the value, everywhere in the
> driver (IMHO, using tab(s) here too much relies on editor's tab size.
> Putting white spaces here fixes that... but it's my humble opinion only).

For kernel code a tab is 8 spaces, see Documentation/CodingStyle

>                 Chapter 1: Indentation
> 
> Tabs are 8 characters, and thus indentations are also 8 characters.
> There are heretic movements that try to make indentations 4 (or even 2!)
> characters deep, and that is akin to trying to define the value of PI to
> be 3.

[...]

>>> +    printk("btr0=0x%02x btr1=0x%02x", btr0, btr1);
>> Please use netdev_LEVEL() instead of printk
> I talked about that in my previous e-mail to Sebastian: these printk()s
> are called in sequence: this one (for example) ISNOT the first one but
> occurs next to a netdev_info() macro (please see function
> "peak_usb_set_bittiming()" in "pcan_usb_core.c") ...
> According to that, if you think that the occurrence of some kernel
> event(s) in between the two messages (which could printk() msgs too) is
> to be taken into account, I'll think about a new way of putting bitrate
> info.

+1, You should always end prints with "\n", continuation of print should
be avoided.

>> +
>> +    err = pcan_usb_wait_response(dev, 6, 1, args);
>> +    if (err)
>> +        netdev_err(dev->netdev, "getting serial number failure:
>> %d\n", err);
>> +    else {
>> +        if (serial_number) memcpy(serial_number,&args[0], 4);
>> +    }
>> please remove the { }, no command after the if, please.
>>
>> Hmmm that serial number to u32 memcpy looks fishy, I smell endianess
>> problem. Can you test your driver on an big endian machine, like powerpc?
> Ok no need to test, you're right (not very proud about that ;-)
>>> +            u8 sl = *ibuf++;
>>> +            u8 rec_len = (sl&  PCAN_USB_STATUSLEN_DLC);
>>> +
>>> +            /* handle error frames here */
>> I suggest to put the error handling in a sub function. If you have
> 
> Hmmm... I also thought about that, but the amount of contextual
> variables to pass as arguments is quite large (i, ibuf, dts...): for
> that kind of hw, for example, timestamp coding depends on its place
> (rank) in the message. TODO

You can put all these arguments into a struct and just pass the pointer
to the struct.

>> +                switch (f) {
>> +
>> +                case 1:
>> +
>> please reove these two empty lines. I think you should define an enum
>> for the functions.
> Ok.
>>> +                    /* allocate an skb to store the error frame */
>>> +                    skb = alloc_can_err_skb(netdev,&cf);
>>> +                    if (skb == NULL) {
>> !skb
>>
>> please talk to Wolfgang for the error handling, he is the expert now :)
> 
> ???
> What should I talk about? Is the way errors are handling wrong?

I haven't looked at it, but Wolfgang has really twisted his mind around
error handling, so he's the expert now.

[...]

>>> +
>>> +                for (; d<  8; d++) {
>>> +                    cf->data[d] = 0;
>>> +                }
>> memset?
> 
> memset(cf->data+d, '\0', 8 - d); /* and not memset(&cf->data[d],...;-),
> even if I personally think that this last syntax is more explicit */

or even:

memset(cf->data, 0x0, sizeof(cf-data) - d);

>>> +
>>> +                peak_usb_get_timestamp_tv(&pdev->time_ref,
>>> ts16+dts,&tv);
>>> +                skb->tstamp = timeval_to_ktime(tv);
>>> +                netif_rx(skb);
>>> +
>>> +                stats->rx_packets++;
>>> +                stats->rx_bytes += cf->can_dlc;
>>> +            }
>>> +
>>> +            if ((ibuf - (u8 *)urb->transfer_buffer)> 
>>> urb->transfer_buffer_length) {
>> What does this check do? You should do bounds checking of ibuf before
>> accessing it.
> Yes you're right... My problem is that the size of the records (in the
> message) to decode is difficult to predict (see timestamp management for
> example). Well "difficult" is not the good word, but I thought it would
> cost too much code for checking next ibuf each time.. But ok, I'll fix
> that too..

You know these security guys? They'll modify your mouse so that the
kernel would load the can driver, then send specially prepared messages
and then boom, root exploit.

>>> +                netdev_err(netdev, "usb message format error\n");
>>> +                err = -EINVAL;
>>> +                break;
>>> +            }
>>> +        }
>>> +    }
>>> +    else if (urb->actual_length>  0) {
>>> +        netdev_err(netdev, "usb message length error (%u)\n",
>>> urb->actual_length);
>>> +        err = -EINVAL;
>>> +    }
>> I'd move the error checking to the beginning of the function. Return
>> early if you detect an error, so that you can get rid of (at least) one
>> indention level.
> I agree with you but for the indentation argument only: I think current
> code enables to generally do only a single test  (actual_length >
> PCAN_USB_MSG_HEADER_LEN) than the one you're proposing. Moreover, even
> if I know that gcc does some quite incredible optimizations sometimes, I
> always prefer defining strict local variables (I mean, defining variable
> only in the block where it is used). But once again, if the linux kernel
> coding style strictly requests it (or if any other arg regarding
> optimization comes from the list), I don't see any problem to move these
> tests where  you propose.



>>> +
>>> +    return err;
>>> +}
>>> +
>>> +static int pcan_usb_encode_msg(struct peak_usb_device *dev, struct
>>> sk_buff *skb, u8 *obuf, size_t *size)
>>> +{
>>> +    struct net_device *netdev = dev->netdev;
>>> +    struct net_device_stats *stats =&netdev->stats;
>>> +    struct can_frame *cf = (struct can_frame *)skb->data;
>>> +    u8 *pc;
>>> +
>>> +    obuf[0] = 2;
>>> +    obuf[1] = 1;
>>> +
>>> +    pc =&obuf[PCAN_USB_MSG_HEADER_LEN];
>>> +
> I think I also have to change the above $obuf[] into:
> 
> pc = obuf + PCAN_USB_MSG_HEADER_LEN;
> 
> right?

yes, would be consistent.

> 
>>> +    /* status/len byte */
>>> +    *pc = cf->can_dlc;
>>> +    if (cf->can_id&  CAN_RTR_FLAG)
>>> +        *pc |= PCAN_USB_STATUSLEN_RTR;
>>> +   
>>> +    /* can id */
>>> +    if (cf->can_id&  CAN_EFF_FLAG) {
>>> +        u32 tmp32 = cpu_to_le32(cf->can_id&  CAN_ERR_MASK);
>>                  ^^^ should be __le32 then
> Ok.
>>> +        tmp32<<= 3;
>>> +        *pc |= PCAN_USB_STATUSLEN_EXT_ID;
>>> +        memcpy(++pc,&tmp32, 4);
>>> +        pc += 4;
>>> +    }
>>> +    else {
>> } else {
> Ok.
>>> +        u16 tmp16 = (u16 )cpu_to_le32(cf->can_id&  CAN_ERR_MASK);
>>                              ^^^^^^^^^^^^^^^^^
>> why convert to le32 and then cast to u16?
> ...because can_id is u32 and I only want to keep the lowest 16 bits . Is
> there another way to do that?

there's no need for an explicit cast

>>> +        tmp16<<= 5;
>>> +        memcpy(++pc,&tmp16, 2);
>>> +        pc += 2;
>>> +    }
>>> +
>>> +    /* can data */
>>> +    if ((cf->can_id&  CAN_RTR_FLAG) == 0) {
>> if (cf->can_id&  CAN_RTR_FLAG)
> Ok, but only if you wanted to say:
> 
> if (!(cf->can_id&  CAN_RTR_FLAG)) {
> 
> ;-)

Opps, yes :)

> 
>>> +        memcpy(pc,&cf->data[0], cf->can_dlc);
>>         memcpy(pc, cf->data, cf->can_dlc);
> Ok.
>> +    /* check endpoint addresses (numbers) and associated max data
>> length */
>> +    /* (only from setting 0) */
>> +    iface_desc =&intf->altsetting[0];
>> +
> 
> What about the above &altsetting[0] ? Should it be also changed into:
> 
> iface_desc = intf->altsetting;

Yes

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]