Re: [PATCH v2] bug: fix problem including from linux/kernel.h

[Ian Abbott]

On 2017-05-24 17:06, Ian Abbott wrote:

If "include/linux/kernel.h" includes , a circular
dependency is introduced when "include/asm-generic/bug.h" includes
.  This results in build breakage when something
includes  before  for architectures that
select `CONFIG_GENERIC_BUG` because `struct bug_entry` is not fully
declared (not declared at all in fact) before its members are accessed
by `is_warning_bug()`.

To avoid this problem, remove the inclusion of  from
"include/asm-generic/bug.h", but include  from
"include/linux/bug.h" because it needs the `bool` type.

A consequence of this change is that since most bug-related,
function-link macros (`BUG()`, `WARN()` etc.) make use of facilities
provided by , something else needs to include that
before those macros are called.

Signed-off-by: Ian Abbott 
Cc: Arnd Bergmann 
Cc: Andrew Morton 
Cc: Michal Nazarewicz 
Cc: Peter Zijlstra 
---
v2: Fix typo in patch subject line.
---
 include/asm-generic/bug.h | 1 -
 include/linux/bug.h   | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)


This patch and its previous version can be dropped now, since the patch 
that caused the breakage that this patch fixes up 
(<https://lkml.org/lkml/2017/5/23/641>) has been dropped (for now). 
I'll avoid the breakage in the way suggested by Rasmus in 
<https://lkml.org/lkml/2017/5/24/553>.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

[PATCH v2] bug: fix problem including from linux/kernel.h

[Ian Abbott]

If "include/linux/kernel.h" includes , a circular
dependency is introduced when "include/asm-generic/bug.h" includes
.  This results in build breakage when something
includes  before  for architectures that
select `CONFIG_GENERIC_BUG` because `struct bug_entry` is not fully
declared (not declared at all in fact) before its members are accessed
by `is_warning_bug()`.

To avoid this problem, remove the inclusion of  from
"include/asm-generic/bug.h", but include  from
"include/linux/bug.h" because it needs the `bool` type.

A consequence of this change is that since most bug-related,
function-link macros (`BUG()`, `WARN()` etc.) make use of facilities
provided by , something else needs to include that
before those macros are called.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Arnd Bergmann <a...@arndb.de>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Michal Nazarewicz <min...@mina86.com>
Cc: Peter Zijlstra <pet...@infradead.org>
---
v2: Fix typo in patch subject line.
---
 include/asm-generic/bug.h | 1 -
 include/linux/bug.h   | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 87191357d303..074a66de47f6 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -12,7 +12,6 @@
 #endif
 
 #ifndef __ASSEMBLY__
-#include 
 
 #ifdef CONFIG_BUG
 
diff --git a/include/linux/bug.h b/include/linux/bug.h
index 687b557fc5eb..68692b775343 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -3,6 +3,7 @@
 
 #include 
 #include 
+#include 
 
 enum bug_trap_type {
BUG_TRAP_TYPE_NONE = 0,
-- 
2.11.0

[PATCH v2] bug: fix problem including from linux/kernel.h

[Ian Abbott]

If "include/linux/kernel.h" includes , a circular
dependency is introduced when "include/asm-generic/bug.h" includes
.  This results in build breakage when something
includes  before  for architectures that
select `CONFIG_GENERIC_BUG` because `struct bug_entry` is not fully
declared (not declared at all in fact) before its members are accessed
by `is_warning_bug()`.

To avoid this problem, remove the inclusion of  from
"include/asm-generic/bug.h", but include  from
"include/linux/bug.h" because it needs the `bool` type.

A consequence of this change is that since most bug-related,
function-link macros (`BUG()`, `WARN()` etc.) make use of facilities
provided by , something else needs to include that
before those macros are called.

Signed-off-by: Ian Abbott 
Cc: Arnd Bergmann 
Cc: Andrew Morton 
Cc: Michal Nazarewicz 
Cc: Peter Zijlstra 
---
v2: Fix typo in patch subject line.
---
 include/asm-generic/bug.h | 1 -
 include/linux/bug.h   | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 87191357d303..074a66de47f6 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -12,7 +12,6 @@
 #endif
 
 #ifndef __ASSEMBLY__
-#include 
 
 #ifdef CONFIG_BUG
 
diff --git a/include/linux/bug.h b/include/linux/bug.h
index 687b557fc5eb..68692b775343 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -3,6 +3,7 @@
 
 #include 
 #include 
+#include 
 
 enum bug_trap_type {
BUG_TRAP_TYPE_NONE = 0,
-- 
2.11.0

Re: [PATCH 1/1] bug: fix problem including from linux/kernel.h

[Ian Abbott]

On 24/05/17 14:37, Rasmus Villemoes wrote:

On 24 May 2017 at 15:21, Ian Abbott <abbo...@mev.co.uk> wrote:

If "include/linux/kernel.h" includes , a circular
dependency is introduced


Then don't. Can't we just create linux/build_bug.h and have that
contain the BUILD_BUG related macros - they're really completely
unrelated to all the asm-cruft bug.h needs to know about. build_bug.h
can then include compiler.h and not much else to figure out how it may
implement its macros. Then kernel.h can include build_bug.h, and we
don't force each and every translation unit to include bug.h and
everything that then pulls in. We just got rid of a lot of header
bloat, let's try to keep it that way.


Thanks, I'll take a look at that.  In the meantime, I noticed the title 
line of this patch contains a nasty typo, so I'll send a v2 patch to fix 
that.


--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 1/1] bug: fix problem including from linux/kernel.h

[Ian Abbott]

On 24/05/17 14:37, Rasmus Villemoes wrote:

On 24 May 2017 at 15:21, Ian Abbott  wrote:

If "include/linux/kernel.h" includes , a circular
dependency is introduced


Then don't. Can't we just create linux/build_bug.h and have that
contain the BUILD_BUG related macros - they're really completely
unrelated to all the asm-cruft bug.h needs to know about. build_bug.h
can then include compiler.h and not much else to figure out how it may
implement its macros. Then kernel.h can include build_bug.h, and we
don't force each and every translation unit to include bug.h and
everything that then pulls in. We just got rid of a lot of header
bloat, let's try to keep it that way.


Thanks, I'll take a look at that.  In the meantime, I noticed the title 
line of this patch contains a nasty typo, so I'll send a v2 patch to fix 
that.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

[PATCH 0/1] bug: fix problem including from linux/kernel.h

[Ian Abbott]

PATCH 1 is a follow-up patch to my previous patch "[PATCH v4 2/2]
kernel.h: handle pointers to arrays better in container_of()":
.  The previous patch introduced a
build failure:
.  The
build failure was due to a circular dependency introduced by the
previous patch and occurs when  is included before
 for architectures that select `CONFIG_GENERIC_BUG`.
PATCH 1 fixes this circular dependency.  Ideally, it should be applied
before my previous patch to avoid breakage during git bisect builds, but
can be applied after if that is not a concern.

1) bug: fix problem including  from linux/kernel.h

 include/asm-generic/bug.h | 1 -
 include/linux/bug.h   | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

[PATCH 0/1] bug: fix problem including from linux/kernel.h

[Ian Abbott]

PATCH 1 is a follow-up patch to my previous patch "[PATCH v4 2/2]
kernel.h: handle pointers to arrays better in container_of()":
.  The previous patch introduced a
build failure:
.  The
build failure was due to a circular dependency introduced by the
previous patch and occurs when  is included before
 for architectures that select `CONFIG_GENERIC_BUG`.
PATCH 1 fixes this circular dependency.  Ideally, it should be applied
before my previous patch to avoid breakage during git bisect builds, but
can be applied after if that is not a concern.

1) bug: fix problem including  from linux/kernel.h

 include/asm-generic/bug.h | 1 -
 include/linux/bug.h   | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

[PATCH 1/1] bug: fix problem including from linux/kernel.h

[Ian Abbott]

If "include/linux/kernel.h" includes , a circular
dependency is introduced when "include/asm-generic/bug.h" includes
.  This results in build breakage when something
includes  before  for architectures that
select `CONFIG_GENERIC_BUG` because `struct bug_entry` is not fully
declared (not declared at all in fact) before its members are accessed
by `is_warning_bug()`.

To avoid this problem, remove the inclusion of  from
"include/asm-generic/bug.h", but include  from
"include/linux/bug.h" because it needs the `bool` type.

A consequence of this change is that since most bug-related,
function-link macros (`BUG()`, `WARN()` etc.) make use of facilities
provided by , something else needs to include that
before those macros are called.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Arnd Bergmann <a...@arndb.de>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Michal Nazarewicz <min...@mina86.com>
Cc: Peter Zijlstra <pet...@infradead.org>
---
 include/asm-generic/bug.h | 1 -
 include/linux/bug.h   | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 87191357d303..074a66de47f6 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -12,7 +12,6 @@
 #endif
 
 #ifndef __ASSEMBLY__
-#include 
 
 #ifdef CONFIG_BUG
 
diff --git a/include/linux/bug.h b/include/linux/bug.h
index 687b557fc5eb..68692b775343 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -3,6 +3,7 @@
 
 #include 
 #include 
+#include 
 
 enum bug_trap_type {
BUG_TRAP_TYPE_NONE = 0,
-- 
2.11.0

[PATCH 1/1] bug: fix problem including from linux/kernel.h

[Ian Abbott]

If "include/linux/kernel.h" includes , a circular
dependency is introduced when "include/asm-generic/bug.h" includes
.  This results in build breakage when something
includes  before  for architectures that
select `CONFIG_GENERIC_BUG` because `struct bug_entry` is not fully
declared (not declared at all in fact) before its members are accessed
by `is_warning_bug()`.

To avoid this problem, remove the inclusion of  from
"include/asm-generic/bug.h", but include  from
"include/linux/bug.h" because it needs the `bool` type.

A consequence of this change is that since most bug-related,
function-link macros (`BUG()`, `WARN()` etc.) make use of facilities
provided by , something else needs to include that
before those macros are called.

Signed-off-by: Ian Abbott 
Cc: Arnd Bergmann 
Cc: Andrew Morton 
Cc: Michal Nazarewicz 
Cc: Peter Zijlstra 
---
 include/asm-generic/bug.h | 1 -
 include/linux/bug.h   | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 87191357d303..074a66de47f6 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -12,7 +12,6 @@
 #endif
 
 #ifndef __ASSEMBLY__
-#include 
 
 #ifdef CONFIG_BUG
 
diff --git a/include/linux/bug.h b/include/linux/bug.h
index 687b557fc5eb..68692b775343 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -3,6 +3,7 @@
 
 #include 
 #include 
+#include 
 
 enum bug_trap_type {
BUG_TRAP_TYPE_NONE = 0,
-- 
2.11.0

Re: [PATCH v4 2/2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 24/05/17 01:54, kbuild test robot wrote:

Hi Ian,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.12-rc2 next-20170523]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Ian-Abbott/asm-generic-bug-h-declare-struct-pt_regs-before-function-prototype/20170524-070310
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:14:0,
from include/asm-generic/bug.h:15,
from arch/x86/include/asm/bug.h:81,
from drivers/block/drbd/drbd_interval.c:1:

include/linux/bug.h:103:47: warning: 'struct bug_entry' declared inside 
parameter list will not be visible outside of this definition or declaration

static inline int is_warning_bug(const struct bug_entry *bug)
  ^
   include/linux/bug.h: In function 'is_warning_bug':

include/linux/bug.h:105:12: error: dereferencing pointer to incomplete type 
'const struct bug_entry'

 return bug->flags & BUGFLAG_WARNING;
   ^~

vim +105 include/linux/bug.h

ff20c2e0 Kirill A. Shutemov  2016-03-01   97
35edd910 Paul Gortmaker  2011-11-16   98  #endif/* __CHECKER__ */
35edd910 Paul Gortmaker  2011-11-16   99
7664c5a1 Jeremy Fitzhardinge 2006-12-08  100  #ifdef CONFIG_GENERIC_BUG
7664c5a1 Jeremy Fitzhardinge 2006-12-08  101  #include 
7664c5a1 Jeremy Fitzhardinge 2006-12-08  102
7664c5a1 Jeremy Fitzhardinge 2006-12-08 @103  static inline int 
is_warning_bug(const struct bug_entry *bug)
7664c5a1 Jeremy Fitzhardinge 2006-12-08  104  {
7664c5a1 Jeremy Fitzhardinge 2006-12-08 @105return bug->flags & 
BUGFLAG_WARNING;
7664c5a1 Jeremy Fitzhardinge 2006-12-08  106  }
7664c5a1 Jeremy Fitzhardinge 2006-12-08  107
19d43626 Peter Zijlstra  2017-02-25  108  struct bug_entry 
*find_bug(unsigned long bugaddr);

:: The code at line 105 was first introduced by commit
:: 7664c5a1da4711bb6383117f51b94c8dc8f3f1cd [PATCH] Generic BUG 
implementation

:: TO: Jeremy Fitzhardinge <jer...@goop.org>
:: CC: Linus Torvalds <torva...@woody.osdl.org>


This breakage occurs when  is included before 
 due to a circular dependancy I introduced with this 
commit.


It can be fixed by replacing the `#include ` with 
`#include ` in "include/asm-generic/bug.h".  I can send a 
patch to fix that, but ideally, it should be committed before this 
commit to avoid breakage during git bisect builds.  So my question is, 
should I send a single patch to fix this breakage, or resend the series 
of patches to incorporate this fix before the current commit?


--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v4 2/2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 24/05/17 01:54, kbuild test robot wrote:

Hi Ian,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.12-rc2 next-20170523]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Ian-Abbott/asm-generic-bug-h-declare-struct-pt_regs-before-function-prototype/20170524-070310
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:14:0,
from include/asm-generic/bug.h:15,
from arch/x86/include/asm/bug.h:81,
from drivers/block/drbd/drbd_interval.c:1:

include/linux/bug.h:103:47: warning: 'struct bug_entry' declared inside 
parameter list will not be visible outside of this definition or declaration

static inline int is_warning_bug(const struct bug_entry *bug)
  ^
   include/linux/bug.h: In function 'is_warning_bug':

include/linux/bug.h:105:12: error: dereferencing pointer to incomplete type 
'const struct bug_entry'

 return bug->flags & BUGFLAG_WARNING;
   ^~

vim +105 include/linux/bug.h

ff20c2e0 Kirill A. Shutemov  2016-03-01   97
35edd910 Paul Gortmaker  2011-11-16   98  #endif/* __CHECKER__ */
35edd910 Paul Gortmaker  2011-11-16   99
7664c5a1 Jeremy Fitzhardinge 2006-12-08  100  #ifdef CONFIG_GENERIC_BUG
7664c5a1 Jeremy Fitzhardinge 2006-12-08  101  #include 
7664c5a1 Jeremy Fitzhardinge 2006-12-08  102
7664c5a1 Jeremy Fitzhardinge 2006-12-08 @103  static inline int 
is_warning_bug(const struct bug_entry *bug)
7664c5a1 Jeremy Fitzhardinge 2006-12-08  104  {
7664c5a1 Jeremy Fitzhardinge 2006-12-08 @105return bug->flags & 
BUGFLAG_WARNING;
7664c5a1 Jeremy Fitzhardinge 2006-12-08  106  }
7664c5a1 Jeremy Fitzhardinge 2006-12-08  107
19d43626 Peter Zijlstra  2017-02-25  108  struct bug_entry 
*find_bug(unsigned long bugaddr);

:: The code at line 105 was first introduced by commit
:: 7664c5a1da4711bb6383117f51b94c8dc8f3f1cd [PATCH] Generic BUG 
implementation

:: TO: Jeremy Fitzhardinge 
:: CC: Linus Torvalds 


This breakage occurs when  is included before 
 due to a circular dependancy I introduced with this 
commit.


It can be fixed by replacing the `#include ` with 
`#include ` in "include/asm-generic/bug.h".  I can send a 
patch to fix that, but ideally, it should be committed before this 
commit to avoid breakage during git bisect builds.  So my question is, 
should I send a single patch to fix this breakage, or resend the series 
of patches to incorporate this fix before the current commit?


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

[PATCH v4 1/2] asm-generic/bug.h: declare struct pt_regs; before function prototype

[Ian Abbott]

The declaration of `__warn()` has `struct pt_regs *regs` as one of its
parameters.  This can result in compiler warnings if `struct regs` is
not already declared.  Add an empty declaration of `struct pt_regs` to
avoid the warnings.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Arnd Bergmann <a...@arndb.de>
Acked-by: Arnd Bergmann <a...@arndb.de>
---
v3: Actually, there was no v1 or v2.  I called this v3 to match the
series.
v4: Corrected 'Acked-by:' line in patch description.
---
 include/asm-generic/bug.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index d6f4aed479a1..87191357d303 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -97,6 +97,7 @@ extern void warn_slowpath_null(const char *file, const int 
line);
 
 /* used internally by panic.c */
 struct warn_args;
+struct pt_regs;
 
 void __warn(const char *file, int line, void *caller, unsigned taint,
struct pt_regs *regs, struct warn_args *args);
-- 
2.11.0

[PATCH v4 2/2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

If the first parameter of container_of() is a pointer to a
non-const-qualified array type (and the third parameter names a
non-const-qualified array member), the local variable __mptr will be
defined with a const-qualified array type.  In ISO C, these types are
incompatible.  They work as expected in GNU C, but some versions will
issue warnings.  For example, GCC 4.9 produces the warning
"initialization from incompatible pointer type".

Here is an example of where the problem occurs:

---
 #include 
 #include 

MODULE_LICENSE("GPL");

struct st {
int a;
char b[16];
};

static int __init example_init(void) {
struct st t = { .a = 101, .b = "hello" };
char (*p)[16] = 
struct st *x = container_of(p, struct st, b);
printk(KERN_DEBUG "%p %p\n", (void *), (void *)x);
return 0;
}

static void __exit example_exit(void) {
}

module_init(example_init);
module_exit(example_exit);
---

Building the module with gcc-4.9 results in these warnings (where '{m}'
is the module source and '{k}' is the kernel source):

---
In file included from {m}/example.c:1:0:
{m}/example.c: In function ‘example_init’:
{k}/include/linux/kernel.h:854:48: warning: initialization from
incompatible pointer type
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
{k}/include/linux/kernel.h:854:48: warning: (near initialization for
‘x’)
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
---

Replace the type checking performed by the macro to avoid these
warnings.  Make sure `*(ptr)` either has type compatible with the
member, or has type compatible with `void`, ignoring qualifiers.  Raise
compiler errors if this is not true.  This is stronger than the previous
behaviour, which only resulted in compiler warnings for a type mismatch.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Michal Nazarewicz <min...@mina86.com>
Cc: Hidehiro Kawai <hidehiro.kawai...@hitachi.com>
Cc: Borislav Petkov <b...@suse.de>
Cc: Rasmus Villemoes <li...@rasmusvillemoes.dk>
Cc: Johannes Berg <johannes.b...@intel.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Alexander Potapenko <gli...@google.com>
---
v2: Rebased and altered description to provide an example of when the
compiler warnings occur.  v1 (from 2016-10-10) also modified a
'container_of_safe()' macro that never made it out of "linux-next".
v3: Added back some type checking at the suggestion of Michal
Nazarewicz with some helpful hints by Peter Zijlstra.
v4: No change.
---
 include/linux/kernel.h | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 13bc08aba704..f5dba37aaa60 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -11,6 +11,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -850,9 +851,11 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
oops_dump_mode) { }
  * @member:the name of the member within the struct.
  *
  */
-#define container_of(ptr, type, member) ({ \
-   const typeof( ((type *)0)->member ) *__mptr = (ptr);\
-   (type *)( (char *)__mptr - offsetof(type,member) );})
+#define container_of(ptr, type, member) ({ \
+   BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
+!__same_type(*(ptr), void),\
+"pointer type mismatch in container_of()");\
+   ((type *)((char *)(ptr) - offsetof(type, member))); })
 
 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
-- 
2.11.0

[PATCH v4 1/2] asm-generic/bug.h: declare struct pt_regs; before function prototype

[Ian Abbott]

The declaration of `__warn()` has `struct pt_regs *regs` as one of its
parameters.  This can result in compiler warnings if `struct regs` is
not already declared.  Add an empty declaration of `struct pt_regs` to
avoid the warnings.

Signed-off-by: Ian Abbott 
Cc: Arnd Bergmann 
Acked-by: Arnd Bergmann 
---
v3: Actually, there was no v1 or v2.  I called this v3 to match the
series.
v4: Corrected 'Acked-by:' line in patch description.
---
 include/asm-generic/bug.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index d6f4aed479a1..87191357d303 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -97,6 +97,7 @@ extern void warn_slowpath_null(const char *file, const int 
line);
 
 /* used internally by panic.c */
 struct warn_args;
+struct pt_regs;
 
 void __warn(const char *file, int line, void *caller, unsigned taint,
struct pt_regs *regs, struct warn_args *args);
-- 
2.11.0

