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

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

Hi Marc,

Thanks for your review. Please, find my answers and comments below...

Le 16/12/2011 13:41, Marc Kleine-Budde a écrit :
> On 12/16/2011 11:19 AM, s.grosjean@peak-system.com wrote:
>>  From 1cee3be3875f27a2ee3942b00d611450f4369325 Mon Sep 17 00:00:00 2001
>> From: Stephane Grosjean<s.grosjean@peak-system.com>
>> Date: Fri, 16 Dec 2011 11:11:37 +0100
>> Subject: [PATCH] Add support for PEAK-System GmbH CAN USB adapters (v2)
> The driver has some unusal coding style, please use checkpatch and do
> sparse checking (compile the drivers with C=2). Make use of C99
> initialisers. Use foo->bar not&foo->bar[0] to get the pointer for the
> first array element
Ok, I did use the C99 style and removed my &foo->bar[0] too, everywhere 
in the driver.
> +
> +ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
>   ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
>
> ^^^^^^^^^^
>
> one ccflag should be enough :)

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?

> +#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).
> +
> +	cmd.function = f;
> +	cmd.number = n;
> you can use C99 initialisers instead:
>
> 	struct pcan_usb_parameter cmd = {
> 		.function = f,
> 		.number = n,
> 	};
>
> then parameters in automatically set with 0x0....
Ok.
>> +
>> +	if (p != NULL)
> if (p)
Ok.
>> +		memcpy(&cmd.parameters[0], p, PCAN_USB_PARAMETER_LEN);
> I'd write memcpy(cmd.parameters, p, ARRAY_SIZE(cmd.parameters))
So did I...
>> +	else
>> +		memset(&cmd.parameters[0], '\0', PCAN_USB_PARAMETER_LEN);
> ...and you can remove the memset here.
Ok.
> +
> +	/* usb device unregistered? */
> +	if (!(dev->state&  PCAN_USB_STATE_CONNECTED)) return 0;
> please put the return in a seperate line.
Ok (done everywhere in the driver too).
> +	err = pcan_usb_send_command(dev, f, n, NULL);
> +	if (err) {
> +		return err;
> +	}
> please remove the { }
Ok (done everywhere in the driver too).
>
> the array can be initializes in C99 style, too:
>
> 	u8 args[PCAN_USB_PARAMETER_LEN] = {
> 		[0] = 0,
> 		[1] = mode,
> 	};
>
> The [0] can be skipped, by you might want to include it for
> documentation purpose.
No, I don't... So I removed the [0] = 0 too...
> Please add a newline in before return
Ok
> dito
>
> I see some magic numbers for the command here, does it make sense to
> define an enum for them?
TODO...
>> +}
>> +
>> +/*
>> + * This routine was stolen from drivers/net/can/sja1000/sja1000.c
>> + */
> Nice for crediting this, but IMHO no need to write it into the code.
Ok, removed now (FYI it's not so nice to steal something which was 
already stolen ;-)
>> +static int pcan_usb_set_bittiming(struct peak_usb_device *dev, struct can_bittiming *bt)
>> +{
>> +	u8 args[PCAN_USB_PARAMETER_LEN];
>> +	u8 btr0, btr1;
>> +
>> +	btr0 = ((bt->brp - 1)&  0x3f) | (((bt->sjw - 1)&  0x3)<<  6);
>> +	btr1 = ((bt->prop_seg + bt->phase_seg1 - 1)&  0xf) \
>> +	     | (((bt->phase_seg2 - 1)&  0x7)<<  4);
>> +	if (dev->can.ctrlmode&  CAN_CTRLMODE_3_SAMPLES)
>> +		btr1 |= 0x80;
>> +
>> +	args[0] = btr1;
>> +	args[1] = btr0;
>> +
>> +	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.
> +
> +	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
> +				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?

> +
> +			/* handle normal can frames here */
> please use an extra fucntion for normal frames

Same answer than error handling (see above): not so easy nor so obvious...
>> +				}
>> +				else {
> the preferred coding style is
> } else {
Ok (tried to change the whole driver files)
>> +					u8 ts8 = *ibuf++;
>> +
>> +					if (dts) {
>> +						dts&= 0xff00;
>> +						if (ts8<  prev_ts8)
>> +							dts += 0x100;
>> +					}
>> +
>> +					dts |= ts8;
>> +					prev_ts8 = ts8;
> I think I've seen this code before, please add a function for it.
Ok, this is done now.

>
>> +				else {
> } else { - or better get remove the { }
>
>> +					for (; d<  rec_len; d++)
>> +						cf->data[d] = *ibuf++;
>> +				}
Ok ({} removed)

>> +
>> +				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 */

>> +
>> +				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..
>> +				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?

>> +	/* 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?
>> +		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)) {

;-)

>> +		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;


?

>
> that's it for now
>
> Marc
>
Many thanks for all of your work, Marc.

Stéphane