Re: [PATCH v5] staging: comedi: Improved readability of function comedi_nsamples_left.

[Ian Abbott]

On 13/06/18 18:14, Chris Opperman wrote:

Improve readability of comedi_nsamples_left:
a) Reduce nesting by using more return statements.
b) Declare variables scans_left and samples_left at start of function.
c) Change type of scans_Left to unsigned long long to avoid cast.

Signed-off-by: Chris Opperman 
---

Changes v5:
a) Moved additional text to below the cut-off line.



The "Changes v5" text is a bit incomplete without the earlier change 
history, but let's forget that since the previous change history was a 
bit messed up anyway.



  drivers/staging/comedi/drivers.c | 26 +-
  1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 9d73347..57dd63d 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -473,21 +473,21 @@ unsigned int comedi_nsamples_left(struct comedi_subdevice 
*s,
  {
struct comedi_async *async = s->async;
struct comedi_cmd *cmd = >cmd;
+   unsigned long long scans_left;
+   unsigned long long samples_left;

-   if (cmd->stop_src == TRIG_COUNT) {
-   unsigned int scans_left = __comedi_nscans_left(s, 
cmd->stop_arg);
-   unsigned int scan_pos =
-   comedi_bytes_to_samples(s, async->scan_progress);
-   unsigned long long samples_left = 0;
-
-   if (scans_left) {
-   samples_left = ((unsigned long long)scans_left *
-   cmd->scan_end_arg) - scan_pos;
-   }
+   if (cmd->stop_src != TRIG_COUNT)
+   return nsamples;

-   if (samples_left < nsamples)
-   nsamples = samples_left;
-   }
+   scans_left = __comedi_nscans_left(s, cmd->stop_arg);
+   if (!scans_left)
+   return 0;
+
+   samples_left = scans_left * cmd->scan_end_arg -
+   comedi_bytes_to_samples(s, async->scan_progress);
+
+   if (samples_left < nsamples)
+   return samples_left;
return nsamples;
  }
  EXPORT_SYMBOL_GPL(comedi_nsamples_left);
--
2.1.4



The actual patch looks fine thanks!

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott  || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH v4] staging: comedi: Improved readability of function comedi_nsamples_left.

[Ian Abbott]

On 13/06/18 19:26, Chris Opperman wrote:

Hi Dan/Ian,

Noted your comments regarding additional text, thanks! Just curious whether
the "scissors" format given at the link below is valid?

https://kernelnewbies.org/PatchTipsAndTricks

It is given as an alternative to placing additional text below the
cut-off line.


I don't know.  Maybe?  I haven't seen it used before.

--
-=( Ian Abbott  || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH v4] staging: comedi: Improved readability of function comedi_nsamples_left.

[Ian Abbott]

On 13/06/18 19:26, Chris Opperman wrote:

Hi Dan/Ian,

Noted your comments regarding additional text, thanks! Just curious whether
the "scissors" format given at the link below is valid?

https://kernelnewbies.org/PatchTipsAndTricks

It is given as an alternative to placing additional text below the
cut-off line.


I don't know.  Maybe?  I haven't seen it used before.

--
-=( Ian Abbott  || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH v4] staging: comedi: Improved readability of function comedi_nsamples_left.

[Ian Abbott]

On 12/06/18 22:09, Chris Opperman wrote:

Changes since v3:
a) Reverted u64 to unsigned long long and u32 to unsigned int.
b) Added patch versioning.
c) Changed type of scans_left to unsigned long long to avoid cast.
d) Clarified and updated changelog.


8---8<


Improve readability of comedi_nsamples_left:
a) Reduce nesting by using more return statements.
b) Declare variables scans_left and samples_left at start of function.
c) Change type of scans_Left to unsigned long long to avoid cast.

Signed-off-by: Chris Opperman 
---
  drivers/staging/comedi/drivers.c | 26 +-
  1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 9d73347..57dd63d 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -473,21 +473,21 @@ unsigned int comedi_nsamples_left(struct comedi_subdevice 
*s,
  {
struct comedi_async *async = s->async;
struct comedi_cmd *cmd = >cmd;
+   unsigned long long scans_left;
+   unsigned long long samples_left;

-   if (cmd->stop_src == TRIG_COUNT) {
-   unsigned int scans_left = __comedi_nscans_left(s, 
cmd->stop_arg);
-   unsigned int scan_pos =
-   comedi_bytes_to_samples(s, async->scan_progress);
-   unsigned long long samples_left = 0;
-
-   if (scans_left) {
-   samples_left = ((unsigned long long)scans_left *
-   cmd->scan_end_arg) - scan_pos;
-   }
+   if (cmd->stop_src != TRIG_COUNT)
+   return nsamples;

-   if (samples_left < nsamples)
-   nsamples = samples_left;
-   }
+   scans_left = __comedi_nscans_left(s, cmd->stop_arg);
+   if (!scans_left)
+   return 0;
+
+   samples_left = scans_left * cmd->scan_end_arg -
+   comedi_bytes_to_samples(s, async->scan_progress);
+
+   if (samples_left < nsamples)
+   return samples_left;
return nsamples;
  }
  EXPORT_SYMBOL_GPL(comedi_nsamples_left);
--
2.1.4



The code changes look fine.  You just need to correct the commit 
message, as pointed out by Dan Carpenter.


For reference, everything below the "---" cut-off line gets stripped out 
of the commit message in the git repository, so that is a good place to 
add comments about the patch itself (such as change logs) that do not 
belong in the commit message.


--
-=( Ian Abbott  || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH v4] staging: comedi: Improved readability of function comedi_nsamples_left.

[Ian Abbott]

On 12/06/18 22:09, Chris Opperman wrote:

Changes since v3:
a) Reverted u64 to unsigned long long and u32 to unsigned int.
b) Added patch versioning.
c) Changed type of scans_left to unsigned long long to avoid cast.
d) Clarified and updated changelog.


8---8<


Improve readability of comedi_nsamples_left:
a) Reduce nesting by using more return statements.
b) Declare variables scans_left and samples_left at start of function.
c) Change type of scans_Left to unsigned long long to avoid cast.

Signed-off-by: Chris Opperman 
---
  drivers/staging/comedi/drivers.c | 26 +-
  1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 9d73347..57dd63d 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -473,21 +473,21 @@ unsigned int comedi_nsamples_left(struct comedi_subdevice 
*s,
  {
struct comedi_async *async = s->async;
struct comedi_cmd *cmd = >cmd;
+   unsigned long long scans_left;
+   unsigned long long samples_left;

-   if (cmd->stop_src == TRIG_COUNT) {
-   unsigned int scans_left = __comedi_nscans_left(s, 
cmd->stop_arg);
-   unsigned int scan_pos =
-   comedi_bytes_to_samples(s, async->scan_progress);
-   unsigned long long samples_left = 0;
-
-   if (scans_left) {
-   samples_left = ((unsigned long long)scans_left *
-   cmd->scan_end_arg) - scan_pos;
-   }
+   if (cmd->stop_src != TRIG_COUNT)
+   return nsamples;

-   if (samples_left < nsamples)
-   nsamples = samples_left;
-   }
+   scans_left = __comedi_nscans_left(s, cmd->stop_arg);
+   if (!scans_left)
+   return 0;
+
+   samples_left = scans_left * cmd->scan_end_arg -
+   comedi_bytes_to_samples(s, async->scan_progress);
+
+   if (samples_left < nsamples)
+   return samples_left;
return nsamples;
  }
  EXPORT_SYMBOL_GPL(comedi_nsamples_left);
--
2.1.4



The code changes look fine.  You just need to correct the commit 
message, as pointed out by Dan Carpenter.


For reference, everything below the "---" cut-off line gets stripped out 
of the commit message in the git repository, so that is a good place to 
add comments about the patch itself (such as change logs) that do not 
belong in the commit message.


--
-=( Ian Abbott  || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH] staging: comedi: comedidev.h: Fix SPDX-License-Identifier tag style

[Ian Abbott]

On 24/05/18 18:44, Nishad Kamdar wrote:

Replace // SPDX-License-Identifier: GPL-2.0+ by
/* SPDX-License-Identifier: GPL-2.0+ */ as per licensing rule
for C header files. Issue found by checkpatch. Part of
Eudyptula Challenge.

Signed-off-by: Nishad Kamdar <nishadkam...@gmail.com>
---
  drivers/staging/comedi/comedidev.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedidev.h 
b/drivers/staging/comedi/comedidev.h
index f3474a4ba69e..c54ac94d89d2 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0+
+/* SPDX-License-Identifier: GPL-2.0+ */
  /*
   * comedidev.h
   * header file for kernel-only structures, variables, and constants



That looks fine thanks.

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

--
-=( Ian Abbott <abbo...@mev.co.uk> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH] staging: comedi: comedidev.h: Fix SPDX-License-Identifier tag style

[Ian Abbott]

On 24/05/18 18:44, Nishad Kamdar wrote:

Replace // SPDX-License-Identifier: GPL-2.0+ by
/* SPDX-License-Identifier: GPL-2.0+ */ as per licensing rule
for C header files. Issue found by checkpatch. Part of
Eudyptula Challenge.

Signed-off-by: Nishad Kamdar 
---
  drivers/staging/comedi/comedidev.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedidev.h 
b/drivers/staging/comedi/comedidev.h
index f3474a4ba69e..c54ac94d89d2 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0+
+/* SPDX-License-Identifier: GPL-2.0+ */
  /*
   * comedidev.h
   * header file for kernel-only structures, variables, and constants



That looks fine thanks.

Reviewed-by: Ian Abbott 

--
-=( Ian Abbott  || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH] Staging:Comedi:comedi_compat32.c: Lindent changes

[Ian Abbott]

On 15/05/18 06:20, Pratik Jain wrote:

Recommended indentation by Lindent on file comedi_compat32.c

Signed-off-by: Pratik Jain <pratik.jain0...@gmail.com>
---
  drivers/staging/comedi/comedi_compat32.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/comedi_compat32.c 
b/drivers/staging/comedi/comedi_compat32.c
index 97fb9388bc22..fa9d239474ee 100644
--- a/drivers/staging/comedi/comedi_compat32.c
+++ b/drivers/staging/comedi/comedi_compat32.c
@@ -34,7 +34,7 @@
  struct comedi32_chaninfo_struct {
unsigned int subdev;
compat_uptr_t maxdata_list; /* 32-bit 'unsigned int *' */
-   compat_uptr_t flaglist; /* 32-bit 'unsigned int *' */
+   compat_uptr_t flaglist; /* 32-bit 'unsigned int *' */
compat_uptr_t rangelist;/* 32-bit 'unsigned int *' */
unsigned int unused[4];
  };
@@ -57,16 +57,16 @@ struct comedi32_cmd_struct {
unsigned int scan_end_arg;
unsigned int stop_src;
unsigned int stop_arg;
-   compat_uptr_t chanlist; /* 32-bit 'unsigned int *' */
+   compat_uptr_t chanlist; /* 32-bit 'unsigned int *' */
unsigned int chanlist_len;
-   compat_uptr_t data; /* 32-bit 'short *' */
+   compat_uptr_t data; /* 32-bit 'short *' */
unsigned int data_len;
  };
  
  struct comedi32_insn_struct {

unsigned int insn;
unsigned int n;
-   compat_uptr_t data; /* 32-bit 'unsigned int *' */
+   compat_uptr_t data; /* 32-bit 'unsigned int *' */
unsigned int subdev;
unsigned int chanspec;
unsigned int unused[3];
@@ -74,7 +74,7 @@ struct comedi32_insn_struct {
  
  struct comedi32_insnlist_struct {

unsigned int n_insns;
-   compat_uptr_t insns;/* 32-bit 'struct comedi_insn *' */
+   compat_uptr_t insns;/* 32-bit 'struct comedi_insn *' */
  };


Those all make the code look less pretty because all those comments are 
nicely aligned currently.


  
  /* Handle translated ioctl. */

@@ -194,7 +194,7 @@ static int get_compat_cmd(struct comedi_cmd __user *cmd,
err |= __put_user(temp.uint, >stop_arg);
err |= __get_user(temp.uptr, >chanlist);
err |= __put_user((unsigned int __force *)compat_ptr(temp.uptr),
-   >chanlist);
+ >chanlist);
err |= __get_user(temp.uint, >chanlist_len);
err |= __put_user(temp.uint, >chanlist_len);
err |= __get_user(temp.uptr, >data);



That is the only one that is an improvement, IMHO.

--
-=( Ian Abbott <abbo...@mev.co.uk> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH] Staging:Comedi:comedi_compat32.c: Lindent changes

[Ian Abbott]

On 15/05/18 06:20, Pratik Jain wrote:

Recommended indentation by Lindent on file comedi_compat32.c

Signed-off-by: Pratik Jain 
---
  drivers/staging/comedi/comedi_compat32.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/comedi_compat32.c 
b/drivers/staging/comedi/comedi_compat32.c
index 97fb9388bc22..fa9d239474ee 100644
--- a/drivers/staging/comedi/comedi_compat32.c
+++ b/drivers/staging/comedi/comedi_compat32.c
@@ -34,7 +34,7 @@
  struct comedi32_chaninfo_struct {
unsigned int subdev;
compat_uptr_t maxdata_list; /* 32-bit 'unsigned int *' */
-   compat_uptr_t flaglist; /* 32-bit 'unsigned int *' */
+   compat_uptr_t flaglist; /* 32-bit 'unsigned int *' */
compat_uptr_t rangelist;/* 32-bit 'unsigned int *' */
unsigned int unused[4];
  };
@@ -57,16 +57,16 @@ struct comedi32_cmd_struct {
unsigned int scan_end_arg;
unsigned int stop_src;
unsigned int stop_arg;
-   compat_uptr_t chanlist; /* 32-bit 'unsigned int *' */
+   compat_uptr_t chanlist; /* 32-bit 'unsigned int *' */
unsigned int chanlist_len;
-   compat_uptr_t data; /* 32-bit 'short *' */
+   compat_uptr_t data; /* 32-bit 'short *' */
unsigned int data_len;
  };
  
  struct comedi32_insn_struct {

unsigned int insn;
unsigned int n;
-   compat_uptr_t data; /* 32-bit 'unsigned int *' */
+   compat_uptr_t data; /* 32-bit 'unsigned int *' */
unsigned int subdev;
unsigned int chanspec;
unsigned int unused[3];
@@ -74,7 +74,7 @@ struct comedi32_insn_struct {
  
  struct comedi32_insnlist_struct {

unsigned int n_insns;
-   compat_uptr_t insns;/* 32-bit 'struct comedi_insn *' */
+   compat_uptr_t insns;/* 32-bit 'struct comedi_insn *' */
  };


Those all make the code look less pretty because all those comments are 
nicely aligned currently.


  
  /* Handle translated ioctl. */

@@ -194,7 +194,7 @@ static int get_compat_cmd(struct comedi_cmd __user *cmd,
err |= __put_user(temp.uint, >stop_arg);
err |= __get_user(temp.uptr, >chanlist);
err |= __put_user((unsigned int __force *)compat_ptr(temp.uptr),
-   >chanlist);
+ >chanlist);
err |= __get_user(temp.uint, >chanlist_len);
err |= __put_user(temp.uint, >chanlist_len);
err |= __get_user(temp.uptr, >data);



That is the only one that is an improvement, IMHO.

--
-=( Ian Abbott  || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH] staging: comedi: cb_pcidas64: fix alignment of function parameters

[Ian Abbott]

On 10/04/18 23:50, Gabriel Francisco Mandaji wrote:

Fix most checkpatch.pl issues of type:

CHECK: Alignment should match open parenthesis

Signed-off-by: Gabriel Francisco Mandaji <gfmand...@gmail.com>
---
  drivers/staging/comedi/drivers/cb_pcidas64.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index fdd81c3..631a703 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -2268,14 +2268,14 @@ static inline unsigned int dma_transfer_size(struct 
comedi_device *dev)
  }
  
  static u32 ai_convert_counter_6xxx(const struct comedi_device *dev,

-   const struct comedi_cmd *cmd)
+  const struct comedi_cmd *cmd)
  {
/* supposed to load counter with desired divisor minus 3 */
return cmd->convert_arg / TIMER_BASE - 3;
  }
  
  static u32 ai_scan_counter_6xxx(struct comedi_device *dev,

-struct comedi_cmd *cmd)
+   struct comedi_cmd *cmd)
  {
u32 count;
  
@@ -2296,7 +2296,7 @@ static u32 ai_scan_counter_6xxx(struct comedi_device *dev,

  }
  
  static u32 ai_convert_counter_4020(struct comedi_device *dev,

-   struct comedi_cmd *cmd)
+  struct comedi_cmd *cmd)
  {
struct pcidas64_private *devpriv = dev->private;
unsigned int divisor;



Looks good, thanks.  I guess the remaining case of open parentheses 
alignment wasn't changed because that would exceed 80 columns.


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: cb_pcidas64: fix alignment of function parameters

[Ian Abbott]

On 10/04/18 23:50, Gabriel Francisco Mandaji wrote:

Fix most checkpatch.pl issues of type:

CHECK: Alignment should match open parenthesis

Signed-off-by: Gabriel Francisco Mandaji 
---
  drivers/staging/comedi/drivers/cb_pcidas64.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index fdd81c3..631a703 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -2268,14 +2268,14 @@ static inline unsigned int dma_transfer_size(struct 
comedi_device *dev)
  }
  
  static u32 ai_convert_counter_6xxx(const struct comedi_device *dev,

-   const struct comedi_cmd *cmd)
+  const struct comedi_cmd *cmd)
  {
/* supposed to load counter with desired divisor minus 3 */
return cmd->convert_arg / TIMER_BASE - 3;
  }
  
  static u32 ai_scan_counter_6xxx(struct comedi_device *dev,

-struct comedi_cmd *cmd)
+   struct comedi_cmd *cmd)
  {
u32 count;
  
@@ -2296,7 +2296,7 @@ static u32 ai_scan_counter_6xxx(struct comedi_device *dev,

  }
  
  static u32 ai_convert_counter_4020(struct comedi_device *dev,

-   struct comedi_cmd *cmd)
+  struct comedi_cmd *cmd)
  {
struct pcidas64_private *devpriv = dev->private;
unsigned int divisor;



Looks good, thanks.  I guess the remaining case of open parentheses 
alignment wasn't changed because that would exceed 80 columns.


Reviewed-by: Ian Abbott 

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

Re: [PATCH] staging: comedi: cb_pcidas64: fix alignment of function parameters

[Ian Abbott]

On 11/04/18 06:07, Julia Lawall wrote:

On Tue, 10 Apr 2018, Gabriel Francisco Mandaji wrote:

[snip]

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index fdd81c3..631a703 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -2268,14 +2268,14 @@ static inline unsigned int dma_transfer_size(struct 
comedi_device *dev)
  }

  static u32 ai_convert_counter_6xxx(const struct comedi_device *dev,
-   const struct comedi_cmd *cmd)
+  const struct comedi_cmd *cmd)


Someone has made the effort to put const on these parameters, but not on
the other similar functions.  Perhaps this can be improved as well.


Perhaps, but that should be in a separate patch.

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

Re: [PATCH] staging: comedi: cb_pcidas64: fix alignment of function parameters

[Ian Abbott]

On 11/04/18 06:07, Julia Lawall wrote:

On Tue, 10 Apr 2018, Gabriel Francisco Mandaji wrote:

[snip]

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index fdd81c3..631a703 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -2268,14 +2268,14 @@ static inline unsigned int dma_transfer_size(struct 
comedi_device *dev)
  }

  static u32 ai_convert_counter_6xxx(const struct comedi_device *dev,
-   const struct comedi_cmd *cmd)
+  const struct comedi_cmd *cmd)


Someone has made the effort to put const on these parameters, but not on
the other similar functions.  Perhaps this can be improved as well.


Perhaps, but that should be in a separate patch.

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

Re: [PATCH] Staging: comedi: drivers: ni_atmio.c: fixed multi-line derefernce issue

[Ian Abbott]

On 16/03/2018 10:48, Pratik Jain wrote:

Fixed coding style issue.

Signed-off-by: Pratik Jain <pratik.jain0...@gmail.com>
---
  drivers/staging/comedi/drivers/ni_atmio.c | 9 +
  1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c
index b9e9ab548c4b..2b7bfe0dd7f3 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -224,10 +224,11 @@ static int ni_isapnp_find_board(struct pnp_dev **dev)
int i;
  
  	for (i = 0; i < ARRAY_SIZE(ni_boards); i++) {

-   isapnp_dev = pnp_find_dev(NULL,
- ISAPNP_VENDOR('N', 'I', 'C'),
- ISAPNP_FUNCTION(ni_boards[i].
- isapnp_id), NULL);
+   isapnp_dev =
+   pnp_find_dev(NULL,
+ISAPNP_VENDOR('N', 'I', 'C'),
+ISAPNP_FUNCTION(ni_boards[i].isapnp_id),
+NULL);
  
  		if (!isapnp_dev || !isapnp_dev->card)

continue;



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] Staging: comedi: drivers: ni_atmio.c: fixed multi-line derefernce issue

[Ian Abbott]

On 16/03/2018 10:48, Pratik Jain wrote:

Fixed coding style issue.

Signed-off-by: Pratik Jain 
---
  drivers/staging/comedi/drivers/ni_atmio.c | 9 +
  1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c