[PATCH v4 2/2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

If the first parameter of container_of() is a pointer to a
non-const-qualified array type (and the third parameter names a
non-const-qualified array member), the local variable __mptr will be
defined with a const-qualified array type.  In ISO C, these types are
incompatible.  They work as expected in GNU C, but some versions will
issue warnings.  For example, GCC 4.9 produces the warning
"initialization from incompatible pointer type".

Here is an example of where the problem occurs:

---
 #include 
 #include 

MODULE_LICENSE("GPL");

struct st {
int a;
char b[16];
};

static int __init example_init(void) {
struct st t = { .a = 101, .b = "hello" };
char (*p)[16] = 
struct st *x = container_of(p, struct st, b);
printk(KERN_DEBUG "%p %p\n", (void *), (void *)x);
return 0;
}

static void __exit example_exit(void) {
}

module_init(example_init);
module_exit(example_exit);
---

Building the module with gcc-4.9 results in these warnings (where '{m}'
is the module source and '{k}' is the kernel source):

---
In file included from {m}/example.c:1:0:
{m}/example.c: In function ‘example_init’:
{k}/include/linux/kernel.h:854:48: warning: initialization from
incompatible pointer type
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
{k}/include/linux/kernel.h:854:48: warning: (near initialization for
‘x’)
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
---

Replace the type checking performed by the macro to avoid these
warnings.  Make sure `*(ptr)` either has type compatible with the
member, or has type compatible with `void`, ignoring qualifiers.  Raise
compiler errors if this is not true.  This is stronger than the previous
behaviour, which only resulted in compiler warnings for a type mismatch.

Signed-off-by: Ian Abbott 
Cc: Andrew Morton 
Cc: Michal Nazarewicz 
Cc: Hidehiro Kawai 
Cc: Borislav Petkov 
Cc: Rasmus Villemoes 
Cc: Johannes Berg 
Cc: Peter Zijlstra 
Cc: Alexander Potapenko 
---
v2: Rebased and altered description to provide an example of when the
compiler warnings occur.  v1 (from 2016-10-10) also modified a
'container_of_safe()' macro that never made it out of "linux-next".
v3: Added back some type checking at the suggestion of Michal
Nazarewicz with some helpful hints by Peter Zijlstra.
v4: No change.
---
 include/linux/kernel.h | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 13bc08aba704..f5dba37aaa60 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -11,6 +11,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -850,9 +851,11 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
oops_dump_mode) { }
  * @member:the name of the member within the struct.
  *
  */
-#define container_of(ptr, type, member) ({ \
-   const typeof( ((type *)0)->member ) *__mptr = (ptr);\
-   (type *)( (char *)__mptr - offsetof(type,member) );})
+#define container_of(ptr, type, member) ({ \
+   BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
+!__same_type(*(ptr), void),\
+"pointer type mismatch in container_of()");\
+   ((type *)((char *)(ptr) - offsetof(type, member))); })
 
 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
-- 
2.11.0

[PATCH v4 0/2] kernel.h: container_of() pointer checking

[Ian Abbott]

[I screwed up an 'Acked-by:' line on v3.]

Patch 2 changes the container_of() macro to improve the compatibility
checking when the member has array type.  As a bonus (?), if the pointer
neither points to a type compatible with the member nor points to a type
compatible with void, compiler errors are produced instead of warnings.

Patch 1 is a prerequisite to avoid a lot of warnings when 
is included by .

1) asm-generic/bug.h: declare struct pt_regs; before function prototype
2) kernel.h: handle pointers to arrays better in container_of()

 include/asm-generic/bug.h | 1 +
 include/linux/kernel.h| 9 ++---
 2 files changed, 7 insertions(+), 3 deletions(-)

[PATCH v4 0/2] kernel.h: container_of() pointer checking

[Ian Abbott]

[I screwed up an 'Acked-by:' line on v3.]

Patch 2 changes the container_of() macro to improve the compatibility
checking when the member has array type.  As a bonus (?), if the pointer
neither points to a type compatible with the member nor points to a type
compatible with void, compiler errors are produced instead of warnings.

Patch 1 is a prerequisite to avoid a lot of warnings when 
is included by .

1) asm-generic/bug.h: declare struct pt_regs; before function prototype
2) kernel.h: handle pointers to arrays better in container_of()

 include/asm-generic/bug.h | 1 +
 include/linux/kernel.h| 9 ++---
 2 files changed, 7 insertions(+), 3 deletions(-)

[PATCH v3 1/2] asm-generic/bug.h: declare struct pt_regs; before function prototype

[Ian Abbott]

The declaration of `__warn()` has `struct pt_regs *regs` as one of its
parameters.  This can result in compiler warnings if `struct regs` is
not already declared.  Add an empty declaration of `struct pt_regs` to
avoid the warnings.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Arnd Bergmann <a...@arndb.de>
Acked-by: Acked-by: Arnd Bergmann <a...@arndb.de>
---
v3: Actually, there was no v1 or v2.  I called this v3 to match the
series.
---
 include/asm-generic/bug.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index d6f4aed479a1..87191357d303 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -97,6 +97,7 @@ extern void warn_slowpath_null(const char *file, const int 
line);
 
 /* used internally by panic.c */
 struct warn_args;
+struct pt_regs;
 
 void __warn(const char *file, int line, void *caller, unsigned taint,
struct pt_regs *regs, struct warn_args *args);
-- 
2.11.0

[PATCH v3 0/2] kernel.h: container_of() pointer checking

[Ian Abbott]

[Apologies for the resend.  I got both mailing list addresses wrong!]

Patch 2 changes the container_of() macro to improve the compatibility
checking when the member has array type.  As a bonus (?), if the pointer
neither points to a type compatible with the member nor points to a type
compatible with void, compiler errors are produced instead of warnings.

Patch 1 is a prerequisite to avoid a lot of warnings when 
is included by .

1) asm-generic/bug.h: declare struct pt_regs; before function prototype
2) kernel.h: handle pointers to arrays better in container_of()

 include/asm-generic/bug.h | 1 +
 include/linux/kernel.h| 9 ++---
 2 files changed, 7 insertions(+), 3 deletions(-)

[PATCH v3 1/2] asm-generic/bug.h: declare struct pt_regs; before function prototype

[Ian Abbott]

The declaration of `__warn()` has `struct pt_regs *regs` as one of its
parameters.  This can result in compiler warnings if `struct regs` is
not already declared.  Add an empty declaration of `struct pt_regs` to
avoid the warnings.

Signed-off-by: Ian Abbott 
Cc: Arnd Bergmann 
Acked-by: Acked-by: Arnd Bergmann 
---
v3: Actually, there was no v1 or v2.  I called this v3 to match the
series.
---
 include/asm-generic/bug.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index d6f4aed479a1..87191357d303 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -97,6 +97,7 @@ extern void warn_slowpath_null(const char *file, const int 
line);
 
 /* used internally by panic.c */
 struct warn_args;
+struct pt_regs;
 
 void __warn(const char *file, int line, void *caller, unsigned taint,
struct pt_regs *regs, struct warn_args *args);
-- 
2.11.0

[PATCH v3 0/2] kernel.h: container_of() pointer checking

[Ian Abbott]

[Apologies for the resend.  I got both mailing list addresses wrong!]

Patch 2 changes the container_of() macro to improve the compatibility
checking when the member has array type.  As a bonus (?), if the pointer
neither points to a type compatible with the member nor points to a type
compatible with void, compiler errors are produced instead of warnings.

Patch 1 is a prerequisite to avoid a lot of warnings when 
is included by .

1) asm-generic/bug.h: declare struct pt_regs; before function prototype
2) kernel.h: handle pointers to arrays better in container_of()

 include/asm-generic/bug.h | 1 +
 include/linux/kernel.h| 9 ++---
 2 files changed, 7 insertions(+), 3 deletions(-)

[PATCH v3 2/2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

If the first parameter of container_of() is a pointer to a
non-const-qualified array type (and the third parameter names a
non-const-qualified array member), the local variable __mptr will be
defined with a const-qualified array type.  In ISO C, these types are
incompatible.  They work as expected in GNU C, but some versions will
issue warnings.  For example, GCC 4.9 produces the warning
"initialization from incompatible pointer type".

Here is an example of where the problem occurs:

---
 #include 
 #include 

MODULE_LICENSE("GPL");

struct st {
int a;
char b[16];
};

static int __init example_init(void) {
struct st t = { .a = 101, .b = "hello" };
char (*p)[16] = 
struct st *x = container_of(p, struct st, b);
printk(KERN_DEBUG "%p %p\n", (void *), (void *)x);
return 0;
}

static void __exit example_exit(void) {
}

module_init(example_init);
module_exit(example_exit);
---

Building the module with gcc-4.9 results in these warnings (where '{m}'
is the module source and '{k}' is the kernel source):

---
In file included from {m}/example.c:1:0:
{m}/example.c: In function ‘example_init’:
{k}/include/linux/kernel.h:854:48: warning: initialization from
incompatible pointer type
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
{k}/include/linux/kernel.h:854:48: warning: (near initialization for
‘x’)
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
---

Replace the type checking performed by the macro to avoid these
warnings.  Make sure `*(ptr)` either has type compatible with the
member, or has type compatible with `void`, ignoring qualifiers.  Raise
compiler errors if this is not true.  This is stronger than the previous
behaviour, which only resulted in compiler warnings for a type mismatch.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Michal Nazarewicz <min...@mina86.com>
Cc: Hidehiro Kawai <hidehiro.kawai...@hitachi.com>
Cc: Borislav Petkov <b...@suse.de>
Cc: Rasmus Villemoes <li...@rasmusvillemoes.dk>
Cc: Johannes Berg <johannes.b...@intel.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Alexander Potapenko <gli...@google.com>
---
v2: Rebased and altered description to provide an example of when the
compiler warnings occur.  v1 (from 2016-10-10) also modified a
'container_of_safe()' macro that never made it out of "linux-next".
v3: Added back some type checking at the suggestion of Michal
Nazarewicz with some helpful hints by Peter Zijlstra.
---
 include/linux/kernel.h | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 13bc08aba704..f5dba37aaa60 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -11,6 +11,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -850,9 +851,11 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
oops_dump_mode) { }
  * @member:the name of the member within the struct.
  *
  */
-#define container_of(ptr, type, member) ({ \
-   const typeof( ((type *)0)->member ) *__mptr = (ptr);\
-   (type *)( (char *)__mptr - offsetof(type,member) );})
+#define container_of(ptr, type, member) ({ \
+   BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
+!__same_type(*(ptr), void),\
+"pointer type mismatch in container_of()");\
+   ((type *)((char *)(ptr) - offsetof(type, member))); })
 
 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
-- 
2.11.0

[PATCH v3 2/2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

If the first parameter of container_of() is a pointer to a
non-const-qualified array type (and the third parameter names a
non-const-qualified array member), the local variable __mptr will be
defined with a const-qualified array type.  In ISO C, these types are
incompatible.  They work as expected in GNU C, but some versions will
issue warnings.  For example, GCC 4.9 produces the warning
"initialization from incompatible pointer type".

Here is an example of where the problem occurs:

---
 #include 
 #include 

MODULE_LICENSE("GPL");

struct st {
int a;
char b[16];
};

static int __init example_init(void) {
struct st t = { .a = 101, .b = "hello" };
char (*p)[16] = 
struct st *x = container_of(p, struct st, b);
printk(KERN_DEBUG "%p %p\n", (void *), (void *)x);
return 0;
}

static void __exit example_exit(void) {
}

module_init(example_init);
module_exit(example_exit);
---

Building the module with gcc-4.9 results in these warnings (where '{m}'
is the module source and '{k}' is the kernel source):

---
In file included from {m}/example.c:1:0:
{m}/example.c: In function ‘example_init’:
{k}/include/linux/kernel.h:854:48: warning: initialization from
incompatible pointer type
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
{k}/include/linux/kernel.h:854:48: warning: (near initialization for
‘x’)
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
---

Replace the type checking performed by the macro to avoid these
warnings.  Make sure `*(ptr)` either has type compatible with the
member, or has type compatible with `void`, ignoring qualifiers.  Raise
compiler errors if this is not true.  This is stronger than the previous
behaviour, which only resulted in compiler warnings for a type mismatch.

Signed-off-by: Ian Abbott 
Cc: Andrew Morton 
Cc: Michal Nazarewicz 
Cc: Hidehiro Kawai 
Cc: Borislav Petkov 
Cc: Rasmus Villemoes 
Cc: Johannes Berg 
Cc: Peter Zijlstra 
Cc: Alexander Potapenko 
---
v2: Rebased and altered description to provide an example of when the
compiler warnings occur.  v1 (from 2016-10-10) also modified a
'container_of_safe()' macro that never made it out of "linux-next".
v3: Added back some type checking at the suggestion of Michal
Nazarewicz with some helpful hints by Peter Zijlstra.
---
 include/linux/kernel.h | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 13bc08aba704..f5dba37aaa60 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -11,6 +11,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -850,9 +851,11 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
oops_dump_mode) { }
  * @member:the name of the member within the struct.
  *
  */
-#define container_of(ptr, type, member) ({ \
-   const typeof( ((type *)0)->member ) *__mptr = (ptr);\
-   (type *)( (char *)__mptr - offsetof(type,member) );})
+#define container_of(ptr, type, member) ({ \
+   BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
+!__same_type(*(ptr), void),\
+"pointer type mismatch in container_of()");\
+   ((type *)((char *)(ptr) - offsetof(type, member))); })
 
 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
-- 
2.11.0

Re: [PATCH v2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 23/05/17 14:23, Ian Abbott wrote:

On 23/05/17 13:02, Ian Abbott wrote:

On 23/05/17 12:24, Peter Zijlstra wrote:

On Tue, May 23, 2017 at 11:32:02AM +0100, Ian Abbott wrote:


#define container_of(ptr, type, member) (\
_Static_assert(__builtin_types_compatible_p(\
typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),\
((type *)((char *)(ptr) - offsetof(type, member)));\
)


It's a fine suggestion (if more parentheses are added), but
_Static_assert
is a C11 feature, and I thought the kernel was using gnu89 (unless
it's been
updated since).


We have BUILD_BUG_ON() that should be similar in functionality.


Yes, I think BUILD_BUG_ON_ZERO() will be better in this case to avoid
some baggage.  Also, __same_type() can be used for type checking.



BUILD_BUG_ON_ZERO() in the left-hand side of a comma expression produced
lots of warnings about it having no effect, so I tried using
BUILD_BUG_ON() instead as follows:

#define container_of(ptr, type, member) ({\
BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member));\
((type *)((char *)(ptr) - offsetof(type, member)));})

Unfortunately, this does result in compiler failures in places where the
types do not in fact match, for example `slab_show()` in
"mm/slab_common.c" (`list_entry()` uses `container_of()`):

static int slab_show(struct seq_file *m, void *p)
{
struct kmem_cache *s = list_entry(p, struct kmem_cache,
root_caches_node);


I then changed it to allow `ptr` to be a `void *` too:

#define container_of(ptr, type, member) ({\
BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member) &&\
 !__same_type((ptr), void *));\
((type *)((char *)(ptr) - offsetof(type, member)));})

Now it fails on `external_name()` in "fs/dcache.c":

static inline struct external_name *external_name(struct dentry *dentry)
{
return container_of(dentry->d_name.name, struct external_name,
name[0]);
}

Here, `dentry->d_name.name` is of type `const unsigned char *` and
`name[0]` in `struct external_name` is of type `unsigned char`.  The
error is:

error: call to ‘__compiletime_assert_258’ declared with attribute error:
BUILD_BUG_ON failed: !__same_type((dentry->d_name.name), &((struct
external_name *)0)->name[0]) && !__same_type((dentry->d_name.name), void *)
  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
  ^

I'm not quite sure why that is failing yet.


It fails because `const unsigned char *` is not compatible with 
`unsigned char *` as fast as `__builtin_types_compatible_p()` is 
concerned (quite rightly so).  However, a simple change fixes that:


#define container_of(ptr, type, member) ({  \
BUILD_BUG_ON(!__same_type(*(ptr), ((type *)0)->member) &&\
 !__same_type((ptr), void *));  \
((type *)((char *)(ptr) - offsetof(type, member)));})

That one seems to work fine (after including  in kernel.h 
and fixing up include/asm-generic/bug.h a little bit, which will be in a 
separate patch.


Thanks for the help guys.  v3 will be a series of 2 patches.

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 23/05/17 14:23, Ian Abbott wrote:

On 23/05/17 13:02, Ian Abbott wrote:

On 23/05/17 12:24, Peter Zijlstra wrote:

On Tue, May 23, 2017 at 11:32:02AM +0100, Ian Abbott wrote:


#define container_of(ptr, type, member) (\
_Static_assert(__builtin_types_compatible_p(\
typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),\
((type *)((char *)(ptr) - offsetof(type, member)));\
)


It's a fine suggestion (if more parentheses are added), but
_Static_assert
is a C11 feature, and I thought the kernel was using gnu89 (unless
it's been
updated since).


We have BUILD_BUG_ON() that should be similar in functionality.


Yes, I think BUILD_BUG_ON_ZERO() will be better in this case to avoid
some baggage.  Also, __same_type() can be used for type checking.



BUILD_BUG_ON_ZERO() in the left-hand side of a comma expression produced
lots of warnings about it having no effect, so I tried using
BUILD_BUG_ON() instead as follows:

#define container_of(ptr, type, member) ({\
BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member));\
((type *)((char *)(ptr) - offsetof(type, member)));})

Unfortunately, this does result in compiler failures in places where the
types do not in fact match, for example `slab_show()` in
"mm/slab_common.c" (`list_entry()` uses `container_of()`):

static int slab_show(struct seq_file *m, void *p)
{
struct kmem_cache *s = list_entry(p, struct kmem_cache,
root_caches_node);


I then changed it to allow `ptr` to be a `void *` too:

#define container_of(ptr, type, member) ({\
BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member) &&\
 !__same_type((ptr), void *));\
((type *)((char *)(ptr) - offsetof(type, member)));})

Now it fails on `external_name()` in "fs/dcache.c":

static inline struct external_name *external_name(struct dentry *dentry)
{
return container_of(dentry->d_name.name, struct external_name,
name[0]);
}

Here, `dentry->d_name.name` is of type `const unsigned char *` and
`name[0]` in `struct external_name` is of type `unsigned char`.  The
error is:

error: call to ‘__compiletime_assert_258’ declared with attribute error:
BUILD_BUG_ON failed: !__same_type((dentry->d_name.name), &((struct
external_name *)0)->name[0]) && !__same_type((dentry->d_name.name), void *)
  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
  ^

I'm not quite sure why that is failing yet.


It fails because `const unsigned char *` is not compatible with 
`unsigned char *` as fast as `__builtin_types_compatible_p()` is 
concerned (quite rightly so).  However, a simple change fixes that:


#define container_of(ptr, type, member) ({  \
BUILD_BUG_ON(!__same_type(*(ptr), ((type *)0)->member) &&\
 !__same_type((ptr), void *));  \
((type *)((char *)(ptr) - offsetof(type, member)));})

That one seems to work fine (after including  in kernel.h 
and fixing up include/asm-generic/bug.h a little bit, which will be in a 
separate patch.


Thanks for the help guys.  v3 will be a series of 2 patches.

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 23/05/17 13:02, Ian Abbott wrote:

On 23/05/17 12:24, Peter Zijlstra wrote:

On Tue, May 23, 2017 at 11:32:02AM +0100, Ian Abbott wrote:


#define container_of(ptr, type, member) (\
_Static_assert(__builtin_types_compatible_p(\
typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),\
((type *)((char *)(ptr) - offsetof(type, member)));\
)


It's a fine suggestion (if more parentheses are added), but
_Static_assert
is a C11 feature, and I thought the kernel was using gnu89 (unless
it's been
updated since).


We have BUILD_BUG_ON() that should be similar in functionality.


Yes, I think BUILD_BUG_ON_ZERO() will be better in this case to avoid
some baggage.  Also, __same_type() can be used for type checking.



BUILD_BUG_ON_ZERO() in the left-hand side of a comma expression produced 
lots of warnings about it having no effect, so I tried using 
BUILD_BUG_ON() instead as follows:


#define container_of(ptr, type, member) ({  \
BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member));\
((type *)((char *)(ptr) - offsetof(type, member)));})

Unfortunately, this does result in compiler failures in places where the 
types do not in fact match, for example `slab_show()` in 
"mm/slab_common.c" (`list_entry()` uses `container_of()`):


static int slab_show(struct seq_file *m, void *p)
{
struct kmem_cache *s = list_entry(p, struct kmem_cache, 
root_caches_node);


I then changed it to allow `ptr` to be a `void *` too:

#define container_of(ptr, type, member) ({  \
BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member) &&\
 !__same_type((ptr), void *));  \
((type *)((char *)(ptr) - offsetof(type, member)));})

Now it fails on `external_name()` in "fs/dcache.c":

static inline struct external_name *external_name(struct dentry *dentry)
{
return container_of(dentry->d_name.name, struct external_name, name[0]);
}

Here, `dentry->d_name.name` is of type `const unsigned char *` and 
`name[0]` in `struct external_name` is of type `unsigned char`.  The 
error is:


error: call to ‘__compiletime_assert_258’ declared with attribute error: 
BUILD_BUG_ON failed: !__same_type((dentry->d_name.name), &((struct 
external_name *)0)->name[0]) && !__same_type((dentry->d_name.name), void *)

  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
          ^

I'm not quite sure why that is failing yet.

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 23/05/17 13:02, Ian Abbott wrote:

On 23/05/17 12:24, Peter Zijlstra wrote:

On Tue, May 23, 2017 at 11:32:02AM +0100, Ian Abbott wrote:


#define container_of(ptr, type, member) (\
_Static_assert(__builtin_types_compatible_p(\
typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),\
((type *)((char *)(ptr) - offsetof(type, member)));\
)


It's a fine suggestion (if more parentheses are added), but
_Static_assert
is a C11 feature, and I thought the kernel was using gnu89 (unless
it's been
updated since).


We have BUILD_BUG_ON() that should be similar in functionality.


Yes, I think BUILD_BUG_ON_ZERO() will be better in this case to avoid
some baggage.  Also, __same_type() can be used for type checking.



BUILD_BUG_ON_ZERO() in the left-hand side of a comma expression produced 
lots of warnings about it having no effect, so I tried using 
BUILD_BUG_ON() instead as follows:


#define container_of(ptr, type, member) ({  \
BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member));\
((type *)((char *)(ptr) - offsetof(type, member)));})

