netdev - Re: [PATCH v2] can: add Renesas R-Car CAN driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <5276A63E.3070505@grandegger.com>
Date:	Sun, 03 Nov 2013 20:38:38 +0100
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	netdev@...r.kernel.org, mkl@...gutronix.de,
	linux-can@...r.kernel.org
CC:	linux-sh@...r.kernel.org, vksavl@...il.com
Subject: Re: [PATCH v2] can: add Renesas R-Car CAN driver

On 11/01/2013 11:24 PM, Sergei Shtylyov wrote:
> On 10/25/2013 11:28 PM, Wolfgang Grandegger wrote:
> 
>>> Add support for the CAN controller found in Renesas R-Car SoCs.
> 
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
> 
> [...]
> 
>>> Index: linux-can-next/drivers/net/can/rcar_can.c
>>> ===================================================================
>>> --- /dev/null
>>> +++ linux-can-next/drivers/net/can/rcar_can.c
>>> @@ -0,0 +1,920 @@
> [...]
>>> +static bool autorecovery;
>>> +module_param(autorecovery, bool, 0644);
>>> +MODULE_PARM_DESC(autorecovery, "Automatic hardware recovery from
>>> bus-off");
> 
>> Software recovery is the preferred solution. No need to support
>> automatic recovery by the hardware.
> 
>    OK, removed it.
> 
>>> +/* Mailbox registers structure */
>>> +struct rcar_can_mbox_regs {
>>> +    u32 id;        /* IDE and RTR bits, SID and EID */
>>> +    u8 stub;    /* Not used */
>>> +    u8 dlc;        /* Data Length Code - bits [0..3] */
>>> +    u8 data[8];    /* Data Bytes */
>>> +    u8 tsh;        /* Time Stamp Higher Byte */
>>> +    u8 tsl;        /* Time Stamp Lower Byte */
> 
>> I would add padding bytes here to ensure alignment.
> 
>    What padding? This is how the hardware registers are laid out. I
> think I should rather add __packed after }.

OK, I was confused that data does not start on a 4 byte boundary.


>>> +            rcar_can_writeb(priv, RCAR_CAN_ECSR, (u8)~ECSR_ADEF);
> 
>> Please avoid casts here and below.
> 
>    These casts help avoid compiler warnings.

Well instead of using casts the declaration of ECSR_ADEF and otheres
should be fixed. I think the problem is that BIT is declared as shown below:

 #define BIT(nr)                 (1UL << (nr))

Using BIT seems not appropriate here.

Wolfgang,

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