netdev - Re: [PATCH] can: Driver for the Microchip MCP251x SPI CAN controllers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cabda6420911030053r2dce5cd9wd416757e5f1b6574@mail.gmail.com>
Date:	Tue, 3 Nov 2009 09:53:47 +0100
From:	christian pellegrin <chripell@...e.org>
To:	Wolfgang Grandegger <wg@...ndegger.com>
Cc:	socketcan-core@...ts.berlios.de,
	spi-devel-general@...ts.sourceforge.net, netdev@...r.kernel.org
Subject: Re: [PATCH] can: Driver for the Microchip MCP251x SPI CAN controllers

Hi,

On Mon, Nov 2, 2009 at 8:49 PM, Wolfgang Grandegger <wg@...ndegger.com> wrote:
> I assume this is v2 of the patch.
>

Yes I put the version in the subject. Will reply with v3 shortly

>> +     buf[TXBDLC_OFF]  = (rtr << DLC_RTR_SHIFT) | frame->can_dlc;
>
> Two spaces before "=".
>

ack. This could be a good idea for a checkpatch.pl rule. Unfortunately
I don't know much about perlre. I used some emacs regexp so I hope
this is the last time you find double whitespaces or tabs around
assignment in this patch

>> +     }
>
> Here the transceiver should be switched off!?
>

ack, all the error paths now seem checked now.

>> +                     mcp251x_write_bits(spi, CANINTF, intf, 0x00);
>
> Assigning variables within if or while expressions is not allowed. I
> wonder why checkpatch did not spot it.
>

ack. Out of curiosity I checked checkpatch.pl, unfortunately it looks
like it looks only for ifs:

if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) {
	ERROR("do not use assignment in if condition\n" . $herecurr);
}

anyway I looked at this loop and tested: there is no need for it.

>> +     net->flags              |= IFF_ECHO;
>
> Remove spaces, please.
>

ack, sorry

>> +     if (ret >= 0) {
>
> if (!ret) ?
>

ack

>> +             dev_info(&spi->dev, "probed\n");
>> +             return ret;
>> +     }
>
> Shouldn't the power be switched off?
>

ack on the error patch

>
> We are close...
>

thank you for the patience


-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html