Unfortunately, this does result in compiler failures in places where the 
types do not in fact match, for example `slab_show()` in 
"mm/slab_common.c" (`list_entry()` uses `container_of()`):


static int slab_show(struct seq_file *m, void *p)
{
struct kmem_cache *s = list_entry(p, struct kmem_cache, 
root_caches_node);


I then changed it to allow `ptr` to be a `void *` too:

#define container_of(ptr, type, member) ({  \
BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member) &&\
 !__same_type((ptr), void *));  \
((type *)((char *)(ptr) - offsetof(type, member)));})

Now it fails on `external_name()` in "fs/dcache.c":

static inline struct external_name *external_name(struct dentry *dentry)
{
return container_of(dentry->d_name.name, struct external_name, name[0]);
}

Here, `dentry->d_name.name` is of type `const unsigned char *` and 
`name[0]` in `struct external_name` is of type `unsigned char`.  The 
error is:


error: call to ‘__compiletime_assert_258’ declared with attribute error: 
BUILD_BUG_ON failed: !__same_type((dentry->d_name.name), &((struct 
external_name *)0)->name[0]) && !__same_type((dentry->d_name.name), void *)

  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
          ^

I'm not quite sure why that is failing yet.

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 23/05/17 12:24, Peter Zijlstra wrote:

On Tue, May 23, 2017 at 11:32:02AM +0100, Ian Abbott wrote:


#define container_of(ptr, type, member) (   \
_Static_assert(__builtin_types_compatible_p(\
typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),  \
((type *)((char *)(ptr) - offsetof(type, member))); \
)


It's a fine suggestion (if more parentheses are added), but _Static_assert
is a C11 feature, and I thought the kernel was using gnu89 (unless it's been
updated since).


We have BUILD_BUG_ON() that should be similar in functionality.


Yes, I think BUILD_BUG_ON_ZERO() will be better in this case to avoid 
some baggage.  Also, __same_type() can be used for type checking.


--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 23/05/17 12:24, Peter Zijlstra wrote:

On Tue, May 23, 2017 at 11:32:02AM +0100, Ian Abbott wrote:


#define container_of(ptr, type, member) (   \
_Static_assert(__builtin_types_compatible_p(\
typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),  \
((type *)((char *)(ptr) - offsetof(type, member))); \
)


It's a fine suggestion (if more parentheses are added), but _Static_assert
is a C11 feature, and I thought the kernel was using gnu89 (unless it's been
updated since).


We have BUILD_BUG_ON() that should be similar in functionality.


Yes, I think BUILD_BUG_ON_ZERO() will be better in this case to avoid 
some baggage.  Also, __same_type() can be used for type checking.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 22/05/17 18:58, Michal Nazarewicz wrote:

On Mon, May 22 2017, Ian Abbott wrote:

If the first parameter of container_of() is a pointer to a
non-const-qualified array type (and the third parameter names a
non-const-qualified array member), the local variable __mptr will be
defined with a const-qualified array type.  In ISO C, these types are
incompatible.  They work as expected in GNU C, but some versions will
issue warnings.  For example, GCC 4.9 produces the warning
"initialization from incompatible pointer type".

Here is an example of where the problem occurs:

---
 #include 
 #include 

MODULE_LICENSE("GPL");

struct st {
int a;
char b[16];
};

static int __init example_init(void) {
struct st t = { .a = 101, .b = "hello" };
char (*p)[16] = 
struct st *x = container_of(p, struct st, b);
printk(KERN_DEBUG "%p %p\n", (void *), (void *)x);
return 0;
}

static void __exit example_exit(void) {
}

module_init(example_init);
module_exit(example_exit);
---

Building the module with gcc-4.9 results in these warnings (where '{m}'
is the module source and '{k}' is the kernel source):

---
In file included from {m}/example.c:1:0:
{m}/example.c: In function ‘example_init’:
{k}/include/linux/kernel.h:854:48: warning: initialization from
incompatible pointer type
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
{k}/include/linux/kernel.h:854:48: warning: (near initialization for
‘x’)
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
---

Fix it by avoiding defining the __mptr variable.  This also avoids other
GCC extensions.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Michal Nazarewicz <min...@mina86.com>
Cc: Hidehiro Kawai <hidehiro.kawai...@hitachi.com>
Cc: Borislav Petkov <b...@suse.de>
Cc: Rasmus Villemoes <li...@rasmusvillemoes.dk>
Cc: Johannes Berg <johannes.b...@intel.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Alexander Potapenko <gli...@google.com>
---
v2: Rebased and altered description to provide an example of when the
compiler warnings occur.  v1 (from 2016-10-10) also modified a
'container_of_safe()' macro that never made it out of "linux-next".
---
 include/linux/kernel.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 13bc08aba704..169fe6f51b7b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -850,9 +850,8 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
oops_dump_mode) { }
  * @member:the name of the member within the struct.
  *
  */
-#define container_of(ptr, type, member) ({ \
-   const typeof( ((type *)0)->member ) *__mptr = (ptr); \
-   (type *)( (char *)__mptr - offsetof(type,member) );})
+#define container_of(ptr, type, member) \
+   ((type *)((char *)(ptr) - offsetof(type, member)))


Now the type of ptr is not checked though.  Using your example, I can
now write:

struct st t = { .a = 101, .b = "hello" };
int *p = 
struct st *x = container_of(p, struct st, b);

and it will compile with no warnings.  Previously it would fail.  The
best I can think of would be (not tested):

#define container_of(ptr, type, member) (   \
_Static_assert(__builtin_types_compatible_p(\
typeof(ptr), typeof( ((type *)0)->member )*), "WUT"),  \
((type *)((char *)(ptr) - offsetof(type, member))); \
)

or maybe:

#define container_of(ptr, type, member) (   \
_Static_assert(__builtin_types_compatible_p(\
typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),  \
((type *)((char *)(ptr) - offsetof(type, member))); \
)


It's a fine suggestion (if more parentheses are added), but 
_Static_assert is a C11 feature, and I thought the kernel was using 
gnu89 (unless it's been updated since).


--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 22/05/17 18:58, Michal Nazarewicz wrote:

On Mon, May 22 2017, Ian Abbott wrote:

If the first parameter of container_of() is a pointer to a
non-const-qualified array type (and the third parameter names a
non-const-qualified array member), the local variable __mptr will be
defined with a const-qualified array type.  In ISO C, these types are
incompatible.  They work as expected in GNU C, but some versions will
issue warnings.  For example, GCC 4.9 produces the warning
"initialization from incompatible pointer type".

Here is an example of where the problem occurs:

---
 #include 
 #include 

MODULE_LICENSE("GPL");

struct st {
int a;
char b[16];
};

static int __init example_init(void) {
struct st t = { .a = 101, .b = "hello" };
char (*p)[16] = 
struct st *x = container_of(p, struct st, b);
printk(KERN_DEBUG "%p %p\n", (void *), (void *)x);
return 0;
}

static void __exit example_exit(void) {
}

module_init(example_init);
module_exit(example_exit);
---

Building the module with gcc-4.9 results in these warnings (where '{m}'
is the module source and '{k}' is the kernel source):

---
In file included from {m}/example.c:1:0:
{m}/example.c: In function ‘example_init’:
{k}/include/linux/kernel.h:854:48: warning: initialization from
incompatible pointer type
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
{k}/include/linux/kernel.h:854:48: warning: (near initialization for
‘x’)
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
---

Fix it by avoiding defining the __mptr variable.  This also avoids other
GCC extensions.

Signed-off-by: Ian Abbott 
Cc: Andrew Morton 
Cc: Michal Nazarewicz 
Cc: Hidehiro Kawai 
Cc: Borislav Petkov 
Cc: Rasmus Villemoes 
Cc: Johannes Berg 
Cc: Peter Zijlstra 
Cc: Alexander Potapenko 
---
v2: Rebased and altered description to provide an example of when the
compiler warnings occur.  v1 (from 2016-10-10) also modified a
'container_of_safe()' macro that never made it out of "linux-next".
---
 include/linux/kernel.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 13bc08aba704..169fe6f51b7b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -850,9 +850,8 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
oops_dump_mode) { }
  * @member:the name of the member within the struct.
  *
  */
-#define container_of(ptr, type, member) ({ \
-   const typeof( ((type *)0)->member ) *__mptr = (ptr); \
-   (type *)( (char *)__mptr - offsetof(type,member) );})
+#define container_of(ptr, type, member) \
+   ((type *)((char *)(ptr) - offsetof(type, member)))


Now the type of ptr is not checked though.  Using your example, I can
now write:

struct st t = { .a = 101, .b = "hello" };
int *p = 
struct st *x = container_of(p, struct st, b);

and it will compile with no warnings.  Previously it would fail.  The
best I can think of would be (not tested):

#define container_of(ptr, type, member) (   \
_Static_assert(__builtin_types_compatible_p(\
typeof(ptr), typeof( ((type *)0)->member )*), "WUT"),  \
((type *)((char *)(ptr) - offsetof(type, member))); \
)

or maybe:

#define container_of(ptr, type, member) (   \
_Static_assert(__builtin_types_compatible_p(\
typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),  \
((type *)((char *)(ptr) - offsetof(type, member))); \
)


It's a fine suggestion (if more parentheses are added), but 
_Static_assert is a C11 feature, and I thought the kernel was using 
gnu89 (unless it's been updated since).


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

[PATCH v2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

If the first parameter of container_of() is a pointer to a
non-const-qualified array type (and the third parameter names a
non-const-qualified array member), the local variable __mptr will be
defined with a const-qualified array type.  In ISO C, these types are
incompatible.  They work as expected in GNU C, but some versions will
issue warnings.  For example, GCC 4.9 produces the warning
"initialization from incompatible pointer type".

Here is an example of where the problem occurs:

---
 #include 
 #include 

MODULE_LICENSE("GPL");

struct st {
int a;
char b[16];
};

static int __init example_init(void) {
struct st t = { .a = 101, .b = "hello" };
char (*p)[16] = 
struct st *x = container_of(p, struct st, b);
printk(KERN_DEBUG "%p %p\n", (void *), (void *)x);
return 0;
}

static void __exit example_exit(void) {
}

module_init(example_init);
module_exit(example_exit);
---

Building the module with gcc-4.9 results in these warnings (where '{m}'
is the module source and '{k}' is the kernel source):

---
In file included from {m}/example.c:1:0:
{m}/example.c: In function ‘example_init’:
{k}/include/linux/kernel.h:854:48: warning: initialization from
incompatible pointer type
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
{k}/include/linux/kernel.h:854:48: warning: (near initialization for
‘x’)
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
---

Fix it by avoiding defining the __mptr variable.  This also avoids other
GCC extensions.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Michal Nazarewicz <min...@mina86.com>
Cc: Hidehiro Kawai <hidehiro.kawai...@hitachi.com>
Cc: Borislav Petkov <b...@suse.de>
Cc: Rasmus Villemoes <li...@rasmusvillemoes.dk>
Cc: Johannes Berg <johannes.b...@intel.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Alexander Potapenko <gli...@google.com>
---
v2: Rebased and altered description to provide an example of when the
compiler warnings occur.  v1 (from 2016-10-10) also modified a
'container_of_safe()' macro that never made it out of "linux-next".
---
 include/linux/kernel.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 13bc08aba704..169fe6f51b7b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -850,9 +850,8 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
oops_dump_mode) { }
  * @member:the name of the member within the struct.
  *
  */
-#define container_of(ptr, type, member) ({ \
-   const typeof( ((type *)0)->member ) *__mptr = (ptr);\
-   (type *)( (char *)__mptr - offsetof(type,member) );})
+#define container_of(ptr, type, member) \
+   ((type *)((char *)(ptr) - offsetof(type, member)))
 
 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
-- 
2.11.0

[PATCH v2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

If the first parameter of container_of() is a pointer to a
non-const-qualified array type (and the third parameter names a
non-const-qualified array member), the local variable __mptr will be
defined with a const-qualified array type.  In ISO C, these types are
incompatible.  They work as expected in GNU C, but some versions will
issue warnings.  For example, GCC 4.9 produces the warning
"initialization from incompatible pointer type".

Here is an example of where the problem occurs:

---
 #include 
 #include 

MODULE_LICENSE("GPL");

struct st {
int a;
char b[16];
};

static int __init example_init(void) {
struct st t = { .a = 101, .b = "hello" };
char (*p)[16] = 
struct st *x = container_of(p, struct st, b);
printk(KERN_DEBUG "%p %p\n", (void *), (void *)x);
return 0;
}

static void __exit example_exit(void) {
}

module_init(example_init);
module_exit(example_exit);
---

Building the module with gcc-4.9 results in these warnings (where '{m}'
is the module source and '{k}' is the kernel source):

---
In file included from {m}/example.c:1:0:
{m}/example.c: In function ‘example_init’:
{k}/include/linux/kernel.h:854:48: warning: initialization from
incompatible pointer type
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
{k}/include/linux/kernel.h:854:48: warning: (near initialization for
‘x’)
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
 ^
---

Fix it by avoiding defining the __mptr variable.  This also avoids other
GCC extensions.

Signed-off-by: Ian Abbott 
Cc: Andrew Morton 
Cc: Michal Nazarewicz 
Cc: Hidehiro Kawai 
Cc: Borislav Petkov 
Cc: Rasmus Villemoes 
Cc: Johannes Berg 
Cc: Peter Zijlstra 
Cc: Alexander Potapenko 
---
v2: Rebased and altered description to provide an example of when the
compiler warnings occur.  v1 (from 2016-10-10) also modified a
'container_of_safe()' macro that never made it out of "linux-next".
---
 include/linux/kernel.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 13bc08aba704..169fe6f51b7b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -850,9 +850,8 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
oops_dump_mode) { }
  * @member:the name of the member within the struct.
  *
  */
-#define container_of(ptr, type, member) ({ \
-   const typeof( ((type *)0)->member ) *__mptr = (ptr);\
-   (type *)( (char *)__mptr - offsetof(type,member) );})
+#define container_of(ptr, type, member) \
+   ((type *)((char *)(ptr) - offsetof(type, member)))
 
 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
-- 
2.11.0

Re: [PATCH 1/1] staging: comedi: addi_apci_3xxx: check return value

[Ian Abbott]

On 23/04/17 10:52, Pan Bian wrote:

From: Pan Bian <bianpan2...@163.com>

Function pci_ioremap_bar() will return a NULL pointer if there is no
enough memory. However, in function apci3xxx_auto_attach(), the return
value of function pci_ioremap_bar() is not validated. This may result in
NULL dereference in following access to dev->mmio. This patch fixes the
bug.

Signed-off-by: Pan Bian <bianpan2...@163.com>
---
 drivers/staging/comedi/drivers/addi_apci_3xxx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/comedi/drivers/addi_apci_3xxx.c 
