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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