index b9e9ab548c4b..2b7bfe0dd7f3 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -224,10 +224,11 @@ static int ni_isapnp_find_board(struct pnp_dev **dev)
int i;
  
  	for (i = 0; i < ARRAY_SIZE(ni_boards); i++) {

-   isapnp_dev = pnp_find_dev(NULL,
- ISAPNP_VENDOR('N', 'I', 'C'),
- ISAPNP_FUNCTION(ni_boards[i].
- isapnp_id), NULL);
+   isapnp_dev =
+   pnp_find_dev(NULL,
+ISAPNP_VENDOR('N', 'I', 'C'),
+ISAPNP_FUNCTION(ni_boards[i].isapnp_id),
+NULL);
  
  		if (!isapnp_dev || !isapnp_dev->card)

continue;



Looks good, thanks!

Reviewed-by: Ian Abbott 

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

Re: [PATCH] Staging: comedi: drivers: ni_atmio.c: fixed multi-line derefernce issue

[Ian Abbott]

On 15/03/2018 18:59, Pratik Jain wrote:

Fixed coding style issue.

Signed-off-by: Pratik Jain <pratik.jain0...@gmail.com>
---
  drivers/staging/comedi/drivers/ni_atmio.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c
index b9e9ab548c4b..4e27a2959b64 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -226,8 +226,8 @@ static int ni_isapnp_find_board(struct pnp_dev **dev)
for (i = 0; i < ARRAY_SIZE(ni_boards); i++) {
isapnp_dev = pnp_find_dev(NULL,
  ISAPNP_VENDOR('N', 'I', 'C'),
- ISAPNP_FUNCTION(ni_boards[i].
- isapnp_id), NULL);
+ 
ISAPNP_FUNCTION(ni_boards[i].isapnp_id),
+ NULL);
  
  		if (!isapnp_dev || !isapnp_dev->card)

continue;



I suggest splitting the expression just after the '=' to avoid going 
over 80 columns.


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

Re: [PATCH] Staging: comedi: drivers: ni_atmio.c: fixed multi-line derefernce issue

[Ian Abbott]

On 15/03/2018 18:59, Pratik Jain wrote:

Fixed coding style issue.

Signed-off-by: Pratik Jain 
---
  drivers/staging/comedi/drivers/ni_atmio.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c
index b9e9ab548c4b..4e27a2959b64 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -226,8 +226,8 @@ static int ni_isapnp_find_board(struct pnp_dev **dev)
for (i = 0; i < ARRAY_SIZE(ni_boards); i++) {
isapnp_dev = pnp_find_dev(NULL,
  ISAPNP_VENDOR('N', 'I', 'C'),
- ISAPNP_FUNCTION(ni_boards[i].
- isapnp_id), NULL);
+ 
ISAPNP_FUNCTION(ni_boards[i].isapnp_id),
+ NULL);
  
  		if (!isapnp_dev || !isapnp_dev->card)

continue;



I suggest splitting the expression just after the '=' to avoid going 
over 80 columns.


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

Re: [PATCH] staging: comedi: adl_pci6208: remove redundant initialization of 'val'

[Ian Abbott]

On 12/03/18 23:36, Colin King wrote:

From: Colin Ian King <colin.k...@canonical.com>

Variable 'val' is initialized with a value that is never read, it is
updated with a new value again after intitialization. Remove the
redundant initialization and move the declaration and assignment into
the scope of the for-loop.

Cleans up clang warning:
drivers/staging/comedi/drivers/adl_pci6208.c:61:15: warning: Value
stored to 'val' during its initialization is never read

Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
  drivers/staging/comedi/drivers/adl_pci6208.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/adl_pci6208.c 
b/drivers/staging/comedi/drivers/adl_pci6208.c
index d39b4eabce8d..e21840e9002d 100644
--- a/drivers/staging/comedi/drivers/adl_pci6208.c
+++ b/drivers/staging/comedi/drivers/adl_pci6208.c
@@ -58,12 +58,11 @@ static int pci6208_ao_insn_write(struct comedi_device *dev,
 unsigned int *data)
  {
unsigned int chan = CR_CHAN(insn->chanspec);
-   unsigned int val = s->readback[chan];
int ret;
int i;
  
  	for (i = 0; i < insn->n; i++) {

-   val = data[i];
+   unsigned int val = data[i];
  
  		/* D/A transfer rate is 2.2us */

ret = comedi_timeout(dev, s, insn, pci6208_ao_eoc, 0);



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] staging: comedi: adl_pci6208: remove redundant initialization of 'val'

[Ian Abbott]

On 12/03/18 23:36, Colin King wrote:

From: Colin Ian King 

Variable 'val' is initialized with a value that is never read, it is
updated with a new value again after intitialization. Remove the
redundant initialization and move the declaration and assignment into
the scope of the for-loop.

Cleans up clang warning:
drivers/staging/comedi/drivers/adl_pci6208.c:61:15: warning: Value
stored to 'val' during its initialization is never read

Signed-off-by: Colin Ian King 
---
  drivers/staging/comedi/drivers/adl_pci6208.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/adl_pci6208.c 
b/drivers/staging/comedi/drivers/adl_pci6208.c
index d39b4eabce8d..e21840e9002d 100644
--- a/drivers/staging/comedi/drivers/adl_pci6208.c
+++ b/drivers/staging/comedi/drivers/adl_pci6208.c
@@ -58,12 +58,11 @@ static int pci6208_ao_insn_write(struct comedi_device *dev,
 unsigned int *data)
  {
unsigned int chan = CR_CHAN(insn->chanspec);
-   unsigned int val = s->readback[chan];
int ret;
int i;
  
  	for (i = 0; i < insn->n; i++) {

-   val = data[i];
+   unsigned int val = data[i];
  
  		/* D/A transfer rate is 2.2us */

ret = comedi_timeout(dev, s, insn, pci6208_ao_eoc, 0);



Looks good, thanks!

Reviewed-by: Ian Abbott 

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

Re: [PATCH] Staging: comedi: das16: Fixed a const struct coding style issue

[Ian Abbott]

On 26/11/17 01:50, Alex Frappier Lachapelle wrote:

Fixed a coding style issue.

Signed-off-by: Alex Frappier Lachapelle <alex.frappierlachape...@gmail.com>
---
  drivers/staging/comedi/drivers/das16.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/das16.c 
b/drivers/staging/comedi/drivers/das16.c
index ddd4aeab6365..ed1e9e9b651d 100644
--- a/drivers/staging/comedi/drivers/das16.c
+++ b/drivers/staging/comedi/drivers/das16.c
@@ -967,7 +967,7 @@ static const struct comedi_lrange *das16_ai_range(struct 
comedi_device *dev,
  
  	/* get any user-defined input range */

if (pg_type == das16_pg_none && (min || max)) {
-   struct comedi_lrange *lrange;
+   const struct comedi_lrange *lrange;
struct comedi_krange *krange;
  
  		/* allocate single-range range table */


NAK.  The following lines of source code allocate memory pointed to by 
'lrange' and modify it, so 'const' is not appropriate here.



@@ -1001,7 +1001,7 @@ static const struct comedi_lrange *das16_ao_range(struct 
comedi_device *dev,
  
  	/* get any user-defined output range */

if (min || max) {
-   struct comedi_lrange *lrange;
+   const struct comedi_lrange *lrange;
struct comedi_krange *krange;
  
  		/* allocate single-range range table */




Ditto.

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

Re: [PATCH] Staging: comedi: das16: Fixed a const struct coding style issue

[Ian Abbott]

On 26/11/17 01:50, Alex Frappier Lachapelle wrote:

Fixed a coding style issue.

Signed-off-by: Alex Frappier Lachapelle 
---
  drivers/staging/comedi/drivers/das16.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/das16.c 
b/drivers/staging/comedi/drivers/das16.c
index ddd4aeab6365..ed1e9e9b651d 100644
--- a/drivers/staging/comedi/drivers/das16.c
+++ b/drivers/staging/comedi/drivers/das16.c
@@ -967,7 +967,7 @@ static const struct comedi_lrange *das16_ai_range(struct 
comedi_device *dev,
  
  	/* get any user-defined input range */

if (pg_type == das16_pg_none && (min || max)) {
-   struct comedi_lrange *lrange;
+   const struct comedi_lrange *lrange;
struct comedi_krange *krange;
  
  		/* allocate single-range range table */


NAK.  The following lines of source code allocate memory pointed to by 
'lrange' and modify it, so 'const' is not appropriate here.



@@ -1001,7 +1001,7 @@ static const struct comedi_lrange *das16_ao_range(struct 
comedi_device *dev,
  
  	/* get any user-defined output range */

if (min || max) {
-   struct comedi_lrange *lrange;
+   const struct comedi_lrange *lrange;
struct comedi_krange *krange;
  
  		/* allocate single-range range table */




Ditto.

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

Re: [PATCH] Staging: comedi: adl_pci9118.c : fixed code style issue

[Ian Abbott]

On 18/11/17 17:46, Fabian Baumanis wrote:

Removed uneccessary parantheses which were sorrounding two
if-statements.


There is only one 'if' statement changed by the patch.  Apart from that 
(and the typos in the patch description), the patch is fine as far as it 
goes.  However, the checkpatch script identifies several similar 
"unnecessary parentheses" issues in this file, and it would be better to 
take care of them all with a single patch rather than patching them one 
at a time.




Signed-off-by: Fabian Baumanis <bauman...@gmail.com>
---
  drivers/staging/comedi/drivers/adl_pci9118.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/adl_pci9118.c 
b/drivers/staging/comedi/drivers/adl_pci9118.c
index 1cc9b7e..c77b994 100644
--- a/drivers/staging/comedi/drivers/adl_pci9118.c
+++ b/drivers/staging/comedi/drivers/adl_pci9118.c
@@ -946,8 +946,7 @@ static int pci9118_ai_cmd(struct comedi_device *dev, struct 
comedi_subdevice *s)
     devpriv->ai_add_back = 0;
     if (devpriv->master) {
     devpriv->usedma = 1;
-   if ((cmd->flags & CMDF_WAKE_EOS) &&
-   (cmd->scan_end_arg == 1)) {
+   if (cmd->flags & CMDF_WAKE_EOS && cmd->scan_end_arg == 1) {
     if (cmd->convert_src == TRIG_NOW)
     devpriv->ai_add_back = 1;
             if (cmd->convert_src == TRIG_TIMER) {




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

Re: [PATCH] Staging: comedi: adl_pci9118.c : fixed code style issue

[Ian Abbott]

On 18/11/17 17:46, Fabian Baumanis wrote:

Removed uneccessary parantheses which were sorrounding two
if-statements.


There is only one 'if' statement changed by the patch.  Apart from that 
(and the typos in the patch description), the patch is fine as far as it 
goes.  However, the checkpatch script identifies several similar 
"unnecessary parentheses" issues in this file, and it would be better to 
take care of them all with a single patch rather than patching them one 
at a time.




Signed-off-by: Fabian Baumanis 
---
  drivers/staging/comedi/drivers/adl_pci9118.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/adl_pci9118.c 
b/drivers/staging/comedi/drivers/adl_pci9118.c
index 1cc9b7e..c77b994 100644
--- a/drivers/staging/comedi/drivers/adl_pci9118.c
+++ b/drivers/staging/comedi/drivers/adl_pci9118.c
@@ -946,8 +946,7 @@ static int pci9118_ai_cmd(struct comedi_device *dev, struct 
comedi_subdevice *s)
     devpriv->ai_add_back = 0;
     if (devpriv->master) {
     devpriv->usedma = 1;
-   if ((cmd->flags & CMDF_WAKE_EOS) &&
-   (cmd->scan_end_arg == 1)) {
+   if (cmd->flags & CMDF_WAKE_EOS && cmd->scan_end_arg == 1) {
     if (cmd->convert_src == TRIG_NOW)
     devpriv->ai_add_back = 1;
         if (cmd->convert_src == TRIG_TIMER) {




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

Re: [PATCH v2] staging: comedi: add missing MODULE_DESCRIPTION/LICENSE

[Ian Abbott]

On 20/11/17 10:29, Ian Abbott wrote:

On 20/11/17 07:50, Jesse Chan wrote:

This change resolves a new compile-time warning
when built as a loadable module:

WARNING: modpost: missing MODULE_LICENSE() in 
drivers/staging/comedi/drivers/ni_atmio.o

see include/linux/module.h for more information

This adds the license as "GPL", which matches the header of the file.

MODULE_DESCRIPTION is also added.

Signed-off-by: Jesse Chan <j...@linux.com>
---
  drivers/staging/comedi/drivers/ni_atmio.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c

index 2d62a8c57332..b61d56367773 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -361,3 +361,6 @@ static struct comedi_driver ni_atmio_driver = {
  .detach    = ni_atmio_detach,
  };
  module_comedi_driver(ni_atmio_driver);
+
+MODULE_DESCRIPTION("Comedi low-level driver");
+MODULE_LICENSE("GPL");


Thanks!  I wonder how I managed to miss out this driver in commit 
3c323c01b6bd ("Staging: comedi: Add MODULE_LICENSE and similar to NI 
modules")?


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


Despite my above comment, we should probably give precedence to Matthew 
Giassa's patch for the same issue, since it was sent earlier.


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

Re: [PATCH v2] staging: comedi: add missing MODULE_DESCRIPTION/LICENSE

[Ian Abbott]

On 20/11/17 10:29, Ian Abbott wrote:

On 20/11/17 07:50, Jesse Chan wrote:

This change resolves a new compile-time warning
when built as a loadable module:

WARNING: modpost: missing MODULE_LICENSE() in 
drivers/staging/comedi/drivers/ni_atmio.o

see include/linux/module.h for more information

This adds the license as "GPL", which matches the header of the file.

MODULE_DESCRIPTION is also added.

Signed-off-by: Jesse Chan 
---
  drivers/staging/comedi/drivers/ni_atmio.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c

index 2d62a8c57332..b61d56367773 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -361,3 +361,6 @@ static struct comedi_driver ni_atmio_driver = {
  .detach    = ni_atmio_detach,
  };
  module_comedi_driver(ni_atmio_driver);
+
+MODULE_DESCRIPTION("Comedi low-level driver");
+MODULE_LICENSE("GPL");


Thanks!  I wonder how I managed to miss out this driver in commit 
3c323c01b6bd ("Staging: comedi: Add MODULE_LICENSE and similar to NI 
modules")?


Reviewed-by: Ian Abbott 


Despite my above comment, we should probably give precedence to Matthew 
Giassa's patch for the same issue, since it was sent earlier.


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

Re: [PATCH] staging: comedi: ni_atmio: fix license warning.

[Ian Abbott]

On 18/11/17 16:26, Matthew Giassa wrote:

Resolving license check warning for drivers/staging/comedi. Added the
license definitions present in the rest of the module and made sure it's
aligned with the license (GPL) in the comments for the affected file
(ni_atmio.c). Original warning:

WARNING: modpost: missing MODULE_LICENSE() in 
drivers/staging/comedi//drivers/ni_atmio.o
see include/linux/module.h for more information.

No longer present after change.

Signed-off-by: Matthew Giassa <matt...@giassa.net>
---
  drivers/staging/comedi/drivers/ni_atmio.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c
index 2d62a8c..ae6ed96 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -361,3 +361,8 @@ static struct comedi_driver ni_atmio_driver = {
.detach = ni_atmio_detach,
  };
  module_comedi_driver(ni_atmio_driver);
+
+MODULE_AUTHOR("Comedi http://www.comedi.org;);
+MODULE_DESCRIPTION("Comedi low-level driver");
+MODULE_LICENSE("GPL");
+



Thanks.  I already reviewed a similar patch for the same driver by Jesse 
Chan, but that one never had the MODULE_AUTHOR line.  Also, your patch 
arrived earlier, so should take precedence.


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_atmio: fix license warning.

[Ian Abbott]

On 18/11/17 16:26, Matthew Giassa wrote:

Resolving license check warning for drivers/staging/comedi. Added the
license definitions present in the rest of the module and made sure it's
aligned with the license (GPL) in the comments for the affected file
(ni_atmio.c). Original warning:

WARNING: modpost: missing MODULE_LICENSE() in 
drivers/staging/comedi//drivers/ni_atmio.o
see include/linux/module.h for more information.

No longer present after change.

Signed-off-by: Matthew Giassa 
---
  drivers/staging/comedi/drivers/ni_atmio.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c
index 2d62a8c..ae6ed96 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -361,3 +361,8 @@ static struct comedi_driver ni_atmio_driver = {
.detach = ni_atmio_detach,
  };
  module_comedi_driver(ni_atmio_driver);
+
+MODULE_AUTHOR("Comedi http://www.comedi.org;);
+MODULE_DESCRIPTION("Comedi low-level driver");
+MODULE_LICENSE("GPL");
+



Thanks.  I already reviewed a similar patch for the same driver by Jesse 
Chan, but that one never had the MODULE_AUTHOR line.  Also, your patch 
arrived earlier, so should take precedence.


Reviewed-by: Ian Abbott 

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

Re: [PATCH v2] staging: comedi: add missing MODULE_DESCRIPTION/LICENSE

[Ian Abbott]

On 20/11/17 07:50, Jesse Chan wrote:

This change resolves a new compile-time warning
when built as a loadable module:

WARNING: modpost: missing MODULE_LICENSE() in 
drivers/staging/comedi/drivers/ni_atmio.o
see include/linux/module.h for more information

This adds the license as "GPL", which matches the header of the file.

MODULE_DESCRIPTION is also added.

Signed-off-by: Jesse Chan <j...@linux.com>
---
  drivers/staging/comedi/drivers/ni_atmio.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c
index 2d62a8c57332..b61d56367773 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -361,3 +361,6 @@ static struct comedi_driver ni_atmio_driver = {
.detach = ni_atmio_detach,
  };
  module_comedi_driver(ni_atmio_driver);
+
+MODULE_DESCRIPTION("Comedi low-level driver");
+MODULE_LICENSE("GPL");


Thanks!  I wonder how I managed to miss out this driver in commit 
3c323c01b6bd ("Staging: comedi: Add MODULE_LICENSE and similar to NI 
modules")?


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: add missing MODULE_DESCRIPTION/LICENSE

[Ian Abbott]

On 20/11/17 07:50, Jesse Chan wrote:

This change resolves a new compile-time warning
when built as a loadable module:

WARNING: modpost: missing MODULE_LICENSE() in 
drivers/staging/comedi/drivers/ni_atmio.o
see include/linux/module.h for more information

This adds the license as "GPL", which matches the header of the file.

MODULE_DESCRIPTION is also added.

Signed-off-by: Jesse Chan 
---
  drivers/staging/comedi/drivers/ni_atmio.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c
index 2d62a8c57332..b61d56367773 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -361,3 +361,6 @@ static struct comedi_driver ni_atmio_driver = {
.detach = ni_atmio_detach,
  };
  module_comedi_driver(ni_atmio_driver);
+
+MODULE_DESCRIPTION("Comedi low-level driver");
+MODULE_LICENSE("GPL");


Thanks!  I wonder how I managed to miss out this driver in commit 
3c323c01b6bd ("Staging: comedi: Add MODULE_LICENSE and similar to NI 
modules")?


Reviewed-by: Ian Abbott 

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

Re: [PATCH] staging: comedi: ni_atmio: Handle return value of pnp_*

[Ian Abbott]

On 16/11/17 04:32, Arvind Yadav wrote:

pnp_irq() and pnp_port_start() can fail here and we must check
its return value.

Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>
---
  drivers/staging/comedi/drivers/ni_atmio.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c
index 2d62a8c..dead159 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -308,6 +308,9 @@ static int ni_atmio_attach(struct comedi_device *dev,
  
  		iobase = pnp_port_start(isapnp_dev, 0);

irq = pnp_irq(isapnp_dev, 0);
+   if (irq == -1 || !iobase)
+   return -ENOMEM;
+
comedi_set_hw_dev(dev, _dev->dev);
}
  



Can they fail here?  ni_isapnp_find_board() has already checked they are 
valid.


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

Re: [PATCH] staging: comedi: ni_atmio: Handle return value of pnp_*

[Ian Abbott]

On 16/11/17 04:32, Arvind Yadav wrote:

pnp_irq() and pnp_port_start() can fail here and we must check
its return value.

Signed-off-by: Arvind Yadav 
---
  drivers/staging/comedi/drivers/ni_atmio.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c 
b/drivers/staging/comedi/drivers/ni_atmio.c
index 2d62a8c..dead159 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -308,6 +308,9 @@ static int ni_atmio_attach(struct comedi_device *dev,
  
  		iobase = pnp_port_start(isapnp_dev, 0);

irq = pnp_irq(isapnp_dev, 0);
+   if (irq == -1 || !iobase)
+   return -ENOMEM;
+
comedi_set_hw_dev(dev, _dev->dev);
}
  



Can they fail here?  ni_isapnp_find_board() has already checked they are 
valid.


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

Re: [PATCH] staging: comedi: usbdux: remove redundant initialization of val

[Ian Abbott]

On 07/11/17 19:07, Colin King wrote:

From: Colin Ian King <colin.k...@canonical.com>

The early initialization of val is redundant as the value is never
read and is updated inside a for-loop. Remove the initialization
and move the declaration and initialization to the for-loop scope.
Cleans up clang warning:

drivers/staging/comedi/drivers/usbdux.c:812:15: warning: Value stored
to 'val' during its initialization is never read

Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
  drivers/staging/comedi/drivers/usbdux.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbdux.c 
b/drivers/staging/comedi/drivers/usbdux.c
index f4f05d29d30d..ede064b47aac 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -809,7 +809,6 @@ static int usbdux_ao_insn_write(struct comedi_device *dev,
  {
struct usbdux_private *devpriv = dev->private;
unsigned int chan = CR_CHAN(insn->chanspec);
-   unsigned int val = s->readback[chan];
__le16 *p = (__le16 *)>dux_commands[2];
int ret = -EBUSY;
int i;
@@ -825,7 +824,7 @@ static int usbdux_ao_insn_write(struct comedi_device *dev,
devpriv->dux_commands[4] = chan << 6;
  
  	for (i = 0; i < insn->n; i++) {

-   val = data[i];
+   unsigned int val = data[i];
  
  		/* one 16 bit value */

*p = cpu_to_le16(val);



Thanks for catching that.  It looks like it has been redundant since 
commit 65a847477f63 ("staging: comedi: usbdux: use comedi_subdevice 
'readback'").


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: usbdux: remove redundant initialization of val

[Ian Abbott]

On 07/11/17 19:07, Colin King wrote:

From: Colin Ian King 

The early initialization of val is redundant as the value is never
read and is updated inside a for-loop. Remove the initialization
and move the declaration and initialization to the for-loop scope.
Cleans up clang warning:

drivers/staging/comedi/drivers/usbdux.c:812:15: warning: Value stored
to 'val' during its initialization is never read

Signed-off-by: Colin Ian King 
---
  drivers/staging/comedi/drivers/usbdux.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbdux.c 
b/drivers/staging/comedi/drivers/usbdux.c
index f4f05d29d30d..ede064b47aac 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -809,7 +809,6 @@ static int usbdux_ao_insn_write(struct comedi_device *dev,
  {
struct usbdux_private *devpriv = dev->private;
unsigned int chan = CR_CHAN(insn->chanspec);
-   unsigned int val = s->readback[chan];
__le16 *p = (__le16 *)>dux_commands[2];
int ret = -EBUSY;
int i;
@@ -825,7 +824,7 @@ static int usbdux_ao_insn_write(struct comedi_device *dev,
devpriv->dux_commands[4] = chan << 6;
  
  	for (i = 0; i < insn->n; i++) {

-   val = data[i];
+   unsigned int val = data[i];
  
  		/* one 16 bit value */

*p = cpu_to_le16(val);



Thanks for catching that.  It looks like it has been redundant since 
commit 65a847477f63 ("staging: comedi: usbdux: use comedi_subdevice 
'readback'").


Reviewed-by: Ian Abbott 

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

Re: [PATCH] staging: comedi: usbduxsigma: Improve unlocking of a mutex in three functions

[Ian Abbott]

On 02/11/17 20:23, SF Markus Elfring wrote:

From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Thu, 2 Nov 2017 21:16:50 +0100

* Add a jump target so that a call of the function "mutex_unlock" is stored
   only twice in these function implementations.

* Replace seven calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
---
  drivers/staging/comedi/drivers/usbduxsigma.c | 48 +++-
  1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c 
b/drivers/staging/comedi/drivers/usbduxsigma.c
index 456e9f13becb..7e8284ed265a 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -672,10 +672,8 @@ static int usbduxsigma_ai_cmd(struct comedi_device *dev,
devpriv->dux_commands[8] = sysred;
  
  	ret = usbbuxsigma_send_cmd(dev, USBBUXSIGMA_AD_CMD);

-   if (ret < 0) {
-   mutex_unlock(>mut);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
  
  	devpriv->ai_counter = devpriv->ai_timer;
  
@@ -686,8 +684,7 @@ static int usbduxsigma_ai_cmd(struct comedi_device *dev,

  devpriv->n_ai_urbs, 1);
if (ret < 0) {
devpriv->ai_cmd_running = 0;
-   mutex_unlock(>mut);
-   return ret;
+   goto unlock;
}
s->async->inttrig = NULL;
} else {/* TRIG_INT */
@@ -697,6 +694,10 @@ static int usbduxsigma_ai_cmd(struct comedi_device *dev,
mutex_unlock(>mut);
  
  	return 0;


You could also remove that call by replacing the above three lines with:

ret = 0;

and falling through to the 'unlock:' label.  (Actually, you don't even 
need the above line, as ret should already be 0 at this point, but I 
think it is easier to understand the code if ret is set to 0 explicitly.)



+
+unlock:
+   mutex_unlock(>mut);
+   return ret;
  }
  
  static int usbduxsigma_ai_insn_read(struct comedi_device *dev,

@@ -714,8 +715,8 @@ static int usbduxsigma_ai_insn_read(struct comedi_device 
*dev,
  
  	mutex_lock(>mut);

if (devpriv->ai_cmd_running) {
-   mutex_unlock(>mut);
-   return -EBUSY;
+   ret = -EBUSY;
+   goto unlock;
}
  
  	create_adc_command(chan, , );

@@ -730,19 +731,15 @@ static int usbduxsigma_ai_insn_read(struct comedi_device 
*dev,
  
  	/* adc commands */

ret = usbbuxsigma_send_cmd(dev, USBDUXSIGMA_SINGLE_AD_CMD);
-   if (ret < 0) {
-   mutex_unlock(>mut);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
  
  	for (i = 0; i < insn->n; i++) {

u32 val;
  
  		ret = usbduxsigma_receive_cmd(dev, USBDUXSIGMA_SINGLE_AD_CMD);

-   if (ret < 0) {
-   mutex_unlock(>mut);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
  
  		/* 32 bits big endian from the A/D converter */

val = be32_to_cpu(get_unaligned((__be32
@@ -753,6 +750,10 @@ static int usbduxsigma_ai_insn_read(struct comedi_device 
*dev,
mutex_unlock(>mut);
  
  	return insn->n;


Similarly, you could replace the above three lines with:

ret = insn->n;

and fall through to the label.


+
+unlock:
+   mutex_unlock(>mut);
+   return ret;
  }
  
  static int usbduxsigma_ao_insn_read(struct comedi_device *dev,

@@ -782,8 +783,8 @@ static int usbduxsigma_ao_insn_write(struct comedi_device 
*dev,
  
  	mutex_lock(>mut);

if (devpriv->ao_cmd_running) {
-   mutex_unlock(>mut);
-   return -EBUSY;
+   ret = -EBUSY;
+   goto unlock;
}
  
  	for (i = 0; i < insn->n; i++) {

@@ -791,15 +792,18 @@ static int usbduxsigma_ao_insn_write(struct comedi_device 
*dev,
devpriv->dux_commands[2] = data[i];  /* value */
devpriv->dux_commands[3] = chan; /* channel number */
ret = usbbuxsigma_send_cmd(dev, USBDUXSIGMA_DA_CMD);
-   if (ret < 0) {
-   mutex_unlock(>mut);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
s->readback[chan] = data[i];
}
mutex_unlock(>mut);
  
  	return insn->n;


Ditto.


+
+unlock:
+   mutex_unlock(>mut);
+   return ret;
  }
  
  static int usbduxsigma_ao_inttrig(struct comedi_device *dev,





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

Re: [PATCH] staging: comedi: usbduxsigma: Improve unlocking of a mutex in three functions

[Ian Abbott]

On 02/11/17 20:23, SF Markus Elfring wrote:

From: Markus Elfring 
Date: Thu, 2 Nov 2017 21:16:50 +0100

* Add a jump target so that a call of the function "mutex_unlock" is stored
   only twice in these function implementations.

* Replace seven calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
  drivers/staging/comedi/drivers/usbduxsigma.c | 48 +++-
  1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c 
b/drivers/staging/comedi/drivers/usbduxsigma.c
index 456e9f13becb..7e8284ed265a 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -672,10 +672,8 @@ static int usbduxsigma_ai_cmd(struct comedi_device *dev,
devpriv->dux_commands[8] = sysred;
  
  	ret = usbbuxsigma_send_cmd(dev, USBBUXSIGMA_AD_CMD);

-   if (ret < 0) {
-   mutex_unlock(>mut);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
  
  	devpriv->ai_counter = devpriv->ai_timer;
  
@@ -686,8 +684,7 @@ static int usbduxsigma_ai_cmd(struct comedi_device *dev,

  devpriv->n_ai_urbs, 1);
if (ret < 0) {
devpriv->ai_cmd_running = 0;
-   mutex_unlock(>mut);
-   return ret;
+   goto unlock;
}
s->async->inttrig = NULL;
} else {/* TRIG_INT */
@@ -697,6 +694,10 @@ static int usbduxsigma_ai_cmd(struct comedi_device *dev,
mutex_unlock(>mut);
  
  	return 0;


You could also remove that call by replacing the above three lines with:

ret = 0;

and falling through to the 'unlock:' label.  (Actually, you don't even 
need the above line, as ret should already be 0 at this point, but I 
think it is easier to understand the code if ret is set to 0 explicitly.)



+
+unlock:
+   mutex_unlock(>mut);
+   return ret;
  }
  
  static int usbduxsigma_ai_insn_read(struct comedi_device *dev,

@@ -714,8 +715,8 @@ static int usbduxsigma_ai_insn_read(struct comedi_device 
*dev,
  
  	mutex_lock(>mut);

if (devpriv->ai_cmd_running) {
-   mutex_unlock(>mut);
-   return -EBUSY;
+   ret = -EBUSY;
+   goto unlock;
}
  
  	create_adc_command(chan, , );

@@ -730,19 +731,15 @@ static int usbduxsigma_ai_insn_read(struct comedi_device 
*dev,
  
  	/* adc commands */

ret = usbbuxsigma_send_cmd(dev, USBDUXSIGMA_SINGLE_AD_CMD);
-   if (ret < 0) {
-   mutex_unlock(>mut);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
  
  	for (i = 0; i < insn->n; i++) {

u32 val;
  
  		ret = usbduxsigma_receive_cmd(dev, USBDUXSIGMA_SINGLE_AD_CMD);

-   if (ret < 0) {
-   mutex_unlock(>mut);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
  
  		/* 32 bits big endian from the A/D converter */

val = be32_to_cpu(get_unaligned((__be32
@@ -753,6 +750,10 @@ static int usbduxsigma_ai_insn_read(struct comedi_device 
*dev,
mutex_unlock(>mut);
  
  	return insn->n;


Similarly, you could replace the above three lines with:

ret = insn->n;

and fall through to the label.


+
+unlock:
+   mutex_unlock(>mut);
+   return ret;
  }
  
  static int usbduxsigma_ao_insn_read(struct comedi_device *dev,

@@ -782,8 +783,8 @@ static int usbduxsigma_ao_insn_write(struct comedi_device 
*dev,
  
  	mutex_lock(>mut);

if (devpriv->ao_cmd_running) {
-   mutex_unlock(>mut);
-   return -EBUSY;
+   ret = -EBUSY;
+   goto unlock;
}
  
  	for (i = 0; i < insn->n; i++) {

@@ -791,15 +792,18 @@ static int usbduxsigma_ao_insn_write(struct comedi_device 
*dev,
devpriv->dux_commands[2] = data[i];  /* value */
devpriv->dux_commands[3] = chan; /* channel number */
ret = usbbuxsigma_send_cmd(dev, USBDUXSIGMA_DA_CMD);
-   if (ret < 0) {
-   mutex_unlock(>mut);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
s->readback[chan] = data[i];
}
mutex_unlock(>mut);
  
  	return insn->n;


Ditto.


+
+unlock:
+   mutex_unlock(>mut);
+   return ret;
  }
  
  static int usbduxsigma_ao_inttrig(struct comedi_device *dev,





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

Re: [PATCH] staging: comedi: usbduxfast: Improve unlocking of a mutex in usbduxfast_ai_insn_read()

[Ian Abbott]

On 02/11/17 19:40, SF Markus Elfring wrote:

From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Thu, 2 Nov 2017 20:30:31 +0100

* Add a jump target so that a call of the function "mutex_unlock" is stored
   only twice in this function implementation.

* Replace five calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
---
  drivers/staging/comedi/drivers/usbduxfast.c | 24 
  1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxfast.c 
b/drivers/staging/comedi/drivers/usbduxfast.c
index 608403c7586b..e5884faf7275 100644
--- a/drivers/staging/comedi/drivers/usbduxfast.c
+++ b/drivers/staging/comedi/drivers/usbduxfast.c
@@ -777,8 +777,8 @@ static int usbduxfast_ai_insn_read(struct comedi_device 
*dev,
if (devpriv->ai_cmd_running) {
dev_err(dev->class_dev,
"ai_insn_read not possible, async cmd is running\n");
-   mutex_unlock(>mut);
-   return -EBUSY;
+   ret = -EBUSY;
+   goto unlock;
}
  
  	/* set command for the first channel */

@@ -798,10 +798,8 @@ static int usbduxfast_ai_insn_read(struct comedi_device 
*dev,
usbduxfast_cmd_data(dev, 6, 0x01, 0x00, rngmask, 0x00);
  
  	ret = usbduxfast_send_cmd(dev, SENDADCOMMANDS);

-   if (ret < 0) {
-   mutex_unlock(>mut);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
  
  	for (i = 0; i < PACKETS_TO_IGNORE; i++) {

ret = usb_bulk_msg(usb, usb_rcvbulkpipe(usb, BULKINEP),
@@ -809,8 +807,7 @@ static int usbduxfast_ai_insn_read(struct comedi_device 
*dev,
   _length, 1);
if (ret < 0) {
dev_err(dev->class_dev, "insn timeout, no data\n");
-   mutex_unlock(>mut);
-   return ret;
+   goto unlock;
}
}
  
@@ -820,14 +817,13 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,

   _length, 1);
if (ret < 0) {
dev_err(dev->class_dev, "insn data error: %d\n", ret);
-   mutex_unlock(>mut);
-   return ret;
+   goto unlock;
}
n = actual_length / sizeof(u16);
if ((n % 16) != 0) {
dev_err(dev->class_dev, "insn data packet corrupted\n");
-   mutex_unlock(>mut);
-   return -EINVAL;
+   ret = -EINVAL;
+   goto unlock;
}
for (j = chan; (j < n) && (i < insn->n); j = j + 16) {
data[i] = ((u16 *)(devpriv->inbuf))[j];
@@ -838,6 +834,10 @@ static int usbduxfast_ai_insn_read(struct comedi_device 
*dev,
mutex_unlock(>mut);
  
  	return insn->n;


Minor niggle: You could also remove that call to mutex_unlock() by 
replacing the above three lines with:


ret = insn->n;

which will fall through to the 'unlock:' label below.


+
+unlock:
+   mutex_unlock(>mut);
+   return ret;
  }
  
  static int usbduxfast_upload_firmware(struct comedi_device *dev,





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

Re: [PATCH] staging: comedi: usbduxfast: Improve unlocking of a mutex in usbduxfast_ai_insn_read()

[Ian Abbott]

On 02/11/17 19:40, SF Markus Elfring wrote:

From: Markus Elfring 
Date: Thu, 2 Nov 2017 20:30:31 +0100

* Add a jump target so that a call of the function "mutex_unlock" is stored
   only twice in this function implementation.

* Replace five calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
  drivers/staging/comedi/drivers/usbduxfast.c | 24 
  1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxfast.c 
b/drivers/staging/comedi/drivers/usbduxfast.c
index 608403c7586b..e5884faf7275 100644
--- a/drivers/staging/comedi/drivers/usbduxfast.c
+++ b/drivers/staging/comedi/drivers/usbduxfast.c
@@ -777,8 +777,8 @@ static int usbduxfast_ai_insn_read(struct comedi_device 
*dev,
if (devpriv->ai_cmd_running) {
dev_err(dev->class_dev,
"ai_insn_read not possible, async cmd is running\n");
-   mutex_unlock(>mut);
-   return -EBUSY;
+   ret = -EBUSY;
+   goto unlock;
}
  
  	/* set command for the first channel */

@@ -798,10 +798,8 @@ static int usbduxfast_ai_insn_read(struct comedi_device 
*dev,
usbduxfast_cmd_data(dev, 6, 0x01, 0x00, rngmask, 0x00);
  
  	ret = usbduxfast_send_cmd(dev, SENDADCOMMANDS);

-   if (ret < 0) {
-   mutex_unlock(>mut);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
  
  	for (i = 0; i < PACKETS_TO_IGNORE; i++) {

ret = usb_bulk_msg(usb, usb_rcvbulkpipe(usb, BULKINEP),
@@ -809,8 +807,7 @@ static int usbduxfast_ai_insn_read(struct comedi_device 
*dev,
   _length, 1);
if (ret < 0) {
dev_err(dev->class_dev, "insn timeout, no data\n");
-   mutex_unlock(>mut);
-   return ret;
+   goto unlock;
}
}
  
@@ -820,14 +817,13 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,

   _length, 1);
if (ret < 0) {
dev_err(dev->class_dev, "insn data error: %d\n", ret);
-   mutex_unlock(>mut);
-   return ret;
+   goto unlock;
}
n = actual_length / sizeof(u16);
if ((n % 16) != 0) {
dev_err(dev->class_dev, "insn data packet corrupted\n");
-   mutex_unlock(>mut);
-   return -EINVAL;
+   ret = -EINVAL;
+   goto unlock;
}
for (j = chan; (j < n) && (i < insn->n); j = j + 16) {
data[i] = ((u16 *)(devpriv->inbuf))[j];
@@ -838,6 +834,10 @@ static int usbduxfast_ai_insn_read(struct comedi_device 
*dev,
mutex_unlock(>mut);
  
  	return insn->n;


Minor niggle: You could also remove that call to mutex_unlock() by 
replacing the above three lines with:


ret = insn->n;

which will fall through to the 'unlock:' label below.


+
+unlock:
+   mutex_unlock(>mut);
+   return ret;
  }
  
  static int usbduxfast_upload_firmware(struct comedi_device *dev,





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

[PATCH] fpga: region: release of_parse_phandle nodes after use

[Ian Abbott]

Both fpga_region_get_manager() and fpga_region_get_bridges() call
of_parse_phandle(), but nothing calls of_node_put() on the returned
struct device_node pointers.  Make sure to do that to stop their
reference counters getting out of whack.

Fixes: 0fa20cdfcc1f ("fpga: fpga-region: device tree control for FPGA")
Cc: <sta...@vger.kernel.org> # 4.10+
Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
---
 drivers/fpga/fpga-region.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index d9ab7c75b14f..e0c73ceba2ed 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -147,6 +147,7 @@ static struct fpga_manager *fpga_region_get_manager(struct 
fpga_region *region)
mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
if (mgr_node) {
mgr = of_fpga_mgr_get(mgr_node);
+   of_node_put(mgr_node);
of_node_put(np);
return mgr;
}
@@ -192,10 +193,13 @@ static int fpga_region_get_bridges(struct fpga_region 
*region,
parent_br = region_np->parent;
 
/* If overlay has a list of bridges, use it. */
-   if (of_parse_phandle(overlay, "fpga-bridges", 0))
+   br = of_parse_phandle(overlay, "fpga-bridges", 0);
+   if (br) {
+   of_node_put(br);
np = overlay;
-   else
+   } else {
np = region_np;
+   }
 
for (i = 0; ; i++) {
br = of_parse_phandle(np, "fpga-bridges", i);
@@ -203,12 +207,15 @@ static int fpga_region_get_bridges(struct fpga_region 
*region,
break;
 
/* If parent bridge is in list, skip it. */
-   if (br == parent_br)
+   if (br == parent_br) {
+   of_node_put(br);
continue;
+   }
 
/* If node is a bridge, get it and add to list */
ret = fpga_bridge_get_to_list(br, region->info,
  >bridge_list);
+   of_node_put(br);
 
/* If any of the bridges are in use, give up */
if (ret == -EBUSY) {
-- 
2.14.2

[PATCH] fpga: region: release of_parse_phandle nodes after use

[Ian Abbott]

Both fpga_region_get_manager() and fpga_region_get_bridges() call
of_parse_phandle(), but nothing calls of_node_put() on the returned
struct device_node pointers.  Make sure to do that to stop their
reference counters getting out of whack.

Fixes: 0fa20cdfcc1f ("fpga: fpga-region: device tree control for FPGA")
Cc:  # 4.10+
Signed-off-by: Ian Abbott 
---
 drivers/fpga/fpga-region.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index d9ab7c75b14f..e0c73ceba2ed 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -147,6 +147,7 @@ static struct fpga_manager *fpga_region_get_manager(struct 
fpga_region *region)
mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
if (mgr_node) {
mgr = of_fpga_mgr_get(mgr_node);
+   of_node_put(mgr_node);
of_node_put(np);
return mgr;
}
@@ -192,10 +193,13 @@ static int fpga_region_get_bridges(struct fpga_region 
*region,
parent_br = region_np->parent;
 
/* If overlay has a list of bridges, use it. */
-   if (of_parse_phandle(overlay, "fpga-bridges", 0))
+   br = of_parse_phandle(overlay, "fpga-bridges", 0);
+   if (br) {
+   of_node_put(br);
np = overlay;
-   else
+   } else {
np = region_np;
+   }
 
for (i = 0; ; i++) {
br = of_parse_phandle(np, "fpga-bridges", i);
@@ -203,12 +207,15 @@ static int fpga_region_get_bridges(struct fpga_region 
*region,
break;
 
/* If parent bridge is in list, skip it. */
-   if (br == parent_br)
+   if (br == parent_br) {
+   of_node_put(br);
continue;
+   }
 
/* If node is a bridge, get it and add to list */
ret = fpga_bridge_get_to_list(br, region->info,
  >bridge_list);
+   of_node_put(br);
 
/* If any of the bridges are in use, give up */
if (ret == -EBUSY) {
-- 
2.14.2

Re: [PATCH] staging/comedi: Convert timers to use timer_setup()

[Ian Abbott]

On 17/10/17 00:25, Kees Cook wrote:

In preparation for unconditionally passing the struct timer_list pointer
to all timer callbacks, switch to using the new timer_setup() and
from_timer() to pass the timer pointer explicitly. Adds pointer back to
comedi device from private struct.

Cc: Ian Abbott <abbo...@mev.co.uk>
Cc: H Hartley Sweeten <hswee...@visionengravers.com>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: de...@driverdev.osuosl.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
  drivers/staging/comedi/drivers/comedi_test.c | 18 ++
  drivers/staging/comedi/drivers/das16.c   | 11 ++-
  drivers/staging/comedi/drivers/jr3_pci.c | 10 ++
  3 files changed, 22 insertions(+), 17 deletions(-)


It looks fine to me.

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: Convert timers to use timer_setup()

[Ian Abbott]

On 17/10/17 00:25, Kees Cook wrote:

In preparation for unconditionally passing the struct timer_list pointer
to all timer callbacks, switch to using the new timer_setup() and
from_timer() to pass the timer pointer explicitly. Adds pointer back to
comedi device from private struct.

Cc: Ian Abbott 
Cc: H Hartley Sweeten 
Cc: Greg Kroah-Hartman 
Cc: de...@driverdev.osuosl.org
Signed-off-by: Kees Cook 
---
  drivers/staging/comedi/drivers/comedi_test.c | 18 ++
  drivers/staging/comedi/drivers/das16.c   | 11 ++-
  drivers/staging/comedi/drivers/jr3_pci.c | 10 ++
  3 files changed, 22 insertions(+), 17 deletions(-)


It looks fine to me.

Reviewed-by: Ian Abbott 

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

Re: [PATCH v2] staging: comedi: dt282x: fix IRQ assignment for dev->irq.

[Ian Abbott]

On 06/10/17 19:25, Arvind Yadav wrote:

Here, dev->irq is not assigned with irq. comedi_legacy_detach()
is using dev->irq for release irq and dt282x_attach() is using dev->irq
for initialize comedi_subdevice.

Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>
---
changes in v2:
comedi_isadma_alloc() can fail. adding dev->irq assignment after
 successful return of comedi_isadma_alloc().

  drivers/staging/comedi/drivers/dt282x.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/staging/comedi/drivers/dt282x.c 
b/drivers/staging/comedi/drivers/dt282x.c
index d5295bb..217a4b8 100644
--- a/drivers/staging/comedi/drivers/dt282x.c
+++ b/drivers/staging/comedi/drivers/dt282x.c
@@ -1062,6 +1062,8 @@ static void dt282x_alloc_dma(struct comedi_device *dev,
   PAGE_SIZE, 0);
if (!devpriv->dma)
free_irq(irq_num, dev);
+   else
+   dev->irq = irq_num;
  }
  
  static void dt282x_free_dma(struct comedi_device *dev)




Good catch. 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: dt282x: fix IRQ assignment for dev->irq.

[Ian Abbott]

On 06/10/17 19:25, Arvind Yadav wrote:

Here, dev->irq is not assigned with irq. comedi_legacy_detach()
is using dev->irq for release irq and dt282x_attach() is using dev->irq
for initialize comedi_subdevice.

Signed-off-by: Arvind Yadav 
---
changes in v2:
comedi_isadma_alloc() can fail. adding dev->irq assignment after
 successful return of comedi_isadma_alloc().

  drivers/staging/comedi/drivers/dt282x.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/staging/comedi/drivers/dt282x.c 
b/drivers/staging/comedi/drivers/dt282x.c
index d5295bb..217a4b8 100644
--- a/drivers/staging/comedi/drivers/dt282x.c
+++ b/drivers/staging/comedi/drivers/dt282x.c
@@ -1062,6 +1062,8 @@ static void dt282x_alloc_dma(struct comedi_device *dev,
   PAGE_SIZE, 0);
if (!devpriv->dma)
free_irq(irq_num, dev);
+   else
+   dev->irq = irq_num;
  }
  
  static void dt282x_free_dma(struct comedi_device *dev)




Good catch. Thanks!

Reviewed-by: Ian Abbott 

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

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

 to the member.
854  * @type:   the type of the container struct this is embedded in.
855  * @member: the name of the member within the struct.
856  *
857  */
858 #define container_of(ptr, type, member) ({  
\
859 void *__mptr = (void *)(ptr);   
\
  > 860  BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) 
&&\
861  !__same_type(*(ptr), void),
\
862  "pointer type mismatch in container_of()");  \
863 ((type *)(__mptr - offsetof(type, member))); })
864 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation



Cc'ing Daniel Lezcano and Thomas Gleixner, since this seems to be a 
problem with configurations selecting 'TIMER_OF' even though 
'GENERIC_CLOCKEVENTS' is not selected.


There was a recent-ish commit 599dc457c79b 
("clocksource/drivers/Kconfig: Fix CLKSRC_PISTACHIO dependencies") to 
address this problem for one particular clocksource driver, but some 
other clocksource drivers seem to share the same problem.  There are 
several clocksource config options in "drivers/clocksource/Kconfig" that 
select 'TIMER_OF' without depending on 'GENERIC_CLOCKEVENTS'.  Some of 
them are only manually selectable when 'COMPILE_TEST' is selected.  This 
particular failure seems to be at least partly due to 'ARMV7M_SYSTICK' 
getting selected.


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

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

 to the member.
854  * @type:   the type of the container struct this is embedded in.
855  * @member: the name of the member within the struct.
856  *
857  */
858 #define container_of(ptr, type, member) ({  
\
859 void *__mptr = (void *)(ptr);   
\
  > 860  BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) 
&&\
861  !__same_type(*(ptr), void),
\
862  "pointer type mismatch in container_of()");  \
863 ((type *)(__mptr - offsetof(type, member))); })
864 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation



Cc'ing Daniel Lezcano and Thomas Gleixner, since this seems to be a 
problem with configurations selecting 'TIMER_OF' even though 
'GENERIC_CLOCKEVENTS' is not selected.


There was a recent-ish commit 599dc457c79b 
("clocksource/drivers/Kconfig: Fix CLKSRC_PISTACHIO dependencies") to 
address this problem for one particular clocksource driver, but some 
other clocksource drivers seem to share the same problem.  There are 
several clocksource config options in "drivers/clocksource/Kconfig" that 
select 'TIMER_OF' without depending on 'GENERIC_CLOCKEVENTS'.  Some of 
them are only manually selectable when 'COMPILE_TEST' is selected.  This 
particular failure seems to be at least partly due to 'ARMV7M_SYSTICK' 
getting selected.


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

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

is is embedded in.
855  * @member: the name of the member within the struct.
856  *
857  */
858 #define container_of(ptr, type, member) ({  
\
859 void *__mptr = (void *)(ptr);   
\
  > 860  BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) 
&&\
861  !__same_type(*(ptr), void),
\
862  "pointer type mismatch in container_of()");  \
863 ((type *)(__mptr - offsetof(type, member))); })
864 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation



Cc'ing Daniel Lezcano and Thomas Gleixner, since this seems to be a 
problem with configurations selecting 'TIMER_OF' even though 
'GENERIC_CLOCKEVENTS' is not selected.


There was a recent-ish commit 599dc457c79b 
("clocksource/drivers/Kconfig: Fix CLKSRC_PISTACHIO dependencies") to 
address this problem for one particular clocksource driver, but some 
other clocksource drivers seem to share the same problem.  There are 
several clocksource config options in "drivers/clocksource/Kconfig" that 
select 'TIMER_OF' without depending on 'GENERIC_CLOCKEVENTS'.  Some of 
them are only manually selectable when 'COMPILE_TEST' is selected.  This 
particular failure seems to be at least partly due to 'ARMV7M_SYSTICK' 
getting selected.


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

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

is is embedded in.
855  * @member: the name of the member within the struct.
856  *
857  */
858 #define container_of(ptr, type, member) ({  
\
859 void *__mptr = (void *)(ptr);   
\
  > 860  BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) 
&&\
861  !__same_type(*(ptr), void),
\
862  "pointer type mismatch in container_of()");  \
863 ((type *)(__mptr - offsetof(type, member))); })
864 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation



Cc'ing Daniel Lezcano and Thomas Gleixner, since this seems to be a 
problem with configurations selecting 'TIMER_OF' even though 
'GENERIC_CLOCKEVENTS' is not selected.


There was a recent-ish commit 599dc457c79b 
("clocksource/drivers/Kconfig: Fix CLKSRC_PISTACHIO dependencies") to 
address this problem for one particular clocksource driver, but some 
other clocksource drivers seem to share the same problem.  There are 
several clocksource config options in "drivers/clocksource/Kconfig" that 
select 'TIMER_OF' without depending on 'GENERIC_CLOCKEVENTS'.  Some of 
them are only manually selectable when 'COMPILE_TEST' is selected.  This 
particular failure seems to be at least partly due to 'ARMV7M_SYSTICK' 
getting selected.


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

Re: [PATCH v2 26/31] staging/comedi/das16: Make timer initialization unconditional

[Ian Abbott]

On 21/09/17 00:27, Kees Cook wrote:

With timer initialization made unconditional, there is no reason to
make del_timer_sync() calls conditionally, there by removing the test
of the .data field.

Cc: Ian Abbott <abbo...@mev.co.uk>
Cc: H Hartley Sweeten <hswee...@visionengravers.com>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: de...@driverdev.osuosl.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
  drivers/staging/comedi/drivers/das16.c | 10 --
  1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/drivers/das16.c 
b/drivers/staging/comedi/drivers/das16.c
index 5d157951f63f..2b2a446af3f5 100644
--- a/drivers/staging/comedi/drivers/das16.c
+++ b/drivers/staging/comedi/drivers/das16.c
@@ -934,6 +934,9 @@ static void das16_alloc_dma(struct comedi_device *dev, 
unsigned int dma_chan)
  {
struct das16_private_struct *devpriv = dev->private;
  
+	setup_timer(>timer, das16_timer_interrupt,

+   (unsigned long)dev);
+
/* only DMA channels 3 and 1 are valid */
if (!(dma_chan == 1 || dma_chan == 3))
return;
@@ -941,10 +944,6 @@ static void das16_alloc_dma(struct comedi_device *dev, 
unsigned int dma_chan)
/* DMA uses two buffers */
devpriv->dma = comedi_isadma_alloc(dev, 2, dma_chan, dma_chan,
   DAS16_DMA_SIZE, COMEDI_ISADMA_READ);
-   if (devpriv->dma) {
-   setup_timer(>timer, das16_timer_interrupt,
-   (unsigned long)dev);
-   }
  }
  
  static void das16_free_dma(struct comedi_device *dev)

@@ -952,8 +951,7 @@ static void das16_free_dma(struct comedi_device *dev)
struct das16_private_struct *devpriv = dev->private;
  
  	if (devpriv) {

-   if (devpriv->timer.data)
-   del_timer_sync(>timer);
+   del_timer_sync(>timer);
comedi_isadma_free(devpriv->dma);
}
  }



That's fine.

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 26/31] staging/comedi/das16: Make timer initialization unconditional

[Ian Abbott]

On 21/09/17 00:27, Kees Cook wrote:

With timer initialization made unconditional, there is no reason to
make del_timer_sync() calls conditionally, there by removing the test
of the .data field.

Cc: Ian Abbott 
Cc: H Hartley Sweeten 
Cc: Greg Kroah-Hartman 
Cc: de...@driverdev.osuosl.org
Signed-off-by: Kees Cook 
---
  drivers/staging/comedi/drivers/das16.c | 10 --
  1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/drivers/das16.c 
b/drivers/staging/comedi/drivers/das16.c
index 5d157951f63f..2b2a446af3f5 100644
--- a/drivers/staging/comedi/drivers/das16.c
+++ b/drivers/staging/comedi/drivers/das16.c
@@ -934,6 +934,9 @@ static void das16_alloc_dma(struct comedi_device *dev, 
unsigned int dma_chan)
  {
struct das16_private_struct *devpriv = dev->private;
  
+	setup_timer(>timer, das16_timer_interrupt,

+   (unsigned long)dev);
+
/* only DMA channels 3 and 1 are valid */
if (!(dma_chan == 1 || dma_chan == 3))
return;
@@ -941,10 +944,6 @@ static void das16_alloc_dma(struct comedi_device *dev, 
unsigned int dma_chan)
/* DMA uses two buffers */
devpriv->dma = comedi_isadma_alloc(dev, 2, dma_chan, dma_chan,
   DAS16_DMA_SIZE, COMEDI_ISADMA_READ);
-   if (devpriv->dma) {
-   setup_timer(>timer, das16_timer_interrupt,
-   (unsigned long)dev);
-   }
  }
  
  static void das16_free_dma(struct comedi_device *dev)

@@ -952,8 +951,7 @@ static void das16_free_dma(struct comedi_device *dev)
struct das16_private_struct *devpriv = dev->private;
  
  	if (devpriv) {

-   if (devpriv->timer.data)
-   del_timer_sync(>timer);
+   del_timer_sync(>timer);
comedi_isadma_free(devpriv->dma);
}
  }



That's fine.

Reviewed-by: Ian Abbott 

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

Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

[Ian Abbott]

On 01/09/17 10:29, Ian Abbott wrote:

On 01/09/17 00:29, Kees Cook wrote:

With timer initialization made unconditional, there is no reason to
make del_timer_sync() calls conditionally, there by removing the test
of the .data field.

Cc: Ian Abbott <abbo...@mev.co.uk>
Cc: H Hartley Sweeten <hswee...@visionengravers.com>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: de...@driverdev.osuosl.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
  drivers/staging/comedi/drivers/das16.c | 9 +++--
  1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/drivers/das16.c 
b/drivers/staging/comedi/drivers/das16.c

index 5d157951f63f..4514179b2007 100644
--- a/drivers/staging/comedi/drivers/das16.c
+++ b/drivers/staging/comedi/drivers/das16.c
@@ -941,10 +941,8 @@ static void das16_alloc_dma(struct comedi_device 
*dev, unsigned int dma_chan)

  /* DMA uses two buffers */
  devpriv->dma = comedi_isadma_alloc(dev, 2, dma_chan, dma_chan,
 DAS16_DMA_SIZE, COMEDI_ISADMA_READ);
-if (devpriv->dma) {
-setup_timer(>timer, das16_timer_interrupt,
-(unsigned long)dev);
-}
+setup_timer(>timer, das16_timer_interrupt,
+(unsigned long)dev);
  }


das16_alloc_dma() returns before the call to comedi_isadma_alloc() if 
the dma_chan parameter is not one of the values 1 or 3, so setup_timer() 
will not be called in that case.



  static void das16_free_dma(struct comedi_device *dev)
@@ -952,8 +950,7 @@ static void das16_free_dma(struct comedi_device *dev)
  struct das16_private_struct *devpriv = dev->private;
  if (devpriv) {
-if (devpriv->timer.data)
-del_timer_sync(>timer);
+del_timer_sync(>timer);


If setup_timer() has not been called (see remark above), this change 
will break.



  comedi_isadma_free(devpriv->dma);
  }
  }



If you want to avoid testing devpriv->timer.data for some reason, you 
could make the calls to setup_timer() and del_timer_sync() depend on 
devpriv->dma.


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

Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

[Ian Abbott]

On 01/09/17 10:29, Ian Abbott wrote:

On 01/09/17 00:29, Kees Cook wrote:

With timer initialization made unconditional, there is no reason to
make del_timer_sync() calls conditionally, there by removing the test
of the .data field.

Cc: Ian Abbott 
Cc: H Hartley Sweeten 
Cc: Greg Kroah-Hartman 
Cc: de...@driverdev.osuosl.org
Signed-off-by: Kees Cook 
---
  drivers/staging/comedi/drivers/das16.c | 9 +++--
  1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/drivers/das16.c 
b/drivers/staging/comedi/drivers/das16.c

index 5d157951f63f..4514179b2007 100644
--- a/drivers/staging/comedi/drivers/das16.c
+++ b/drivers/staging/comedi/drivers/das16.c
@@ -941,10 +941,8 @@ static void das16_alloc_dma(struct comedi_device 
*dev, unsigned int dma_chan)

  /* DMA uses two buffers */
  devpriv->dma = comedi_isadma_alloc(dev, 2, dma_chan, dma_chan,
 DAS16_DMA_SIZE, COMEDI_ISADMA_READ);
-if (devpriv->dma) {
-setup_timer(>timer, das16_timer_interrupt,
-(unsigned long)dev);
-}
+setup_timer(>timer, das16_timer_interrupt,
+(unsigned long)dev);
  }


das16_alloc_dma() returns before the call to comedi_isadma_alloc() if 
the dma_chan parameter is not one of the values 1 or 3, so setup_timer() 
will not be called in that case.



  static void das16_free_dma(struct comedi_device *dev)
@@ -952,8 +950,7 @@ static void das16_free_dma(struct comedi_device *dev)
  struct das16_private_struct *devpriv = dev->private;
  if (devpriv) {
-if (devpriv->timer.data)
-del_timer_sync(>timer);
+del_timer_sync(>timer);


If setup_timer() has not been called (see remark above), this change 
will break.



  comedi_isadma_free(devpriv->dma);
  }
  }



If you want to avoid testing devpriv->timer.data for some reason, you 
could make the calls to setup_timer() and del_timer_sync() depend on 
devpriv->dma.


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

Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

[Ian Abbott]

On 01/09/17 00:29, Kees Cook wrote:

With timer initialization made unconditional, there is no reason to
make del_timer_sync() calls conditionally, there by removing the test
of the .data field.

Cc: Ian Abbott <abbo...@mev.co.uk>
Cc: H Hartley Sweeten <hswee...@visionengravers.com>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: de...@driverdev.osuosl.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
  drivers/staging/comedi/drivers/das16.c | 9 +++--
  1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/drivers/das16.c 
b/drivers/staging/comedi/drivers/das16.c
index 5d157951f63f..4514179b2007 100644
--- a/drivers/staging/comedi/drivers/das16.c
+++ b/drivers/staging/comedi/drivers/das16.c
@@ -941,10 +941,8 @@ static void das16_alloc_dma(struct comedi_device *dev, 
unsigned int dma_chan)
/* DMA uses two buffers */
devpriv->dma = comedi_isadma_alloc(dev, 2, dma_chan, dma_chan,
   DAS16_DMA_SIZE, COMEDI_ISADMA_READ);
-   if (devpriv->dma) {
-   setup_timer(>timer, das16_timer_interrupt,
-   (unsigned long)dev);
-   }
+   setup_timer(>timer, das16_timer_interrupt,
+   (unsigned long)dev);
  }


das16_alloc_dma() returns before the call to comedi_isadma_alloc() if 
the dma_chan parameter is not one of the values 1 or 3, so setup_timer() 
will not be called in that case.


  
  static void das16_free_dma(struct comedi_device *dev)

@@ -952,8 +950,7 @@ static void das16_free_dma(struct comedi_device *dev)
struct das16_private_struct *devpriv = dev->private;
  
  	if (devpriv) {

-   if (devpriv->timer.data)
-   del_timer_sync(>timer);
+   del_timer_sync(>timer);


If setup_timer() has not been called (see remark above), this change 
will break.



comedi_isadma_free(devpriv->dma);
}
  }




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

Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

[Ian Abbott]

On 01/09/17 00:29, Kees Cook wrote:

With timer initialization made unconditional, there is no reason to
make del_timer_sync() calls conditionally, there by removing the test
of the .data field.

Cc: Ian Abbott 
Cc: H Hartley Sweeten 
Cc: Greg Kroah-Hartman 
Cc: de...@driverdev.osuosl.org
Signed-off-by: Kees Cook 
---
  drivers/staging/comedi/drivers/das16.c | 9 +++--
  1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/drivers/das16.c 
b/drivers/staging/comedi/drivers/das16.c
index 5d157951f63f..4514179b2007 100644
--- a/drivers/staging/comedi/drivers/das16.c
+++ b/drivers/staging/comedi/drivers/das16.c
@@ -941,10 +941,8 @@ static void das16_alloc_dma(struct comedi_device *dev, 
unsigned int dma_chan)
/* DMA uses two buffers */
devpriv->dma = comedi_isadma_alloc(dev, 2, dma_chan, dma_chan,
   DAS16_DMA_SIZE, COMEDI_ISADMA_READ);
-   if (devpriv->dma) {
-   setup_timer(>timer, das16_timer_interrupt,
-   (unsigned long)dev);
-   }
+   setup_timer(>timer, das16_timer_interrupt,
+   (unsigned long)dev);
  }


das16_alloc_dma() returns before the call to comedi_isadma_alloc() if 
the dma_chan parameter is not one of the values 1 or 3, so setup_timer() 
will not be called in that case.


  
  static void das16_free_dma(struct comedi_device *dev)

@@ -952,8 +950,7 @@ static void das16_free_dma(struct comedi_device *dev)
struct das16_private_struct *devpriv = dev->private;
  
  	if (devpriv) {

-   if (devpriv->timer.data)
-   del_timer_sync(>timer);
+   del_timer_sync(>timer);


If setup_timer() has not been called (see remark above), this change 
will break.



comedi_isadma_free(devpriv->dma);
    }
  }




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

Re: [PATCH] Staging: comedi: addi_apci_3501: fixed a CamelCase coding style issue

[Ian Abbott]

On 08/08/17 08:42, Chi-Chun Hsu wrote:

Fixed a coding style issue.

Signed-off-by: Chi-Chun Hsu <justma...@gmail.com>
---
  drivers/staging/comedi/drivers/addi_apci_3501.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi_apci_3501.c 
b/drivers/staging/comedi/drivers/addi_apci_3501.c
index 40ff914..b2f6712 100644
--- a/drivers/staging/comedi/drivers/addi_apci_3501.c
+++ b/drivers/staging/comedi/drivers/addi_apci_3501.c
@@ -68,7 +68,7 @@
  struct apci3501_private {
unsigned long amcc;
unsigned long tcw;
-   struct task_struct *tsk_Current;
+   struct task_struct *tsk_current;
unsigned char timer_mode;
  };
  
@@ -273,7 +273,7 @@ static irqreturn_t apci3501_interrupt(int irq, void *d)

}
  
  	/* Enable Interrupt Send a signal to from kernel to user space */

-   send_sig(SIGIO, devpriv->tsk_Current, 0);
+   send_sig(SIGIO, devpriv->tsk_current, 0);
ctrl = inl(devpriv->tcw + ADDI_TCW_CTRL_REG);
ctrl &= ~(ADDI_TCW_CTRL_GATE | ADDI_TCW_CTRL_TRIG |
  ADDI_TCW_CTRL_IRQ_ENA);



This patch is incomplete (the 'tsk_Current' member is also used in 
"drivers/staging/comedi/addi-data/hwdrv_apci3501.c" which is #include'd 
by "addi_apci_3501.c") and out-of-date (the 'tsk_Current' member was 
removed in kernel version 4.9 by commit a6672530f6fc ("staging: comedi: 
addi_apci_3501: remove timer/counter subdevice support").


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

Re: [PATCH] Staging: comedi: addi_apci_3501: fixed a CamelCase coding style issue

[Ian Abbott]

On 08/08/17 08:42, Chi-Chun Hsu wrote:

Fixed a coding style issue.

Signed-off-by: Chi-Chun Hsu 
---
  drivers/staging/comedi/drivers/addi_apci_3501.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi_apci_3501.c 
b/drivers/staging/comedi/drivers/addi_apci_3501.c
index 40ff914..b2f6712 100644
--- a/drivers/staging/comedi/drivers/addi_apci_3501.c
+++ b/drivers/staging/comedi/drivers/addi_apci_3501.c
@@ -68,7 +68,7 @@
  struct apci3501_private {
unsigned long amcc;
unsigned long tcw;
-   struct task_struct *tsk_Current;
+   struct task_struct *tsk_current;
unsigned char timer_mode;
  };
  
@@ -273,7 +273,7 @@ static irqreturn_t apci3501_interrupt(int irq, void *d)

}
  
  	/* Enable Interrupt Send a signal to from kernel to user space */

-   send_sig(SIGIO, devpriv->tsk_Current, 0);
+   send_sig(SIGIO, devpriv->tsk_current, 0);
ctrl = inl(devpriv->tcw + ADDI_TCW_CTRL_REG);
ctrl &= ~(ADDI_TCW_CTRL_GATE | ADDI_TCW_CTRL_TRIG |
  ADDI_TCW_CTRL_IRQ_ENA);



This patch is incomplete (the 'tsk_Current' member is also used in 
"drivers/staging/comedi/addi-data/hwdrv_apci3501.c" which is #include'd 
by "addi_apci_3501.c") and out-of-date (the 'tsk_Current' member was 
removed in kernel version 4.9 by commit a6672530f6fc ("staging: comedi: 
addi_apci_3501: remove timer/counter subdevice support").


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

[PATCH] staging: comedi: comedi_fops: do not call blocking ops when !TASK_RUNNING

[Ian Abbott]

Comedi's read and write file operation handlers (`comedi_read()` and
`comedi_write()`) currently call `copy_to_user()` or `copy_from_user()`
whilst in the `TASK_INTERRUPTIBLE` state, which falls foul of the
`might_fault()` checks when enabled.  Fix it by setting the current task
state back to `TASK_RUNNING` a bit earlier before calling these
functions.

Reported-by: Piotr Gregor <piotrgre...@rsyncme.org>
Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: <sta...@vger.kernel.org> # 4.5+
---
Note: stable kernel versions 4.4 and earlier will need a slightly more
extensive change in `comedi_write()` than provided by this patch due to
a call to `mutex_lock()` whilst in the `TASK_INTERRUPTIBLE` state.
---
 drivers/staging/comedi/comedi_fops.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index ca11be21f64b..34ca7823255d 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2396,6 +2396,7 @@ static ssize_t comedi_write(struct file *file, const char 
__user *buf,
continue;
}
 
+   set_current_state(TASK_RUNNING);
wp = async->buf_write_ptr;
n1 = min(n, async->prealloc_bufsz - wp);
n2 = n - n1;
@@ -2528,6 +2529,8 @@ static ssize_t comedi_read(struct file *file, char __user 
*buf, size_t nbytes,
}
continue;
}
+
+   set_current_state(TASK_RUNNING);
rp = async->buf_read_ptr;
n1 = min(n, async->prealloc_bufsz - rp);
n2 = n - n1;
-- 
2.13.2

[PATCH] staging: comedi: comedi_fops: do not call blocking ops when !TASK_RUNNING

[Ian Abbott]

Comedi's read and write file operation handlers (`comedi_read()` and
`comedi_write()`) currently call `copy_to_user()` or `copy_from_user()`
whilst in the `TASK_INTERRUPTIBLE` state, which falls foul of the
`might_fault()` checks when enabled.  Fix it by setting the current task
state back to `TASK_RUNNING` a bit earlier before calling these
functions.

Reported-by: Piotr Gregor 
Signed-off-by: Ian Abbott 
Cc:  # 4.5+
---
Note: stable kernel versions 4.4 and earlier will need a slightly more
extensive change in `comedi_write()` than provided by this patch due to
a call to `mutex_lock()` whilst in the `TASK_INTERRUPTIBLE` state.
---
 drivers/staging/comedi/comedi_fops.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index ca11be21f64b..34ca7823255d 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2396,6 +2396,7 @@ static ssize_t comedi_write(struct file *file, const char 
__user *buf,
continue;
}
 
+   set_current_state(TASK_RUNNING);
wp = async->buf_write_ptr;
n1 = min(n, async->prealloc_bufsz - wp);
n2 = n - n1;
@@ -2528,6 +2529,8 @@ static ssize_t comedi_read(struct file *file, char __user 
*buf, size_t nbytes,
}
continue;
}
+
+   set_current_state(TASK_RUNNING);
rp = async->buf_read_ptr;
n1 = min(n, async->prealloc_bufsz - rp);
n2 = n - n1;
-- 
2.13.2

Re: [PATCH] staging: comedi: ni_mio_common.c: fix coding style issue

[Ian Abbott]

On 23/07/17 13:14, Christopher Mårtensson wrote:

 From 3e90ab52ad9b437d7c09cc667161cdb855c0cc7b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christopher=20M=C3=A5rtensson?= <criba...@gmail.com>
Date: Sun, 23 Jul 2017 13:05:09 +0200
Subject: [PATCH] staging: comedi: ni_mio_common.c: fix coding style issue

"checkpatch.pl -f ..." gave
ERROR: open brace '{' following function definitions go on the next line

Signed-off-by: Christopher Mårtensson <criba...@gmail.com>
---
  drivers/staging/comedi/drivers/ni_mio_common.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c 
b/drivers/staging/comedi/drivers/ni_mio_common.c
index 2f7bfc1..398347f 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -1962,7 +1962,8 @@ static unsigned int ni_timer_to_ns(const struct 
comedi_device *dev, int timer)
  static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
 struct comedi_subdevice *sdev,
 const struct comedi_cmd *cmd,
-unsigned int max_count) {
+unsigned int max_count)
+{
  #ifdef PCIDMA
unsigned int nbytes = max_count;
  



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] staging: comedi: ni_mio_common.c: fix coding style issue

[Ian Abbott]

On 23/07/17 13:14, Christopher Mårtensson wrote:

 From 3e90ab52ad9b437d7c09cc667161cdb855c0cc7b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christopher=20M=C3=A5rtensson?= 
Date: Sun, 23 Jul 2017 13:05:09 +0200
Subject: [PATCH] staging: comedi: ni_mio_common.c: fix coding style issue

"checkpatch.pl -f ..." gave
ERROR: open brace '{' following function definitions go on the next line

Signed-off-by: Christopher Mårtensson 
---
  drivers/staging/comedi/drivers/ni_mio_common.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c 
b/drivers/staging/comedi/drivers/ni_mio_common.c
index 2f7bfc1..398347f 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -1962,7 +1962,8 @@ static unsigned int ni_timer_to_ns(const struct 
comedi_device *dev, int timer)
  static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
 struct comedi_subdevice *sdev,
 const struct comedi_cmd *cmd,
-unsigned int max_count) {
+unsigned int max_count)
+{
  #ifdef PCIDMA
unsigned int nbytes = max_count;
  



Thanks for the fix.

Reviewed-by: Ian Abbott 

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

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

ntainer_of(ptr, type, member) ({  
\
859 void *__mptr = (void *)(ptr);   
\
  > 860  BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) 
&&\
861  !__same_type(*(ptr), void),
\
862  "pointer type mismatch in container_of()");  \
863 ((type *)(__mptr - offsetof(type, member))); })
864 


Matt Redfearn sent a fix 6 days ago.  It didn't get Cc'ed to LKML, but 
it did get Cc'd to the CLOCKSOURCE maintainers.  Does it need re-posting?


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

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

ntainer_of(ptr, type, member) ({  
\
859 void *__mptr = (void *)(ptr);   
\
  > 860  BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) 
&&\
861  !__same_type(*(ptr), void),
\
862  "pointer type mismatch in container_of()");  \
863 ((type *)(__mptr - offsetof(type, member))); })
864 


Matt Redfearn sent a fix 6 days ago.  It didn't get Cc'ed to LKML, but 
it did get Cc'd to the CLOCKSOURCE maintainers.  Does it need re-posting?


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

Re: [BUG] drivers: staging: comedi: do not call blocking ops when !TASK_RUNNING; state=1

[Ian Abbott]

[Top-posting fixed.  Please don't top-post!]
On 17/07/17 19:14, Piotr Gregor wrote:

On Mon, Jul 17, 2017 at 05:02:45PM +0100, Ian Abbott wrote:

On 17/07/17 15:48, Piotr Gregor wrote:

Calling blocking operations from wrong context.
Kernel:

Linux piotrpc 4.4.70-rt83 #1 SMP PREEMPT RT Thu Jul 13 08:42:02 BST 2017 x86_64 
GNU/Linux

[   80.542018] NOHZ: local_softirq_pending 80
[  125.175471] [ cut here ]
[  125.175491] WARNING: CPU: 0 PID: 1497 at kernel/sched/core.c:7833 
__might_sleep+0x9f/0xb0()
[  125.175728] do not call blocking ops when !TASK_RUNNING; state=1 set at 
[] comedi_read+0x1a1/0x610 [comedi]

[snip]

[  125.175926] CPU: 0 PID: 1497 Comm: txrx Tainted: GWC  
4.4.70-rt83 #1
[  125.175928] Hardware name: NOVATECH LTD PC-XB04472/H110M-C, BIOS 3019 
01/06/2017
[  125.175935]   8802205b3cb0 81387160 
8802205b3cf8
[  125.175940]  0009 8802205b3ce8 81089766 
c057ca60
[  125.175944]  02dc  0001 
0001
[  125.175945] Call Trace:
[  125.175955]  [] dump_stack+0x85/0xc5
[  125.175963]  [] warn_slowpath_common+0x86/0xe0
[  125.175971]  [] warn_slowpath_fmt+0x4c/0x50
[  125.175980]  [] ? trace_preempt_on+0x1a7/0x2b0
[  125.175985]  [] ? schedule+0x55/0xe0
[  125.175994]  [] ? comedi_read+0x1a1/0x610 [comedi]
[  125.176001]  [] ? comedi_read+0x1a1/0x610 [comedi]
[  125.176005]  [] __might_sleep+0x9f/0xb0
[  125.176012]  [] __might_fault+0x3b/0xb0
[  125.176020]  [] comedi_read+0x3df/0x610 [comedi]
[  125.176034]  [] ? wake_up_process+0x20/0x20
[  125.176044]  [] __vfs_read+0x28/0xe0
[  125.176053]  [] ? security_file_permission+0xa6/0xc0
[  125.176060]  [] ? rw_verify_area+0x53/0xf0
[  125.176066]  [] vfs_read+0x89/0x130
[  125.176074]  [] SyS_read+0x49/0xb0
[  125.176084]  [] entry_SYSCALL_64_fastpath+0x16/0x7a
[  125.176095] ---[ end trace 0003 ]---


cheers,
Piotr


Thanks for the bug report.  I think comedi_read() in
"drivers/staging/comedi/comedi_fops.c" needs a call to
set_current_state(TASK_RUNNING) just before the call to copy_to_user(...).

Similarly, comedi_write() needs a call to set_current_task(TASK_RUNNING)
just before the call to copy_from_user(...), and another call to
set_current_task(TASK_RUNNING) before the call to mutex_lock().

I'll work on a patch.


> Hi Ian,
>
> I am afraid I have more problems with running Comedi on rt-patched 
kernel than
> only that. I am still trying to figure out the specific reason - 
hopefully it is

> my fault and I scrued up my own code, but so far it seems pcie215
> support doesn't work at least in the same way on rt-patched kernel
> as it was on non-rt. I will dig it a little bit harder, likely coming 
with another

> bug reports shoud they be observed.
>
> Do you know about other experience other people may have using Comedi
> for pcie215 on rt-patched kernels, specifically 4.4.70?

Not in particular.  People have used Comedi with RTAI in the past, but 
that's a different animal to the RT patch-set.


> Has Comedi been tested on rt kernels by yourself?

Not up until now.  Initial testing with the "comedi_test" module 
suggests there may be some issues to be resolved (e.g. it locks up when 
running an asynchronous command), although "comedi_test" isn't a typical 
Comedi driver as it makes heavy use of kernel timers.  I do have a 
pcie215, but haven't tried it on an RT kernel yet.


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

Re: [BUG] drivers: staging: comedi: do not call blocking ops when !TASK_RUNNING; state=1

[Ian Abbott]

[Top-posting fixed.  Please don't top-post!]
On 17/07/17 19:14, Piotr Gregor wrote:

On Mon, Jul 17, 2017 at 05:02:45PM +0100, Ian Abbott wrote:

On 17/07/17 15:48, Piotr Gregor wrote:

Calling blocking operations from wrong context.
Kernel:

Linux piotrpc 4.4.70-rt83 #1 SMP PREEMPT RT Thu Jul 13 08:42:02 BST 2017 x86_64 
GNU/Linux

[   80.542018] NOHZ: local_softirq_pending 80
[  125.175471] [ cut here ]
[  125.175491] WARNING: CPU: 0 PID: 1497 at kernel/sched/core.c:7833 
__might_sleep+0x9f/0xb0()
[  125.175728] do not call blocking ops when !TASK_RUNNING; state=1 set at 
[] comedi_read+0x1a1/0x610 [comedi]

[snip]

[  125.175926] CPU: 0 PID: 1497 Comm: txrx Tainted: GWC  
4.4.70-rt83 #1
[  125.175928] Hardware name: NOVATECH LTD PC-XB04472/H110M-C, BIOS 3019 
01/06/2017
[  125.175935]   8802205b3cb0 81387160 
8802205b3cf8
[  125.175940]  0009 8802205b3ce8 81089766 
c057ca60
[  125.175944]  02dc  0001 
0001
[  125.175945] Call Trace:
[  125.175955]  [] dump_stack+0x85/0xc5
[  125.175963]  [] warn_slowpath_common+0x86/0xe0
[  125.175971]  [] warn_slowpath_fmt+0x4c/0x50
[  125.175980]  [] ? trace_preempt_on+0x1a7/0x2b0
[  125.175985]  [] ? schedule+0x55/0xe0
[  125.175994]  [] ? comedi_read+0x1a1/0x610 [comedi]
[  125.176001]  [] ? comedi_read+0x1a1/0x610 [comedi]
[  125.176005]  [] __might_sleep+0x9f/0xb0
[  125.176012]  [] __might_fault+0x3b/0xb0
[  125.176020]  [] comedi_read+0x3df/0x610 [comedi]
[  125.176034]  [] ? wake_up_process+0x20/0x20
[  125.176044]  [] __vfs_read+0x28/0xe0
[  125.176053]  [] ? security_file_permission+0xa6/0xc0
[  125.176060]  [] ? rw_verify_area+0x53/0xf0
[  125.176066]  [] vfs_read+0x89/0x130
[  125.176074]  [] SyS_read+0x49/0xb0
[  125.176084]  [] entry_SYSCALL_64_fastpath+0x16/0x7a
[  125.176095] ---[ end trace 0003 ]---


cheers,
Piotr


Thanks for the bug report.  I think comedi_read() in
"drivers/staging/comedi/comedi_fops.c" needs a call to
set_current_state(TASK_RUNNING) just before the call to copy_to_user(...).

Similarly, comedi_write() needs a call to set_current_task(TASK_RUNNING)
just before the call to copy_from_user(...), and another call to
set_current_task(TASK_RUNNING) before the call to mutex_lock().

I'll work on a patch.


> Hi Ian,
>
> I am afraid I have more problems with running Comedi on rt-patched 
kernel than
> only that. I am still trying to figure out the specific reason - 
hopefully it is

> my fault and I scrued up my own code, but so far it seems pcie215
> support doesn't work at least in the same way on rt-patched kernel
> as it was on non-rt. I will dig it a little bit harder, likely coming 
with another

> bug reports shoud they be observed.
>
> Do you know about other experience other people may have using Comedi
> for pcie215 on rt-patched kernels, specifically 4.4.70?

Not in particular.  People have used Comedi with RTAI in the past, but 
that's a different animal to the RT patch-set.


> Has Comedi been tested on rt kernels by yourself?

Not up until now.  Initial testing with the "comedi_test" module 
suggests there may be some issues to be resolved (e.g. it locks up when 
running an asynchronous command), although "comedi_test" isn't a typical 
Comedi driver as it makes heavy use of kernel timers.  I do have a 
pcie215, but haven't tried it on an RT kernel yet.


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

Re: [BUG] drivers: staging: comedi: do not call blocking ops when !TASK_RUNNING; state=1

[Ian Abbott]

On 17/07/17 15:48, Piotr Gregor wrote:

Calling blocking operations from wrong context.
Kernel:

Linux piotrpc 4.4.70-rt83 #1 SMP PREEMPT RT Thu Jul 13 08:42:02 BST 2017 x86_64 
GNU/Linux

[   80.542018] NOHZ: local_softirq_pending 80
[  125.175471] [ cut here ]
[  125.175491] WARNING: CPU: 0 PID: 1497 at kernel/sched/core.c:7833 
__might_sleep+0x9f/0xb0()
[  125.175728] do not call blocking ops when !TASK_RUNNING; state=1 set at 
[] comedi_read+0x1a1/0x610 [comedi]
[  125.175735] Modules linked in: cpufreq_conservative cpufreq_powersave 
cpufreq_userspace cfg80211 nfsd auth_rpcgss oid_registry nfs_acl nfs lockd 
grace fscache sunrpc nls_ascii nls_cp437 vfat fat snd_hda_codec_hdmi 
snd_hda_codec_realtek snd_hda_codec_generic intel_rapl joydev intel_powerclamp 
coretemp kvm drbg efi_pstore ansi_cprng irqbypass crct10dif_pclmul crc32_pclmul 
ghash_clmulni_intel aesni_intel aes_x86_64 lrw gf128mul glue_helper ablk_helper 
cryptd i915 serio_raw pcspkr efivars snd_hda_intel amplc_dio200_pci(C) 
amplc_dio200_common(C) snd_hda_codec iTCO_wdt comedi_8254(C) comedi_pci(C) 
iTCO_vendor_support comedi(C) snd_hda_core snd_hwdep snd_pcm drm_kms_helper 
snd_timer drm snd mei_me soundcore mei i2c_algo_bit shpchp eeepc_wmi asus_wmi 
sparse_keymap mxm_wmi battery hci_uart btbcm btqca btintel
[  125.175824]  bluetooth
[  125.175825]  wmi
[  125.175826]  rfkill
[  125.175827]  video
[  125.175828]  intel_lpss_acpi
[  125.175830]  intel_lpss
[  125.175831]  evdev
[  125.175832]  mfd_core
[  125.175833]  acpi_als
[  125.175834]  acpi_pad
[  125.175835]  kfifo_buf
[  125.175836]  button
[  125.175838]  industrialio
[  125.175839]  fuse
[  125.175840]  parport_pc
[  125.175841]  ppdev
[  125.175842]  lp
[  125.175843]  parport
[  125.175844]  autofs4
[  125.175845]  ext4
[  125.175846]  crc16
[  125.175847]  mbcache
[  125.175848]  jbd2
[  125.175849]  hid_generic
[  125.175851]  usbhid
[  125.175852]  sg
[  125.175853]  sr_mod
[  125.175854]  cdrom
[  125.175855]  sd_mod
[  125.175856]  crc32c_intel
[  125.175857]  ahci
[  125.175858]  libahci
[  125.175859]  psmouse
[  125.175860]  xhci_pci
[  125.175861]  libata
[  125.175862]  r8169
[  125.175864]  i2c_i801
[  125.175865]  mii
[  125.175866]  xhci_hcd
[  125.175867]  tg3
[  125.175868]  ptp
[  125.175869]  scsi_mod
[  125.175870]  usbcore
[  125.175871]  pps_core
[  125.175872]  libphy
[  125.175873]  usb_common
[  125.175874]  fan
[  125.175875]  thermal
[  125.175876]  i2c_hid
[  125.175877]  hid
[  125.175878]  fjes

[  125.175926] CPU: 0 PID: 1497 Comm: txrx Tainted: GWC  
4.4.70-rt83 #1
[  125.175928] Hardware name: NOVATECH LTD PC-XB04472/H110M-C, BIOS 3019 
01/06/2017
[  125.175935]   8802205b3cb0 81387160 
8802205b3cf8
[  125.175940]  0009 8802205b3ce8 81089766 
c057ca60
[  125.175944]  02dc  0001 
0001
[  125.175945] Call Trace:
[  125.175955]  [] dump_stack+0x85/0xc5
[  125.175963]  [] warn_slowpath_common+0x86/0xe0
[  125.175971]  [] warn_slowpath_fmt+0x4c/0x50
[  125.175980]  [] ? trace_preempt_on+0x1a7/0x2b0
[  125.175985]  [] ? schedule+0x55/0xe0
[  125.175994]  [] ? comedi_read+0x1a1/0x610 [comedi]
[  125.176001]  [] ? comedi_read+0x1a1/0x610 [comedi]
[  125.176005]  [] __might_sleep+0x9f/0xb0
[  125.176012]  [] __might_fault+0x3b/0xb0
[  125.176020]  [] comedi_read+0x3df/0x610 [comedi]
[  125.176034]  [] ? wake_up_process+0x20/0x20
[  125.176044]  [] __vfs_read+0x28/0xe0
[  125.176053]  [] ? security_file_permission+0xa6/0xc0
[  125.176060]  [] ? rw_verify_area+0x53/0xf0
[  125.176066]  [] vfs_read+0x89/0x130
[  125.176074]  [] SyS_read+0x49/0xb0
[  125.176084]  [] entry_SYSCALL_64_fastpath+0x16/0x7a
[  125.176095] ---[ end trace 0003 ]---


cheers,
Piotr


Thanks for the bug report.  I think comedi_read() in 
"drivers/staging/comedi/comedi_fops.c" needs a call to 
set_current_state(TASK_RUNNING) just before the call to copy_to_user(...).


Similarly, comedi_write() needs a call to set_current_task(TASK_RUNNING) 
just before the call to copy_from_user(...), and another call to 
set_current_task(TASK_RUNNING) before the call to mutex_lock().


I'll work on a patch.

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

Re: [BUG] drivers: staging: comedi: do not call blocking ops when !TASK_RUNNING; state=1

[Ian Abbott]

On 17/07/17 15:48, Piotr Gregor wrote:

Calling blocking operations from wrong context.
Kernel:

Linux piotrpc 4.4.70-rt83 #1 SMP PREEMPT RT Thu Jul 13 08:42:02 BST 2017 x86_64 
GNU/Linux

[   80.542018] NOHZ: local_softirq_pending 80
[  125.175471] [ cut here ]
[  125.175491] WARNING: CPU: 0 PID: 1497 at kernel/sched/core.c:7833 
__might_sleep+0x9f/0xb0()
[  125.175728] do not call blocking ops when !TASK_RUNNING; state=1 set at 
[] comedi_read+0x1a1/0x610 [comedi]
[  125.175735] Modules linked in: cpufreq_conservative cpufreq_powersave 
cpufreq_userspace cfg80211 nfsd auth_rpcgss oid_registry nfs_acl nfs lockd 
grace fscache sunrpc nls_ascii nls_cp437 vfat fat snd_hda_codec_hdmi 
snd_hda_codec_realtek snd_hda_codec_generic intel_rapl joydev intel_powerclamp 
coretemp kvm drbg efi_pstore ansi_cprng irqbypass crct10dif_pclmul crc32_pclmul 
ghash_clmulni_intel aesni_intel aes_x86_64 lrw gf128mul glue_helper ablk_helper 
cryptd i915 serio_raw pcspkr efivars snd_hda_intel amplc_dio200_pci(C) 
amplc_dio200_common(C) snd_hda_codec iTCO_wdt comedi_8254(C) comedi_pci(C) 
iTCO_vendor_support comedi(C) snd_hda_core snd_hwdep snd_pcm drm_kms_helper 
snd_timer drm snd mei_me soundcore mei i2c_algo_bit shpchp eeepc_wmi asus_wmi 
sparse_keymap mxm_wmi battery hci_uart btbcm btqca btintel
[  125.175824]  bluetooth
[  125.175825]  wmi
[  125.175826]  rfkill
[  125.175827]  video
[  125.175828]  intel_lpss_acpi
[  125.175830]  intel_lpss
[  125.175831]  evdev
[  125.175832]  mfd_core
[  125.175833]  acpi_als
[  125.175834]  acpi_pad
[  125.175835]  kfifo_buf
[  125.175836]  button
[  125.175838]  industrialio
[  125.175839]  fuse
[  125.175840]  parport_pc
[  125.175841]  ppdev
[  125.175842]  lp
[  125.175843]  parport
[  125.175844]  autofs4
[  125.175845]  ext4
[  125.175846]  crc16
[  125.175847]  mbcache
[  125.175848]  jbd2
[  125.175849]  hid_generic
[  125.175851]  usbhid
[  125.175852]  sg
[  125.175853]  sr_mod
[  125.175854]  cdrom
[  125.175855]  sd_mod
[  125.175856]  crc32c_intel
[  125.175857]  ahci
[  125.175858]  libahci
[  125.175859]  psmouse
[  125.175860]  xhci_pci
[  125.175861]  libata
[  125.175862]  r8169
[  125.175864]  i2c_i801
[  125.175865]  mii
[  125.175866]  xhci_hcd
[  125.175867]  tg3
[  125.175868]  ptp
[  125.175869]  scsi_mod
[  125.175870]  usbcore
[  125.175871]  pps_core
[  125.175872]  libphy
[  125.175873]  usb_common
[  125.175874]  fan
[  125.175875]  thermal
[  125.175876]  i2c_hid
[  125.175877]  hid
[  125.175878]  fjes

[  125.175926] CPU: 0 PID: 1497 Comm: txrx Tainted: GWC  
4.4.70-rt83 #1
[  125.175928] Hardware name: NOVATECH LTD PC-XB04472/H110M-C, BIOS 3019 
01/06/2017
[  125.175935]   8802205b3cb0 81387160 
8802205b3cf8
[  125.175940]  0009 8802205b3ce8 81089766 
c057ca60
[  125.175944]  02dc  0001 
0001
[  125.175945] Call Trace:
[  125.175955]  [] dump_stack+0x85/0xc5
[  125.175963]  [] warn_slowpath_common+0x86/0xe0
[  125.175971]  [] warn_slowpath_fmt+0x4c/0x50
[  125.175980]  [] ? trace_preempt_on+0x1a7/0x2b0
[  125.175985]  [] ? schedule+0x55/0xe0
[  125.175994]  [] ? comedi_read+0x1a1/0x610 [comedi]
[  125.176001]  [] ? comedi_read+0x1a1/0x610 [comedi]
[  125.176005]  [] __might_sleep+0x9f/0xb0
[  125.176012]  [] __might_fault+0x3b/0xb0
[  125.176020]  [] comedi_read+0x3df/0x610 [comedi]
[  125.176034]  [] ? wake_up_process+0x20/0x20
[  125.176044]  [] __vfs_read+0x28/0xe0
[  125.176053]  [] ? security_file_permission+0xa6/0xc0
[  125.176060]  [] ? rw_verify_area+0x53/0xf0
[  125.176066]  [] vfs_read+0x89/0x130
[  125.176074]  [] SyS_read+0x49/0xb0
[  125.176084]  [] entry_SYSCALL_64_fastpath+0x16/0x7a
[  125.176095] ---[ end trace 0003 ]---


cheers,
Piotr


Thanks for the bug report.  I think comedi_read() in 
"drivers/staging/comedi/comedi_fops.c" needs a call to 
set_current_state(TASK_RUNNING) just before the call to copy_to_user(...).


Similarly, comedi_write() needs a call to set_current_task(TASK_RUNNING) 
just before the call to copy_from_user(...), and another call to 
set_current_task(TASK_RUNNING) before the call to mutex_lock().


I'll work on a patch.

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

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

On 16/07/17 14:50, Ian Abbott wrote:

On 16/07/17 04:24, kbuild test robot wrote:
tree:   
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

head:   5771a8c08880cdca3bfb4a3fc6d309d6bba20877
commit: c7acec713d14c6ce8a20154f9dfda258d6bcad3b kernel.h: handle 
pointers to arrays better in container_of()

date:   3 days ago
config: ia64-allyesconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross 
-O ~/bin/make.cross

chmod +x ~/bin/make.cross
git checkout c7acec713d14c6ce8a20154f9dfda258d6bcad3b
# save the attached .config to linux build tree
make.cross ARCH=ia64

All errors (new ones prefixed by >>):

   In file included from drivers/clocksource/timer-of.c:25:0:
   drivers/clocksource/timer-of.h:35:28: error: field 'clkevt' has 
incomplete type

 struct clock_event_device clkevt;
   ^~
   In file included from include/linux/err.h:4:0,
from include/linux/clk.h:15,
from drivers/clocksource/timer-of.c:18:
   drivers/clocksource/timer-of.h: In function 'to_timer_of':
include/linux/kernel.h:860:32: error: dereferencing pointer to 
incomplete type 'struct clock_event_device'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~
   include/linux/compiler.h:517:19: note: in definition of macro 
'__compiletime_assert'

  bool __cond = !(condition);\
  ^
   include/linux/compiler.h:537:2: note: in expansion of macro 
'_compiletime_assert'

 _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
 ^~~
   include/linux/build_bug.h:46:37: note: in expansion of macro 
'compiletime_assert'

#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
   include/linux/kernel.h:860:2: note: in expansion of macro 
'BUILD_BUG_ON_MSG'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
 ^~~~
   include/linux/kernel.h:860:20: note: in expansion of macro 
'__same_type'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~~
   drivers/clocksource/timer-of.h:44:9: note: in expansion of macro 
'container_of'

 return container_of(clkevt, struct timer_of, clkevt);
^~~~
--
   In file included from drivers//clocksource/timer-of.c:25:0:
   drivers//clocksource/timer-of.h:35:28: error: field 'clkevt' has 
incomplete type

 struct clock_event_device clkevt;
   ^~
   In file included from include/linux/err.h:4:0,
from include/linux/clk.h:15,
from drivers//clocksource/timer-of.c:18:
   drivers//clocksource/timer-of.h: In function 'to_timer_of':
include/linux/kernel.h:860:32: error: dereferencing pointer to 
incomplete type 'struct clock_event_device'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~
   include/linux/compiler.h:517:19: note: in definition of macro 
'__compiletime_assert'

  bool __cond = !(condition);\
  ^
   include/linux/compiler.h:537:2: note: in expansion of macro 
'_compiletime_assert'

 _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
 ^~~
   include/linux/build_bug.h:46:37: note: in expansion of macro 
'compiletime_assert'

#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
   include/linux/kernel.h:860:2: note: in expansion of macro 
'BUILD_BUG_ON_MSG'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
 ^~~~
   include/linux/kernel.h:860:20: note: in expansion of macro 
'__same_type'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~~
   drivers//clocksource/timer-of.h:44:9: note: in expansion of macro 
'container_of'

 return container_of(clkevt, struct timer_of, clkevt);
^~~~

vim +860 include/linux/kernel.h

   843
   844
   845/*
   846 * swap - swap value of @a and @b
   847 */
   848#define swap(a, b) \
   849do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } 
while (0)

   850
   851/**
   852 * container_of - cast a member of a structure out to the 
containing structure

   853 * @ptr:the pointer to the member.
   854 * @type:the type of the container struct this is 
embedded in.

   855 * @member:the name of the member within the struct.
   856 *
   857 */
   858#define container_of(ptr, type, member) ({\

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

On 16/07/17 14:50, Ian Abbott wrote:

On 16/07/17 04:24, kbuild test robot wrote:
tree:   
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

head:   5771a8c08880cdca3bfb4a3fc6d309d6bba20877
commit: c7acec713d14c6ce8a20154f9dfda258d6bcad3b kernel.h: handle 
pointers to arrays better in container_of()

date:   3 days ago
config: ia64-allyesconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross 
-O ~/bin/make.cross

chmod +x ~/bin/make.cross
git checkout c7acec713d14c6ce8a20154f9dfda258d6bcad3b
# save the attached .config to linux build tree
make.cross ARCH=ia64

All errors (new ones prefixed by >>):

   In file included from drivers/clocksource/timer-of.c:25:0:
   drivers/clocksource/timer-of.h:35:28: error: field 'clkevt' has 
incomplete type

 struct clock_event_device clkevt;
   ^~
   In file included from include/linux/err.h:4:0,
from include/linux/clk.h:15,
from drivers/clocksource/timer-of.c:18:
   drivers/clocksource/timer-of.h: In function 'to_timer_of':
include/linux/kernel.h:860:32: error: dereferencing pointer to 
incomplete type 'struct clock_event_device'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~
   include/linux/compiler.h:517:19: note: in definition of macro 
'__compiletime_assert'

  bool __cond = !(condition);\
  ^
   include/linux/compiler.h:537:2: note: in expansion of macro 
'_compiletime_assert'

 _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
 ^~~
   include/linux/build_bug.h:46:37: note: in expansion of macro 
'compiletime_assert'

#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
   include/linux/kernel.h:860:2: note: in expansion of macro 
'BUILD_BUG_ON_MSG'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
 ^~~~
   include/linux/kernel.h:860:20: note: in expansion of macro 
'__same_type'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~~
   drivers/clocksource/timer-of.h:44:9: note: in expansion of macro 
'container_of'

 return container_of(clkevt, struct timer_of, clkevt);
^~~~
--
   In file included from drivers//clocksource/timer-of.c:25:0:
   drivers//clocksource/timer-of.h:35:28: error: field 'clkevt' has 
incomplete type

 struct clock_event_device clkevt;
   ^~
   In file included from include/linux/err.h:4:0,
from include/linux/clk.h:15,
from drivers//clocksource/timer-of.c:18:
   drivers//clocksource/timer-of.h: In function 'to_timer_of':
include/linux/kernel.h:860:32: error: dereferencing pointer to 
incomplete type 'struct clock_event_device'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~
   include/linux/compiler.h:517:19: note: in definition of macro 
'__compiletime_assert'

  bool __cond = !(condition);\
  ^
   include/linux/compiler.h:537:2: note: in expansion of macro 
'_compiletime_assert'

 _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
 ^~~
   include/linux/build_bug.h:46:37: note: in expansion of macro 
'compiletime_assert'

#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
   include/linux/kernel.h:860:2: note: in expansion of macro 
'BUILD_BUG_ON_MSG'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
 ^~~~
   include/linux/kernel.h:860:20: note: in expansion of macro 
'__same_type'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~~
   drivers//clocksource/timer-of.h:44:9: note: in expansion of macro 
'container_of'

 return container_of(clkevt, struct timer_of, clkevt);
^~~~

vim +860 include/linux/kernel.h

   843
   844
   845/*
   846 * swap - swap value of @a and @b
   847 */
   848#define swap(a, b) \
   849do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } 
while (0)

   850
   851/**
   852 * container_of - cast a member of a structure out to the 
containing structure

   853 * @ptr:the pointer to the member.
   854 * @type:the type of the container struct this is 
embedded in.

   855 * @member:the name of the member within the struct.
   856 *
   857 */
   858#define container_of(ptr, type, member) ({\

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

On 16/07/17 15:14, Ian Abbott wrote:

On 16/07/17 14:50, Ian Abbott wrote:

On 16/07/17 04:24, kbuild test robot wrote:
tree: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master

head:   5771a8c08880cdca3bfb4a3fc6d309d6bba20877
commit: c7acec713d14c6ce8a20154f9dfda258d6bcad3b kernel.h: handle 
pointers to arrays better in container_of()

date:   3 days ago
config: ia64-allyesconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross 
-O ~/bin/make.cross

chmod +x ~/bin/make.cross
git checkout c7acec713d14c6ce8a20154f9dfda258d6bcad3b
# save the attached .config to linux build tree
make.cross ARCH=ia64

All errors (new ones prefixed by >>):

   In file included from drivers/clocksource/timer-of.c:25:0:
   drivers/clocksource/timer-of.h:35:28: error: field 'clkevt' has 
incomplete type

 struct clock_event_device clkevt;
   ^~
   In file included from include/linux/err.h:4:0,
from include/linux/clk.h:15,
from drivers/clocksource/timer-of.c:18:
   drivers/clocksource/timer-of.h: In function 'to_timer_of':
include/linux/kernel.h:860:32: error: dereferencing pointer to 
incomplete type 'struct clock_event_device'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~
   include/linux/compiler.h:517:19: note: in definition of macro 
'__compiletime_assert'

  bool __cond = !(condition);\
  ^
   include/linux/compiler.h:537:2: note: in expansion of macro 
'_compiletime_assert'
 _compiletime_assert(condition, msg, __compiletime_assert_, 
__LINE__)

 ^~~
   include/linux/build_bug.h:46:37: note: in expansion of macro 
'compiletime_assert'

#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
   include/linux/kernel.h:860:2: note: in expansion of macro 
'BUILD_BUG_ON_MSG'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
 ^~~~
   include/linux/kernel.h:860:20: note: in expansion of macro 
'__same_type'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~~
   drivers/clocksource/timer-of.h:44:9: note: in expansion of macro 
'container_of'

 return container_of(clkevt, struct timer_of, clkevt);
^~~~
--
   In file included from drivers//clocksource/timer-of.c:25:0:
   drivers//clocksource/timer-of.h:35:28: error: field 'clkevt' has 
incomplete type

 struct clock_event_device clkevt;
   ^~
   In file included from include/linux/err.h:4:0,
from include/linux/clk.h:15,
from drivers//clocksource/timer-of.c:18:
   drivers//clocksource/timer-of.h: In function 'to_timer_of':
include/linux/kernel.h:860:32: error: dereferencing pointer to 
incomplete type 'struct clock_event_device'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~
   include/linux/compiler.h:517:19: note: in definition of macro 
'__compiletime_assert'

  bool __cond = !(condition);\
  ^
   include/linux/compiler.h:537:2: note: in expansion of macro 
'_compiletime_assert'
 _compiletime_assert(condition, msg, __compiletime_assert_, 
__LINE__)

 ^~~
   include/linux/build_bug.h:46:37: note: in expansion of macro 
'compiletime_assert'

#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
   include/linux/kernel.h:860:2: note: in expansion of macro 
'BUILD_BUG_ON_MSG'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
 ^~~~
   include/linux/kernel.h:860:20: note: in expansion of macro 
'__same_type'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~~
   drivers//clocksource/timer-of.h:44:9: note: in expansion of macro 
'container_of'

 return container_of(clkevt, struct timer_of, clkevt);
^~~~

vim +860 include/linux/kernel.h

   843
   844
   845/*
   846 * swap - swap value of @a and @b
   847 */
   848#define swap(a, b) \
   849do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } 
while (0)

   850
   851/**
   852 * container_of - cast a member of a structure out to the 
containing structure

   853 * @ptr:the pointer to the member.
   854 * @type:the type of the container struct this is 
embedded in.

   855 * @member:the name of the member within the struct.
   856 *
   857 */
   858#define container_of(p

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

On 16/07/17 15:14, Ian Abbott wrote:

On 16/07/17 14:50, Ian Abbott wrote:

On 16/07/17 04:24, kbuild test robot wrote:
tree: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master

head:   5771a8c08880cdca3bfb4a3fc6d309d6bba20877
commit: c7acec713d14c6ce8a20154f9dfda258d6bcad3b kernel.h: handle 
pointers to arrays better in container_of()

date:   3 days ago
config: ia64-allyesconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross 
-O ~/bin/make.cross

chmod +x ~/bin/make.cross
git checkout c7acec713d14c6ce8a20154f9dfda258d6bcad3b
# save the attached .config to linux build tree
make.cross ARCH=ia64

All errors (new ones prefixed by >>):

   In file included from drivers/clocksource/timer-of.c:25:0:
   drivers/clocksource/timer-of.h:35:28: error: field 'clkevt' has 
incomplete type

 struct clock_event_device clkevt;
   ^~
   In file included from include/linux/err.h:4:0,
from include/linux/clk.h:15,
from drivers/clocksource/timer-of.c:18:
   drivers/clocksource/timer-of.h: In function 'to_timer_of':
include/linux/kernel.h:860:32: error: dereferencing pointer to 
incomplete type 'struct clock_event_device'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~
   include/linux/compiler.h:517:19: note: in definition of macro 
'__compiletime_assert'

  bool __cond = !(condition);\
  ^
   include/linux/compiler.h:537:2: note: in expansion of macro 
'_compiletime_assert'
 _compiletime_assert(condition, msg, __compiletime_assert_, 
__LINE__)

 ^~~
   include/linux/build_bug.h:46:37: note: in expansion of macro 
'compiletime_assert'

#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
   include/linux/kernel.h:860:2: note: in expansion of macro 
'BUILD_BUG_ON_MSG'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
 ^~~~
   include/linux/kernel.h:860:20: note: in expansion of macro 
'__same_type'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~~
   drivers/clocksource/timer-of.h:44:9: note: in expansion of macro 
'container_of'

 return container_of(clkevt, struct timer_of, clkevt);
^~~~
--
   In file included from drivers//clocksource/timer-of.c:25:0:
   drivers//clocksource/timer-of.h:35:28: error: field 'clkevt' has 
incomplete type

 struct clock_event_device clkevt;
   ^~
   In file included from include/linux/err.h:4:0,
from include/linux/clk.h:15,
from drivers//clocksource/timer-of.c:18:
   drivers//clocksource/timer-of.h: In function 'to_timer_of':
include/linux/kernel.h:860:32: error: dereferencing pointer to 
incomplete type 'struct clock_event_device'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~
   include/linux/compiler.h:517:19: note: in definition of macro 
'__compiletime_assert'

  bool __cond = !(condition);\
  ^
   include/linux/compiler.h:537:2: note: in expansion of macro 
'_compiletime_assert'
 _compiletime_assert(condition, msg, __compiletime_assert_, 
__LINE__)

 ^~~
   include/linux/build_bug.h:46:37: note: in expansion of macro 
'compiletime_assert'

#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
   include/linux/kernel.h:860:2: note: in expansion of macro 
'BUILD_BUG_ON_MSG'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
 ^~~~
   include/linux/kernel.h:860:20: note: in expansion of macro 
'__same_type'

 BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
   ^~~
   drivers//clocksource/timer-of.h:44:9: note: in expansion of macro 
'container_of'

 return container_of(clkevt, struct timer_of, clkevt);
^~~~

vim +860 include/linux/kernel.h

   843
   844
   845/*
   846 * swap - swap value of @a and @b
   847 */
   848#define swap(a, b) \
   849do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } 
while (0)

   850
   851/**
   852 * container_of - cast a member of a structure out to the 
containing structure

   853 * @ptr:the pointer to the member.
   854 * @type:the type of the container struct this is 
embedded in.

   855 * @member:the name of the member within the struct.
   856 *
   857 */
   858#define container_of(p

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

\
 > 860   BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) 
&&\
   861   !__same_type(*(ptr), void),
\
   862   "pointer type mismatch in container_of()");  \
   863  ((type *)(__mptr - offsetof(type, member))); })
   864  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation



struct clock_event_device is only completely defined when 
CONFIG_GENERIC_CLOCKEVENTS is defined, which it isn't.  But I'm confused 
as to why TIMER_OF getting selected by allyesconfig since it depends on 
GENERIC_CLOCKEVENTS.


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

Re: include/linux/kernel.h:860:32: error: dereferencing pointer to incomplete type 'struct clock_event_device'

[Ian Abbott]

\
 > 860   BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) 
&&\
   861   !__same_type(*(ptr), void),
\
   862   "pointer type mismatch in container_of()");  \
   863  ((type *)(__mptr - offsetof(type, member))); })
   864  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation



struct clock_event_device is only completely defined when 
CONFIG_GENERIC_CLOCKEVENTS is defined, which it isn't.  But I'm confused 
as to why TIMER_OF getting selected by allyesconfig since it depends on 
GENERIC_CLOCKEVENTS.


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

Re: [PATCH] staging: comedi: Use offset_in_page macro

[Ian Abbott]

On 04/07/17 00:13, Amitoj Kaur Chawla wrote:

Use offset_in_page macro instead of (var & ~PAGE_MASK)

The Coccinelle semantic patch used to make this change is as follows:
// 
@@
unsigned long p;
@@
- p & ~PAGE_MASK
+ offset_in_page(p)
// 

Signed-off-by: Amitoj Kaur Chawla <amitoj1...@gmail.com>
---
 drivers/staging/comedi/comedi_buf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_buf.c 
b/drivers/staging/comedi/comedi_buf.c
index 8e9b30b..b455ff6 100644
--- a/drivers/staging/comedi/comedi_buf.c
+++ b/drivers/staging/comedi/comedi_buf.c
@@ -165,7 +165,7 @@ int comedi_buf_map_put(struct comedi_buf_map *bm)
 int comedi_buf_map_access(struct comedi_buf_map *bm, unsigned long offset,
  void *buf, int len, int write)
 {
-   unsigned int pgoff = offset & ~PAGE_MASK;
+   unsigned int pgoff = offset_in_page(offset);
unsigned long pg = offset >> PAGE_SHIFT;
int done = 0;




Seems reasonable, thanks.  Being picky, I'd prefer it if the line 
`#include ` was added, since that is where the 
`offset_in_page` macro is defined.  But it doesn't matter that much as 
the "mm.h" header file gets included indirectly, and there are plenty of 
examples where `offset_in_page` is used without including  
directly.


Signed-off-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: Use offset_in_page macro

[Ian Abbott]

On 04/07/17 00:13, Amitoj Kaur Chawla wrote:

Use offset_in_page macro instead of (var & ~PAGE_MASK)

The Coccinelle semantic patch used to make this change is as follows:
// 
@@
unsigned long p;
@@
- p & ~PAGE_MASK
+ offset_in_page(p)
// 

Signed-off-by: Amitoj Kaur Chawla 
---
 drivers/staging/comedi/comedi_buf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_buf.c 
b/drivers/staging/comedi/comedi_buf.c
index 8e9b30b..b455ff6 100644
--- a/drivers/staging/comedi/comedi_buf.c
+++ b/drivers/staging/comedi/comedi_buf.c
@@ -165,7 +165,7 @@ int comedi_buf_map_put(struct comedi_buf_map *bm)
 int comedi_buf_map_access(struct comedi_buf_map *bm, unsigned long offset,
  void *buf, int len, int write)
 {
-   unsigned int pgoff = offset & ~PAGE_MASK;
+   unsigned int pgoff = offset_in_page(offset);
unsigned long pg = offset >> PAGE_SHIFT;
int done = 0;




Seems reasonable, thanks.  Being picky, I'd prefer it if the line 
`#include ` was added, since that is where the 
`offset_in_page` macro is defined.  But it doesn't matter that much as 
the "mm.h" header file gets included indirectly, and there are plenty of 
examples where `offset_in_page` is used without including  
directly.


Signed-off-by: Ian Abbott 

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

[PATCH] staging: comedi: ni_mio_common: fix AO timer off-by-one regression

[Ian Abbott]

As reported by Éric Piel on the Comedi mailing list (see
<https://groups.google.com/forum/#!topic/comedi_list/ueZiR7vTLOU/discussion>),
the analog output asynchronous commands are running too fast with a
period 50 ns shorter than it should be.  This affects all boards with AO
command support that are supported by the "ni_pcimio", "ni_atmio", and
"ni_mio_cs" drivers.

This is a regression bug introduced by commit 080e6795cba3 ("staging:
comedi: ni_mio_common: Cleans up/clarifies ni_ao_cmd"), specifically,
this line in `ni_ao_cmd_set_update()`:

/* following line: N-1 per STC */
ni_stc_writel(dev, trigvar - 1, NISTC_AO_UI_LOADA_REG);

The `trigvar` variable value comes from a call to `ni_ns_to_timer()`
which converts a timer period in nanoseconds to a hardware divisor
value. The function already reduces the divisor by 1 as required by the
hardware, so the above line should not reduce it further by 1.  Fix it
by replacing `trigvar` by `trigvar - 1` in the above line, and remove
the misleading comment.

Reported-by: Éric Piel <p...@delmic.com>
Fixes: 080e6795cba3 ("staging: comedi: ni_mio_common: Cleans up/clarifies 
ni_ao_cmd")
Cc: Éric Piel <p...@delmic.com>
Cc: Spencer E. Olson <olso...@umich.edu>
Cc: <sta...@vger.kernel.org> # 4.7+
Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
---
The bug is also in 4.6.x, but this patch does not apply to it cleanly.
---
 drivers/staging/comedi/drivers/ni_mio_common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c 
b/drivers/staging/comedi/drivers/ni_mio_common.c
index b2e382888981..2f7bfc1c59e5 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -3116,8 +3116,7 @@ static void ni_ao_cmd_set_update(struct comedi_device 
*dev,
/* following line: 2-1 per STC */
ni_stc_writel(dev, 1, NISTC_AO_UI_LOADA_REG);
ni_stc_writew(dev, NISTC_AO_CMD1_UI_LOAD, NISTC_AO_CMD1_REG);
-   /* following line: N-1 per STC */
-   ni_stc_writel(dev, trigvar - 1, NISTC_AO_UI_LOADA_REG);
+   ni_stc_writel(dev, trigvar, NISTC_AO_UI_LOADA_REG);
} else { /* TRIG_EXT */
/* FIXME:  assert scan_begin_arg != 0, ret failure otherwise */
devpriv->ao_cmd2  |= NISTC_AO_CMD2_BC_GATE_ENA;
-- 
2.11.0

[PATCH] staging: comedi: ni_mio_common: fix AO timer off-by-one regression

[Ian Abbott]

As reported by Éric Piel on the Comedi mailing list (see
<https://groups.google.com/forum/#!topic/comedi_list/ueZiR7vTLOU/discussion>),
the analog output asynchronous commands are running too fast with a
period 50 ns shorter than it should be.  This affects all boards with AO
command support that are supported by the "ni_pcimio", "ni_atmio", and
"ni_mio_cs" drivers.

This is a regression bug introduced by commit 080e6795cba3 ("staging:
comedi: ni_mio_common: Cleans up/clarifies ni_ao_cmd"), specifically,
this line in `ni_ao_cmd_set_update()`:

/* following line: N-1 per STC */
ni_stc_writel(dev, trigvar - 1, NISTC_AO_UI_LOADA_REG);

The `trigvar` variable value comes from a call to `ni_ns_to_timer()`
which converts a timer period in nanoseconds to a hardware divisor
value. The function already reduces the divisor by 1 as required by the
hardware, so the above line should not reduce it further by 1.  Fix it
by replacing `trigvar` by `trigvar - 1` in the above line, and remove
the misleading comment.

Reported-by: Éric Piel 
Fixes: 080e6795cba3 ("staging: comedi: ni_mio_common: Cleans up/clarifies 
ni_ao_cmd")
Cc: Éric Piel 
Cc: Spencer E. Olson 
Cc:  # 4.7+
Signed-off-by: Ian Abbott 
---
The bug is also in 4.6.x, but this patch does not apply to it cleanly.
---
 drivers/staging/comedi/drivers/ni_mio_common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c 
b/drivers/staging/comedi/drivers/ni_mio_common.c
index b2e382888981..2f7bfc1c59e5 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -3116,8 +3116,7 @@ static void ni_ao_cmd_set_update(struct comedi_device 
*dev,
/* following line: 2-1 per STC */
ni_stc_writel(dev, 1, NISTC_AO_UI_LOADA_REG);
ni_stc_writew(dev, NISTC_AO_CMD1_UI_LOAD, NISTC_AO_CMD1_REG);
-   /* following line: N-1 per STC */
-   ni_stc_writel(dev, trigvar - 1, NISTC_AO_UI_LOADA_REG);
+   ni_stc_writel(dev, trigvar, NISTC_AO_UI_LOADA_REG);
} else { /* TRIG_EXT */
/* FIXME:  assert scan_begin_arg != 0, ret failure otherwise */
devpriv->ao_cmd2  |= NISTC_AO_CMD2_BC_GATE_ENA;
-- 
2.11.0

Re: [PATCH] kernel.h: fix new warnings for container_of()

[Ian Abbott]

On 20/06/17 21:09, Arnd Bergmann wrote:

I see new warnings with gcc-7.0.1 with the modified container_of():

fs/f2fs/dir.c: In function 'F2FS_I':
fs/f2fs/f2fs.h:1122:385: note: found mismatched ssa struct pointer types: 
'struct f2fs_inode_info' and 'struct inode'


Is that actually a warning, or just informational?  In any case, it 
seems like a good idea to avoid it.




This seems to happen for all structures that have a zero offset
between the member and the container structure, i.e. idential
pointers.

Reverting to an intermediate pointer avoids the warning, and using
a void pointer instead of the target type should also avoid
regressing on the previous patch again.

Fixes: mmotm ("kernel.h: handle pointers to arrays better in container_of()")
Signed-off-by: Arnd Bergmann <a...@arndb.de>
---
 include/linux/kernel.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d043adadcf33..bd6d96cf80b1 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -856,10 +856,11 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
oops_dump_mode) { }
  *
  */
 #define container_of(ptr, type, member) ({ \
+   void *__mptr = (void *)(ptr);   \
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))); })
+   ((type *)(__mptr - offsetof(type, member))); })

 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD



Acked-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] kernel.h: fix new warnings for container_of()

[Ian Abbott]

On 20/06/17 21:09, Arnd Bergmann wrote:

I see new warnings with gcc-7.0.1 with the modified container_of():

fs/f2fs/dir.c: In function 'F2FS_I':
fs/f2fs/f2fs.h:1122:385: note: found mismatched ssa struct pointer types: 
'struct f2fs_inode_info' and 'struct inode'


Is that actually a warning, or just informational?  In any case, it 
seems like a good idea to avoid it.




This seems to happen for all structures that have a zero offset
between the member and the container structure, i.e. idential
pointers.

Reverting to an intermediate pointer avoids the warning, and using
a void pointer instead of the target type should also avoid
regressing on the previous patch again.

Fixes: mmotm ("kernel.h: handle pointers to arrays better in container_of()")
Signed-off-by: Arnd Bergmann 
---
 include/linux/kernel.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d043adadcf33..bd6d96cf80b1 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -856,10 +856,11 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
oops_dump_mode) { }
  *
  */
 #define container_of(ptr, type, member) ({ \
+   void *__mptr = (void *)(ptr);   \
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))); })
+   ((type *)(__mptr - offsetof(type, member))); })

 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD



Acked-by: Ian Abbott 

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

Re: [PATCH v5 1/6] asm-generic/bug.h: declare struct pt_regs; before function prototype

[Ian Abbott]

On 08/06/17 15:07, Steven Rostedt wrote:

On Thu, 25 May 2017 13:03:11 +0100
Ian Abbott <abbo...@mev.co.uk> wrote:


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.



Not sure if this has been pulled already or not, but I have a small nit.


It's already in linux-next master.




Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Arnd Bergmann <a...@arndb.de>
Acked-by: Arnd Bergmann <a...@arndb.de>
Acked-by: Michal Nazarewicz <min...@mina86.com>
---
v3: Actually, there was no v1 or v2.  I called this v3 to match the
series.
v4: Corrected 'Acked-by:' line in patch description.
v5: Added Acked-by for Michal Nazarewicz.
---
 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;


Probably be better to move pt_regs above the comment. For one, it is
used before warn_args in the function below. Two, it's not defined
internally by panic.c like warn_args is.


Alternatively, the comment could be placed on the same line as `struct 
warn_args;`.  I don't think it's a big enough of a deal to patch it for 
the sake of it, tbh.




-- Steve




 void __warn(const char *file, int line, void *caller, unsigned taint,
struct pt_regs *regs, struct warn_args *args);





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

Re: [PATCH v5 1/6] asm-generic/bug.h: declare struct pt_regs; before function prototype

[Ian Abbott]

On 08/06/17 15:07, Steven Rostedt wrote:

On Thu, 25 May 2017 13:03:11 +0100
Ian Abbott  wrote:


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.



Not sure if this has been pulled already or not, but I have a small nit.


It's already in linux-next master.




Signed-off-by: Ian Abbott 
Cc: Arnd Bergmann 
Acked-by: Arnd Bergmann 
Acked-by: Michal Nazarewicz 
---
v3: Actually, there was no v1 or v2.  I called this v3 to match the
series.
v4: Corrected 'Acked-by:' line in patch description.
v5: Added Acked-by for Michal Nazarewicz.
---
 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;


Probably be better to move pt_regs above the comment. For one, it is
used before warn_args in the function below. Two, it's not defined
internally by panic.c like warn_args is.


Alternatively, the comment could be placed on the same line as `struct 
warn_args;`.  I don't think it's a big enough of a deal to patch it for 
the sake of it, tbh.




-- Steve




 void __warn(const char *file, int line, void *caller, unsigned taint,
struct pt_regs *regs, struct warn_args *args);





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

Re: [PATCH] Staging: comedi: ni_labpc_regs: fixed a block comment alignment issue

[Ian Abbott]

On 2017-06-07 19:31, Amisha Singh wrote:

Fixed a coding style issue.

Signed-off-by: Amisha Singh <amisha.s...@gmail.com>
---
 drivers/staging/comedi/drivers/ni_labpc_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_labpc_regs.h 
b/drivers/staging/comedi/drivers/ni_labpc_regs.h
index 8c52179..6003e9d 100644
--- a/drivers/staging/comedi/drivers/ni_labpc_regs.h
+++ b/drivers/staging/comedi/drivers/ni_labpc_regs.h
@@ -1,6 +1,6 @@
 /*
  * ni_labpc register definitions.
-*/
+ */

 #ifndef _NI_LABPC_REGS_H
 #define _NI_LABPC_REGS_H



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_labpc_regs: fixed a block comment alignment issue

[Ian Abbott]

On 2017-06-07 19:31, Amisha Singh wrote:

Fixed a coding style issue.

Signed-off-by: Amisha Singh 
---
 drivers/staging/comedi/drivers/ni_labpc_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_labpc_regs.h 
b/drivers/staging/comedi/drivers/ni_labpc_regs.h
index 8c52179..6003e9d 100644
--- a/drivers/staging/comedi/drivers/ni_labpc_regs.h
+++ b/drivers/staging/comedi/drivers/ni_labpc_regs.h
@@ -1,6 +1,6 @@
 /*
  * ni_labpc register definitions.
-*/
+ */

 #ifndef _NI_LABPC_REGS_H
 #define _NI_LABPC_REGS_H



Thanks!

Reviewed-by: Ian Abbott 

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

Re: [PATCH v2] xen: avoid type warning in xchg_xen_ulong

[Ian Abbott]

On 2017-06-08 09:53, Arnd Bergmann wrote:

The improved type-checking version of container_of() triggers a warning for
xchg_xen_ulong, pointing out that 'xen_ulong_t' is unsigned, but atomic64_t
contains a signed value:

drivers/xen/events/events_2l.c: In function 'evtchn_2l_handle_events':
drivers/xen/events/events_2l.c:187:1020: error: call to 
'__compiletime_assert_187' declared with attribute error: pointer type mismatch 
in container_of()

This adds a cast to work around the warning.

Cc: Ian Abbott <abbo...@mev.co.uk>
Fixes: 85323a991d40 ("xen: arm: mandate EABI and use generic atomic 
operations.")
Fixes: daa2ac80834d ("kernel.h: handle pointers to arrays better in 
container_of()")
Signed-off-by: Arnd Bergmann <a...@arndb.de>
---
v2: found the correct warning message and updated the changelog
---
 arch/arm/include/asm/xen/events.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/xen/events.h 
b/arch/arm/include/asm/xen/events.h
index 71e473d05fcc..620dc75362e5 100644
--- a/arch/arm/include/asm/xen/events.h
+++ b/arch/arm/include/asm/xen/events.h
@@ -16,7 +16,7 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
return raw_irqs_disabled_flags(regs->ARM_cpsr);
 }

-#define xchg_xen_ulong(ptr, val) atomic64_xchg(container_of((ptr), \
+#define xchg_xen_ulong(ptr, val) atomic64_xchg(container_of((long long*)(ptr),\
atomic64_t, \
    counter), (val))




Acked-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] xen: avoid type warning in xchg_xen_ulong

[Ian Abbott]

On 2017-06-08 09:53, Arnd Bergmann wrote:

The improved type-checking version of container_of() triggers a warning for
xchg_xen_ulong, pointing out that 'xen_ulong_t' is unsigned, but atomic64_t
contains a signed value:

drivers/xen/events/events_2l.c: In function 'evtchn_2l_handle_events':
drivers/xen/events/events_2l.c:187:1020: error: call to 
'__compiletime_assert_187' declared with attribute error: pointer type mismatch 
in container_of()

This adds a cast to work around the warning.

Cc: Ian Abbott 
Fixes: 85323a991d40 ("xen: arm: mandate EABI and use generic atomic 
operations.")
Fixes: daa2ac80834d ("kernel.h: handle pointers to arrays better in 
container_of()")
Signed-off-by: Arnd Bergmann 
---
v2: found the correct warning message and updated the changelog
---
 arch/arm/include/asm/xen/events.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/xen/events.h 
b/arch/arm/include/asm/xen/events.h
index 71e473d05fcc..620dc75362e5 100644
--- a/arch/arm/include/asm/xen/events.h
+++ b/arch/arm/include/asm/xen/events.h
@@ -16,7 +16,7 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
return raw_irqs_disabled_flags(regs->ARM_cpsr);
 }

-#define xchg_xen_ulong(ptr, val) atomic64_xchg(container_of((ptr), \
+#define xchg_xen_ulong(ptr, val) atomic64_xchg(container_of((long long*)(ptr),\
atomic64_t, \
counter), (val))




Acked-by: Ian Abbott 

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

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

[Ian Abbott]

On 25/05/17 19:35, Kees Cook wrote:

On Thu, May 25, 2017 at 5:03 AM, Ian Abbott <abbo...@mev.co.uk> 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);
 ^
---

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>
Acked-by: Michal Nazarewicz <min...@mina86.com>


Seems reasonable to me. I think this actually improves the errors
reported when something is mismatched in container_of(). Silly
question: does this pass an allyesconfig?


Yes for ARCH=i386 at least.



Acked-by: Kees Cook <keesc...@chromium.org>


Thanks!

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

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

[Ian Abbott]

On 25/05/17 19:35, Kees Cook wrote:

On Thu, May 25, 2017 at 5:03 AM, 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);
 ^
---

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 
Acked-by: Michal Nazarewicz 


Seems reasonable to me. I think this actually improves the errors
reported when something is mismatched in container_of(). Silly
question: does this pass an allyesconfig?


Yes for ARCH=i386 at least.



Acked-by: Kees Cook 


Thanks!

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

[PATCH v5 0/6] kernel.h: container_of() pointer checking

[Ian Abbott]

TL;DR: This series of patches splits BUILD_BUG related macros out of
"include/linux/bug.h" into new file "include/linux/build_bug.h" (patch
5), and changes the pointer type checking in the `container_of()` macro
to deal with pointers of array type better (patch 6).  Patches 1 to 4
are prerequisites.

Patches 2, 3, 4, and 5 have been inserted since the previous version of
this patch series.  Patch 6 here corresponds to v3 and v4's patch 2.

Patch 1 was a prerequisite in v3 of this series to avoid a lot of
warnings when  was included by .  That is
no longer relevant for v5 of the series, but I left it in because it was
acked by a Arnd Bergmann and Michal Nazarewicz.

Patches 2, 3, and 4 are some checkpatch clean-ups on
"include/linux/bug.h" before splitting out the BUILD_BUG stuff in patch
5.

Patch 5 splits the BUILD_BUG related macros out of
"include/linux/bug.h" into new file "include/linux/build_bug.h" because
including  in "include/linux/kernel.h" would result in
build failures due to circular dependencies.

Patch 6 changes the pointer type checking by `container_of()` to avoid
some incompatible pointer warnings when the dereferenced pointer has
array type.

1) asm-generic/bug.h: declare struct pt_regs; before function prototype
2) linux/bug.h: correct formatting of block comment
3) linux/bug.h: correct "(foo*)" should be "(foo *)"
4) linux/bug.h: correct "space required before that '-'"
5) bug: split BUILD_BUG stuff out into 
6) kernel.h: handle pointers to arrays better in container_of()

 include/asm-generic/bug.h |  1 +
 include/linux/bug.h   | 72 +---
 include/linux/build_bug.h | 84 +++
 include/linux/kernel.h|  9 +++--
 4 files changed, 92 insertions(+), 74 deletions(-)

[PATCH v5 0/6] kernel.h: container_of() pointer checking

[Ian Abbott]

TL;DR: This series of patches splits BUILD_BUG related macros out of
"include/linux/bug.h" into new file "include/linux/build_bug.h" (patch
5), and changes the pointer type checking in the `container_of()` macro
to deal with pointers of array type better (patch 6).  Patches 1 to 4
are prerequisites.

Patches 2, 3, 4, and 5 have been inserted since the previous version of
this patch series.  Patch 6 here corresponds to v3 and v4's patch 2.

Patch 1 was a prerequisite in v3 of this series to avoid a lot of
warnings when  was included by .  That is
no longer relevant for v5 of the series, but I left it in because it was
acked by a Arnd Bergmann and Michal Nazarewicz.

Patches 2, 3, and 4 are some checkpatch clean-ups on
"include/linux/bug.h" before splitting out the BUILD_BUG stuff in patch
5.

Patch 5 splits the BUILD_BUG related macros out of
"include/linux/bug.h" into new file "include/linux/build_bug.h" because
including  in "include/linux/kernel.h" would result in
build failures due to circular dependencies.

Patch 6 changes the pointer type checking by `container_of()` to avoid
some incompatible pointer warnings when the dereferenced pointer has
array type.

1) asm-generic/bug.h: declare struct pt_regs; before function prototype
2) linux/bug.h: correct formatting of block comment
3) linux/bug.h: correct "(foo*)" should be "(foo *)"
4) linux/bug.h: correct "space required before that '-'"
5) bug: split BUILD_BUG stuff out into 
6) kernel.h: handle pointers to arrays better in container_of()

 include/asm-generic/bug.h |  1 +
 include/linux/bug.h   | 72 +---
 include/linux/build_bug.h | 84 +++
 include/linux/kernel.h|  9 +++--
 4 files changed, 92 insertions(+), 74 deletions(-)

[PATCH v5 4/6] linux/bug.h: correct "space required before that '-'"

[Ian Abbott]

Correct these checkpatch.pl errors:

|ERROR: space required before that '-' (ctx:OxO)
|#37: FILE: include/linux/bug.h:37:
|+#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))

|ERROR: space required before that '-' (ctx:OxO)
|#38: FILE: include/linux/bug.h:38:
|+#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))

I decided to wrap the bitfield expressions that begin with minus signs
in parentheses rather than insert spaces before the minus signs.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Kees Cook <keesc...@chromium.org>
Cc: Steven Rostedt <rost...@goodmis.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Jakub Kicinski <jakub.kicin...@netronome.com>
Cc: Rasmus Villemoes <li...@rasmusvillemoes.dk>
---
v5: Actually, there was no v1 thru v4.  I called this v5 to match the
series.
---
 include/linux/bug.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 216a1b79653d..483207cb99fb 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -36,8 +36,8 @@ struct pt_regs;
  * e.g. in a structure initializer (or where-ever else comma expressions
  * aren't permitted).
  */
-#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
-#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
+#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
+#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:(-!!(e)); }))
 
 /*
  * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the
-- 
2.11.0

[PATCH v5 4/6] linux/bug.h: correct "space required before that '-'"

[Ian Abbott]

Correct these checkpatch.pl errors:

|ERROR: space required before that '-' (ctx:OxO)
|#37: FILE: include/linux/bug.h:37:
|+#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))

|ERROR: space required before that '-' (ctx:OxO)
|#38: FILE: include/linux/bug.h:38:
|+#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))

I decided to wrap the bitfield expressions that begin with minus signs
in parentheses rather than insert spaces before the minus signs.

Signed-off-by: Ian Abbott 
Cc: Andrew Morton 
Cc: Kees Cook 
Cc: Steven Rostedt 
Cc: Peter Zijlstra 
Cc: Jakub Kicinski 
Cc: Rasmus Villemoes 
---
v5: Actually, there was no v1 thru v4.  I called this v5 to match the
series.
---
 include/linux/bug.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 216a1b79653d..483207cb99fb 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -36,8 +36,8 @@ struct pt_regs;
  * e.g. in a structure initializer (or where-ever else comma expressions
  * aren't permitted).
  */
-#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
-#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
+#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
+#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:(-!!(e)); }))
 
 /*
  * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the
-- 
2.11.0

[PATCH v5 5/6] bug: split BUILD_BUG stuff out into

[Ian Abbott]

Including  pulls in a lot of bloat from  and
 that is not needed to call the BUILD_BUG() family of
macros.  Split them out into their own header, .

Also correct some checkpatch.pl errors for the BUILD_BUG_ON_ZERO() and
BUILD_BUG_ON_NULL() macros by adding parentheses around the bitfield
widths that begin with a minus sign.

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Kees Cook <keesc...@chromium.org>
Cc: Steven Rostedt <rost...@goodmis.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Jakub Kicinski <jakub.kicin...@netronome.com>
Cc: Rasmus Villemoes <li...@rasmusvillemoes.dk>
---
v5: Actually, there was no v1 thru v4.  I called this v5 to match the
series.
---
 include/linux/bug.h   | 74 +
 include/linux/build_bug.h | 84 +++
 2 files changed, 85 insertions(+), 73 deletions(-)
 create mode 100644 include/linux/build_bug.h

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 483207cb99fb..5d5554c874fd 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,
@@ -13,82 +14,9 @@ enum bug_trap_type {
 struct pt_regs;
 
 #ifdef __CHECKER__
-#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
-#define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
-#define BUILD_BUG_ON_ZERO(e) (0)
-#define BUILD_BUG_ON_NULL(e) ((void *)0)
-#define BUILD_BUG_ON_INVALID(e) (0)
-#define BUILD_BUG_ON_MSG(cond, msg) (0)
-#define BUILD_BUG_ON(condition) (0)
-#define BUILD_BUG() (0)
 #define MAYBE_BUILD_BUG_ON(cond) (0)
 #else /* __CHECKER__ */
 
-/* Force a compilation error if a constant expression is not a power of 2 */
-#define __BUILD_BUG_ON_NOT_POWER_OF_2(n)   \
-   BUILD_BUG_ON(((n) & ((n) - 1)) != 0)
-#define BUILD_BUG_ON_NOT_POWER_OF_2(n) \
-   BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
-
-/*
- * Force a compilation error if condition is true, but also produce a
- * result (of value 0 and type size_t), so the expression can be used
- * e.g. in a structure initializer (or where-ever else comma expressions
- * aren't permitted).
- */
-#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
-#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:(-!!(e)); }))
-
-/*
- * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the
- * expression but avoids the generation of any code, even if that expression
- * has side-effects.
- */
-#define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e
-
-/**
- * BUILD_BUG_ON_MSG - break compile if a condition is true & emit supplied
- *   error message.
- * @condition: the condition which the compiler should know is false.
- *
- * See BUILD_BUG_ON for description.
- */
-#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
-
-/**
- * BUILD_BUG_ON - break compile if a condition is true.
- * @condition: the condition which the compiler should know is false.
- *
- * If you have some code which relies on certain constants being equal, or
- * some other compile-time-evaluated condition, you should use BUILD_BUG_ON to
- * detect if someone changes it.
- *
- * The implementation uses gcc's reluctance to create a negative array, but gcc
- * (as of 4.4) only emits that error for obvious cases (e.g. not arguments to
- * inline functions).  Luckily, in 4.3 they added the "error" function
- * attribute just for this type of case.  Thus, we use a negative sized array
- * (should always create an error on gcc versions older than 4.4) and then call
- * an undefined function with the error attribute (should always create an
- * error on gcc 4.3 and later).  If for some reason, neither creates a
- * compile-time error, we'll still have a link-time error, which is harder to
- * track down.
- */
-#ifndef __OPTIMIZE__
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-#else
-#define BUILD_BUG_ON(condition) \
-   BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
-#endif
-
-/**
- * BUILD_BUG - break compile if used.
- *
- * If you have some code that you expect the compiler to eliminate at
- * build time, you should use BUILD_BUG to detect if it is
- * unexpectedly used.
- */
-#define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
-
 #define MAYBE_BUILD_BUG_ON(cond)   \
do {\
if (__builtin_constant_p((cond)))   \
diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
new file mode 100644
index ..b7d22d60008a
--- /dev/null
+++ b/include/linux/build_bug.h
@@ -0,0 +1,84 @@
+#ifndef _LINUX_BUILD_BUG_H
+#define _LINUX_BUILD_BUG_H
+
+#include 
+
+#ifdef __CHECKER__
+#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
+#define BUILD_BUG_ON_NOT_POWE

[PATCH v5 5/6] bug: split BUILD_BUG stuff out into

[Ian Abbott]

Including  pulls in a lot of bloat from  and
 that is not needed to call the BUILD_BUG() family of
macros.  Split them out into their own header, .

Also correct some checkpatch.pl errors for the BUILD_BUG_ON_ZERO() and
BUILD_BUG_ON_NULL() macros by adding parentheses around the bitfield
widths that begin with a minus sign.

Signed-off-by: Ian Abbott 
Cc: Andrew Morton 
Cc: Kees Cook 
Cc: Steven Rostedt 
Cc: Peter Zijlstra 
Cc: Jakub Kicinski 
Cc: Rasmus Villemoes 
---
v5: Actually, there was no v1 thru v4.  I called this v5 to match the
series.
---
 include/linux/bug.h   | 74 +
 include/linux/build_bug.h | 84 +++
 2 files changed, 85 insertions(+), 73 deletions(-)
 create mode 100644 include/linux/build_bug.h

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 483207cb99fb..5d5554c874fd 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,
@@ -13,82 +14,9 @@ enum bug_trap_type {
 struct pt_regs;
 
 #ifdef __CHECKER__
-#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
-#define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
-#define BUILD_BUG_ON_ZERO(e) (0)
-#define BUILD_BUG_ON_NULL(e) ((void *)0)
-#define BUILD_BUG_ON_INVALID(e) (0)
-#define BUILD_BUG_ON_MSG(cond, msg) (0)
-#define BUILD_BUG_ON(condition) (0)
-#define BUILD_BUG() (0)
 #define MAYBE_BUILD_BUG_ON(cond) (0)
 #else /* __CHECKER__ */
 
-/* Force a compilation error if a constant expression is not a power of 2 */
-#define __BUILD_BUG_ON_NOT_POWER_OF_2(n)   \
-   BUILD_BUG_ON(((n) & ((n) - 1)) != 0)
-#define BUILD_BUG_ON_NOT_POWER_OF_2(n) \
-   BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
-
-/*
- * Force a compilation error if condition is true, but also produce a
- * result (of value 0 and type size_t), so the expression can be used
- * e.g. in a structure initializer (or where-ever else comma expressions
- * aren't permitted).
- */
-#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
-#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:(-!!(e)); }))
-
-/*
- * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the
- * expression but avoids the generation of any code, even if that expression
- * has side-effects.
- */
-#define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e
-
-/**
- * BUILD_BUG_ON_MSG - break compile if a condition is true & emit supplied
- *   error message.
- * @condition: the condition which the compiler should know is false.
- *
- * See BUILD_BUG_ON for description.
- */
-#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
-
-/**
- * BUILD_BUG_ON - break compile if a condition is true.
- * @condition: the condition which the compiler should know is false.
- *
- * If you have some code which relies on certain constants being equal, or
- * some other compile-time-evaluated condition, you should use BUILD_BUG_ON to
- * detect if someone changes it.
- *
- * The implementation uses gcc's reluctance to create a negative array, but gcc
- * (as of 4.4) only emits that error for obvious cases (e.g. not arguments to
- * inline functions).  Luckily, in 4.3 they added the "error" function
- * attribute just for this type of case.  Thus, we use a negative sized array
- * (should always create an error on gcc versions older than 4.4) and then call
- * an undefined function with the error attribute (should always create an
- * error on gcc 4.3 and later).  If for some reason, neither creates a
- * compile-time error, we'll still have a link-time error, which is harder to
- * track down.
- */
-#ifndef __OPTIMIZE__
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-#else
-#define BUILD_BUG_ON(condition) \
-   BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
-#endif
-
-/**
- * BUILD_BUG - break compile if used.
- *
- * If you have some code that you expect the compiler to eliminate at
- * build time, you should use BUILD_BUG to detect if it is
- * unexpectedly used.
- */
-#define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
-
 #define MAYBE_BUILD_BUG_ON(cond)   \
do {\
if (__builtin_constant_p((cond)))   \
diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
new file mode 100644
index ..b7d22d60008a
--- /dev/null
+++ b/include/linux/build_bug.h
@@ -0,0 +1,84 @@
+#ifndef _LINUX_BUILD_BUG_H
+#define _LINUX_BUILD_BUG_H
+
+#include 
+
+#ifdef __CHECKER__
+#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
+#define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
+#define BUILD_BUG_ON_ZERO(e) (0)
+#define BUILD_BUG_ON_NULL(e) ((void *)0)
+#define BUILD_BUG_ON_INVALID(e) (0)
+#define BUILD_BUG_ON_MSG(cond, msg) (0)
+#define BUILD_BUG_ON(condition) (0)
+#de

[PATCH v5 6/6] 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>
Acked-by: Michal Nazarewicz <min...@mina86.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.
v5: Added Acked-by for Michal Nazarewicz.  Included 
instead of  to avoid a circular dependency that resulted in
build failures when  was included before .
---
 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..1c9c11c9f1a8 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 v5 3/6] linux/bug.h: correct "(foo*)" should be "(foo *)"

[Ian Abbott]

Correct this checkpatch.pl error:

|ERROR: "(foo*)" should be "(foo *)"
|#19: FILE: include/linux/bug.h:19:
|+#define BUILD_BUG_ON_NULL(e) ((void*)0)

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Kees Cook <keesc...@chromium.org>
Cc: Steven Rostedt <rost...@goodmis.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Jakub Kicinski <jakub.kicin...@netronome.com>
Cc: Rasmus Villemoes <li...@rasmusvillemoes.dk>
---
v5: Actually, there was no v1 thru v4.  I called this v5 to match the
series.
---
 include/linux/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index ca24007e2dc3..216a1b79653d 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -16,7 +16,7 @@ struct pt_regs;
 #define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
 #define BUILD_BUG_ON_ZERO(e) (0)
-#define BUILD_BUG_ON_NULL(e) ((void*)0)
+#define BUILD_BUG_ON_NULL(e) ((void *)0)
 #define BUILD_BUG_ON_INVALID(e) (0)
 #define BUILD_BUG_ON_MSG(cond, msg) (0)
 #define BUILD_BUG_ON(condition) (0)
-- 
2.11.0

[PATCH v5 6/6] 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 
Acked-by: Michal Nazarewicz 
---
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.
v5: Added Acked-by for Michal Nazarewicz.  Included 
instead of  to avoid a circular dependency that resulted in
build failures when  was included before .
---
 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..1c9c11c9f1a8 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 v5 3/6] linux/bug.h: correct "(foo*)" should be "(foo *)"

[Ian Abbott]

Correct this checkpatch.pl error:

|ERROR: "(foo*)" should be "(foo *)"
|#19: FILE: include/linux/bug.h:19:
|+#define BUILD_BUG_ON_NULL(e) ((void*)0)

Signed-off-by: Ian Abbott 
Cc: Andrew Morton 
Cc: Kees Cook 
Cc: Steven Rostedt 
Cc: Peter Zijlstra 
Cc: Jakub Kicinski 
Cc: Rasmus Villemoes 
---
v5: Actually, there was no v1 thru v4.  I called this v5 to match the
series.
---
 include/linux/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index ca24007e2dc3..216a1b79653d 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -16,7 +16,7 @@ struct pt_regs;
 #define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
 #define BUILD_BUG_ON_ZERO(e) (0)
-#define BUILD_BUG_ON_NULL(e) ((void*)0)
+#define BUILD_BUG_ON_NULL(e) ((void *)0)
 #define BUILD_BUG_ON_INVALID(e) (0)
 #define BUILD_BUG_ON_MSG(cond, msg) (0)
 #define BUILD_BUG_ON(condition) (0)
-- 
2.11.0

[PATCH v5 2/6] linux/bug.h: correct formatting of block comment

[Ian Abbott]

Correct these checkpatch.pl warnings:

|WARNING: Block comments use * on subsequent lines
|#34: FILE: include/linux/bug.h:34:
|+/* Force a compilation error if condition is true, but also produce a
|+   result (of value 0 and type size_t), so the expression can be used

|WARNING: Block comments use a trailing */ on a separate line
|#36: FILE: include/linux/bug.h:36:
|+   aren't permitted). */

Signed-off-by: Ian Abbott <abbo...@mev.co.uk>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Kees Cook <keesc...@chromium.org>
Cc: Steven Rostedt <rost...@goodmis.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Jakub Kicinski <jakub.kicin...@netronome.com>
Cc: Rasmus Villemoes <li...@rasmusvillemoes.dk>
---
v5: Actually, there was no v1 thru v4.  I called this v5 to match the
series.
---
 include/linux/bug.h | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 687b557fc5eb..ca24007e2dc3 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -30,10 +30,12 @@ struct pt_regs;
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n) \
BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
 
-/* Force a compilation error if condition is true, but also produce a
-   result (of value 0 and type size_t), so the expression can be used
-   e.g. in a structure initializer (or where-ever else comma expressions
-   aren't permitted). */
+/*
+ * Force a compilation error if condition is true, but also produce a
+ * result (of value 0 and type size_t), so the expression can be used
+ * e.g. in a structure initializer (or where-ever else comma expressions
+ * aren't permitted).
+ */
 #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
 #define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
 
-- 
2.11.0

[PATCH v5 1/6] 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>
Acked-by: Michal Nazarewicz <min...@mina86.com>
---
v3: Actually, there was no v1 or v2.  I called this v3 to match the
series.
v4: Corrected 'Acked-by:' line in patch description.
v5: Added Acked-by for Michal Nazarewicz.
---
 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 v5 2/6] linux/bug.h: correct formatting of block comment

[Ian Abbott]

Correct these checkpatch.pl warnings:

|WARNING: Block comments use * on subsequent lines
|#34: FILE: include/linux/bug.h:34:
|+/* Force a compilation error if condition is true, but also produce a
|+   result (of value 0 and type size_t), so the expression can be used

|WARNING: Block comments use a trailing */ on a separate line
|#36: FILE: include/linux/bug.h:36:
|+   aren't permitted). */

Signed-off-by: Ian Abbott 
Cc: Andrew Morton 
Cc: Kees Cook 
Cc: Steven Rostedt 
Cc: Peter Zijlstra 
Cc: Jakub Kicinski 
Cc: Rasmus Villemoes 
---
v5: Actually, there was no v1 thru v4.  I called this v5 to match the
series.
---
 include/linux/bug.h | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 687b557fc5eb..ca24007e2dc3 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -30,10 +30,12 @@ struct pt_regs;
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n) \
BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
 
-/* Force a compilation error if condition is true, but also produce a
-   result (of value 0 and type size_t), so the expression can be used
-   e.g. in a structure initializer (or where-ever else comma expressions
-   aren't permitted). */
+/*
+ * Force a compilation error if condition is true, but also produce a
+ * result (of value 0 and type size_t), so the expression can be used
+ * e.g. in a structure initializer (or where-ever else comma expressions
+ * aren't permitted).
+ */
 #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
 #define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
 
-- 
2.11.0

[PATCH v5 1/6] 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 
Acked-by: Michal Nazarewicz 
---
v3: Actually, there was no v1 or v2.  I called this v3 to match the
series.
v4: Corrected 'Acked-by:' line in patch description.
v5: Added Acked-by for Michal Nazarewicz.
---
 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

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


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: <abbo...@mev.co.uk> )=-
-=(  Web: http://www.mev.co.uk/  )=-