netdev - RE: [Uclinux-dist-devel] [PATCH v3] add the driver for Analog Devices Blackfin on-chip CAN controllers

Open Source and information security mailing list archives

Message-ID: <8A42379416420646B9BFAC9682273B6D0EDD9A3C@limkexm3.ad.analog.com>
Date:	Thu, 10 Dec 2009 11:06:07 +0000
From:	"Hennerich, Michael" <Michael.Hennerich@...log.com>
To:	"Wolfgang Grandegger" <wg@...ndegger.com>,
	"Mike Frysinger" <vapier.adi@...il.com>
CC:	<socketcan-core@...ts.berlios.de>, <netdev@...r.kernel.org>,
	<davem@...emloft.net>, "H.J. Oertel" <oe@...t.de>,
	<uclinux-dist-devel@...ckfin.uclinux.org>
Subject: RE: [Uclinux-dist-devel] [PATCH v3] add the driver for Analog Devices Blackfin on-chip CAN controllers

>Wolfgang Grandegger wrote:
>> Mike Frysinger wrote:
>>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote:
>>>> Barry Song wrote:
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/init.h>
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/types.h>
>>>> I think you don't need "types.h" as the code no longer uses
>"uint*_t".
>>> linux/types.h declares all types, like u* which this driver still
>uses
>>
>> I just remember that "linux/types.h" needs to be added for the
uint*_t
>> types. At a first glance I do not see __u8/u8 being defined in that
>> header file but I might have missed something.
>>
>>>> Well, I'm still not a friend of the following inline functions,
>>>> especially the *one-liners* which are called just *once*. With the
>usage
>>>> of structs they seem even more useless.
>>> seems like it would make more sense to not even use the read/write
>>> functions either.  just declare the regs as volatile and assign/read
>>> the struct directly.
>>
>> Two times no. Don't use volatile and proper accessor functions. See:
>>
>> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-
>harmful.txt
>
>I was just wondering if bfin_read/write16 would not be the proper
>accessor functions. readw/writew seems to be implemented differently:
>
>http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44
>
>Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely
>shows why accessor functions should be used to access device registers.
>
>Well, just curious. I don't really know the blackfin arch.

Well - on Blackfin its absolutely ok to access System Memory Mapped
Registers using structs.
At any rate volatile is then required to prevent the compiler to
optimize accesses away.
IMHO this is a pretty legal use of volatile, and used in hundreds of
places all over the kernel. 

When accessing external controllers accessor functions from io.h must be
used.
There are two things to consider here:
1) weak ordering of reads and writes 
2) killed and reissued reads (especially harmful when reading from
FIFOs)

-Michael

>
>Wolfgang.
>
>_______________________________________________
>Uclinux-dist-devel mailing list
>Uclinux-dist-devel@...ckfin.uclinux.org
>https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
--
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

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