Maximilian Falkenstein wrote:
>> Did you run checkpatch.pl, at least? That's what I get:
>>
>> ERROR: Missing Signed-off-by: line(s)
>>
>> total: 8 errors, 54 warnings, 727 lines checked
>>
>> Please fix and resubmit.
> Yes, I've run checkpatch.pl, but because I didn't need this line till now I
> ignored this error.
Some more general comments from quickly browsing the code:
- use u[8|16|32] instead of uint[8|16|32]_t for kernel code.
- try to reduce you indention level by using functions.
- use (static inline) functions for repetitive patterns, e.g.:
buf_to_[std|ext]_can_id(), [std|ext]_can_id_to_buf()
- put labels on a separate line
- don't align values, e.g.:
sl->magic = SLCAN_MAGIC;
sl->dev = dev;
sl->to_ack = 0;
Just one space before and after the "=".
- Use mask definitions from include/linux/can.h (instead of 0x1fffffff)
More comments inline...
[snip]
> + if ((s == 't') | (s == 'T') | (s == 0xe0)) {
| or || ?
> + set_bit(SLF_FRAME_CANDIDAT, &sl->flags);
> + if (s == 't')
> + clear_bit(SLF_EFRAME, &sl->flags);
> + if (s == 'T')
> + set_bit(SLF_EFRAME, &sl->flags);
> + if (s == 0xe0)
> + set_bit(SLF_RESP, &sl->flags);
This pattern exist more than once.
> + sl->rcount = 0;
> + }
> + return;
> +
> +invalid: clear_bit(SLF_FRAME_CANDIDAT, &sl->flags);
> + return;
> +
> +rec_out_decaps:
> + /* Modify buffer so that we can call slc_bump */
> + for (i = 0; i <= sl->rcount; i++)
> + nrbuff[i] = sl->rbuff[sl->rec_rcount -
> sl->rcount - 1 + i];
> + memcpy(sl->rbuff, nrbuff, sl->rcount + 1);
> slc_bump(sl);
> + sl->rcount = 0;
> + sl->rec_rcount = 0;
> + clear_bit(SLF_FRAME_CANDIDAT, &sl->flags);
> + clear_bit(SLF_LOST_SYNC, &sl->flags);
> + return;
> }
> - sl->rcount = 0;
> - } else {
> - if (!test_bit(SLF_ERROR, &sl->flags)) {
> - if (sl->rcount < SLC_MTU) {
> - sl->rbuff[sl->rcount++] = s;
> +
> + if (test_bit(SLF_RESP, &sl->flags)) {
> + if (s != 0xf9) {
> + /* 0xf9 means OK, everything different means
> error */
> + printk(KERN_WARNING "%s: Command failed,
> retval: %X\n",
> + sl->dev->name, s);
> + stats->tx_errors++;
> + stats->tx_dropped++;
> + }
> + sl->to_ack--;
> + if (test_bit(SLF_PAUSED, &sl->flags) && (sl->to_ack <
> maxack)) {
> + /*
> + * The device buffer is large enough for at
> least one
> + * more packet, so let's wake the quere
> + */
> + clear_bit(SLF_PAUSED, &sl->flags);
> + netif_wake_queue(sl->dev);
> + }
> + clear_bit(SLF_RESP, &sl->flags);
> + sl->rcount = 0;
> + return;
> + }
> +
> + if (sl->rcount == 0) {
> + /*
> + * What's coming next: a can frame, an extended can
> frame, or an
> + * ack/nack?
> + */
> + if (s == 't') {
> + clear_bit(SLF_EFRAME, &sl->flags);
> + goto out_write;
> + } else if (s == 'T') {
> + set_bit(SLF_EFRAME, &sl->flags);
> + goto out_write;
> + } else if (s == 0xe0) {
> + /* ACK/NACK */
> + set_bit(SLF_RESP, &sl->flags);
See above.
> + } else {
> + printk(KERN_WARNING "slcan (dev: %s): Unknown
> packet(head: \
> + 0x%X), entering lost sync
> mode!\n", sl->dev->name, s);
> + stats->rx_errors++;
> + sl->rec_rcount = 0;
> + sl->rcount = 0;
> + sl->rec_rcount = 0;
Hm, already assigned two lines above.
> + set_bit(SLF_LOST_SYNC, &sl->flags);
> return;
> - } else {
> - stats->rx_over_errors++;
> - set_bit(SLF_ERROR, &sl->flags);
> }
> + } else if ((test_bit(SLF_EFRAME, &sl->flags) && sl->rcount == 5)
> + | ((!test_bit(SLF_EFRAME, &sl->flags)) &&
> sl->rcount == 3)) {
| or || ?
[snip]
> static int slcan_open(struct tty_struct *tty)
> {
> struct slcan *sl;
> @@ -885,6 +1208,21 @@
> return -EINVAL;
>
> switch (cmd) {
> + case SIOCGIFENCAP:
> + /* Do we use binary encapsulation? */
> + return test_bit(SLF_BINENC, &sl->flags);
> + case SIOCSIFENCAP:
> + /* Let's modify our encapsulation! */
> + if (arg > 0) {
> + set_bit(SLF_BINENC, &sl->flags);
> + printk(KERN_INFO "slcan: Now using binary encapsulation
> on device \
> + %s!", sl->dev->name);
Wired continuation line. Shouldn't it be:
printk(KERN_INFO "slcan: Now using binary encapsulation"
"on device
%s!", sl->dev->name);
> + } else {
> + clear_bit(SLF_BINENC, &sl->flags);
> + printk(KERN_INFO "slcan: Now using ASCII encapsulation
> on device \
> + %s!", sl->dev->name);
Ditto. Also the "\n" is missing, I think.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