b/drivers/staging/comedi/drivers/addi_apci_3xxx.c
index b6af3eb..a07d5bd 100644
--- a/drivers/staging/comedi/drivers/addi_apci_3xxx.c
+++ b/drivers/staging/comedi/drivers/addi_apci_3xxx.c
@@ -787,6 +787,8 @@ static int apci3xxx_auto_attach(struct comedi_device *dev,

dev->iobase = pci_resource_start(pcidev, 2);
dev->mmio = pci_ioremap_bar(pcidev, 3);
+   if (!dev->mmio)
+   return -ENOMEM;

if (pcidev->irq > 0) {
ret = request_irq(pcidev->irq, apci3xxx_irq_handler,



Looks good. Thanks for the fix.

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 1/1] staging: comedi: addi_apci_3xxx: check return value

[Ian Abbott]

On 23/04/17 10:52, Pan Bian wrote:

From: Pan Bian 

Function pci_ioremap_bar() will return a NULL pointer if there is no
enough memory. However, in function apci3xxx_auto_attach(), the return
value of function pci_ioremap_bar() is not validated. This may result in
NULL dereference in following access to dev->mmio. This patch fixes the
bug.

Signed-off-by: Pan Bian 
---
 drivers/staging/comedi/drivers/addi_apci_3xxx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/comedi/drivers/addi_apci_3xxx.c 
b/drivers/staging/comedi/drivers/addi_apci_3xxx.c
index b6af3eb..a07d5bd 100644
--- a/drivers/staging/comedi/drivers/addi_apci_3xxx.c
+++ b/drivers/staging/comedi/drivers/addi_apci_3xxx.c
@@ -787,6 +787,8 @@ static int apci3xxx_auto_attach(struct comedi_device *dev,

dev->iobase = pci_resource_start(pcidev, 2);
dev->mmio = pci_ioremap_bar(pcidev, 3);
+   if (!dev->mmio)
+   return -ENOMEM;

if (pcidev->irq > 0) {
ret = request_irq(pcidev->irq, apci3xxx_irq_handler,



Looks good. Thanks for the fix.

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] staging: comedi: labpc: fix isadma dependency

[Ian Abbott]

On 29/03/17 22:10, Arnd Bergmann wrote:

When COMEDI_NI_LABPC is built-in and COMEDI_NI_LABPC_ISA is a loadable
module, thhe ISA DMA code is not reachable by the common module, causing
a link error:

drivers/staging/built-in.o: In function `labpc_interrupt':
ni_labpc_common.c:(.text+0x1d178): undefined reference to 
`labpc_handle_dma_status'
ni_labpc_common.c:(.text+0x1d1cb): undefined reference to `labpc_drain_dma'
drivers/staging/built-in.o: In function `labpc_ai_cmd':
ni_labpc_common.c:(.text+0x1d8ad): undefined reference to `labpc_setup_dma'

This changes the definition of COMEDI_NI_LABPC_ISADMA so that it will
also be builtin for that case. This looks like a rather old bug, but
I have never seen this in randconfig testing until today.


That's because the comedi stuff has only recently been configurable as 
built-in.




Signed-off-by: Arnd Bergmann <a...@arndb.de>
---
 drivers/staging/comedi/Kconfig | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

v2: fix typo from first version, forgot to amend the patch after successfully
testing until I had sent it out.

diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig
index 6079c23237d5..7a655ed071a3 100644
--- a/drivers/staging/comedi/Kconfig
+++ b/drivers/staging/comedi/Kconfig
@@ -505,7 +505,6 @@ config COMEDI_NI_ATMIO16D
 config COMEDI_NI_LABPC_ISA
tristate "NI Lab-PC and compatibles ISA support"
select COMEDI_NI_LABPC
-   select COMEDI_NI_LABPC_ISADMA if ISA_DMA_API
---help---
  Enable support for National Instruments Lab-PC and compatibles
  Lab-PC-1200, Lab-PC-1200AI, Lab-PC+.
@@ -1315,6 +1314,9 @@ config COMEDI_NI_LABPC

 config COMEDI_NI_LABPC_ISADMA
tristate
+   default COMEDI_NI_LABPC
+   depends on COMEDI_NI_LABPC_ISA != n
+   depends on ISA_DMA_API
select COMEDI_ISADMA

 config COMEDI_NI_TIO



Thanks for the fix!

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] staging: comedi: labpc: fix isadma dependency

[Ian Abbott]

On 29/03/17 22:10, Arnd Bergmann wrote:

When COMEDI_NI_LABPC is built-in and COMEDI_NI_LABPC_ISA is a loadable
module, thhe ISA DMA code is not reachable by the common module, causing
a link error:

drivers/staging/built-in.o: In function `labpc_interrupt':
ni_labpc_common.c:(.text+0x1d178): undefined reference to 
`labpc_handle_dma_status'
ni_labpc_common.c:(.text+0x1d1cb): undefined reference to `labpc_drain_dma'
drivers/staging/built-in.o: In function `labpc_ai_cmd':
ni_labpc_common.c:(.text+0x1d8ad): undefined reference to `labpc_setup_dma'

This changes the definition of COMEDI_NI_LABPC_ISADMA so that it will
also be builtin for that case. This looks like a rather old bug, but
I have never seen this in randconfig testing until today.


That's because the comedi stuff has only recently been configurable as 
built-in.




Signed-off-by: Arnd Bergmann 
---
 drivers/staging/comedi/Kconfig | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

v2: fix typo from first version, forgot to amend the patch after successfully
testing until I had sent it out.

diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig
index 6079c23237d5..7a655ed071a3 100644
--- a/drivers/staging/comedi/Kconfig
+++ b/drivers/staging/comedi/Kconfig
@@ -505,7 +505,6 @@ config COMEDI_NI_ATMIO16D
 config COMEDI_NI_LABPC_ISA
tristate "NI Lab-PC and compatibles ISA support"
select COMEDI_NI_LABPC
-   select COMEDI_NI_LABPC_ISADMA if ISA_DMA_API
---help---
  Enable support for National Instruments Lab-PC and compatibles
  Lab-PC-1200, Lab-PC-1200AI, Lab-PC+.
@@ -1315,6 +1314,9 @@ config COMEDI_NI_LABPC

 config COMEDI_NI_LABPC_ISADMA
tristate
+   default COMEDI_NI_LABPC
+   depends on COMEDI_NI_LABPC_ISA != n
+   depends on ISA_DMA_API
select COMEDI_ISADMA

 config COMEDI_NI_TIO



Thanks for the fix!

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Ian Abbott]

On 07/03/17 18:13, Cheah Kok Cheong wrote:

If comedi module is loaded with the following max allowed parameter
[comedi_num_legacy_minors=48], subsequent loading of an auto-configured
device will fail at auto-configuration. If there's no fall back in
place then module loading will fail.

In this case, a default to auto-configure comedi_test module failed
to auto-configure with the following messages. It loaded but fell back
to unconfigured mode.

comedi_test comedi_testd: ran out of minor numbers for board device files
comedi_test comedi_testd: driver 'comedi_test' could not create device.
comedi_test: unable to auto-configure device

This is due to changes in commit 38b9722a4414
("staging: comedi: avoid releasing legacy minors automatically") which
will not allocate a minor number when comedi_num_legacy_minors equals
COMEDI_NUM_BOARD_MINORS. COMEDI_NUM_BOARD_MINORS is defined to be
0x30 which is 48.


Sorry, I don't consider this to be a bug.  The number of minor device 
numbers available for auto-configured devices is 48 minus 
comedi_num_legacy_minors.  Using up all available minor device numbers 
is a useful test case for running out of minor device numbers, although 
this relies on knowing the limit is 48.  Perhaps the description of the 
module parameter could be improved to mention the limits, as could the 
error message when running out of minor device numbers.


Commit 38b9722a4414 is irrelevant here.  Prior to that commit, the first 
'comedi_num_legacy_minors' minor numbers were still allocated to legacy 
devices created during module initialization, leaving 48 minus 
comedi_num_legacy_minors minors (possibly none) available for 
auto-configured devices.



This goes for a simple fix which limit comedi_num_legacy_minors to 47
instead of tinkering with comedi_alloc_board_minor() and
comedi_release_hardware_device().

Fix: commit 38b9722a4414 ("staging: comedi: avoid releasing legacy minors
automatically")

Signed-off-by: Cheah Kok Cheong <thrus...@gmail.com>
---

V2:
-Amend commit log to specify that comedi_test module failed
 to auto-configure and fell back to unconfigured mode.
 For other devices with no fall back, module loading will fail.


--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Ian Abbott]

On 07/03/17 18:13, Cheah Kok Cheong wrote:

If comedi module is loaded with the following max allowed parameter
[comedi_num_legacy_minors=48], subsequent loading of an auto-configured
device will fail at auto-configuration. If there's no fall back in
place then module loading will fail.

In this case, a default to auto-configure comedi_test module failed
to auto-configure with the following messages. It loaded but fell back
to unconfigured mode.

comedi_test comedi_testd: ran out of minor numbers for board device files
comedi_test comedi_testd: driver 'comedi_test' could not create device.
comedi_test: unable to auto-configure device

This is due to changes in commit 38b9722a4414
("staging: comedi: avoid releasing legacy minors automatically") which
will not allocate a minor number when comedi_num_legacy_minors equals
COMEDI_NUM_BOARD_MINORS. COMEDI_NUM_BOARD_MINORS is defined to be
0x30 which is 48.


Sorry, I don't consider this to be a bug.  The number of minor device 
numbers available for auto-configured devices is 48 minus 
comedi_num_legacy_minors.  Using up all available minor device numbers 
is a useful test case for running out of minor device numbers, although 
this relies on knowing the limit is 48.  Perhaps the description of the 
module parameter could be improved to mention the limits, as could the 
error message when running out of minor device numbers.


Commit 38b9722a4414 is irrelevant here.  Prior to that commit, the first 
'comedi_num_legacy_minors' minor numbers were still allocated to legacy 
devices created during module initialization, leaving 48 minus 
comedi_num_legacy_minors minors (possibly none) available for 
auto-configured devices.



This goes for a simple fix which limit comedi_num_legacy_minors to 47
instead of tinkering with comedi_alloc_board_minor() and
comedi_release_hardware_device().

Fix: commit 38b9722a4414 ("staging: comedi: avoid releasing legacy minors
automatically")

Signed-off-by: Cheah Kok Cheong 
---

V2:
-Amend commit log to specify that comedi_test module failed
 to auto-configure and fell back to unconfigured mode.
 For other devices with no fall back, module loading will fail.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 6/6] staging: comedi: Using macro DIV_ROUND_UP

[Ian Abbott]

On 21/02/17 18:28, simran singhal wrote:

The macro DIV_ROUND_UP performs the computation (((n) + (d) - 1) /(d)).
It clarifies the divisor calculations. This occurence was detected using
the coccinelle script:

@@
expression e1;
expression e2;
@@
(
- ((e1) + e2 - 1) / (e2)
+ DIV_ROUND_UP(e1,e2)
|
- ((e1) + (e2 - 1)) / (e2)
+ DIV_ROUND_UP(e1,e2)
)

Signed-off-by: simran singhal <singhalsimr...@gmail.com>
---
 drivers/staging/comedi/drivers/addi_apci_3xxx.c | 2 +-
 drivers/staging/comedi/drivers/cb_pcidas64.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi_apci_3xxx.c 
b/drivers/staging/comedi/drivers/addi_apci_3xxx.c
index b6af3eb..82c2211 100644
--- a/drivers/staging/comedi/drivers/addi_apci_3xxx.c
+++ b/drivers/staging/comedi/drivers/addi_apci_3xxx.c
@@ -502,7 +502,7 @@ static int apci3xxx_ai_ns_to_timer(struct comedi_device 
*dev,
timer = *ns / base;
break;
case CMDF_ROUND_UP:
-   timer = (*ns + base - 1) / base;
+   timer = DIV_ROUND_UP(*ns, base);
break;
}

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index efbf277..3b98193 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -2007,7 +2007,7 @@ static unsigned int get_divisor(unsigned int ns, unsigned 
int flags)

switch (flags & CMDF_ROUND_MASK) {
case CMDF_ROUND_UP:
-   divisor = (ns + TIMER_BASE - 1) / TIMER_BASE;
+   divisor = DIV_ROUND_UP(ns, TIMER_BASE);
break;
case CMDF_ROUND_DOWN:
divisor = ns / TIMER_BASE;



Thanks.  Ideally, this should be split into two patches, one for each 
driver module, but I guess we can live with a single patch.  (I don't 
know about the other 5 patches in this series.)


Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 6/6] staging: comedi: Using macro DIV_ROUND_UP

[Ian Abbott]

On 21/02/17 18:28, simran singhal wrote:

The macro DIV_ROUND_UP performs the computation (((n) + (d) - 1) /(d)).
It clarifies the divisor calculations. This occurence was detected using
the coccinelle script:

@@
expression e1;
expression e2;
@@
(
- ((e1) + e2 - 1) / (e2)
+ DIV_ROUND_UP(e1,e2)
|
- ((e1) + (e2 - 1)) / (e2)
+ DIV_ROUND_UP(e1,e2)
)

Signed-off-by: simran singhal 
---
 drivers/staging/comedi/drivers/addi_apci_3xxx.c | 2 +-
 drivers/staging/comedi/drivers/cb_pcidas64.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi_apci_3xxx.c 
b/drivers/staging/comedi/drivers/addi_apci_3xxx.c
index b6af3eb..82c2211 100644
--- a/drivers/staging/comedi/drivers/addi_apci_3xxx.c
+++ b/drivers/staging/comedi/drivers/addi_apci_3xxx.c
@@ -502,7 +502,7 @@ static int apci3xxx_ai_ns_to_timer(struct comedi_device 
*dev,
timer = *ns / base;
break;
case CMDF_ROUND_UP:
-   timer = (*ns + base - 1) / base;
+   timer = DIV_ROUND_UP(*ns, base);
break;
}

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index efbf277..3b98193 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -2007,7 +2007,7 @@ static unsigned int get_divisor(unsigned int ns, unsigned 
int flags)

switch (flags & CMDF_ROUND_MASK) {
case CMDF_ROUND_UP:
-   divisor = (ns + TIMER_BASE - 1) / TIMER_BASE;
+   divisor = DIV_ROUND_UP(ns, TIMER_BASE);
break;
case CMDF_ROUND_DOWN:
divisor = ns / TIMER_BASE;



Thanks.  Ideally, this should be split into two patches, one for each 
driver module, but I guess we can live with a single patch.  (I don't 
know about the other 5 patches in this series.)


Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[Ian Abbott]

On 07/03/17 18:13, Cheah Kok Cheong wrote:

Change to unsigned to allow removal of negative value check in
init section. Use smaller data type since the max possible
value currently is 48.

Signed-off-by: Cheah Kok Cheong <thrus...@gmail.com>
---

V2:
-No changes.

 drivers/staging/comedi/comedi_fops.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 57e8599..354d264 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -76,8 +76,8 @@ struct comedi_file {
 #define COMEDI_NUM_SUBDEVICE_MINORS\
(COMEDI_NUM_MINORS - COMEDI_NUM_BOARD_MINORS)

-static int comedi_num_legacy_minors;
-module_param(comedi_num_legacy_minors, int, 0444);
+static unsigned short comedi_num_legacy_minors;
+module_param(comedi_num_legacy_minors, ushort, 0444);
 MODULE_PARM_DESC(comedi_num_legacy_minors,
 "number of comedi minor devices to reserve for non-auto-configured 
devices (default 0)"
);
@@ -2857,8 +2857,7 @@ static int __init comedi_init(void)

pr_info("version " COMEDI_RELEASE " - http://www.comedi.org\n;);

-   if (comedi_num_legacy_minors < 0 ||
-   comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
+   if (comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
pr_err("invalid value for module parameter 
\"comedi_num_legacy_minors\".  Valid values are 0 through %i.\n",
   COMEDI_NUM_BOARD_MINORS);
return -EINVAL;



Thanks.  (There is no harm in making the parameter unsigned short rather 
than unsigned int, although it's probably not worth it as you still need 
to check the value.  It doesn't matter either way.)


Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[Ian Abbott]

On 07/03/17 18:13, Cheah Kok Cheong wrote:

Change to unsigned to allow removal of negative value check in
init section. Use smaller data type since the max possible
value currently is 48.

Signed-off-by: Cheah Kok Cheong 
---

V2:
-No changes.

 drivers/staging/comedi/comedi_fops.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 57e8599..354d264 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -76,8 +76,8 @@ struct comedi_file {
 #define COMEDI_NUM_SUBDEVICE_MINORS\
(COMEDI_NUM_MINORS - COMEDI_NUM_BOARD_MINORS)

-static int comedi_num_legacy_minors;
-module_param(comedi_num_legacy_minors, int, 0444);
+static unsigned short comedi_num_legacy_minors;
+module_param(comedi_num_legacy_minors, ushort, 0444);
 MODULE_PARM_DESC(comedi_num_legacy_minors,
 "number of comedi minor devices to reserve for non-auto-configured 
devices (default 0)"
);
@@ -2857,8 +2857,7 @@ static int __init comedi_init(void)

pr_info("version " COMEDI_RELEASE " - http://www.comedi.org\n;);

-   if (comedi_num_legacy_minors < 0 ||
-   comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
+   if (comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
pr_err("invalid value for module parameter 
\"comedi_num_legacy_minors\".  Valid values are 0 through %i.\n",
   COMEDI_NUM_BOARD_MINORS);
return -EINVAL;



Thanks.  (There is no harm in making the parameter unsigned short rather 
than unsigned int, although it's probably not worth it as you still need 
to check the value.  It doesn't matter either way.)


Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Ian Abbott]

On 08/03/17 10:08, Cheah Kok Cheong wrote:

Dear Dan,
  Thanks for reviewing this patch.

On Wed, Mar 08, 2017 at 08:54:47AM +0300, Dan Carpenter wrote:

On Sun, Mar 05, 2017 at 03:22:33AM +0800, Cheah Kok Cheong wrote:

If comedi module is loaded with the following max allowed parameter
[comedi_num_legacy_minors=48], subsequent loading of an auto-configured
device will fail.


Don't set comedi_num_legacy_minors=48, then?

This doesn't seem like the right fix at all.  Why only allow one auto
configured board?  Why not 5 or 10?



Let me explain, the original intended behaviour is to allow user to
reserve up to 48 minor numbers for legacy devices.

Therefore [sudo modprobe comedi comedi_num_legacy_minors=3]
will allocate minor number 0, 1, 2 for legacy devices.
Subsequent loading of an auto-configured device will use minor number 3.
And the next one number 4 so on and so forth.

Now for the corner case of [comedi_num_legacy_minors=48] which
is supposed to reserve minor number 0 till 47 for legacy devices,
and is supposed to allocate number 48 and so on for auto-configured
devices, does not allocate number 48 anymore after commit
38b9722a4414.


Both legacy and auto-configured devices will have minor numbers less 
than 48, which is the total number of devices currently supported by 
Comedi.  Minor numbers 48 and above have been reserved for dynamic 
allocation to those Comedi subdevices that support asynchronous commands.



This is due to the changes in comedi_alloc_board_minor().

As to why I chose to limit [comedi_num_legacy_minors=47], is given
in the commit log.

This will allow user to allocate 0 till 46 for legacy devices and
subsequent auto-configured devices will start from 47 and so forth.

I don't think anybody will miss one less number for legacy devices
otherwise there'll be complains earlier on.


As Dan implies above, if you want auto-configured devices to work, set 
comedi_num_legacy_minors to a value less than 48.  Further limiting 
comedi_num_legacy_minors to 47 seems a bit arbitrary.


The comedi_test module mentioned in your commit can still be used when 
comedi_num_legacy_minors=48 by configuring a "legacy" comedi_test 
device.  Most of the other Comedi drivers support "legacy" devices 
exclusive-or auto-configured devices.



Thanks.

Brgds,
CheahKC


regards,
dan carpenter




--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Ian Abbott]

On 08/03/17 10:08, Cheah Kok Cheong wrote:

Dear Dan,
  Thanks for reviewing this patch.

On Wed, Mar 08, 2017 at 08:54:47AM +0300, Dan Carpenter wrote:

On Sun, Mar 05, 2017 at 03:22:33AM +0800, Cheah Kok Cheong wrote:

If comedi module is loaded with the following max allowed parameter
[comedi_num_legacy_minors=48], subsequent loading of an auto-configured
device will fail.


Don't set comedi_num_legacy_minors=48, then?

This doesn't seem like the right fix at all.  Why only allow one auto
configured board?  Why not 5 or 10?



Let me explain, the original intended behaviour is to allow user to
reserve up to 48 minor numbers for legacy devices.

Therefore [sudo modprobe comedi comedi_num_legacy_minors=3]
will allocate minor number 0, 1, 2 for legacy devices.
Subsequent loading of an auto-configured device will use minor number 3.
And the next one number 4 so on and so forth.

Now for the corner case of [comedi_num_legacy_minors=48] which
is supposed to reserve minor number 0 till 47 for legacy devices,
and is supposed to allocate number 48 and so on for auto-configured
devices, does not allocate number 48 anymore after commit
38b9722a4414.


Both legacy and auto-configured devices will have minor numbers less 
than 48, which is the total number of devices currently supported by 
Comedi.  Minor numbers 48 and above have been reserved for dynamic 
allocation to those Comedi subdevices that support asynchronous commands.



This is due to the changes in comedi_alloc_board_minor().

As to why I chose to limit [comedi_num_legacy_minors=47], is given
in the commit log.

This will allow user to allocate 0 till 46 for legacy devices and
subsequent auto-configured devices will start from 47 and so forth.

I don't think anybody will miss one less number for legacy devices
otherwise there'll be complains earlier on.


As Dan implies above, if you want auto-configured devices to work, set 
comedi_num_legacy_minors to a value less than 48.  Further limiting 
comedi_num_legacy_minors to 47 seems a bit arbitrary.


The comedi_test module mentioned in your commit can still be used when 
comedi_num_legacy_minors=48 by configuring a "legacy" comedi_test 
device.  Most of the other Comedi drivers support "legacy" devices 
exclusive-or auto-configured devices.



Thanks.

Brgds,
CheahKC


regards,
dan carpenter




--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v1 3/3] staging: comedi: Replace "is is" with "is"

[Ian Abbott]

On 04/03/17 17:41, simran singhal wrote:

This patch replace "is is " with "is". The replacement couldn't be
automated because sometimes the first "is" was meant to be another
word.

Signed-off-by: simran singhal <singhalsimr...@gmail.com>
Acked-by: Julia Lawall <julia.law...@lip6.fr>
---
 drivers/staging/comedi/drivers/ni_usb6501.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_usb6501.c 
b/drivers/staging/comedi/drivers/ni_usb6501.c
index 5036eeb..9a0a963 100644
--- a/drivers/staging/comedi/drivers/ni_usb6501.c
+++ b/drivers/staging/comedi/drivers/ni_usb6501.c
@@ -45,7 +45,7 @@
  * byte 3 is the total packet length
  *
  * byte 4 is always 00
- * byte 5 is is the total packet length - 4
+ * byte 5 is the total packet length - 4
  * byte 6 is always 01
  * byte 7 is the command
  *



Looks good.

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v1 3/3] staging: comedi: Replace "is is" with "is"

[Ian Abbott]

On 04/03/17 17:41, simran singhal wrote:

This patch replace "is is " with "is". The replacement couldn't be
automated because sometimes the first "is" was meant to be another
word.

Signed-off-by: simran singhal 
Acked-by: Julia Lawall 
---
 drivers/staging/comedi/drivers/ni_usb6501.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_usb6501.c 
b/drivers/staging/comedi/drivers/ni_usb6501.c
index 5036eeb..9a0a963 100644
--- a/drivers/staging/comedi/drivers/ni_usb6501.c
+++ b/drivers/staging/comedi/drivers/ni_usb6501.c
@@ -45,7 +45,7 @@
  * byte 3 is the total packet length
  *
  * byte 4 is always 00
- * byte 5 is is the total packet length - 4
+ * byte 5 is the total packet length - 4
  * byte 6 is always 01
  * byte 7 is the command
  *



Looks good.

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [Outreachy kernel] [PATCH 1/3] staging: rtl8192u: Replace "is is" with "is"

[Ian Abbott]

On 04/03/17 16:18, Julia Lawall wrote:



On Sat, 4 Mar 2017, simran singhal wrote:


This patch replace "is is " with "is". The replacement couldn't be
automated because sometimes the first "is" was meant to be another
word.

Signed-off-by: simran singhal <singhalsimr...@gmail.com>
---
 drivers/staging/rtl8192u/r819xU_cmdpkt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8192u/r819xU_cmdpkt.c 
b/drivers/staging/rtl8192u/r819xU_cmdpkt.c
index 3e0731b..231be8f 100644
--- a/drivers/staging/rtl8192u/r819xU_cmdpkt.c
+++ b/drivers/staging/rtl8192u/r819xU_cmdpkt.c
@@ -495,7 +495,7 @@ u32 cmpk_message_handle_rx(struct net_device *dev,
u8  element_id;
u8  *pcmd_buff;

-   /* 0. Check inpt arguments. If is is a command queue message or
+   /* 0. Check inpt arguments. If is a command queue message or


I can't figure out how to parse "If is a command...".


I think it should be "If it is a command...".

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [Outreachy kernel] [PATCH 1/3] staging: rtl8192u: Replace "is is" with "is"

[Ian Abbott]

On 04/03/17 16:18, Julia Lawall wrote:



On Sat, 4 Mar 2017, simran singhal wrote:


This patch replace "is is " with "is". The replacement couldn't be
automated because sometimes the first "is" was meant to be another
word.

Signed-off-by: simran singhal 
---
 drivers/staging/rtl8192u/r819xU_cmdpkt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8192u/r819xU_cmdpkt.c 
b/drivers/staging/rtl8192u/r819xU_cmdpkt.c
index 3e0731b..231be8f 100644
--- a/drivers/staging/rtl8192u/r819xU_cmdpkt.c
+++ b/drivers/staging/rtl8192u/r819xU_cmdpkt.c
@@ -495,7 +495,7 @@ u32 cmpk_message_handle_rx(struct net_device *dev,
u8  element_id;
u8  *pcmd_buff;

-   /* 0. Check inpt arguments. If is is a command queue message or
+   /* 0. Check inpt arguments. If is a command queue message or


I can't figure out how to parse "If is a command...".


I think it should be "If it is a command...".

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Ian Abbott]

On 21/02/17 17:25, Cheah Kok Cheong wrote:

Fix checkpatch warning "Avoid multiple line dereference"
using a pointer variable to avoid line wrap.

Signed-off-by: Cheah Kok Cheong <thrus...@gmail.com>
---

V2:
-Use pointer instead of normal variable - Ian
-Variable is to be used as "write destination" and
 not as "read source" - Ian

 drivers/staging/comedi/drivers/comedi_test.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index 2a063f0..ccfd642 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -480,11 +480,11 @@ static void waveform_ao_timer(unsigned long arg)
/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
+   unsigned short *pd;

-   if (comedi_buf_read_samples(s,
-   >
-ao_loopbacks[chan],
-   1) == 0) {
+   pd = >ao_loopbacks[chan];
+
+   if (!comedi_buf_read_samples(s, pd, 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
    goto underrun;



Looks good, thanks!

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Ian Abbott]

On 21/02/17 17:25, Cheah Kok Cheong wrote:

Fix checkpatch warning "Avoid multiple line dereference"
using a pointer variable to avoid line wrap.

Signed-off-by: Cheah Kok Cheong 
---

V2:
-Use pointer instead of normal variable - Ian
-Variable is to be used as "write destination" and
 not as "read source" - Ian

 drivers/staging/comedi/drivers/comedi_test.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index 2a063f0..ccfd642 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -480,11 +480,11 @@ static void waveform_ao_timer(unsigned long arg)
/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
+   unsigned short *pd;

-   if (comedi_buf_read_samples(s,
-   >
-ao_loopbacks[chan],
-   1) == 0) {
+   pd = >ao_loopbacks[chan];
+
+   if (!comedi_buf_read_samples(s, pd, 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;



Looks good, thanks!

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Ian Abbott]

On 21/02/2017 09:33, Cheah Kok Cheong wrote:

On Mon, Feb 20, 2017 at 05:36:52PM +, Ian Abbott wrote:

On 20/02/17 16:02, Cheah Kok Cheong wrote:

On Mon, Feb 20, 2017 at 10:03:39AM +, Ian Abbott wrote:

On 20/02/17 08:28, Cheah Kok Cheong wrote:

Fix checkpatch warning "Avoid multiple line dereference"
using a local variable to avoid line wrap.

Signed-off-by: Cheah Kok Cheong <thrus...@gmail.com>
---
drivers/staging/comedi/drivers/comedi_test.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index 2a063f0..fde83e0 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg)
/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
+   unsigned short d = devpriv->ao_loopbacks[chan];

-   if (comedi_buf_read_samples(s,
-   >
-ao_loopbacks[chan],
-   1) == 0) {
+   if (!comedi_buf_read_samples(s, , 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;



NAK.  This leaves devpriv->ao_loopbacks[chan] unchanged.



Thanks for pointing this out. In that case will assigning the variable to
devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet.

Otherwise I'll just drop the variable and adjust the lines to avoid
checkpatch warning.

Sorry for the inconvenience caused.

[ Snip ]

/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
unsigned short data;

if (!comedi_buf_read_samples(s, , 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
}

devpriv->ao_loopbacks[chan] = data;
}
/* advance time of last scan */

[ Snip ]


It will work, but you could just use a pointer variable set to
>ao_loopbacks[chan] and pass that to comedi_buf_read_samples().



Thanks for the suggestion. I tried below snippet 1 with the shortest pointer
name but 80 characters is exceeded. The declaration and initialisation
will have to be splitted. Will this be acceptable or am I doing it wrong
again?

Sorry for the trouble.

Snippet 1:
[ Snip ]

/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
unsigned short *p = 
>ao_loopbacks[chan];

if (!comedi_buf_read_samples(s, p, 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
}
}
/* advance time of last scan */

[ Snip ]

Snippet 2:
[ Snip ]

/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
unsigned short *pd;

pd = >ao_loopbacks[chan];

if (!comedi_buf_read_samples(s, pd, 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
}
}

[ Snip ]


Snippet 2 looks fine.  Alternatives are to modify Snippet 1 to split the 
initialization of the pointer variable after the '=', or to shorten the 
the name of the 'chan' variable.


--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Ian Abbott]

On 21/02/2017 09:33, Cheah Kok Cheong wrote:

On Mon, Feb 20, 2017 at 05:36:52PM +, Ian Abbott wrote:

On 20/02/17 16:02, Cheah Kok Cheong wrote:

On Mon, Feb 20, 2017 at 10:03:39AM +, Ian Abbott wrote:

On 20/02/17 08:28, Cheah Kok Cheong wrote:

Fix checkpatch warning "Avoid multiple line dereference"
using a local variable to avoid line wrap.

Signed-off-by: Cheah Kok Cheong 
---
drivers/staging/comedi/drivers/comedi_test.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index 2a063f0..fde83e0 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg)
/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
+   unsigned short d = devpriv->ao_loopbacks[chan];

-   if (comedi_buf_read_samples(s,
-   >
-ao_loopbacks[chan],
-   1) == 0) {
+   if (!comedi_buf_read_samples(s, , 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;



NAK.  This leaves devpriv->ao_loopbacks[chan] unchanged.



Thanks for pointing this out. In that case will assigning the variable to
devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet.

Otherwise I'll just drop the variable and adjust the lines to avoid
checkpatch warning.

Sorry for the inconvenience caused.

[ Snip ]

/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
unsigned short data;

if (!comedi_buf_read_samples(s, , 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
}

devpriv->ao_loopbacks[chan] = data;
}
/* advance time of last scan */

[ Snip ]


It will work, but you could just use a pointer variable set to
>ao_loopbacks[chan] and pass that to comedi_buf_read_samples().



Thanks for the suggestion. I tried below snippet 1 with the shortest pointer
name but 80 characters is exceeded. The declaration and initialisation
will have to be splitted. Will this be acceptable or am I doing it wrong
again?

Sorry for the trouble.

Snippet 1:
[ Snip ]

/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
unsigned short *p = 
>ao_loopbacks[chan];

if (!comedi_buf_read_samples(s, p, 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
}
}
/* advance time of last scan */

[ Snip ]

Snippet 2:
[ Snip ]

/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
unsigned short *pd;

pd = >ao_loopbacks[chan];

if (!comedi_buf_read_samples(s, pd, 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
}
}

[ Snip ]


Snippet 2 looks fine.  Alternatives are to modify Snippet 1 to split the 
initialization of the pointer variable after the '=', or to shorten the 
the name of the 'chan' variable.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Ian Abbott]

On 20/02/17 16:02, Cheah Kok Cheong wrote:

On Mon, Feb 20, 2017 at 10:03:39AM +, Ian Abbott wrote:

On 20/02/17 08:28, Cheah Kok Cheong wrote:

Fix checkpatch warning "Avoid multiple line dereference"
using a local variable to avoid line wrap.

Signed-off-by: Cheah Kok Cheong <thrus...@gmail.com>
---
drivers/staging/comedi/drivers/comedi_test.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index 2a063f0..fde83e0 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg)
/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
+   unsigned short d = devpriv->ao_loopbacks[chan];

-   if (comedi_buf_read_samples(s,
-   >
-ao_loopbacks[chan],
-   1) == 0) {
+   if (!comedi_buf_read_samples(s, , 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;



NAK.  This leaves devpriv->ao_loopbacks[chan] unchanged.



Thanks for pointing this out. In that case will assigning the variable to
devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet.

Otherwise I'll just drop the variable and adjust the lines to avoid
checkpatch warning.

Sorry for the inconvenience caused.

[ Snip ]

/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
unsigned short data;

if (!comedi_buf_read_samples(s, , 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
}

devpriv->ao_loopbacks[chan] = data;
}
/* advance time of last scan */

[ Snip ]


It will work, but you could just use a pointer variable set to 
>ao_loopbacks[chan] and pass that to comedi_buf_read_samples().


--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Ian Abbott]

On 20/02/17 16:02, Cheah Kok Cheong wrote:

On Mon, Feb 20, 2017 at 10:03:39AM +, Ian Abbott wrote:

On 20/02/17 08:28, Cheah Kok Cheong wrote:

Fix checkpatch warning "Avoid multiple line dereference"
using a local variable to avoid line wrap.

Signed-off-by: Cheah Kok Cheong 
---
drivers/staging/comedi/drivers/comedi_test.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index 2a063f0..fde83e0 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg)
/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
+   unsigned short d = devpriv->ao_loopbacks[chan];

-   if (comedi_buf_read_samples(s,
-   >
-ao_loopbacks[chan],
-   1) == 0) {
+   if (!comedi_buf_read_samples(s, , 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;



NAK.  This leaves devpriv->ao_loopbacks[chan] unchanged.



Thanks for pointing this out. In that case will assigning the variable to
devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet.

Otherwise I'll just drop the variable and adjust the lines to avoid
checkpatch warning.

Sorry for the inconvenience caused.

[ Snip ]

/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
unsigned short data;

if (!comedi_buf_read_samples(s, , 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
}

devpriv->ao_loopbacks[chan] = data;
}
/* advance time of last scan */

[ Snip ]


It will work, but you could just use a pointer variable set to 
>ao_loopbacks[chan] and pass that to comedi_buf_read_samples().


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Ian Abbott]

On 20/02/17 08:28, Cheah Kok Cheong wrote:

Fix checkpatch warning "Avoid multiple line dereference"
using a local variable to avoid line wrap.

Signed-off-by: Cheah Kok Cheong <thrus...@gmail.com>
---
 drivers/staging/comedi/drivers/comedi_test.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index 2a063f0..fde83e0 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg)
/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
+   unsigned short d = devpriv->ao_loopbacks[chan];

-   if (comedi_buf_read_samples(s,
-   >
-ao_loopbacks[chan],
-   1) == 0) {
+   if (!comedi_buf_read_samples(s, , 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;



NAK.  This leaves devpriv->ao_loopbacks[chan] unchanged.

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Ian Abbott]

On 20/02/17 08:28, Cheah Kok Cheong wrote:

Fix checkpatch warning "Avoid multiple line dereference"
using a local variable to avoid line wrap.

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/drivers/comedi_test.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index 2a063f0..fde83e0 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg)
/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
+   unsigned short d = devpriv->ao_loopbacks[chan];

-   if (comedi_buf_read_samples(s,
-   >
-ao_loopbacks[chan],
-   1) == 0) {
+   if (!comedi_buf_read_samples(s, , 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;



NAK.  This leaves devpriv->ao_loopbacks[chan] unchanged.

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v3] Staging: comedi: drivers: comedi_test: Add auto-configuration capability

[Ian Abbott]

On 16/02/17 14:05, Cheah Kok Cheong wrote:

Currently this module needs to be manually configured by COMEDI
userspace tool before the test waveform can be read by a COMEDI
compatible application.

This patch adds auto-configuration capability and makes it the default
loading option. This is achieved by creating a device during init
to stand in for a real hardware device. This allows comedi_auto_config()
to perform auto-configuration. With this patch, the test waveform can
be read by a COMEDI compatible application without needing manual
configuration.

Previous behaviour is still selectable via module loading parameter.
Module loading without passing any parameter will default to
auto-configuration with the same default waveform amplitude and
period values. For auto-configuration, different amplitude and
period values can be set via module loading parameters.

Tested on Xubuntu 16.04 using Xoscope ver: 2.0 which is available
in the Ubuntu repository. Xoscope is a COMEDI compatible digital
oscilloscope application. For manual configuration, only module
loading/unloading is tested.

Here are the truncated dmesg output.
[sudo modprobe comedi_test]

comedi_test: 100 microvolt, 10 microsecond waveform attached
driver 'comedi_test' has successfully auto-configured 'comedi_test'.

[sudo modprobe comedi_test amplitude=250 period=15]

comedi_test: 250 microvolt, 15 microsecond waveform attached
driver 'comedi_test' has successfully auto-configured 'comedi_test'.

[sudo modprobe comedi_test noauto=1]

comedi_test: module is from the staging directory, the quality is unknown,
you have been warned.

For those without an actual hardware, the comedi_test module
is as close as one can get to test the COMEDI system.
Having both auto and manual configuration capability will broaden
the test function of this module.
Hopefully this will make it easier for people to check out the
COMEDI system and contribute to its development.

Signed-off-by: Cheah Kok Cheong <thrus...@gmail.com>
---

V3:
-Ensure struct class and struct device pointers are "NULL"
 if auto-configuration fails - Ian

V2:
-Rename module param - Ian
-Rename class - Ian
-Tidy up init error handling - Ian
-Allow module loading to continue when auto-configuration fails - Ian
-Remove redundant "if" statement from module exit
-Edit driver intro to reflect changes

 drivers/staging/comedi/drivers/comedi_test.c | 135 ---
 1 file changed, 123 insertions(+), 12 deletions(-)



It all seems to be in order now.  Thanks!

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v3] Staging: comedi: drivers: comedi_test: Add auto-configuration capability

[Ian Abbott]

On 16/02/17 14:05, Cheah Kok Cheong wrote:

Currently this module needs to be manually configured by COMEDI
userspace tool before the test waveform can be read by a COMEDI
compatible application.

This patch adds auto-configuration capability and makes it the default
loading option. This is achieved by creating a device during init
to stand in for a real hardware device. This allows comedi_auto_config()
to perform auto-configuration. With this patch, the test waveform can
be read by a COMEDI compatible application without needing manual
configuration.

Previous behaviour is still selectable via module loading parameter.
Module loading without passing any parameter will default to
auto-configuration with the same default waveform amplitude and
period values. For auto-configuration, different amplitude and
period values can be set via module loading parameters.

Tested on Xubuntu 16.04 using Xoscope ver: 2.0 which is available
in the Ubuntu repository. Xoscope is a COMEDI compatible digital
oscilloscope application. For manual configuration, only module
loading/unloading is tested.

Here are the truncated dmesg output.
[sudo modprobe comedi_test]

comedi_test: 100 microvolt, 10 microsecond waveform attached
driver 'comedi_test' has successfully auto-configured 'comedi_test'.

[sudo modprobe comedi_test amplitude=250 period=15]

comedi_test: 250 microvolt, 15 microsecond waveform attached
driver 'comedi_test' has successfully auto-configured 'comedi_test'.

[sudo modprobe comedi_test noauto=1]

comedi_test: module is from the staging directory, the quality is unknown,
you have been warned.

For those without an actual hardware, the comedi_test module
is as close as one can get to test the COMEDI system.
Having both auto and manual configuration capability will broaden
the test function of this module.
Hopefully this will make it easier for people to check out the
COMEDI system and contribute to its development.

Signed-off-by: Cheah Kok Cheong 
---

V3:
-Ensure struct class and struct device pointers are "NULL"
 if auto-configuration fails - Ian

V2:
-Rename module param - Ian
-Rename class - Ian
-Tidy up init error handling - Ian
-Allow module loading to continue when auto-configuration fails - Ian
-Remove redundant "if" statement from module exit
-Edit driver intro to reflect changes

 drivers/staging/comedi/drivers/comedi_test.c | 135 ---
 1 file changed, 123 insertions(+), 12 deletions(-)



It all seems to be in order now.  Thanks!

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] Staging: comedi: drivers: comedi_test: Add auto-configuration capability

[Ian Abbott]

On 15/02/17 06:05, Cheah Kok Cheong wrote:

On Mon, Feb 13, 2017 at 11:14:14AM +, Ian Abbott wrote:

On 11/02/17 10:37, Cheah Kok Cheong wrote:

[snip]

+static void __exit comedi_test_exit(void)
+{
+   comedi_auto_unconfig(ctdev);
+
+   device_destroy(ctcls, MKDEV(0, 0));
+
+   class_destroy(ctcls);


If the driver init returned successfully, but failed to set-up the
auto-configured device, the device and class will not exist at this point,
so those three calls need to go in an 'if' statement.  Perhaps you could use
'if (ctcls) {', and set 'ctcls = NULL;' after calling
'class_destroy(ctcls);' in the init function.

Apart from that, it looks fine.



Thanks for pointing out those two pointers have to be "NULL" if
auto-configuration fails.

Please review below snippet.
Sorry for the inconvenience caused.

[ Snip ]

static int __init comedi_test_init(void)
{
int ret;

ret = comedi_driver_register(_driver);
if (ret) {
pr_err("comedi_test: unable to register driver\n");
return ret;
}

if (!config_mode) {
ctcls = class_create(THIS_MODULE, CLASS_NAME);
if (IS_ERR(ctcls)) {
pr_warn("comedi_test: unable to create class\n");
goto clean3;
}

ctdev = device_create(ctcls, NULL, MKDEV(0, 0), NULL, DEV_NAME);
if (IS_ERR(ctdev)) {
pr_warn("comedi_test: unable to create device\n");
goto clean2;
}

ret = comedi_auto_config(ctdev, _driver, 0);
if (ret) {
pr_warn("comedi_test: unable to auto-configure 
device\n");
goto clean;
}
}

return 0;

clean:
device_destroy(ctcls, MKDEV(0, 0));
clean2:
class_destroy(ctcls);
ctdev = NULL;
clean3:
ctcls = NULL;

return 0;
}
module_init(comedi_test_init);

static void __exit comedi_test_exit(void)
{
if (ctdev)
comedi_auto_unconfig(ctdev);

if (ctcls) {
device_destroy(ctcls, MKDEV(0, 0));
class_destroy(ctcls);
}

comedi_driver_unregister(_driver);
}
module_exit(comedi_test_exit);

[ Snip ]

Thks.
Brgds,
CheahKC



Yes, that looks fine.

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] Staging: comedi: drivers: comedi_test: Add auto-configuration capability

[Ian Abbott]

On 15/02/17 06:05, Cheah Kok Cheong wrote:

On Mon, Feb 13, 2017 at 11:14:14AM +, Ian Abbott wrote:

On 11/02/17 10:37, Cheah Kok Cheong wrote:

[snip]

+static void __exit comedi_test_exit(void)
+{
+   comedi_auto_unconfig(ctdev);
+
+   device_destroy(ctcls, MKDEV(0, 0));
+
+   class_destroy(ctcls);


If the driver init returned successfully, but failed to set-up the
auto-configured device, the device and class will not exist at this point,
so those three calls need to go in an 'if' statement.  Perhaps you could use
'if (ctcls) {', and set 'ctcls = NULL;' after calling
'class_destroy(ctcls);' in the init function.

Apart from that, it looks fine.



Thanks for pointing out those two pointers have to be "NULL" if
auto-configuration fails.

Please review below snippet.
Sorry for the inconvenience caused.

[ Snip ]

static int __init comedi_test_init(void)
{
int ret;

ret = comedi_driver_register(_driver);
if (ret) {
pr_err("comedi_test: unable to register driver\n");
return ret;
}

if (!config_mode) {
ctcls = class_create(THIS_MODULE, CLASS_NAME);
if (IS_ERR(ctcls)) {
pr_warn("comedi_test: unable to create class\n");
goto clean3;
}

ctdev = device_create(ctcls, NULL, MKDEV(0, 0), NULL, DEV_NAME);
if (IS_ERR(ctdev)) {
pr_warn("comedi_test: unable to create device\n");
goto clean2;
}

ret = comedi_auto_config(ctdev, _driver, 0);
if (ret) {
pr_warn("comedi_test: unable to auto-configure 
device\n");
goto clean;
}
}

return 0;

clean:
device_destroy(ctcls, MKDEV(0, 0));
clean2:
class_destroy(ctcls);
ctdev = NULL;
clean3:
ctcls = NULL;

return 0;
}
module_init(comedi_test_init);

static void __exit comedi_test_exit(void)
{
if (ctdev)
comedi_auto_unconfig(ctdev);

if (ctcls) {
device_destroy(ctcls, MKDEV(0, 0));
class_destroy(ctcls);
}

comedi_driver_unregister(_driver);
}
module_exit(comedi_test_exit);

[ Snip ]

Thks.
Brgds,
CheahKC



Yes, that looks fine.

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] Staging: comedi: drivers: comedi_test: Add auto-configuration capability

[Ian Abbott]

plitude and period */
-   if (amplitude <= 0)
-   amplitude = 100;/* 1 volt */
-   if (period <= 0)
-   period = 10;/* 0.1 sec */
-
devpriv->wf_amplitude = amplitude;
devpriv->wf_period = period;

@@ -678,6 +697,36 @@ static int waveform_attach(struct comedi_device *dev,
return 0;
 }

+static int waveform_attach(struct comedi_device *dev,
+  struct comedi_devconfig *it)
+{
+   int amplitude = it->options[0];
+   int period = it->options[1];
+
+   /* set default amplitude and period */
+   if (amplitude <= 0)
+   amplitude = 100;/* 1 volt */
+   if (period <= 0)
+   period = 10;/* 0.1 sec */
+
+   return waveform_common_attach(dev, amplitude, period);
+}
+
+static int waveform_auto_attach(struct comedi_device *dev,
+   unsigned long context_unused)
+{
+   int amplitude = set_amplitude;
+   int period = set_period;
+
+   /* set default amplitude and period */
+   if (!amplitude)
+   amplitude = 100;/* 1 volt */
+   if (!period)
+   period = 10;/* 0.1 sec */
+
+   return waveform_common_attach(dev, amplitude, period);
+}
+
 static void waveform_detach(struct comedi_device *dev)
 {
struct waveform_private *devpriv = dev->private;
@@ -692,9 +741,66 @@ static struct comedi_driver waveform_driver = {
.driver_name= "comedi_test",
.module = THIS_MODULE,
.attach = waveform_attach,
+   .auto_attach= waveform_auto_attach,
.detach = waveform_detach,
 };
-module_comedi_driver(waveform_driver);
+
+/*
+ * For auto-configuration, a device is created to stand in for a
+ * real hardware device.
+ */
+static int __init comedi_test_init(void)
+{
+   int ret;
+
+   ret = comedi_driver_register(_driver);
+   if (ret) {
+   pr_err("comedi_test: unable to register driver\n");
+   return ret;
+   }
+
+   if (!config_mode) {
+   ctcls = class_create(THIS_MODULE, CLASS_NAME);
+   if (IS_ERR(ctcls)) {
+   pr_warn("comedi_test: unable to create class\n");
+   return ret;
+   }
+
+   ctdev = device_create(ctcls, NULL, MKDEV(0, 0), NULL, DEV_NAME);
+   if (IS_ERR(ctdev)) {
+   pr_warn("comedi_test: unable to create device\n");
+   goto clean2;
+   }
+
+   ret = comedi_auto_config(ctdev, _driver, 0);
+   if (ret) {
+   pr_warn("comedi_test: unable to auto-configure 
device\n");
+   goto clean;
+   }
+   }
+
+   return 0;
+
+clean:
+   device_destroy(ctcls, MKDEV(0, 0));
+clean2:
+   class_destroy(ctcls);
+
+   return 0;
+}
+module_init(comedi_test_init);
+
+static void __exit comedi_test_exit(void)
+{
+   comedi_auto_unconfig(ctdev);
+
+   device_destroy(ctcls, MKDEV(0, 0));
+
+   class_destroy(ctcls);


If the driver init returned successfully, but failed to set-up the 
auto-configured device, the device and class will not exist at this 
point, so those three calls need to go in an 'if' statement.  Perhaps 
you could use 'if (ctcls) {', and set 'ctcls = NULL;' after calling 
'class_destroy(ctcls);' in the init function.


Apart from that, it looks fine.


+
+   comedi_driver_unregister(_driver);
+}
+module_exit(comedi_test_exit);

 MODULE_AUTHOR("Comedi http://www.comedi.org;);
 MODULE_DESCRIPTION("Comedi low-level driver");




--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH v2] Staging: comedi: drivers: comedi_test: Add auto-configuration capability

[Ian Abbott]

/
-   if (amplitude <= 0)
-   amplitude = 100;/* 1 volt */
-   if (period <= 0)
-   period = 10;/* 0.1 sec */
-
devpriv->wf_amplitude = amplitude;
devpriv->wf_period = period;

@@ -678,6 +697,36 @@ static int waveform_attach(struct comedi_device *dev,
return 0;
 }

+static int waveform_attach(struct comedi_device *dev,
+  struct comedi_devconfig *it)
+{
+   int amplitude = it->options[0];
+   int period = it->options[1];
+
+   /* set default amplitude and period */
+   if (amplitude <= 0)
+   amplitude = 100;/* 1 volt */
+   if (period <= 0)
+   period = 10;/* 0.1 sec */
+
+   return waveform_common_attach(dev, amplitude, period);
+}
+
+static int waveform_auto_attach(struct comedi_device *dev,
+   unsigned long context_unused)
+{
+   int amplitude = set_amplitude;
+   int period = set_period;
+
+   /* set default amplitude and period */
+   if (!amplitude)
+   amplitude = 100;/* 1 volt */
+   if (!period)
+   period = 10;/* 0.1 sec */
+
+   return waveform_common_attach(dev, amplitude, period);
+}
+
 static void waveform_detach(struct comedi_device *dev)
 {
struct waveform_private *devpriv = dev->private;
@@ -692,9 +741,66 @@ static struct comedi_driver waveform_driver = {
.driver_name= "comedi_test",
.module = THIS_MODULE,
.attach = waveform_attach,
+   .auto_attach= waveform_auto_attach,
.detach = waveform_detach,
 };
-module_comedi_driver(waveform_driver);
+
+/*
+ * For auto-configuration, a device is created to stand in for a
+ * real hardware device.
+ */
+static int __init comedi_test_init(void)
+{
+   int ret;
+
+   ret = comedi_driver_register(_driver);
+   if (ret) {
+   pr_err("comedi_test: unable to register driver\n");
+   return ret;
+   }
+
+   if (!config_mode) {
+   ctcls = class_create(THIS_MODULE, CLASS_NAME);
+   if (IS_ERR(ctcls)) {
+   pr_warn("comedi_test: unable to create class\n");
+   return ret;
+   }
+
+   ctdev = device_create(ctcls, NULL, MKDEV(0, 0), NULL, DEV_NAME);
+   if (IS_ERR(ctdev)) {
+   pr_warn("comedi_test: unable to create device\n");
+   goto clean2;
+   }
+
+   ret = comedi_auto_config(ctdev, _driver, 0);
+   if (ret) {
+   pr_warn("comedi_test: unable to auto-configure 
device\n");
+   goto clean;
+   }
+   }
+
+   return 0;
+
+clean:
+   device_destroy(ctcls, MKDEV(0, 0));
+clean2:
+   class_destroy(ctcls);
+
+   return 0;
+}
+module_init(comedi_test_init);
+
+static void __exit comedi_test_exit(void)
+{
+   comedi_auto_unconfig(ctdev);
+
+   device_destroy(ctcls, MKDEV(0, 0));
+
+   class_destroy(ctcls);


If the driver init returned successfully, but failed to set-up the 
auto-configured device, the device and class will not exist at this 
point, so those three calls need to go in an 'if' statement.  Perhaps 
you could use 'if (ctcls) {', and set 'ctcls = NULL;' after calling 
'class_destroy(ctcls);' in the init function.


Apart from that, it looks fine.


+
+   comedi_driver_unregister(_driver);
+}
+module_exit(comedi_test_exit);

 MODULE_AUTHOR("Comedi http://www.comedi.org;);
 MODULE_DESCRIPTION("Comedi low-level driver");




--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: drivers: comedi_test: Set max input value for auto config

[Ian Abbott]

On 27/01/17 15:55, Cheah Kok Cheong wrote:

Currently user can input any value for amplitude and period.
This patch set a sane max value for auto-configuration mode.

For manual configuration mode, it is assumed this is taken care of
by the COMEDI userspace tool since there's no limit set here from
day one in the staging tree. If otherwise then maybe this can be
looked at separately.

Signed-off-by: Cheah Kok Cheong <thrus...@gmail.com>


I don't think there is any need to limit these unless it results in 
arithmetic overflow, since they only affect the fake sample data values 
produced by the driver, not system performance.


--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: drivers: comedi_test: Set max input value for auto config

[Ian Abbott]

On 27/01/17 15:55, Cheah Kok Cheong wrote:

Currently user can input any value for amplitude and period.
This patch set a sane max value for auto-configuration mode.

For manual configuration mode, it is assumed this is taken care of
by the COMEDI userspace tool since there's no limit set here from
day one in the staging tree. If otherwise then maybe this can be
looked at separately.

Signed-off-by: Cheah Kok Cheong 


I don't think there is any need to limit these unless it results in 
arithmetic overflow, since they only affect the fake sample data values 
produced by the driver, not system performance.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: drivers: comedi_test: Add auto-configuration capability

[Ian Abbott]

atic int waveform_ao_insn_write(struct comedi_device 
*dev,
return insn->n;
 }

-static int waveform_attach(struct comedi_device *dev,
-  struct comedi_devconfig *it)
+static int waveform_common_attach(struct comedi_device *dev,
+ int amplitude, int period)
 {
struct waveform_private *devpriv;
struct comedi_subdevice *s;
-   int amplitude = it->options[0];
-   int period = it->options[1];
int i;
int ret;

@@ -621,12 +646,6 @@ static int waveform_attach(struct comedi_device *dev,
if (!devpriv)
return -ENOMEM;

-   /* set default amplitude and period */
-   if (amplitude <= 0)
-   amplitude = 100;/* 1 volt */
-   if (period <= 0)
-   period = 10;/* 0.1 sec */
-
devpriv->wf_amplitude = amplitude;
devpriv->wf_period = period;

@@ -678,6 +697,36 @@ static int waveform_attach(struct comedi_device *dev,
return 0;
 }

+static int waveform_attach(struct comedi_device *dev,
+  struct comedi_devconfig *it)
+{
+   int amplitude = it->options[0];
+   int period = it->options[1];
+
+   /* set default amplitude and period */
+   if (amplitude <= 0)
+   amplitude = 100;/* 1 volt */
+   if (period <= 0)
+   period = 10;/* 0.1 sec */
+
+   return waveform_common_attach(dev, amplitude, period);
+}
+
+static int waveform_auto_attach(struct comedi_device *dev,
+   unsigned long context_unused)
+{
+   int amplitude = set_amplitude;
+   int period = set_period;
+
+   /* set default amplitude and period */
+   if (!amplitude)
+   amplitude = 100;/* 1 volt */
+   if (!period)
+   period = 10;/* 0.1 sec */
+
+   return waveform_common_attach(dev, amplitude, period);
+}
+
 static void waveform_detach(struct comedi_device *dev)
 {
struct waveform_private *devpriv = dev->private;
@@ -692,9 +741,76 @@ static struct comedi_driver waveform_driver = {
.driver_name= "comedi_test",
.module = THIS_MODULE,
.attach = waveform_attach,
+   .auto_attach= waveform_auto_attach,
.detach = waveform_detach,
 };
-module_comedi_driver(waveform_driver);
+
+/*
+ * For auto configuration, a device is created to stand in for a
+ * real hardware device.
+ */
+static int __init comedi_test_init(void)
+{
+   int ret;
+
+   if (!config_mode) {
+   ctcls = class_create(THIS_MODULE, CLASS_NAME);
+   if (IS_ERR(ctcls)) {
+   pr_err("comedi_test: unable to create class\n");
+   return PTR_ERR(ctcls);
+   }
+
+   ctdev = device_create(ctcls, NULL, MKDEV(0, 0), NULL, DEV_NAME);
+   if (IS_ERR(ctdev)) {
+   pr_err("comedi_test: unable to create device\n");
+   ret = PTR_ERR(ctdev);
+   goto clean3;
+   }
+   }
+
+   ret = comedi_driver_register(_driver);
+   if (ret) {
+   pr_err("comedi_test: unable to register driver\n");
+   if (!config_mode)
+   goto clean2;
+   else
+   return ret;
+   }
+
+   if (!config_mode) {
+   ret = comedi_auto_config(ctdev, _driver, 0);
+   if (ret) {
+   pr_err("comedi_test: unable to auto configure 
device\n");
+   goto clean;
+   }
+   }
+
+   return 0;
+
+clean:
+   comedi_driver_unregister(_driver);
+clean2:
+   device_destroy(ctcls, MKDEV(0, 0));
+clean3:
+   class_destroy(ctcls);
+
+   return ret;
+}
+module_init(comedi_test_init);


I think the error handling paths look a bit untidy.  I think calling 
comedi_driver_register() first would help.  It would also be nice if 
failure to create the device class and test device did not prevent the 
module from initializing completely, since the module could still be 
used by manually configured COMEDI devices even if they fail.



+
+static void __exit comedi_test_exit(void)
+{
+   if (!config_mode)
+   comedi_auto_unconfig(ctdev);
+
+   comedi_driver_unregister(_driver);
+
+   if (!config_mode) {
+   device_destroy(ctcls, MKDEV(0, 0));
+   class_destroy(ctcls);
+   }
+}
+module_exit(comedi_test_exit);

 MODULE_AUTHOR("Comedi http://www.comedi.org;);
 MODULE_DESCRIPTION("Comedi low-level driver");




--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: drivers: comedi_test: Add auto-configuration capability

[Ian Abbott]

omedi_device 
*dev,
return insn->n;
 }

-static int waveform_attach(struct comedi_device *dev,
-  struct comedi_devconfig *it)
+static int waveform_common_attach(struct comedi_device *dev,
+ int amplitude, int period)
 {
struct waveform_private *devpriv;
struct comedi_subdevice *s;
-   int amplitude = it->options[0];
-   int period = it->options[1];
int i;
int ret;

@@ -621,12 +646,6 @@ static int waveform_attach(struct comedi_device *dev,
if (!devpriv)
return -ENOMEM;

-   /* set default amplitude and period */
-   if (amplitude <= 0)
-   amplitude = 100;/* 1 volt */
-   if (period <= 0)
-   period = 10;/* 0.1 sec */
-
devpriv->wf_amplitude = amplitude;
devpriv->wf_period = period;

@@ -678,6 +697,36 @@ static int waveform_attach(struct comedi_device *dev,
return 0;
 }

+static int waveform_attach(struct comedi_device *dev,
+  struct comedi_devconfig *it)
+{
+   int amplitude = it->options[0];
+   int period = it->options[1];
+
+   /* set default amplitude and period */
+   if (amplitude <= 0)
+   amplitude = 100;/* 1 volt */
+   if (period <= 0)
+   period = 10;/* 0.1 sec */
+
+   return waveform_common_attach(dev, amplitude, period);
+}
+
+static int waveform_auto_attach(struct comedi_device *dev,
+   unsigned long context_unused)
+{
+   int amplitude = set_amplitude;
+   int period = set_period;
+
+   /* set default amplitude and period */
+   if (!amplitude)
+   amplitude = 100;/* 1 volt */
+   if (!period)
+   period = 10;/* 0.1 sec */
+
+   return waveform_common_attach(dev, amplitude, period);
+}
+
 static void waveform_detach(struct comedi_device *dev)
 {
struct waveform_private *devpriv = dev->private;
@@ -692,9 +741,76 @@ static struct comedi_driver waveform_driver = {
.driver_name= "comedi_test",
.module = THIS_MODULE,
.attach = waveform_attach,
+   .auto_attach= waveform_auto_attach,
.detach = waveform_detach,
 };
-module_comedi_driver(waveform_driver);
+
+/*
+ * For auto configuration, a device is created to stand in for a
+ * real hardware device.
+ */
+static int __init comedi_test_init(void)
+{
+   int ret;
+
+   if (!config_mode) {
+   ctcls = class_create(THIS_MODULE, CLASS_NAME);
+   if (IS_ERR(ctcls)) {
+   pr_err("comedi_test: unable to create class\n");
+   return PTR_ERR(ctcls);
+   }
+
+   ctdev = device_create(ctcls, NULL, MKDEV(0, 0), NULL, DEV_NAME);
+   if (IS_ERR(ctdev)) {
+   pr_err("comedi_test: unable to create device\n");
+   ret = PTR_ERR(ctdev);
+   goto clean3;
+   }
+   }
+
+   ret = comedi_driver_register(_driver);
+   if (ret) {
+   pr_err("comedi_test: unable to register driver\n");
+   if (!config_mode)
+   goto clean2;
+   else
+   return ret;
+   }
+
+   if (!config_mode) {
+   ret = comedi_auto_config(ctdev, _driver, 0);
+   if (ret) {
+   pr_err("comedi_test: unable to auto configure 
device\n");
+   goto clean;
+   }
+   }
+
+   return 0;
+
+clean:
+   comedi_driver_unregister(_driver);
+clean2:
+   device_destroy(ctcls, MKDEV(0, 0));
+clean3:
+   class_destroy(ctcls);
+
+   return ret;
+}
+module_init(comedi_test_init);


I think the error handling paths look a bit untidy.  I think calling 
comedi_driver_register() first would help.  It would also be nice if 
failure to create the device class and test device did not prevent the 
module from initializing completely, since the module could still be 
used by manually configured COMEDI devices even if they fail.



+
+static void __exit comedi_test_exit(void)
+{
+   if (!config_mode)
+   comedi_auto_unconfig(ctdev);
+
+   comedi_driver_unregister(_driver);
+
+   if (!config_mode) {
+   device_destroy(ctcls, MKDEV(0, 0));
+   class_destroy(ctcls);
+   }
+}
+module_exit(comedi_test_exit);

 MODULE_AUTHOR("Comedi http://www.comedi.org;);
 MODULE_DESCRIPTION("Comedi low-level driver");




--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: Fix incorrect type assignment

[Ian Abbott]

On 09/02/2017 11:04, Greg KH wrote:

On Thu, Feb 09, 2017 at 01:53:56AM +0530, Karthik Nayak wrote:

This patch fixes the following sparse error:
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32: warning: incorrect type in 
assignment (different base types)
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:expected restricted 
__be32 [usertype] serial_number
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:got unsigned int

This is done by removing the whole code block, since the variable
'serial_number' is only assigned but never used.

Helped-by: Ian Abbott <abbo...@mev.co.uk>


There's no such tag, sorry :(

And does this obsolete all of your other ones?  Please make it obvious
what I am supposed to do here.

I've now dropped all of these patches from my queue.  please resend the
proper one.

thanks,

greg k-h



To add to that, I think the emphasis of the patch title and description 
should now be on the removal of serial_number, with fixing the sparse 
error as a useful side-effect/inspiration.  The patch title should also 
mention ni_pcimio.


Thanks,
Ian.

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: Fix incorrect type assignment

[Ian Abbott]

On 09/02/2017 11:04, Greg KH wrote:

On Thu, Feb 09, 2017 at 01:53:56AM +0530, Karthik Nayak wrote:

This patch fixes the following sparse error:
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32: warning: incorrect type in 
assignment (different base types)
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:expected restricted 
__be32 [usertype] serial_number
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:got unsigned int

This is done by removing the whole code block, since the variable
'serial_number' is only assigned but never used.

Helped-by: Ian Abbott 


There's no such tag, sorry :(

And does this obsolete all of your other ones?  Please make it obvious
what I am supposed to do here.

I've now dropped all of these patches from my queue.  please resend the
proper one.

thanks,

greg k-h



To add to that, I think the emphasis of the patch title and description 
should now be on the removal of serial_number, with fixing the sparse 
error as a useful side-effect/inspiration.  The patch title should also 
mention ni_pcimio.


Thanks,
Ian.

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 2/2] staging: comedi: drop unused variable from struct 'ni_private'

[Ian Abbott]

On 08/02/2017 16:55, Karthik Nayak wrote:

Drop the 'serial_number' variable from the struct 'ni_private' since
its never used after assignment.

Signed-off-by: Karthik Nayak <karthik@gmail.com>
---

This is to be based on top of "staging: comedi: Fix incorrect type assignment"
to which this is replied to.

 drivers/staging/comedi/drivers/ni_pcimio.c | 3 +--
 drivers/staging/comedi/drivers/ni_stc.h| 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_pcimio.c 
b/drivers/staging/comedi/drivers/ni_pcimio.c
index 4f45a5c230ad..da4d3da071eb 100644
--- a/drivers/staging/comedi/drivers/ni_pcimio.c
+++ b/drivers/staging/comedi/drivers/ni_pcimio.c
@@ -1222,12 +1222,11 @@ static void m_series_init_eeprom_buffer(struct 
comedi_device *dev)
writel(0x1 | old_iodwcr1_bits, mite->mmio + MITE_IODWCR_1);
writel(0xf, mite->mmio + 0x30);



I think it would be preferable to remove the code from here ...


-   BUG_ON(serial_number_eeprom_length > sizeof(devpriv->serial_number));
+   BUG_ON(serial_number_eeprom_length > sizeof(serial_number));
for (i = 0; i < serial_number_eeprom_length; ++i) {
char *byte_ptr = (char *)_number + i;
*byte_ptr = ni_readb(dev, serial_number_eeprom_offset + i);
}
-   devpriv->serial_number = be32_to_cpu(serial_number);


... to here.  And remove the serial_number_eeprom_length, 
serial_number_eeprom_offset, and serial_number variables too.  There is 
no need to continue reading the serial number bytes from the EEPROM.




for (i = 0; i < M_SERIES_EEPROM_SIZE; ++i)
devpriv->eeprom_buffer[i] = ni_readb(dev, Start_Cal_EEPROM + i);
diff --git a/drivers/staging/comedi/drivers/ni_stc.h 
b/drivers/staging/comedi/drivers/ni_stc.h
index b5eca0da71eb..61138e86a455 100644
--- a/drivers/staging/comedi/drivers/ni_stc.h
+++ b/drivers/staging/comedi/drivers/ni_stc.h
@@ -1031,7 +1031,6 @@ struct ni_private {

unsigned short ai_fifo_buffer[0x2000];
u8 eeprom_buffer[M_SERIES_EEPROM_SIZE];
-   unsigned int serial_number;

struct mite *mite;
struct mite_channel *ai_mite_chan;



--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 2/2] staging: comedi: drop unused variable from struct 'ni_private'

[Ian Abbott]

On 08/02/2017 16:55, Karthik Nayak wrote:

Drop the 'serial_number' variable from the struct 'ni_private' since
its never used after assignment.

Signed-off-by: Karthik Nayak 
---

This is to be based on top of "staging: comedi: Fix incorrect type assignment"
to which this is replied to.

 drivers/staging/comedi/drivers/ni_pcimio.c | 3 +--
 drivers/staging/comedi/drivers/ni_stc.h| 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_pcimio.c 
b/drivers/staging/comedi/drivers/ni_pcimio.c
index 4f45a5c230ad..da4d3da071eb 100644
--- a/drivers/staging/comedi/drivers/ni_pcimio.c
+++ b/drivers/staging/comedi/drivers/ni_pcimio.c
@@ -1222,12 +1222,11 @@ static void m_series_init_eeprom_buffer(struct 
comedi_device *dev)
writel(0x1 | old_iodwcr1_bits, mite->mmio + MITE_IODWCR_1);
writel(0xf, mite->mmio + 0x30);



I think it would be preferable to remove the code from here ...


-   BUG_ON(serial_number_eeprom_length > sizeof(devpriv->serial_number));
+   BUG_ON(serial_number_eeprom_length > sizeof(serial_number));
for (i = 0; i < serial_number_eeprom_length; ++i) {
char *byte_ptr = (char *)_number + i;
*byte_ptr = ni_readb(dev, serial_number_eeprom_offset + i);
}
-   devpriv->serial_number = be32_to_cpu(serial_number);


... to here.  And remove the serial_number_eeprom_length, 
serial_number_eeprom_offset, and serial_number variables too.  There is 
no need to continue reading the serial number bytes from the EEPROM.




for (i = 0; i < M_SERIES_EEPROM_SIZE; ++i)
devpriv->eeprom_buffer[i] = ni_readb(dev, Start_Cal_EEPROM + i);
diff --git a/drivers/staging/comedi/drivers/ni_stc.h 
b/drivers/staging/comedi/drivers/ni_stc.h
index b5eca0da71eb..61138e86a455 100644
--- a/drivers/staging/comedi/drivers/ni_stc.h
+++ b/drivers/staging/comedi/drivers/ni_stc.h
@@ -1031,7 +1031,6 @@ struct ni_private {

unsigned short ai_fifo_buffer[0x2000];
u8 eeprom_buffer[M_SERIES_EEPROM_SIZE];
-   unsigned int serial_number;

struct mite *mite;
struct mite_channel *ai_mite_chan;



--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: Fix incorrect type assignment

[Ian Abbott]

On 08/02/17 13:26, Karthik Nayak wrote:

Hello,

On Wed, Feb 8, 2017 at 6:43 PM, Ian Abbott <abbo...@mev.co.uk> wrote:

On 07/02/17 19:06, Karthik Nayak wrote:


This patch fixes the following sparse error:
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32: warning: incorrect
type in assignment (different base types)
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:expected
restricted __be32 [usertype] serial_number
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:got unsigned int

This is done by introducing a temporary variable which is of type
'__be32' and converting the existing variable to type 'unsigned int'.

Signed-off-by: Karthik Nayak <karthik@gmail.com>
---
 drivers/staging/comedi/drivers/ni_pcimio.c | 5 +++--
 drivers/staging/comedi/drivers/ni_stc.h| 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

[snip]


(On a side note, nothing actually uses serial number, so the code that reads
it from the EEPROM could just be ripped out.)

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>



Yea, I saw that, was assuming there might be a purposed use case scenario.


AFAICT it's never been used - not even to print a kernel log message or 
anything.



Do you want me to send another patch?


If you want.  If you plan to do so, could you indicate whether you are 
going to base the patch on top of this one, or whether this patch should 
be discarded.  Thanks!


--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: Fix incorrect type assignment

[Ian Abbott]

On 08/02/17 13:26, Karthik Nayak wrote:

Hello,

On Wed, Feb 8, 2017 at 6:43 PM, Ian Abbott  wrote:

On 07/02/17 19:06, Karthik Nayak wrote:


This patch fixes the following sparse error:
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32: warning: incorrect
type in assignment (different base types)
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:expected
restricted __be32 [usertype] serial_number
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:got unsigned int

This is done by introducing a temporary variable which is of type
'__be32' and converting the existing variable to type 'unsigned int'.

Signed-off-by: Karthik Nayak 
---
 drivers/staging/comedi/drivers/ni_pcimio.c | 5 +++--
 drivers/staging/comedi/drivers/ni_stc.h| 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

[snip]


(On a side note, nothing actually uses serial number, so the code that reads
it from the EEPROM could just be ripped out.)

Reviewed-by: Ian Abbott 



Yea, I saw that, was assuming there might be a purposed use case scenario.


AFAICT it's never been used - not even to print a kernel log message or 
anything.



Do you want me to send another patch?


If you want.  If you plan to do so, could you indicate whether you are 
going to base the patch on top of this one, or whether this patch should 
be discarded.  Thanks!


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: Fix incorrect type assignment

[Ian Abbott]

On 07/02/17 19:06, Karthik Nayak wrote:

This patch fixes the following sparse error:
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32: warning: incorrect type in 
assignment (different base types)
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:expected restricted 
__be32 [usertype] serial_number
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:got unsigned int

This is done by introducing a temporary variable which is of type
'__be32' and converting the existing variable to type 'unsigned int'.

Signed-off-by: Karthik Nayak <karthik@gmail.com>
---
 drivers/staging/comedi/drivers/ni_pcimio.c | 5 +++--
 drivers/staging/comedi/drivers/ni_stc.h| 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_pcimio.c 
b/drivers/staging/comedi/drivers/ni_pcimio.c
index cdb66eab1292..4f45a5c230ad 100644
--- a/drivers/staging/comedi/drivers/ni_pcimio.c
+++ b/drivers/staging/comedi/drivers/ni_pcimio.c
@@ -1207,6 +1207,7 @@ static void m_series_init_eeprom_buffer(struct 
comedi_device *dev)
unsigned int old_iodwbsr_bits;
unsigned int old_iodwbsr1_bits;
unsigned int old_iodwcr1_bits;
+   __be32 serial_number;
int i;

/* IO Window 1 needs to be temporarily mapped to read the eeprom */
@@ -1223,10 +1224,10 @@ static void m_series_init_eeprom_buffer(struct 
comedi_device *dev)

BUG_ON(serial_number_eeprom_length > sizeof(devpriv->serial_number));
for (i = 0; i < serial_number_eeprom_length; ++i) {
-   char *byte_ptr = (char *)>serial_number + i;
+   char *byte_ptr = (char *)_number + i;
*byte_ptr = ni_readb(dev, serial_number_eeprom_offset + i);
}
-   devpriv->serial_number = be32_to_cpu(devpriv->serial_number);
+   devpriv->serial_number = be32_to_cpu(serial_number);

for (i = 0; i < M_SERIES_EEPROM_SIZE; ++i)
devpriv->eeprom_buffer[i] = ni_readb(dev, Start_Cal_EEPROM + i);
diff --git a/drivers/staging/comedi/drivers/ni_stc.h 
b/drivers/staging/comedi/drivers/ni_stc.h
index f27b545f83eb..b5eca0da71eb 100644
--- a/drivers/staging/comedi/drivers/ni_stc.h
+++ b/drivers/staging/comedi/drivers/ni_stc.h
@@ -1031,7 +1031,7 @@ struct ni_private {

unsigned short ai_fifo_buffer[0x2000];
u8 eeprom_buffer[M_SERIES_EEPROM_SIZE];
-   __be32 serial_number;
+   unsigned int serial_number;

struct mite *mite;
struct mite_channel *ai_mite_chan;



That looks fine, thanks!

(On a side note, nothing actually uses serial number, so the code that 
reads it from the EEPROM could just be ripped out.)


Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: Fix incorrect type assignment

[Ian Abbott]

On 07/02/17 19:06, Karthik Nayak wrote:

This patch fixes the following sparse error:
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32: warning: incorrect type in 
assignment (different base types)
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:expected restricted 
__be32 [usertype] serial_number
drivers/staging/comedi/drivers//ni_pcimio.c:1229:32:got unsigned int

This is done by introducing a temporary variable which is of type
'__be32' and converting the existing variable to type 'unsigned int'.

Signed-off-by: Karthik Nayak 
---
 drivers/staging/comedi/drivers/ni_pcimio.c | 5 +++--
 drivers/staging/comedi/drivers/ni_stc.h| 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_pcimio.c 
b/drivers/staging/comedi/drivers/ni_pcimio.c
index cdb66eab1292..4f45a5c230ad 100644
--- a/drivers/staging/comedi/drivers/ni_pcimio.c
+++ b/drivers/staging/comedi/drivers/ni_pcimio.c
@@ -1207,6 +1207,7 @@ static void m_series_init_eeprom_buffer(struct 
comedi_device *dev)
unsigned int old_iodwbsr_bits;
unsigned int old_iodwbsr1_bits;
unsigned int old_iodwcr1_bits;
+   __be32 serial_number;
int i;

/* IO Window 1 needs to be temporarily mapped to read the eeprom */
@@ -1223,10 +1224,10 @@ static void m_series_init_eeprom_buffer(struct 
comedi_device *dev)

BUG_ON(serial_number_eeprom_length > sizeof(devpriv->serial_number));
for (i = 0; i < serial_number_eeprom_length; ++i) {
-   char *byte_ptr = (char *)>serial_number + i;
+   char *byte_ptr = (char *)_number + i;
*byte_ptr = ni_readb(dev, serial_number_eeprom_offset + i);
}
-   devpriv->serial_number = be32_to_cpu(devpriv->serial_number);
+   devpriv->serial_number = be32_to_cpu(serial_number);

for (i = 0; i < M_SERIES_EEPROM_SIZE; ++i)
devpriv->eeprom_buffer[i] = ni_readb(dev, Start_Cal_EEPROM + i);
diff --git a/drivers/staging/comedi/drivers/ni_stc.h 
b/drivers/staging/comedi/drivers/ni_stc.h
index f27b545f83eb..b5eca0da71eb 100644
--- a/drivers/staging/comedi/drivers/ni_stc.h
+++ b/drivers/staging/comedi/drivers/ni_stc.h
@@ -1031,7 +1031,7 @@ struct ni_private {

unsigned short ai_fifo_buffer[0x2000];
u8 eeprom_buffer[M_SERIES_EEPROM_SIZE];
-   __be32 serial_number;
+   unsigned int serial_number;

struct mite *mite;
struct mite_channel *ai_mite_chan;



That looks fine, thanks!

(On a side note, nothing actually uses serial number, so the code that 
reads it from the EEPROM could just be ripped out.)


Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

[PATCH] staging: comedi: ni_660x: Support PCI-6224

[Ian Abbott]

Add support for the NI PCI-6224 board, assuming it behaves like the NI
PXI-6224 board at the register level.

The PCI device ID comes from the "nitiowv.inf" file in National
Instrument's Windows drivers.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
---
 drivers/staging/comedi/Kconfig   |  2 +-
 drivers/staging/comedi/drivers/ni_660x.c | 10 --
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig
index e7255f811611..4c22e2caa75e 100644
--- a/drivers/staging/comedi/Kconfig
+++ b/drivers/staging/comedi/Kconfig
@@ -1025,7 +1025,7 @@ config COMEDI_NI_660X
select COMEDI_NI_TIOCMD
---help---
  Enable support for National Instruments PCI-6601 (ni_660x), PCI-6602,
- PXI-6602, PXI-6608 and PXI-6624.
+ PXI-6602, PXI-6608, PCI-6624, and PXI-6624.
 
  To compile this driver as a module, choose M here: the module will be
  called ni_660x.
diff --git a/drivers/staging/comedi/drivers/ni_660x.c 
b/drivers/staging/comedi/drivers/ni_660x.c
index 0dcb826a9f1f..6aa755ad3953 100644
--- a/drivers/staging/comedi/drivers/ni_660x.c
+++ b/drivers/staging/comedi/drivers/ni_660x.c
@@ -16,13 +16,13 @@
  * Driver: ni_660x
  * Description: National Instruments 660x counter/timer boards
  * Devices: [National Instruments] PCI-6601 (ni_660x), PCI-6602, PXI-6602,
- *   PXI-6608, PXI-6624
+ *   PXI-6608, PCI-6624, PXI-6624
  * Author: J.P. Mellor <jpmel...@rose-hulman.edu>,
  *   herman.bruynin...@mech.kuleuven.ac.be,
  *   wim.meeus...@mech.kuleuven.ac.be,
  *   klaas.gade...@mech.kuleuven.ac.be,
  *   Frank Mori Hess <fmh...@users.sourceforge.net>
- * Updated: Fri, 15 Mar 2013 10:47:56 +
+ * Updated: Mon, 16 Jan 2017 14:00:43 +
  * Status: experimental
  *
  * Encoders work.  PulseGeneration (both single pulse and pulse train)
@@ -211,6 +211,7 @@ enum ni_660x_boardid {
BOARD_PCI6602,
BOARD_PXI6602,
BOARD_PXI6608,
+   BOARD_PCI6624,
BOARD_PXI6624
 };
 
@@ -236,6 +237,10 @@ static const struct ni_660x_board ni_660x_boards[] = {
.name   = "PXI-6608",
.n_chips= 2,
},
+   [BOARD_PCI6624] = {
+   .name   = "PCI-6624",
+   .n_chips= 2,
+   },
[BOARD_PXI6624] = {
.name   = "PXI-6624",
.n_chips= 2,
@@ -920,6 +925,7 @@ static const struct pci_device_id ni_660x_pci_table[] = {
{ PCI_VDEVICE(NI, 0x1360), BOARD_PXI6602 },
{ PCI_VDEVICE(NI, 0x2c60), BOARD_PCI6601 },
{ PCI_VDEVICE(NI, 0x2cc0), BOARD_PXI6608 },
+   { PCI_VDEVICE(NI, 0x1e30), BOARD_PCI6624 },
{ PCI_VDEVICE(NI, 0x1e40), BOARD_PXI6624 },
{ 0 }
 };
-- 
2.11.0

[PATCH] staging: comedi: ni_660x: Support PCI-6224

[Ian Abbott]

Add support for the NI PCI-6224 board, assuming it behaves like the NI
PXI-6224 board at the register level.

The PCI device ID comes from the "nitiowv.inf" file in National
Instrument's Windows drivers.

Signed-off-by: Ian Abbott 
---
 drivers/staging/comedi/Kconfig   |  2 +-
 drivers/staging/comedi/drivers/ni_660x.c | 10 --
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig
index e7255f811611..4c22e2caa75e 100644
--- a/drivers/staging/comedi/Kconfig
+++ b/drivers/staging/comedi/Kconfig
@@ -1025,7 +1025,7 @@ config COMEDI_NI_660X
select COMEDI_NI_TIOCMD
---help---
  Enable support for National Instruments PCI-6601 (ni_660x), PCI-6602,
- PXI-6602, PXI-6608 and PXI-6624.
+ PXI-6602, PXI-6608, PCI-6624, and PXI-6624.
 
  To compile this driver as a module, choose M here: the module will be
  called ni_660x.
diff --git a/drivers/staging/comedi/drivers/ni_660x.c 
b/drivers/staging/comedi/drivers/ni_660x.c
index 0dcb826a9f1f..6aa755ad3953 100644
--- a/drivers/staging/comedi/drivers/ni_660x.c
+++ b/drivers/staging/comedi/drivers/ni_660x.c
@@ -16,13 +16,13 @@
  * Driver: ni_660x
  * Description: National Instruments 660x counter/timer boards
  * Devices: [National Instruments] PCI-6601 (ni_660x), PCI-6602, PXI-6602,
- *   PXI-6608, PXI-6624
+ *   PXI-6608, PCI-6624, PXI-6624
  * Author: J.P. Mellor ,
  *   herman.bruynin...@mech.kuleuven.ac.be,
  *   wim.meeus...@mech.kuleuven.ac.be,
  *   klaas.gade...@mech.kuleuven.ac.be,
  *   Frank Mori Hess 
- * Updated: Fri, 15 Mar 2013 10:47:56 +
+ * Updated: Mon, 16 Jan 2017 14:00:43 +
  * Status: experimental
  *
  * Encoders work.  PulseGeneration (both single pulse and pulse train)
@@ -211,6 +211,7 @@ enum ni_660x_boardid {
BOARD_PCI6602,
BOARD_PXI6602,
BOARD_PXI6608,
+   BOARD_PCI6624,
BOARD_PXI6624
 };
 
@@ -236,6 +237,10 @@ static const struct ni_660x_board ni_660x_boards[] = {
.name   = "PXI-6608",
.n_chips= 2,
},
+   [BOARD_PCI6624] = {
+   .name   = "PCI-6624",
+   .n_chips= 2,
+   },
[BOARD_PXI6624] = {
.name   = "PXI-6624",
.n_chips= 2,
@@ -920,6 +925,7 @@ static const struct pci_device_id ni_660x_pci_table[] = {
{ PCI_VDEVICE(NI, 0x1360), BOARD_PXI6602 },
{ PCI_VDEVICE(NI, 0x2c60), BOARD_PCI6601 },
{ PCI_VDEVICE(NI, 0x2cc0), BOARD_PXI6608 },
+   { PCI_VDEVICE(NI, 0x1e30), BOARD_PCI6624 },
{ PCI_VDEVICE(NI, 0x1e40), BOARD_PXI6624 },
{ 0 }
 };
-- 
2.11.0

[PATCH] staging: comedi: ni_pcimio: Support more PXI cards

[Ian Abbott]

Add support for NI PXI-6220, PXI-6221, PXI-6229, PXI-6250, PXI-6254,
PXI-6259, PXIe-6259, PXI-6280, PXI-6284, and PXI-6289 boards, treating
them the same as the correspondingly numbered PCI and PCIe boards (apart
from having different Comedi board name strings).  The same has
previously been done for other PXI boards supported by the driver.

The PCI device IDs for the newly supported boards come from the
"nixswv.inf" file in National Instrument's Windows drivers.

Also, sort `ni_pcimio_pci_table[]` by PCI device ID.  It is mostly
sorted already, so only the entries for PXI-6251 and PXIe-6251 need
moving.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
---
 drivers/staging/comedi/Kconfig |   8 +-
 drivers/staging/comedi/drivers/ni_pcimio.c | 173 +++--
 2 files changed, 167 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig
index e7255f811611..557ce96b4463 100644
--- a/drivers/staging/comedi/Kconfig
+++ b/drivers/staging/comedi/Kconfig
@@ -1070,9 +1070,11 @@ config COMEDI_NI_PCIMIO
  PCI-MIO-16E-4, PCI-6014, PCI-6040E, PXI-6040E, PCI-6030E, PCI-6031E,
  PCI-6032E, PCI-6033E, PCI-6071E, PCI-6023E, PCI-6024E, PCI-6025E,
  PXI-6025E, PCI-6034E, PCI-6035E, PCI-6052E, PCI-6110, PCI-6111,
- PCI-6220, PCI-6221, PCI-6224, PXI-6224, PCI-6225, PXI-6225, PCI-6229,
- PCI-6250, PCI-6251, PCIe-6251, PCI-6254, PCI-6259, PCIe-6259,
- PCI-6280, PCI-6281, PXI-6281, PCI-6284, PCI-6289, PCI-6711, PXI-6711,
+ PCI-6220, PXI-6220, PCI-6221, PXI-6221, PCI-6224, PXI-6224, PCI-6225,
+ PXI-6225, PCI-6229, PXI-6229, PCI-6250, PXI-6250, PCI-6251, PXI-6251,
+ PCIe-6251, PXIe-6251, PCI-6254, PXI-6254, PCI-6259, PXI-6259,
+ PCIe-6259, PXIe-6259, PCI-6280, PXI-6280, PCI-6281, PXI-6281,
+ PCI-6284, PXI-6284, PCI-6289, PXI-6289, PCI-6711, PXI-6711,
  PCI-6713, PXI-6713, PXI-6071E, PCI-6070E, PXI-6070E, PXI-6052E,
  PCI-6036E, PCI-6731, PCI-6733, PXI-6733, PCI-6143, PXI-6143
 
diff --git a/drivers/staging/comedi/drivers/ni_pcimio.c 
b/drivers/staging/comedi/drivers/ni_pcimio.c
index f13a2f7360b3..cdb66eab1292 100644
--- a/drivers/staging/comedi/drivers/ni_pcimio.c
+++ b/drivers/staging/comedi/drivers/ni_pcimio.c
@@ -25,17 +25,16 @@
  *   PCI-MIO-16XE-10, PXI-6030E, PCI-MIO-16E-1, PCI-MIO-16E-4, PCI-6014,
  *   PCI-6040E, PXI-6040E, PCI-6030E, PCI-6031E, PCI-6032E, PCI-6033E,
  *   PCI-6071E, PCI-6023E, PCI-6024E, PCI-6025E, PXI-6025E, PCI-6034E,
- *   PCI-6035E, PCI-6052E,
- *   PCI-6110, PCI-6111, PCI-6220, PCI-6221, PCI-6224, PXI-6224,
- *   PCI-6225, PXI-6225, PCI-6229, PCI-6250,
- *   PCI-6251, PXI-6251, PCIe-6251, PXIe-6251,
- *   PCI-6254, PCI-6259, PCIe-6259,
- *   PCI-6280, PCI-6281, PXI-6281, PCI-6284, PCI-6289,
- *   PCI-6711, PXI-6711, PCI-6713, PXI-6713,
- *   PXI-6071E, PCI-6070E, PXI-6070E,
+ *   PCI-6035E, PCI-6052E, PCI-6110, PCI-6111, PCI-6220, PXI-6220,
+ *   PCI-6221, PXI-6221, PCI-6224, PXI-6224, PCI-6225, PXI-6225,
+ *   PCI-6229, PXI-6229, PCI-6250, PXI-6250, PCI-6251, PXI-6251,
+ *   PCIe-6251, PXIe-6251, PCI-6254, PXI-6254, PCI-6259, PXI-6259,
+ *   PCIe-6259, PXIe-6259, PCI-6280, PXI-6280, PCI-6281, PXI-6281,
+ *   PCI-6284, PXI-6284, PCI-6289, PXI-6289, PCI-6711, PXI-6711,
+ *   PCI-6713, PXI-6713, PXI-6071E, PCI-6070E, PXI-6070E,
  *   PXI-6052E, PCI-6036E, PCI-6731, PCI-6733, PXI-6733,
  *   PCI-6143, PXI-6143
- * Updated: Mon, 09 Jan 2012 14:52:48 +
+ * Updated: Mon, 16 Jan 2017 12:56:04 +
  *
  * These boards are almost identical to the AT-MIO E series, except that
  * they use the PCI bus instead of ISA (i.e., AT). See the notes for the
@@ -181,26 +180,36 @@ enum ni_pcimio_boardid {
BOARD_PXI6031E,
BOARD_PCI6036E,
BOARD_PCI6220,
+   BOARD_PXI6220,
BOARD_PCI6221,
BOARD_PCI6221_37PIN,
+   BOARD_PXI6221,
BOARD_PCI6224,
BOARD_PXI6224,
BOARD_PCI6225,
BOARD_PXI6225,
BOARD_PCI6229,
+   BOARD_PXI6229,
BOARD_PCI6250,
+   BOARD_PXI6250,
BOARD_PCI6251,
BOARD_PXI6251,
BOARD_PCIE6251,
BOARD_PXIE6251,
BOARD_PCI6254,
+   BOARD_PXI6254,
BOARD_PCI6259,
+   BOARD_PXI6259,
BOARD_PCIE6259,
+   BOARD_PXIE6259,
BOARD_PCI6280,
+   BOARD_PXI6280,
BOARD_PCI6281,
BOARD_PXI6281,
BOARD_PCI6284,
+   BOARD_PXI6284,
BOARD_PCI6289,
+   BOARD_PXI6289,
BOARD_PCI6143,
BOARD_PXI6143,
 };
@@ -684,6 +693,16 @@ static const struct ni_board_struct ni_boards[] = {
.reg_type   = ni_reg_622x,
.caldac = { caldac_none },
},
+   [BOARD_PXI6220] = {
+   .name   = "pxi-6220",
+   .n_adchan   = 16,
+   .ai_maxdata = 0x,
+   .ai_fifo_depth  = 512,  /* FIXME: guess */

[PATCH] staging: comedi: ni_pcimio: Support more PXI cards

[Ian Abbott]

Add support for NI PXI-6220, PXI-6221, PXI-6229, PXI-6250, PXI-6254,
PXI-6259, PXIe-6259, PXI-6280, PXI-6284, and PXI-6289 boards, treating
them the same as the correspondingly numbered PCI and PCIe boards (apart
from having different Comedi board name strings).  The same has
previously been done for other PXI boards supported by the driver.

The PCI device IDs for the newly supported boards come from the
"nixswv.inf" file in National Instrument's Windows drivers.

Also, sort `ni_pcimio_pci_table[]` by PCI device ID.  It is mostly
sorted already, so only the entries for PXI-6251 and PXIe-6251 need
moving.

Signed-off-by: Ian Abbott 
---
 drivers/staging/comedi/Kconfig |   8 +-
 drivers/staging/comedi/drivers/ni_pcimio.c | 173 +++--
 2 files changed, 167 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig
index e7255f811611..557ce96b4463 100644
--- a/drivers/staging/comedi/Kconfig
+++ b/drivers/staging/comedi/Kconfig
@@ -1070,9 +1070,11 @@ config COMEDI_NI_PCIMIO
  PCI-MIO-16E-4, PCI-6014, PCI-6040E, PXI-6040E, PCI-6030E, PCI-6031E,
  PCI-6032E, PCI-6033E, PCI-6071E, PCI-6023E, PCI-6024E, PCI-6025E,
  PXI-6025E, PCI-6034E, PCI-6035E, PCI-6052E, PCI-6110, PCI-6111,
- PCI-6220, PCI-6221, PCI-6224, PXI-6224, PCI-6225, PXI-6225, PCI-6229,
- PCI-6250, PCI-6251, PCIe-6251, PCI-6254, PCI-6259, PCIe-6259,
- PCI-6280, PCI-6281, PXI-6281, PCI-6284, PCI-6289, PCI-6711, PXI-6711,
+ PCI-6220, PXI-6220, PCI-6221, PXI-6221, PCI-6224, PXI-6224, PCI-6225,
+ PXI-6225, PCI-6229, PXI-6229, PCI-6250, PXI-6250, PCI-6251, PXI-6251,
+ PCIe-6251, PXIe-6251, PCI-6254, PXI-6254, PCI-6259, PXI-6259,
+ PCIe-6259, PXIe-6259, PCI-6280, PXI-6280, PCI-6281, PXI-6281,
+ PCI-6284, PXI-6284, PCI-6289, PXI-6289, PCI-6711, PXI-6711,
  PCI-6713, PXI-6713, PXI-6071E, PCI-6070E, PXI-6070E, PXI-6052E,
  PCI-6036E, PCI-6731, PCI-6733, PXI-6733, PCI-6143, PXI-6143
 
diff --git a/drivers/staging/comedi/drivers/ni_pcimio.c 
b/drivers/staging/comedi/drivers/ni_pcimio.c
index f13a2f7360b3..cdb66eab1292 100644
--- a/drivers/staging/comedi/drivers/ni_pcimio.c
+++ b/drivers/staging/comedi/drivers/ni_pcimio.c
@@ -25,17 +25,16 @@
  *   PCI-MIO-16XE-10, PXI-6030E, PCI-MIO-16E-1, PCI-MIO-16E-4, PCI-6014,
  *   PCI-6040E, PXI-6040E, PCI-6030E, PCI-6031E, PCI-6032E, PCI-6033E,
  *   PCI-6071E, PCI-6023E, PCI-6024E, PCI-6025E, PXI-6025E, PCI-6034E,
- *   PCI-6035E, PCI-6052E,
- *   PCI-6110, PCI-6111, PCI-6220, PCI-6221, PCI-6224, PXI-6224,
- *   PCI-6225, PXI-6225, PCI-6229, PCI-6250,
- *   PCI-6251, PXI-6251, PCIe-6251, PXIe-6251,
- *   PCI-6254, PCI-6259, PCIe-6259,
- *   PCI-6280, PCI-6281, PXI-6281, PCI-6284, PCI-6289,
- *   PCI-6711, PXI-6711, PCI-6713, PXI-6713,
- *   PXI-6071E, PCI-6070E, PXI-6070E,
+ *   PCI-6035E, PCI-6052E, PCI-6110, PCI-6111, PCI-6220, PXI-6220,
+ *   PCI-6221, PXI-6221, PCI-6224, PXI-6224, PCI-6225, PXI-6225,
+ *   PCI-6229, PXI-6229, PCI-6250, PXI-6250, PCI-6251, PXI-6251,
+ *   PCIe-6251, PXIe-6251, PCI-6254, PXI-6254, PCI-6259, PXI-6259,
+ *   PCIe-6259, PXIe-6259, PCI-6280, PXI-6280, PCI-6281, PXI-6281,
+ *   PCI-6284, PXI-6284, PCI-6289, PXI-6289, PCI-6711, PXI-6711,
+ *   PCI-6713, PXI-6713, PXI-6071E, PCI-6070E, PXI-6070E,
  *   PXI-6052E, PCI-6036E, PCI-6731, PCI-6733, PXI-6733,
  *   PCI-6143, PXI-6143
- * Updated: Mon, 09 Jan 2012 14:52:48 +
+ * Updated: Mon, 16 Jan 2017 12:56:04 +
  *
  * These boards are almost identical to the AT-MIO E series, except that
  * they use the PCI bus instead of ISA (i.e., AT). See the notes for the
@@ -181,26 +180,36 @@ enum ni_pcimio_boardid {
BOARD_PXI6031E,
BOARD_PCI6036E,
BOARD_PCI6220,
+   BOARD_PXI6220,
BOARD_PCI6221,
BOARD_PCI6221_37PIN,
+   BOARD_PXI6221,
BOARD_PCI6224,
BOARD_PXI6224,
BOARD_PCI6225,
BOARD_PXI6225,
BOARD_PCI6229,
+   BOARD_PXI6229,
BOARD_PCI6250,
+   BOARD_PXI6250,
BOARD_PCI6251,
BOARD_PXI6251,
BOARD_PCIE6251,
BOARD_PXIE6251,
BOARD_PCI6254,
+   BOARD_PXI6254,
BOARD_PCI6259,
+   BOARD_PXI6259,
BOARD_PCIE6259,
+   BOARD_PXIE6259,
BOARD_PCI6280,
+   BOARD_PXI6280,
BOARD_PCI6281,
BOARD_PXI6281,
BOARD_PCI6284,
+   BOARD_PXI6284,
BOARD_PCI6289,
+   BOARD_PXI6289,
BOARD_PCI6143,
BOARD_PXI6143,
 };
@@ -684,6 +693,16 @@ static const struct ni_board_struct ni_boards[] = {
.reg_type   = ni_reg_622x,
.caldac = { caldac_none },
},
+   [BOARD_PXI6220] = {
+   .name   = "pxi-6220",
+   .n_adchan   = 16,
+   .ai_maxdata = 0x,
+   .ai_fifo_depth  = 512,  /* FIXME: guess */
+   .gainlkup 

Re: [PATCH 0/4] Staging: comedi: comedi_fops: Header cleanup

[Ian Abbott]

On 07/01/17 11:13, Cheah Kok Cheong wrote:

This series does trivial header cleanup for comedi_fops.c

Cheah Kok Cheong (4):
  Staging: comedi: comedi_fops: Remove unused kmod.h header
  Staging: comedi: comedi_fops: Remove redundant init.h header
  Staging: comedi: comedi_fops: Remove unused vmalloc.h header
  Staging: comedi: comedi_fops: Remove unused stat.h header

 drivers/staging/comedi/comedi_fops.c | 4 
 1 file changed, 4 deletions(-)



I wasn't sure about the removal of the #include , but it 
seems to make sense.  Thanks.


Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 0/4] Staging: comedi: comedi_fops: Header cleanup

[Ian Abbott]

On 07/01/17 11:13, Cheah Kok Cheong wrote:

This series does trivial header cleanup for comedi_fops.c

Cheah Kok Cheong (4):
  Staging: comedi: comedi_fops: Remove unused kmod.h header
  Staging: comedi: comedi_fops: Remove redundant init.h header
  Staging: comedi: comedi_fops: Remove unused vmalloc.h header
  Staging: comedi: comedi_fops: Remove unused stat.h header

 drivers/staging/comedi/comedi_fops.c | 4 
 1 file changed, 4 deletions(-)



I wasn't sure about the removal of the #include , but it 
seems to make sense.  Thanks.


Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: comedidev.h: Drop old style zero-length array

[Ian Abbott]

On 21/12/16 19:13, Cheah Kok Cheong wrote:

According to Documentation/Changes, the minimum gcc version required
to compile the kernel is 3.2 (this is probably outdated too).

Signed-off-by: Cheah Kok Cheong <thrus...@gmail.com>
---
 drivers/staging/comedi/comedidev.h | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/staging/comedi/comedidev.h 
b/drivers/staging/comedi/comedidev.h
index 0c7c37a..9713d96 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -612,12 +612,6 @@ extern const struct comedi_lrange range_unknown;

 #define range_digital  range_unipolar5

-#if __GNUC__ >= 3
-#define GCC_ZERO_LENGTH_ARRAY
-#else
-#define GCC_ZERO_LENGTH_ARRAY 0
-#endif
-
 /**
  * struct comedi_lrange - Describes a COMEDI range table
  * @length: Number of entries in the range table.
@@ -631,7 +625,7 @@ extern const struct comedi_lrange range_unknown;
  */
 struct comedi_lrange {
int length;
-   struct comedi_krange range[GCC_ZERO_LENGTH_ARRAY];
+   struct comedi_krange range[];
 };

 /**



Yes, that seems to be redundant.  Thanks!

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: comedidev.h: Drop old style zero-length array

[Ian Abbott]

On 21/12/16 19:13, Cheah Kok Cheong wrote:

According to Documentation/Changes, the minimum gcc version required
to compile the kernel is 3.2 (this is probably outdated too).

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/comedidev.h | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/staging/comedi/comedidev.h 
b/drivers/staging/comedi/comedidev.h
index 0c7c37a..9713d96 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -612,12 +612,6 @@ extern const struct comedi_lrange range_unknown;

 #define range_digital  range_unipolar5

-#if __GNUC__ >= 3
-#define GCC_ZERO_LENGTH_ARRAY
-#else
-#define GCC_ZERO_LENGTH_ARRAY 0
-#endif
-
 /**
  * struct comedi_lrange - Describes a COMEDI range table
  * @length: Number of entries in the range table.
@@ -631,7 +625,7 @@ extern const struct comedi_lrange range_unknown;
  */
 struct comedi_lrange {
int length;
-   struct comedi_krange range[GCC_ZERO_LENGTH_ARRAY];
+   struct comedi_krange range[];
 };

 /**



Yes, that seems to be redundant.  Thanks!

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: comedi_compat32: fixed a syntax error

[Ian Abbott]

On 26/12/16 06:26, Jonathan Villatoro wrote:

Fixed a syntax error in the function definition's parameter.


It's not really a syntax error, just a coding style issue.



Signed-off-by: Jonathan Horacio Villatoro Córdoba <lacho8...@gmail.com>
---
 drivers/staging/comedi/comedi_compat32.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_compat32.h 
b/drivers/staging/comedi/comedi_compat32.h
index 5ce77f3..91d15c6 100644
--- a/drivers/staging/comedi/comedi_compat32.h
+++ b/drivers/staging/comedi/comedi_compat32.h
@@ -25,7 +25,7 @@
 #ifdef CONFIG_COMPAT

 struct file;
-long comedi_compat_ioctl(struct file *, unsigned int cmd, unsigned long arg);
+long comedi_compat_ioctl(struct file *f, unsigned int cmd, unsigned long arg);

 #else /* CONFIG_COMPAT */




Thanks.  There is an earlier patch for this that hasn't been applied yet.

http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2016-December/097854.html

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] Staging: comedi: comedi_compat32: fixed a syntax error

[Ian Abbott]

On 26/12/16 06:26, Jonathan Villatoro wrote:

Fixed a syntax error in the function definition's parameter.


It's not really a syntax error, just a coding style issue.



Signed-off-by: Jonathan Horacio Villatoro Córdoba 
---
 drivers/staging/comedi/comedi_compat32.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_compat32.h 
b/drivers/staging/comedi/comedi_compat32.h
index 5ce77f3..91d15c6 100644
--- a/drivers/staging/comedi/comedi_compat32.h
+++ b/drivers/staging/comedi/comedi_compat32.h
@@ -25,7 +25,7 @@
 #ifdef CONFIG_COMPAT

 struct file;
-long comedi_compat_ioctl(struct file *, unsigned int cmd, unsigned long arg);
+long comedi_compat_ioctl(struct file *f, unsigned int cmd, unsigned long arg);

 #else /* CONFIG_COMPAT */




Thanks.  There is an earlier patch for this that hasn't been applied yet.

http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2016-December/097854.html

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 0/5] Staging: comedi: Proc FS related cleanup

[Ian Abbott]

On 30/12/16 11:24, Cheah Kok Cheong wrote:

This series does trivial cleanup for COMEDI proc fs related stuff.

Cheah Kok Cheong (5):
  Staging: comedi: comedi_fops: Avoid orphaned proc entry
  Staging: comedi: proc: Change file permission to read only
  Staging: comedi: proc: Add __init prefix
  Staging: comedi: proc: Add module owner
  Staging: comedi: proc: Warn if unable to create proc entry

 drivers/staging/comedi/comedi_fops.c | 6 +++---
 drivers/staging/comedi/proc.c| 6 --
 2 files changed, 7 insertions(+), 5 deletions(-)



Looks good, thanks!

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 0/5] Staging: comedi: Proc FS related cleanup

[Ian Abbott]

On 30/12/16 11:24, Cheah Kok Cheong wrote:

This series does trivial cleanup for COMEDI proc fs related stuff.

Cheah Kok Cheong (5):
  Staging: comedi: comedi_fops: Avoid orphaned proc entry
  Staging: comedi: proc: Change file permission to read only
  Staging: comedi: proc: Add __init prefix
  Staging: comedi: proc: Add module owner
  Staging: comedi: proc: Warn if unable to create proc entry

 drivers/staging/comedi/comedi_fops.c | 6 +++---
 drivers/staging/comedi/proc.c| 6 --
 2 files changed, 7 insertions(+), 5 deletions(-)



Looks good, thanks!

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: cb_pcidas64: Fixed coding issue about multiple line dereferencing

[Ian Abbott]

On 19/12/2016 15:14, devendra sharma wrote:

Fixed coding issue about multiple line dereferencing

Signed-off-by: Devendra Sharma <devendra.sharma9...@gmail.com>
---
  drivers/staging/comedi/drivers/cb_pcidas64.c | 13 +
  1 file changed, 5 insertions(+), 8 deletions(-)



I guess this is version 3 of the patch?


diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index cb9c269..9ace9c0 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -2475,20 +2475,17 @@ static int setup_channel_queue(struct comedi_device 
*dev,
/* load external queue */
for (i = 0; i < cmd->chanlist_len; i++) {
bits = 0;
+   unsigned int ch = cmd->chanlist[i];


Please put all declarations within a block before any statements in the 
block (in other words, avoid mixing declarations with code), and use a 
blank line to separate the declarations from the statements.


--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: cb_pcidas64: Fixed coding issue about multiple line dereferencing

[Ian Abbott]

On 19/12/2016 15:14, devendra sharma wrote:

Fixed coding issue about multiple line dereferencing

Signed-off-by: Devendra Sharma 
---
  drivers/staging/comedi/drivers/cb_pcidas64.c | 13 +
  1 file changed, 5 insertions(+), 8 deletions(-)



I guess this is version 3 of the patch?


diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index cb9c269..9ace9c0 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -2475,20 +2475,17 @@ static int setup_channel_queue(struct comedi_device 
*dev,
/* load external queue */
for (i = 0; i < cmd->chanlist_len; i++) {
bits = 0;
+   unsigned int ch = cmd->chanlist[i];


Please put all declarations within a block before any statements in the 
block (in other words, avoid mixing declarations with code), and use a 
blank line to separate the declarations from the statements.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: dyna_pci10xx: fixed check patch warnings about memory barrier comments

[Ian Abbott]

On 19/12/2016 14:36, devendra sharma wrote:

Added comments before memory barrier

Signed-off-by: Devendra Sharma <devendra.sharma9...@gmail.com>
---
  drivers/staging/comedi/drivers/dyna_pci10xx.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/dyna_pci10xx.c 
b/drivers/staging/comedi/drivers/dyna_pci10xx.c
index c9eb26f..c3b5236 100644
--- a/drivers/staging/comedi/drivers/dyna_pci10xx.c
+++ b/drivers/staging/comedi/drivers/dyna_pci10xx.c
@@ -122,6 +122,7 @@ static int dyna_pci10xx_insn_write_ao(struct comedi_device 
*dev,

mutex_lock(>mutex);
for (n = 0; n < insn->n; n++) {
+   /*multi-processor memory barrier*/
smp_mb();


Thanks for the effort, but these comments add no useful information and 
serve only to silence the checkpatch warnings. Also, there is no space 
after the '/*' and before the '*/'.


--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: dyna_pci10xx: fixed check patch warnings about memory barrier comments

[Ian Abbott]

On 19/12/2016 14:36, devendra sharma wrote:

Added comments before memory barrier

Signed-off-by: Devendra Sharma 
---
  drivers/staging/comedi/drivers/dyna_pci10xx.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/dyna_pci10xx.c 
b/drivers/staging/comedi/drivers/dyna_pci10xx.c
index c9eb26f..c3b5236 100644
--- a/drivers/staging/comedi/drivers/dyna_pci10xx.c
+++ b/drivers/staging/comedi/drivers/dyna_pci10xx.c
@@ -122,6 +122,7 @@ static int dyna_pci10xx_insn_write_ao(struct comedi_device 
*dev,

mutex_lock(>mutex);
for (n = 0; n < insn->n; n++) {
+   /*multi-processor memory barrier*/
smp_mb();


Thanks for the effort, but these comments add no useful information and 
serve only to silence the checkpatch warnings. Also, there is no space 
after the '/*' and before the '*/'.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: ni_670x: using the BIT(x) macro

[Ian Abbott]

On 16/12/2016 20:20, Saber Rezvani wrote:

Fix the checkpatch.pl issue:
CHECK: Prefer using the BIT macro
replacing bit shifting on 1 with the BIT(x) macro.

Signed-off-by: Saber Rezvani <irsa...@gmail.com>
---
  drivers/staging/comedi/drivers/ni_670x.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Thanks!

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: ni_670x: using the BIT(x) macro

[Ian Abbott]

On 16/12/2016 20:20, Saber Rezvani wrote:

Fix the checkpatch.pl issue:
CHECK: Prefer using the BIT macro
replacing bit shifting on 1 with the BIT(x) macro.

Signed-off-by: Saber Rezvani 
---
  drivers/staging/comedi/drivers/ni_670x.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Thanks!

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: ni_at_ao: using the BIT(x) macro

[Ian Abbott]

On 16/12/2016 20:06, Saber Rezvani wrote:

Fix the checkpatch.pl issue:
CHECK: Prefer using the BIT macro
replacing bit shifting on 1 with the BIT(x) macro.

Signed-off-by: Saber Rezvani <irsa...@gmail.com>
---
  drivers/staging/comedi/drivers/ni_at_ao.c | 62 +++
  1 file changed, 31 insertions(+), 31 deletions(-)


Thanks!

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: ni_at_ao: using the BIT(x) macro

[Ian Abbott]

On 16/12/2016 20:06, Saber Rezvani wrote:

Fix the checkpatch.pl issue:
CHECK: Prefer using the BIT macro
replacing bit shifting on 1 with the BIT(x) macro.

Signed-off-by: Saber Rezvani 
---
  drivers/staging/comedi/drivers/ni_at_ao.c | 62 +++
  1 file changed, 31 insertions(+), 31 deletions(-)


Thanks!

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 3/3] staging: comedi: cb_pcidas64: use preferred kernel type u32

[Ian Abbott]

On 16/12/2016 19:15, Saber Rezvani wrote:

Fix the checkpatch.pl issue:
CHECK: Prefer kernel type 'u32' over 'uint32_t'

Signed-off-by: Saber Rezvani <irsa...@gmail.com>
---
  drivers/staging/comedi/drivers/cb_pcidas64.c | 38 ++--
  1 file changed, 19 insertions(+), 19 deletions(-)



Thanks!

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 3/3] staging: comedi: cb_pcidas64: use preferred kernel type u32

[Ian Abbott]

On 16/12/2016 19:15, Saber Rezvani wrote:

Fix the checkpatch.pl issue:
CHECK: Prefer kernel type 'u32' over 'uint32_t'

Signed-off-by: Saber Rezvani 
---
  drivers/staging/comedi/drivers/cb_pcidas64.c | 38 ++--
  1 file changed, 19 insertions(+), 19 deletions(-)



Thanks!

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 2/3] staging: comedi: cb_pcidas64: use preferred kernel type u16

[Ian Abbott]

On 16/12/2016 19:15, Saber Rezvani wrote:

Fix the checkpatch.pl issue:
CHECK: Prefer kernel type 'u16' over 'uint16_t'

Signed-off-by: Saber Rezvani <irsa...@gmail.com>
---
  drivers/staging/comedi/drivers/cb_pcidas64.c | 58 ++--
  1 file changed, 29 insertions(+), 29 deletions(-)



Thanks!

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 2/3] staging: comedi: cb_pcidas64: use preferred kernel type u16

[Ian Abbott]

On 16/12/2016 19:15, Saber Rezvani wrote:

Fix the checkpatch.pl issue:
CHECK: Prefer kernel type 'u16' over 'uint16_t'

Signed-off-by: Saber Rezvani 
---
  drivers/staging/comedi/drivers/cb_pcidas64.c | 58 ++--
  1 file changed, 29 insertions(+), 29 deletions(-)



Thanks!

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 1/3] staging: comedi: cb_pcidas64: use preferred kernel type u8

[Ian Abbott]

On 16/12/2016 19:15, Saber Rezvani wrote:

Fix the checkpatch.pl issue:
CHECK: Prefer kernel type 'u8' over 'uint8_t'

Signed-off-by: Saber Rezvani <irsa...@gmail.com>
---
  drivers/staging/comedi/drivers/cb_pcidas64.c | 46 ++--
  1 file changed, 23 insertions(+), 23 deletions(-)



Thanks!

Reviewed-by: Ian Abbott <abbo...@mev.co.uk>

--
-=( Ian Abbott @ MEV Ltd.E-mail: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-