[PATCH] arch: hexagon: Kconfig: add HAVE_DMA_ATTR in Kconfig and remove "linux/dma-mapping.h" from "asm/dma-mapping.h"

[PATCH] arch: hexagon: Kconfig: add HAVE_DMA_ATTR in Kconfig and remove "linux/dma-mapping.h" from "asm/dma-mapping.h"

[Chen Gang]

When HAS_DMA, and also need use generic implementation, HAVE_DMA_ATTR
must be enabled, or can not pass compiling with allmodconfig, the
related error:

    CC [M]  drivers/ata/libata-core.o
  drivers/ata/libata-core.c: In function 'ata_sg_clean':
  drivers/ata/libata-core.c:4598:3: error: implicit declaration of function 'dma_unmap_sg' [-Werror=implicit-function-declaration]
  drivers/ata/libata-core.c: In function 'ata_sg_setup':
  drivers/ata/libata-core.c:4708:2: error: implicit declaration of function 'dma_map_sg' [-Werror=implicit-function-declaration]

"linux/dma-mapping.h" will include "asm/dma-mapping.h", so need remove
"linux/dma-mapping.h" from "asm/dma-mapping.h",


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 arch/hexagon/Kconfig                   |    1 +
 arch/hexagon/include/asm/dma-mapping.h |    1 -
 2 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/hexagon/Kconfig b/arch/hexagon/Kconfig
index 09df260..fbc5c78 100644
--- a/arch/hexagon/Kconfig
+++ b/arch/hexagon/Kconfig
@@ -28,6 +28,7 @@ config HEXAGON
 	select GENERIC_CLOCKEVENTS_BROADCAST
 	select MODULES_USE_ELF_RELA
 	select GENERIC_CPU_DEVICES
+	select HAVE_DMA_ATTRS
 	---help---
 	  Qualcomm Hexagon is a processor architecture designed for high
 	  performance and low power across a wide variety of applications.
diff --git a/arch/hexagon/include/asm/dma-mapping.h b/arch/hexagon/include/asm/dma-mapping.h
index 85e9935..1696542 100644
--- a/arch/hexagon/include/asm/dma-mapping.h
+++ b/arch/hexagon/include/asm/dma-mapping.h
@@ -25,7 +25,6 @@
 #include <linux/cache.h>
 #include <linux/mm.h>
 #include <linux/scatterlist.h>
-#include <linux/dma-mapping.h>
 #include <linux/dma-debug.h>
 #include <linux/dma-attrs.h>
 #include <asm/io.h>
-- 
1.7.7.6

Re: [PATCH] arch: hexagon: Kconfig: add HAVE_DMA_ATTR in Kconfig and remove "linux/dma-mapping.h" from "asm/dma-mapping.h"

[rkuo]

On Tue, Nov 19, 2013 at 12:57:27PM +0800, Chen Gang wrote:
> When HAS_DMA, and also need use generic implementation, HAVE_DMA_ATTR
> must be enabled, or can not pass compiling with allmodconfig, the
> related error:
> 
>     CC [M]  drivers/ata/libata-core.o
>   drivers/ata/libata-core.c: In function 'ata_sg_clean':
>   drivers/ata/libata-core.c:4598:3: error: implicit declaration of function 'dma_unmap_sg' [-Werror=implicit-function-declaration]
>   drivers/ata/libata-core.c: In function 'ata_sg_setup':
>   drivers/ata/libata-core.c:4708:2: error: implicit declaration of function 'dma_map_sg' [-Werror=implicit-function-declaration]
> 
> "linux/dma-mapping.h" will include "asm/dma-mapping.h", so need remove
> "linux/dma-mapping.h" from "asm/dma-mapping.h",
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

Acked-by: Richard Kuo <rkuo@codeaurora.org>

-- 

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 0/2] arch: hexagon: include: asm: add prefix "vm_" for all enum members in "hexagon_vm.h"

[Chen Gang]

Append "vm_" to all enum members (which are too common to make conflict
with another sub-systems). The related error with allmodconfig:

    CC [M]  drivers/md/raid1.o
  drivers/md/raid1.c:1440:13: error: 'status' redeclared as different kind of symbol
  arch/hexagon/include/asm/hexagon_vm.h:76:2: note: previous definition of 'status' was here

Also fix a typo issue: use 'affinity' instead of 'locdis' for
__vmintop_affinity() in "hexagon_vm.h"


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 arch/hexagon/include/asm/hexagon_vm.h |   72 ++++++++++++++++----------------
 1 files changed, 36 insertions(+), 36 deletions(-)

[PATCH 1/2] arch: hexagon: include: asm: add prefix "vm_" for all enum members in "hexagon_vm.h"

[Chen Gang]

Append "vm_" to all enum members (which are too common to make conflict
with another sub-systems). The related error with allmodconfig:

    CC [M]  drivers/md/raid1.o
  drivers/md/raid1.c:1440:13: error: 'status' redeclared as different kind of symbol
  arch/hexagon/include/asm/hexagon_vm.h:76:2: note: previous definition of 'status' was here


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 arch/hexagon/include/asm/hexagon_vm.h |   70 ++++++++++++++++----------------
 1 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/arch/hexagon/include/asm/hexagon_vm.h b/arch/hexagon/include/asm/hexagon_vm.h
index 67bb6d6..e1e0470 100644
--- a/arch/hexagon/include/asm/hexagon_vm.h
+++ b/arch/hexagon/include/asm/hexagon_vm.h
@@ -55,27 +55,27 @@
 #ifndef __ASSEMBLY__
 
 enum VM_CACHE_OPS {
-	ickill,
-	dckill,
-	l2kill,
-	dccleaninva,
-	icinva,
-	idsync,
-	fetch_cfg
+	vm_ickill,
+	vm_dckill,
+	vm_l2kill,
+	vm_dccleaninva,
+	vm_icinva,
+	vm_idsync,
+	vm_fetch_cfg
 };
 
 enum VM_INT_OPS {
-	nop,
-	globen,
-	globdis,
-	locen,
-	locdis,
-	affinity,
-	get,
-	peek,
-	status,
-	post,
-	clear
+	vm_nop,
+	vm_globen,
+	vm_globdis,
+	vm_locen,
+	vm_locdis,
+	vm_affinity,
+	vm_get,
+	vm_peek,
+	vm_status,
+	vm_post,
+	vm_clear
 };
 
 extern void _K_VM_event_vector(void);
@@ -98,65 +98,65 @@ long __vmvpid(void);
 
 static inline long __vmcache_ickill(void)
 {
-	return __vmcache(ickill, 0, 0);
+	return __vmcache(vm_ickill, 0, 0);
 }
 
 static inline long __vmcache_dckill(void)
 {
-	return __vmcache(dckill, 0, 0);
+	return __vmcache(vm_dckill, 0, 0);
 }
 
 static inline long __vmcache_l2kill(void)
 {
-	return __vmcache(l2kill, 0, 0);
+	return __vmcache(vm_l2kill, 0, 0);
 }
 
 static inline long __vmcache_dccleaninva(unsigned long addr, unsigned long len)
 {
-	return __vmcache(dccleaninva, addr, len);
+	return __vmcache(vm_dccleaninva, addr, len);
 }
 
 static inline long __vmcache_icinva(unsigned long addr, unsigned long len)
 {
-	return __vmcache(icinva, addr, len);
+	return __vmcache(vm_icinva, addr, len);
 }
 
 static inline long __vmcache_idsync(unsigned long addr,
 					   unsigned long len)
 {
-	return __vmcache(idsync, addr, len);
+	return __vmcache(vm_idsync, addr, len);
 }
 
 static inline long __vmcache_fetch_cfg(unsigned long val)
 {
-	return __vmcache(fetch_cfg, val, 0);
+	return __vmcache(vm_fetch_cfg, val, 0);
 }
 
 /* interrupt operations  */
 
 static inline long __vmintop_nop(void)
 {
-	return __vmintop(nop, 0, 0, 0, 0);
+	return __vmintop(vm_nop, 0, 0, 0, 0);
 }
 
 static inline long __vmintop_globen(long i)
 {
-	return __vmintop(globen, i, 0, 0, 0);
+	return __vmintop(vm_globen, i, 0, 0, 0);
 }
 
 static inline long __vmintop_globdis(long i)
 {
-	return __vmintop(globdis, i, 0, 0, 0);
+	return __vmintop(vm_globdis, i, 0, 0, 0);
 }
 
 static inline long __vmintop_locen(long i)
 {
-	return __vmintop(locen, i, 0, 0, 0);
+	return __vmintop(vm_locen, i, 0, 0, 0);
 }
 
 static inline long __vmintop_locdis(long i)
 {
-	return __vmintop(locdis, i, 0, 0, 0);
+	return __vmintop(vm_locdis, i, 0, 0, 0);
 }
 
 static inline long __vmintop_affinity(long i, long cpu)
@@ -166,27 +166,27 @@ static inline long __vmintop_affinity(long i, long cpu)
 
 static inline long __vmintop_get(void)
 {
-	return __vmintop(get, 0, 0, 0, 0);
+	return __vmintop(vm_get, 0, 0, 0, 0);
 }
 
 static inline long __vmintop_peek(void)
 {
-	return __vmintop(peek, 0, 0, 0, 0);
+	return __vmintop(vm_peek, 0, 0, 0, 0);
 }
 
 static inline long __vmintop_status(long i)
 {
-	return __vmintop(status, i, 0, 0, 0);
+	return __vmintop(vm_status, i, 0, 0, 0);
 }
 
 static inline long __vmintop_post(long i)
 {
-	return __vmintop(post, i, 0, 0, 0);
+	return __vmintop(vm_post, i, 0, 0, 0);
 }
 
 static inline long __vmintop_clear(long i)
 {
-	return __vmintop(clear, i, 0, 0, 0);
+	return __vmintop(vm_clear, i, 0, 0, 0);
 }
 
 #else /* Only assembly code should reference these */
-- 
1.7.7.6

[PATCH 2/2] arch: hexagon: include: asm: use 'affinity' instead of 'locdis' for __vmintop_affinity() in "hexagon_vm.h"

[Chen Gang]

All __vmintop_*() use  __vmintop(*, ...), so __vmintop_affinity() need
use __vmintop(vm_affinity, ...).


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 arch/hexagon/include/asm/hexagon_vm.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/hexagon/include/asm/hexagon_vm.h b/arch/hexagon/include/asm/hexagon_vm.h
index e1e0470..3ea99c1 100644
--- a/arch/hexagon/include/asm/hexagon_vm.h
+++ b/arch/hexagon/include/asm/hexagon_vm.h
@@ -161,7 +161,7 @@ static inline long __vmintop_locdis(long i)
 
 static inline long __vmintop_affinity(long i, long cpu)
 {
-	return __vmintop(locdis, i, cpu, 0, 0);
+	return __vmintop(vm_affinity, i, cpu, 0, 0);
 }
 
 static inline long __vmintop_get(void)
-- 
1.7.7.6

[PATCH v2] arch: hexagon: include: asm: add prefix "hvm[ci]_" for all enum members in "hexagon_vm.h"

[Chen Gang]

Append "hvmc_" or "hvmi_" to all related enum members (which are too
common to make conflict with another sub-systems). The related error
with allmodconfig:

    CC [M]  drivers/md/raid1.o
  drivers/md/raid1.c:1440:13: error: 'status' redeclared as different kind of symbol
  arch/hexagon/include/asm/hexagon_vm.h:76:2: note: previous definition of 'status' was here

Also use 'affinity' instead of 'locdis' for __vmintop_affinity().


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 arch/hexagon/include/asm/hexagon_vm.h |   72 ++++++++++++++++----------------
 1 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/arch/hexagon/include/asm/hexagon_vm.h b/arch/hexagon/include/asm/hexagon_vm.h
index 67bb6d6..1f6918b 100644
--- a/arch/hexagon/include/asm/hexagon_vm.h
+++ b/arch/hexagon/include/asm/hexagon_vm.h
@@ -55,27 +55,27 @@
 #ifndef __ASSEMBLY__
 
 enum VM_CACHE_OPS {
-	ickill,
-	dckill,
-	l2kill,
-	dccleaninva,
-	icinva,
-	idsync,
-	fetch_cfg
+	hvmc_ickill,
+	hvmc_dckill,
+	hvmc_l2kill,
+	hvmc_dccleaninva,
+	hvmc_icinva,
+	hvmc_idsync,
+	hvmc_fetch_cfg
 };
 
 enum VM_INT_OPS {
-	nop,
-	globen,
-	globdis,
-	locen,
-	locdis,
-	affinity,
-	get,
-	peek,
-	status,
-	post,
-	clear
+	hvmi_nop,
+	hvmi_globen,
+	hvmi_globdis,
+	hvmi_locen,
+	hvmi_locdis,
+	hvmi_affinity,
+	hvmi_get,
+	hvmi_peek,
+	hvmi_status,
+	hvmi_post,
+	hvmi_clear
 };
 
 extern void _K_VM_event_vector(void);
@@ -98,95 +98,95 @@ long __vmvpid(void);
 
 static inline long __vmcache_ickill(void)
 {
-	return __vmcache(ickill, 0, 0);
+	return __vmcache(hvmc_ickill, 0, 0);
 }
 
 static inline long __vmcache_dckill(void)
 {
-	return __vmcache(dckill, 0, 0);
+	return __vmcache(hvmc_dckill, 0, 0);
 }
 
 static inline long __vmcache_l2kill(void)
 {
-	return __vmcache(l2kill, 0, 0);
+	return __vmcache(hvmc_l2kill, 0, 0);
 }
 
 static inline long __vmcache_dccleaninva(unsigned long addr, unsigned long len)
 {
-	return __vmcache(dccleaninva, addr, len);
+	return __vmcache(hvmc_dccleaninva, addr, len);
 }
 
 static inline long __vmcache_icinva(unsigned long addr, unsigned long len)
 {
-	return __vmcache(icinva, addr, len);
+	return __vmcache(hvmc_icinva, addr, len);
 }
 
 static inline long __vmcache_idsync(unsigned long addr,
 					   unsigned long len)
 {
-	return __vmcache(idsync, addr, len);
+	return __vmcache(hvmc_idsync, addr, len);
 }
 
 static inline long __vmcache_fetch_cfg(unsigned long val)
 {
-	return __vmcache(fetch_cfg, val, 0);
+	return __vmcache(hvmc_fetch_cfg, val, 0);
 }
 
 /* interrupt operations  */
 
 static inline long __vmintop_nop(void)
 {
-	return __vmintop(nop, 0, 0, 0, 0);
+	return __vmintop(hvmi_nop, 0, 0, 0, 0);
 }
 
 static inline long __vmintop_globen(long i)
 {
-	return __vmintop(globen, i, 0, 0, 0);
+	return __vmintop(hvmi_globen, i, 0, 0, 0);
 }
 
 static inline long __vmintop_globdis(long i)
 {
-	return __vmintop(globdis, i, 0, 0, 0);
+	return __vmintop(hvmi_globdis, i, 0, 0, 0);
 }
 
 static inline long __vmintop_locen(long i)
 {
-	return __vmintop(locen, i, 0, 0, 0);
+	return __vmintop(hvmi_locen, i, 0, 0, 0);
 }
 
 static inline long __vmintop_locdis(long i)
 {
-	return __vmintop(locdis, i, 0, 0, 0);
+	return __vmintop(hvmi_locdis, i, 0, 0, 0);
 }
 
 static inline long __vmintop_affinity(long i, long cpu)
 {
-	return __vmintop(locdis, i, cpu, 0, 0);
+	return __vmintop(hvmi_affinity, i, cpu, 0, 0);
 }
 
 static inline long __vmintop_get(void)
 {
-	return __vmintop(get, 0, 0, 0, 0);
+	return __vmintop(hvmi_get, 0, 0, 0, 0);
 }
 
 static inline long __vmintop_peek(void)
 {
-	return __vmintop(peek, 0, 0, 0, 0);
+	return __vmintop(hvmi_peek, 0, 0, 0, 0);
 }
 
 static inline long __vmintop_status(long i)
 {
-	return __vmintop(status, i, 0, 0, 0);
+	return __vmintop(hvmi_status, i, 0, 0, 0);
 }
 
 static inline long __vmintop_post(long i)
 {
-	return __vmintop(post, i, 0, 0, 0);
+	return __vmintop(hvmi_post, i, 0, 0, 0);
 }
 
 static inline long __vmintop_clear(long i)
 {
-	return __vmintop(clear, i, 0, 0, 0);
+	return __vmintop(hvmi_clear, i, 0, 0, 0);
 }
 
 #else /* Only assembly code should reference these */
-- 
1.7.7.6

Re: [PATCH v2] arch: hexagon: include: asm: add prefix "hvm[ci]_" for all enum members in "hexagon_vm.h"

[rkuo]

On Thu, Nov 28, 2013 at 04:51:45PM +0800, Chen Gang wrote:
> Append "hvmc_" or "hvmi_" to all related enum members (which are too
> common to make conflict with another sub-systems). The related error
> with allmodconfig:
> 
>     CC [M]  drivers/md/raid1.o
>   drivers/md/raid1.c:1440:13: error: 'status' redeclared as different kind of symbol
>   arch/hexagon/include/asm/hexagon_vm.h:76:2: note: previous definition of 'status' was here
> 
> Also use 'affinity' instead of 'locdis' for __vmintop_affinity().
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  arch/hexagon/include/asm/hexagon_vm.h |   72 ++++++++++++++++----------------
>  1 files changed, 36 insertions(+), 36 deletions(-)
> 

Thanks for catching the affinity typo too.


Acked-by: Richard Kuo <rkuo@codeaurora.org>

-- 

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/2] arch: hexagon: include: asm: add prefix "vm_" for all enum members in "hexagon_vm.h"

[Chen Gang]

On 11/25/2013 10:40 AM, Chen Gang wrote:
> Append "vm_" to all enum members (which are too common to make conflict
> with another sub-systems). The related error with allmodconfig:
> 
>     CC [M]  drivers/md/raid1.o
>   drivers/md/raid1.c:1440:13: error: 'status' redeclared as different kind of symbol
>   arch/hexagon/include/asm/hexagon_vm.h:76:2: note: previous definition of 'status' was here
> 

Oh, sorry, The new prefix "vm_" is still conflict with others. One case
is below (bottom of this mail)

I will use "hvmc_" for VM_CACHE_OPS and "hvmi_" for VM_INT_OPS instead
of. If get no rejections within 1 day, I will send patch v2 for it.

And also I will merge the related 2 patches together (although they are
for 2 issues).

> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  arch/hexagon/include/asm/hexagon_vm.h |   70 ++++++++++++++++----------------
>  1 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/hexagon/include/asm/hexagon_vm.h b/arch/hexagon/include/asm/hexagon_vm.h
> index 67bb6d6..e1e0470 100644
> --- a/arch/hexagon/include/asm/hexagon_vm.h
> +++ b/arch/hexagon/include/asm/hexagon_vm.h
> @@ -55,27 +55,27 @@
>  #ifndef __ASSEMBLY__
>  
>  enum VM_CACHE_OPS {
> -	ickill,
> -	dckill,
> -	l2kill,
> -	dccleaninva,
> -	icinva,
> -	idsync,
> -	fetch_cfg
> +	vm_ickill,
> +	vm_dckill,
> +	vm_l2kill,
> +	vm_dccleaninva,
> +	vm_icinva,
> +	vm_idsync,
> +	vm_fetch_cfg
>  };
>  
>  enum VM_INT_OPS {
> -	nop,
> -	globen,
> -	globdis,
> -	locen,
> -	locdis,
> -	affinity,
> -	get,
> -	peek,
> -	status,
> -	post,
> -	clear
> +	vm_nop,
> +	vm_globen,
> +	vm_globdis,
> +	vm_locen,
> +	vm_locdis,
> +	vm_affinity,
> +	vm_get,

The new prefix "vm_" for "get" is still conflict with others:

    CC [M]  drivers/virtio/virtio_mmio.o
  drivers/virtio/virtio_mmio.c:170:13: error: 'vm_get' redeclared as different kind of symbol
  arch/hexagon/include/asm/hexagon_vm.h:74:2: note: previous definition of 'vm_get' was here


Thanks.
-- 
Chen Gang

[PATCH] drivers: scsi: scsi_lib.c: add prefix "SCSILIB_" to macro "SP"

[Chen Gang]

the macro "SP" is too common to make conflict with some architectures,
so recommend to add prefix for it.

The related warning (with allmodconfig for hexagon):

    CC [M]  drivers/scsi/scsi_lib.o
  drivers/scsi/scsi_lib.c:46:0: warning: "SP" redefined [enabled by default]
  arch/hexagon/include/uapi/asm/registers.h:9:0: note: this is the location of the previous definition


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/scsi/scsi_lib.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bd7f0d..f78e21b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -43,28 +43,28 @@ struct scsi_host_sg_pool {
 	mempool_t	*pool;
 };
 
-#define SP(x) { x, "sgpool-" __stringify(x) }
+#define SCSILIB_SP(x) { x, "sgpool-" __stringify(x) }
 #if (SCSI_MAX_SG_SEGMENTS < 32)
 #error SCSI_MAX_SG_SEGMENTS is too small (must be 32 or greater)
 #endif
 static struct scsi_host_sg_pool scsi_sg_pools[] = {
-	SP(8),
-	SP(16),
+	SCSILIB_SP(8),
+	SCSILIB_SP(16),
 #if (SCSI_MAX_SG_SEGMENTS > 32)
-	SP(32),
+	SCSILIB_SP(32),
 #if (SCSI_MAX_SG_SEGMENTS > 64)
-	SP(64),
+	SCSILIB_SP(64),
 #if (SCSI_MAX_SG_SEGMENTS > 128)
-	SP(128),
+	SCSILIB_SP(128),
 #if (SCSI_MAX_SG_SEGMENTS > 256)
 #error SCSI_MAX_SG_SEGMENTS is too large (256 MAX)
 #endif
 #endif
 #endif
 #endif
-	SP(SCSI_MAX_SG_SEGMENTS)
+	SCSILIB_SP(SCSI_MAX_SG_SEGMENTS)
 };
-#undef SP
+#undef SCSILIB_SP
 
 struct kmem_cache *scsi_sdb_cache;
 
-- 
1.7.7.6

Re: [PATCH] drivers: scsi: scsi_lib.c: add prefix "SCSILIB_" to macro "SP"

[Bart Van Assche]

On 11/27/13 03:29, Chen Gang wrote:
> the macro "SP" is too common to make conflict with some architectures,
> so recommend to add prefix for it.
> 
> The related warning (with allmodconfig for hexagon):
> 
>     CC [M]  drivers/scsi/scsi_lib.o
>   drivers/scsi/scsi_lib.c:46:0: warning: "SP" redefined [enabled by default]
>   arch/hexagon/include/uapi/asm/registers.h:9:0: note: this is the location of the previous definition
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  drivers/scsi/scsi_lib.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7bd7f0d..f78e21b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -43,28 +43,28 @@ struct scsi_host_sg_pool {
>  	mempool_t	*pool;
>  };
>  
> -#define SP(x) { x, "sgpool-" __stringify(x) }
> +#define SCSILIB_SP(x) { x, "sgpool-" __stringify(x) }
>  #if (SCSI_MAX_SG_SEGMENTS < 32)
>  #error SCSI_MAX_SG_SEGMENTS is too small (must be 32 or greater)
>  #endif
>  static struct scsi_host_sg_pool scsi_sg_pools[] = {
> -	SP(8),
> -	SP(16),
> +	SCSILIB_SP(8),
> +	SCSILIB_SP(16),
>  #if (SCSI_MAX_SG_SEGMENTS > 32)
> -	SP(32),
> +	SCSILIB_SP(32),
>  #if (SCSI_MAX_SG_SEGMENTS > 64)
> -	SP(64),
> +	SCSILIB_SP(64),
>  #if (SCSI_MAX_SG_SEGMENTS > 128)
> -	SP(128),
> +	SCSILIB_SP(128),
>  #if (SCSI_MAX_SG_SEGMENTS > 256)
>  #error SCSI_MAX_SG_SEGMENTS is too large (256 MAX)
>  #endif
>  #endif
>  #endif
>  #endif
> -	SP(SCSI_MAX_SG_SEGMENTS)
> +	SCSILIB_SP(SCSI_MAX_SG_SEGMENTS)
>  };
> -#undef SP
> +#undef SCSILIB_SP
>  
>  struct kmem_cache *scsi_sdb_cache;

Sorry but the "SCSILIB_SP" name doesn't look very descriptive to me.
There are probably better choices possible. How about using e.g.
SG_POOL() instead ?

Bart.

Re: [PATCH] drivers: scsi: scsi_lib.c: add prefix "SCSILIB_" to macro "SP"

[Chen Gang]

On 12/02/2013 12:17 AM, Bart Van Assche wrote:
> On 11/27/13 03:29, Chen Gang wrote:
>> the macro "SP" is too common to make conflict with some architectures,
>> so recommend to add prefix for it.
>>
>> The related warning (with allmodconfig for hexagon):
>>
>>     CC [M]  drivers/scsi/scsi_lib.o
>>   drivers/scsi/scsi_lib.c:46:0: warning: "SP" redefined [enabled by default]
>>   arch/hexagon/include/uapi/asm/registers.h:9:0: note: this is the location of the previous definition
>>
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  drivers/scsi/scsi_lib.c |   16 ++++++++--------
>>  1 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 7bd7f0d..f78e21b 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -43,28 +43,28 @@ struct scsi_host_sg_pool {
>>  	mempool_t	*pool;
>>  };
>>  
>> -#define SP(x) { x, "sgpool-" __stringify(x) }
>> +#define SCSILIB_SP(x) { x, "sgpool-" __stringify(x) }
>>  #if (SCSI_MAX_SG_SEGMENTS < 32)
>>  #error SCSI_MAX_SG_SEGMENTS is too small (must be 32 or greater)
>>  #endif
>>  static struct scsi_host_sg_pool scsi_sg_pools[] = {
>> -	SP(8),
>> -	SP(16),
>> +	SCSILIB_SP(8),
>> +	SCSILIB_SP(16),
>>  #if (SCSI_MAX_SG_SEGMENTS > 32)
>> -	SP(32),
>> +	SCSILIB_SP(32),
>>  #if (SCSI_MAX_SG_SEGMENTS > 64)
>> -	SP(64),
>> +	SCSILIB_SP(64),
>>  #if (SCSI_MAX_SG_SEGMENTS > 128)
>> -	SP(128),
>> +	SCSILIB_SP(128),
>>  #if (SCSI_MAX_SG_SEGMENTS > 256)
>>  #error SCSI_MAX_SG_SEGMENTS is too large (256 MAX)
>>  #endif
>>  #endif
>>  #endif
>>  #endif
>> -	SP(SCSI_MAX_SG_SEGMENTS)
>> +	SCSILIB_SP(SCSI_MAX_SG_SEGMENTS)
>>  };
>> -#undef SP
>> +#undef SCSILIB_SP
>>  
>>  struct kmem_cache *scsi_sdb_cache;
> 
> Sorry but the "SCSILIB_SP" name doesn't look very descriptive to me.
> There are probably better choices possible. How about using e.g.
> SG_POOL() instead ?
> 

That sounds good to me, I will send patch v2, tomorrow (today I have
to do some another urgent things, if this patch is also urgent, please
help send). :-)

Thanks.
-- 
Chen Gang

Re: [PATCH] drivers: scsi: scsi_lib.c: add prefix "SCSILIB_" to macro "SP"

[James Bottomley]

On Mon, 2013-12-02 at 08:34 +0800, Chen Gang wrote:
> On 12/02/2013 12:17 AM, Bart Van Assche wrote:
> > On 11/27/13 03:29, Chen Gang wrote:
> >> the macro "SP" is too common to make conflict with some architectures,
> >> so recommend to add prefix for it.
> >>
> >> The related warning (with allmodconfig for hexagon):
> >>
> >>     CC [M]  drivers/scsi/scsi_lib.o
> >>   drivers/scsi/scsi_lib.c:46:0: warning: "SP" redefined [enabled by default]
> >>   arch/hexagon/include/uapi/asm/registers.h:9:0: note: this is the location of the previous definition
> >>
> >>
> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> >> ---
> >>  drivers/scsi/scsi_lib.c |   16 ++++++++--------
> >>  1 files changed, 8 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> >> index 7bd7f0d..f78e21b 100644
> >> --- a/drivers/scsi/scsi_lib.c
> >> +++ b/drivers/scsi/scsi_lib.c
> >> @@ -43,28 +43,28 @@ struct scsi_host_sg_pool {
> >>  	mempool_t	*pool;
> >>  };
> >>  
> >> -#define SP(x) { x, "sgpool-" __stringify(x) }
> >> +#define SCSILIB_SP(x) { x, "sgpool-" __stringify(x) }
> >>  #if (SCSI_MAX_SG_SEGMENTS < 32)
> >>  #error SCSI_MAX_SG_SEGMENTS is too small (must be 32 or greater)
> >>  #endif
> >>  static struct scsi_host_sg_pool scsi_sg_pools[] = {
> >> -	SP(8),
> >> -	SP(16),
> >> +	SCSILIB_SP(8),
> >> +	SCSILIB_SP(16),
> >>  #if (SCSI_MAX_SG_SEGMENTS > 32)
> >> -	SP(32),
> >> +	SCSILIB_SP(32),
> >>  #if (SCSI_MAX_SG_SEGMENTS > 64)
> >> -	SP(64),
> >> +	SCSILIB_SP(64),
> >>  #if (SCSI_MAX_SG_SEGMENTS > 128)
> >> -	SP(128),
> >> +	SCSILIB_SP(128),
> >>  #if (SCSI_MAX_SG_SEGMENTS > 256)
> >>  #error SCSI_MAX_SG_SEGMENTS is too large (256 MAX)
> >>  #endif
> >>  #endif
> >>  #endif
> >>  #endif
> >> -	SP(SCSI_MAX_SG_SEGMENTS)
> >> +	SCSILIB_SP(SCSI_MAX_SG_SEGMENTS)
> >>  };
> >> -#undef SP
> >> +#undef SCSILIB_SP
> >>  
> >>  struct kmem_cache *scsi_sdb_cache;
> > 
> > Sorry but the "SCSILIB_SP" name doesn't look very descriptive to me.
> > There are probably better choices possible. How about using e.g.
> > SG_POOL() instead ?
> > 
> 
> That sounds good to me, I will send patch v2, tomorrow (today I have
> to do some another urgent things, if this patch is also urgent, please
> help send). :-)

No, this is the wrong thing to do.  Exported headers should be namespace
protected, so the thing wrong is what's exporting the problem to us,
namely hexagon.

James

Re: [PATCH] drivers: scsi: scsi_lib.c: add prefix "SCSILIB_" to macro "SP"

[Chen Gang]

On 12/02/2013 08:49 AM, James Bottomley wrote:
> On Mon, 2013-12-02 at 08:34 +0800, Chen Gang wrote:
>> On 12/02/2013 12:17 AM, Bart Van Assche wrote:
>>> On 11/27/13 03:29, Chen Gang wrote:
>>>> the macro "SP" is too common to make conflict with some architectures,
>>>> so recommend to add prefix for it.
>>>>
>>>> The related warning (with allmodconfig for hexagon):
>>>>
>>>>     CC [M]  drivers/scsi/scsi_lib.o
>>>>   drivers/scsi/scsi_lib.c:46:0: warning: "SP" redefined [enabled by default]
>>>>   arch/hexagon/include/uapi/asm/registers.h:9:0: note: this is the location of the previous definition
>>>>
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>> ---
>>>>  drivers/scsi/scsi_lib.c |   16 ++++++++--------
>>>>  1 files changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>>> index 7bd7f0d..f78e21b 100644
>>>> --- a/drivers/scsi/scsi_lib.c
>>>> +++ b/drivers/scsi/scsi_lib.c
>>>> @@ -43,28 +43,28 @@ struct scsi_host_sg_pool {
>>>>  	mempool_t	*pool;
>>>>  };
>>>>  
>>>> -#define SP(x) { x, "sgpool-" __stringify(x) }
>>>> +#define SCSILIB_SP(x) { x, "sgpool-" __stringify(x) }
>>>>  #if (SCSI_MAX_SG_SEGMENTS < 32)
>>>>  #error SCSI_MAX_SG_SEGMENTS is too small (must be 32 or greater)
>>>>  #endif
>>>>  static struct scsi_host_sg_pool scsi_sg_pools[] = {
>>>> -	SP(8),
>>>> -	SP(16),
>>>> +	SCSILIB_SP(8),
>>>> +	SCSILIB_SP(16),
>>>>  #if (SCSI_MAX_SG_SEGMENTS > 32)
>>>> -	SP(32),
>>>> +	SCSILIB_SP(32),
>>>>  #if (SCSI_MAX_SG_SEGMENTS > 64)
>>>> -	SP(64),
>>>> +	SCSILIB_SP(64),
>>>>  #if (SCSI_MAX_SG_SEGMENTS > 128)
>>>> -	SP(128),
>>>> +	SCSILIB_SP(128),
>>>>  #if (SCSI_MAX_SG_SEGMENTS > 256)
>>>>  #error SCSI_MAX_SG_SEGMENTS is too large (256 MAX)
>>>>  #endif
>>>>  #endif
>>>>  #endif
>>>>  #endif
>>>> -	SP(SCSI_MAX_SG_SEGMENTS)
>>>> +	SCSILIB_SP(SCSI_MAX_SG_SEGMENTS)
>>>>  };
>>>> -#undef SP
>>>> +#undef SCSILIB_SP
>>>>  
>>>>  struct kmem_cache *scsi_sdb_cache;
>>>
>>> Sorry but the "SCSILIB_SP" name doesn't look very descriptive to me.
>>> There are probably better choices possible. How about using e.g.
>>> SG_POOL() instead ?
>>>
>>
>> That sounds good to me, I will send patch v2, tomorrow (today I have
>> to do some another urgent things, if this patch is also urgent, please
>> help send). :-)
> 
> No, this is the wrong thing to do.  Exported headers should be namespace
> protected, so the thing wrong is what's exporting the problem to us,
> namely hexagon.
> 

If one issue occurs, normally, both sides need improvement.

For our issue:

 - need try to keep uapi no touch ("arch/hexagon/uapi/asm/registers.h").

 - improving our module is much easier than improving hexagon.

 - for 'SP', it is really short enough to like a register name.
   SG_POOL seems more suitable for our 'sgpool' related operations.


It will be better to improve hexagon too, but it is not quit easy (maybe
have to bear it), it is uapi :-(


Thanks
-- 
Chen Gang

Re: [PATCH] drivers: scsi: scsi_lib.c: add prefix "SCSILIB_" to macro "SP"

[rkuo]

On Mon, Dec 02, 2013 at 06:14:33PM +0800, Chen Gang wrote:
> If one issue occurs, normally, both sides need improvement.
> 
> For our issue:
> 
>  - need try to keep uapi no touch ("arch/hexagon/uapi/asm/registers.h").
> 
>  - improving our module is much easier than improving hexagon.
> 
>  - for 'SP', it is really short enough to like a register name.
>    SG_POOL seems more suitable for our 'sgpool' related operations.
> 
> 
> It will be better to improve hexagon too, but it is not quit easy (maybe
> have to bear it), it is uapi :-(

I can't speak for SCSI, but that define in Hexagon isn't really used outside
of that file anyways (the two SP macros themselves).  I'm also pretty sure
the userspace isn't currently using it anyways.  Seeing as it's not really
buying us anything, I'm fine with just removing it and continuing to review
the other defines.  I'll make that change and test it locally.


Thanks,
Richard Kuo


-- 

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] drivers: scsi: scsi_lib.c: add prefix "SCSILIB_" to macro "SP"

[Chen Gang]

On 12/03/2013 05:32 AM, rkuo wrote:
> On Mon, Dec 02, 2013 at 06:14:33PM +0800, Chen Gang wrote:
>> If one issue occurs, normally, both sides need improvement.
>>
>> For our issue:
>>
>>  - need try to keep uapi no touch ("arch/hexagon/uapi/asm/registers.h").
>>
>>  - improving our module is much easier than improving hexagon.
>>
>>  - for 'SP', it is really short enough to like a register name.
>>    SG_POOL seems more suitable for our 'sgpool' related operations.
>>
>>
>> It will be better to improve hexagon too, but it is not quit easy (maybe
>> have to bear it), it is uapi :-(
> 
> I can't speak for SCSI, but that define in Hexagon isn't really used outside
> of that file anyways (the two SP macros themselves).  I'm also pretty sure
> the userspace isn't currently using it anyways.  Seeing as it's not really
> buying us anything, I'm fine with just removing it and continuing to review
> the other defines.  I'll make that change and test it locally.
> 

OK, thanks.

> 
> Thanks,
> Richard Kuo
> 
> 


-- 
Chen Gang

[PATCH v2] drivers: scsi: scsi_lib.c: use SG_POOL instead of SP

[Chen Gang]

the macro SP is too common to make conflict with others, so recommend
to use another more readable and non-conflict macro SG_POOL instead of
(and recommend others do not use SP either).

The related warning (with allmodconfig for hexagon):

    CC [M]  drivers/scsi/scsi_lib.o
  drivers/scsi/scsi_lib.c:46:0: warning: "SP" redefined [enabled by default]
  arch/hexagon/include/uapi/asm/registers.h:9:0: note: this is the location of the previous definition


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/scsi/scsi_lib.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bd7f0d..19967fa 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -43,28 +43,28 @@ struct scsi_host_sg_pool {
 	mempool_t	*pool;
 };
 
-#define SP(x) { x, "sgpool-" __stringify(x) }
+#define SG_POOL(x) { x, "sgpool-" __stringify(x) }
 #if (SCSI_MAX_SG_SEGMENTS < 32)
 #error SCSI_MAX_SG_SEGMENTS is too small (must be 32 or greater)
 #endif
 static struct scsi_host_sg_pool scsi_sg_pools[] = {
-	SP(8),
-	SP(16),
+	SG_POOL(8),
+	SG_POOL(16),
 #if (SCSI_MAX_SG_SEGMENTS > 32)
-	SP(32),
+	SG_POOL(32),
 #if (SCSI_MAX_SG_SEGMENTS > 64)
-	SP(64),
+	SG_POOL(64),
 #if (SCSI_MAX_SG_SEGMENTS > 128)
-	SP(128),
+	SG_POOL(128),
 #if (SCSI_MAX_SG_SEGMENTS > 256)
 #error SCSI_MAX_SG_SEGMENTS is too large (256 MAX)
 #endif
 #endif
 #endif
 #endif
-	SP(SCSI_MAX_SG_SEGMENTS)
+	SG_POOL(SCSI_MAX_SG_SEGMENTS)
 };
-#undef SP
+#undef SG_POOL
 
 struct kmem_cache *scsi_sdb_cache;
 
-- 
1.7.7.6

[PATCH] drivers: staging: ft1000: ft1000-usb: initialize 'status' with STATUS_SUCCESS in request_code_segment()

[Chen Gang]

If "!bool_case", it returns unexpected value instead of STATUS_SUCCESS,
so need fix it, the related warning (with allmodconfig under hexagon):

    CC [M]  drivers/staging/ft1000/ft1000-usb/ft1000_download.o
  drivers/staging/ft1000/ft1000-usb/ft1000_download.c: In function 'request_code_segment':
  drivers/staging/ft1000/ft1000-usb/ft1000_download.c:581:6: warning: 'status' may be used uninitialized in this function [-Wuninitialized]


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 .../staging/ft1000/ft1000-usb/ft1000_download.c    |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
index 68ded17..15f3062 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
@@ -578,7 +578,7 @@ static int request_code_segment(struct ft1000_usb *ft1000dev, u16 **s_file,
 		 u8 **c_file, const u8 *endpoint, bool boot_case)
 {
 	long word_length;
-	int status;
+	int status = STATUS_SUCCESS;
 
 	/*DEBUG("FT1000:REQUEST_CODE_SEGMENT\n");i*/
 	word_length = get_request_value(ft1000dev);
-- 
1.7.7.6

Re: [PATCH] drivers: staging: ft1000: ft1000-usb: initialize 'status' with STATUS_SUCCESS in request_code_segment()

[Josh Triplett]

On Wed, Nov 27, 2013 at 11:01:18AM +0800, Chen Gang wrote:
> If "!bool_case", it returns unexpected value instead of STATUS_SUCCESS,
> so need fix it, the related warning (with allmodconfig under hexagon):
> 
>     CC [M]  drivers/staging/ft1000/ft1000-usb/ft1000_download.o
>   drivers/staging/ft1000/ft1000-usb/ft1000_download.c: In function 'request_code_segment':
>   drivers/staging/ft1000/ft1000-usb/ft1000_download.c:581:6: warning: 'status' may be used uninitialized in this function [-Wuninitialized]
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  .../staging/ft1000/ft1000-usb/ft1000_download.c    |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
> index 68ded17..15f3062 100644
> --- a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
> +++ b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
> @@ -578,7 +578,7 @@ static int request_code_segment(struct ft1000_usb *ft1000dev, u16 **s_file,
>  		 u8 **c_file, const u8 *endpoint, bool boot_case)
>  {
>  	long word_length;
> -	int status;
> +	int status = STATUS_SUCCESS;
>  
>  	/*DEBUG("FT1000:REQUEST_CODE_SEGMENT\n");i*/
>  	word_length = get_request_value(ft1000dev);
> -- 
> 1.7.7.6

Re: [PATCH] drivers: staging: ft1000: ft1000-usb: initialize 'status' with STATUS_SUCCESS in request_code_segment()

[Chen Gang]

On 11/27/2013 05:18 PM, Josh Triplett wrote:
> On Wed, Nov 27, 2013 at 11:01:18AM +0800, Chen Gang wrote:
>> If "!bool_case", it returns unexpected value instead of STATUS_SUCCESS,
>> so need fix it, the related warning (with allmodconfig under hexagon):
>>
>>     CC [M]  drivers/staging/ft1000/ft1000-usb/ft1000_download.o
>>   drivers/staging/ft1000/ft1000-usb/ft1000_download.c: In function 'request_code_segment':
>>   drivers/staging/ft1000/ft1000-usb/ft1000_download.c:581:6: warning: 'status' may be used uninitialized in this function [-Wuninitialized]
>>
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> 
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> 

Thanks.  :-)

>>  .../staging/ft1000/ft1000-usb/ft1000_download.c    |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
>> index 68ded17..15f3062 100644
>> --- a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
>> +++ b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
>> @@ -578,7 +578,7 @@ static int request_code_segment(struct ft1000_usb *ft1000dev, u16 **s_file,
>>  		 u8 **c_file, const u8 *endpoint, bool boot_case)
>>  {
>>  	long word_length;
>> -	int status;
>> +	int status = STATUS_SUCCESS;
>>  
>>  	/*DEBUG("FT1000:REQUEST_CODE_SEGMENT\n");i*/
>>  	word_length = get_request_value(ft1000dev);
>> -- 
>> 1.7.7.6


-- 
Chen Gang

Re: [PATCH] drivers: staging: ft1000: ft1000-usb: initialize 'status' with STATUS_SUCCESS in request_code_segment()

[Chen Gang]

Oh, another member has already fixed it (found earlier than me), and
integrated it into next-20131203 tree, so this patch is obsoleted.

The related git commit is "8aced95 staging: ft1000: fix use of
potentially uninitialized variable"

Thanks.

On 11/27/2013 05:27 PM, Chen Gang wrote:
> On 11/27/2013 05:18 PM, Josh Triplett wrote:
>> On Wed, Nov 27, 2013 at 11:01:18AM +0800, Chen Gang wrote:
>>> If "!bool_case", it returns unexpected value instead of STATUS_SUCCESS,
>>> so need fix it, the related warning (with allmodconfig under hexagon):
>>>
>>>     CC [M]  drivers/staging/ft1000/ft1000-usb/ft1000_download.o
>>>   drivers/staging/ft1000/ft1000-usb/ft1000_download.c: In function 'request_code_segment':
>>>   drivers/staging/ft1000/ft1000-usb/ft1000_download.c:581:6: warning: 'status' may be used uninitialized in this function [-Wuninitialized]
>>>
>>>
>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>
>> Reviewed-by: Josh Triplett <josh@joshtriplett.org>
>>
> 
> Thanks.  :-)
> 
>>>  .../staging/ft1000/ft1000-usb/ft1000_download.c    |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
>>> index 68ded17..15f3062 100644
>>> --- a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
>>> +++ b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
>>> @@ -578,7 +578,7 @@ static int request_code_segment(struct ft1000_usb *ft1000dev, u16 **s_file,
>>>  		 u8 **c_file, const u8 *endpoint, bool boot_case)
>>>  {
>>>  	long word_length;
>>> -	int status;
>>> +	int status = STATUS_SUCCESS;
>>>  
>>>  	/*DEBUG("FT1000:REQUEST_CODE_SEGMENT\n");i*/
>>>  	word_length = get_request_value(ft1000dev);
>>> -- 
>>> 1.7.7.6
> 
> 


-- 
Chen Gang

[PATCH] drivers: staging: media: go7007: go7007-usb.c use pr_*() instead of dev_*() before 'go' initialized in go7007_usb_probe()

[Chen Gang]

dev_*() assumes 'go' is already initialized, so need use pr_*() instead
of before 'go' initialized. Related warning (with allmodconfig under
hexagon):

    CC [M]  drivers/staging/media/go7007/go7007-usb.o
  drivers/staging/media/go7007/go7007-usb.c: In function 'go7007_usb_probe':
  drivers/staging/media/go7007/go7007-usb.c:1060:2: warning: 'go' may be used uninitialized in this function [-Wuninitialized]

Also remove useless code after 'return' statement.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/staging/media/go7007/go7007-usb.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/go7007/go7007-usb.c b/drivers/staging/media/go7007/go7007-usb.c
index 58684da..30310e9 100644
--- a/drivers/staging/media/go7007/go7007-usb.c
+++ b/drivers/staging/media/go7007/go7007-usb.c
@@ -1057,7 +1057,7 @@ static int go7007_usb_probe(struct usb_interface *intf,
 	char *name;
 	int video_pipe, i, v_urb_len;
 
-	dev_dbg(go->dev, "probing new GO7007 USB board\n");
+	pr_devel("probing new GO7007 USB board\n");
 
 	switch (id->driver_info) {
 	case GO7007_BOARDID_MATRIX_II:
@@ -1097,13 +1097,10 @@ static int go7007_usb_probe(struct usb_interface *intf,
 		board = &board_px_tv402u;
 		break;
 	case GO7007_BOARDID_LIFEVIEW_LR192:
-		dev_err(go->dev, "The Lifeview TV Walker Ultra is not supported. Sorry!\n");
+		pr_err("The Lifeview TV Walker Ultra is not supported. Sorry!\n");
 		return -ENODEV;
-		name = "Lifeview TV Walker Ultra";
-		board = &board_lifeview_lr192;
-		break;
 	case GO7007_BOARDID_SENSORAY_2250:
-		dev_info(go->dev, "Sensoray 2250 found\n");
+		pr_info("Sensoray 2250 found\n");
 		name = "Sensoray 2250/2251";
 		board = &board_sensoray_2250;
 		break;
@@ -1112,7 +1109,7 @@ static int go7007_usb_probe(struct usb_interface *intf,
 		board = &board_ads_usbav_709;
 		break;
 	default:
-		dev_err(go->dev, "unknown board ID %d!\n",
+		pr_err("unknown board ID %d!\n",
 				(unsigned int)id->driver_info);
 		return -ENODEV;
 	}
-- 
1.7.7.6

Re: [PATCH] drivers: staging: media: go7007: go7007-usb.c use pr_*() instead of dev_*() before 'go' initialized in go7007_usb_probe()

[Joe Perches]

On Wed, 2013-11-27 at 11:17 +0800, Chen Gang wrote:
> dev_*() assumes 'go' is already initialized, so need use pr_*() instead
> of before 'go' initialized.
[]
> diff --git a/drivers/staging/media/go7007/go7007-usb.c b/drivers/staging/media/go7007/go7007-usb.c
[]
> @@ -1057,7 +1057,7 @@ static int go7007_usb_probe(struct usb_interface *intf,
>  	char *name;
>  	int video_pipe, i, v_urb_len;
>  
> -	dev_dbg(go->dev, "probing new GO7007 USB board\n");
> +	pr_devel("probing new GO7007 USB board\n");

pr_devel is commonly compiled out completely unless DEBUG is #defined.
You probably want to use pr_debug here.
 

Re: [PATCH] drivers: staging: media: go7007: go7007-usb.c use pr_*() instead of dev_*() before 'go' initialized in go7007_usb_probe()

[Chen Gang]

On 11/27/2013 11:21 AM, Joe Perches wrote:
> On Wed, 2013-11-27 at 11:17 +0800, Chen Gang wrote:
>> dev_*() assumes 'go' is already initialized, so need use pr_*() instead
>> of before 'go' initialized.
> []
>> diff --git a/drivers/staging/media/go7007/go7007-usb.c b/drivers/staging/media/go7007/go7007-usb.c
> []
>> @@ -1057,7 +1057,7 @@ static int go7007_usb_probe(struct usb_interface *intf,
>>  	char *name;
>>  	int video_pipe, i, v_urb_len;
>>  
>> -	dev_dbg(go->dev, "probing new GO7007 USB board\n");
>> +	pr_devel("probing new GO7007 USB board\n");
> 
> pr_devel is commonly compiled out completely unless DEBUG is #defined.
> You probably want to use pr_debug here.
>  
> 

Oh, yes, it is my fault, I will send patch v2.  :-)


Thanks.
-- 
Chen Gang

[PATCH v2] drivers: staging: media: go7007: go7007-usb.c use pr_*() instead of dev_*() before 'go' initialized in go7007_usb_probe()

[Chen Gang]

dev_*() assumes 'go' is already initialized, so need use pr_*() instead
of before 'go' initialized. Related warning (with allmodconfig under
hexagon):

    CC [M]  drivers/staging/media/go7007/go7007-usb.o
  drivers/staging/media/go7007/go7007-usb.c: In function 'go7007_usb_probe':
  drivers/staging/media/go7007/go7007-usb.c:1060:2: warning: 'go' may be used uninitialized in this function [-Wuninitialized]

Also remove useless code after 'return' statement.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/staging/media/go7007/go7007-usb.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/go7007/go7007-usb.c b/drivers/staging/media/go7007/go7007-usb.c
index 58684da..2423643 100644
--- a/drivers/staging/media/go7007/go7007-usb.c
+++ b/drivers/staging/media/go7007/go7007-usb.c
@@ -1057,7 +1057,7 @@ static int go7007_usb_probe(struct usb_interface *intf,
 	char *name;
 	int video_pipe, i, v_urb_len;
 
-	dev_dbg(go->dev, "probing new GO7007 USB board\n");
+	pr_debug("probing new GO7007 USB board\n");
 
 	switch (id->driver_info) {
 	case GO7007_BOARDID_MATRIX_II:
@@ -1097,13 +1097,10 @@ static int go7007_usb_probe(struct usb_interface *intf,
 		board = &board_px_tv402u;
 		break;
 	case GO7007_BOARDID_LIFEVIEW_LR192:
-		dev_err(go->dev, "The Lifeview TV Walker Ultra is not supported. Sorry!\n");
+		pr_err("The Lifeview TV Walker Ultra is not supported. Sorry!\n");
 		return -ENODEV;
-		name = "Lifeview TV Walker Ultra";
-		board = &board_lifeview_lr192;
-		break;
 	case GO7007_BOARDID_SENSORAY_2250:
-		dev_info(go->dev, "Sensoray 2250 found\n");
+		pr_info("Sensoray 2250 found\n");
 		name = "Sensoray 2250/2251";
 		board = &board_sensoray_2250;
 		break;
@@ -1112,7 +1109,7 @@ static int go7007_usb_probe(struct usb_interface *intf,
 		board = &board_ads_usbav_709;
 		break;
 	default:
-		dev_err(go->dev, "unknown board ID %d!\n",
+		pr_err("unknown board ID %d!\n",
 				(unsigned int)id->driver_info);
 		return -ENODEV;
 	}
-- 
1.7.7.6

Re: [PATCH v2] drivers: staging: media: go7007: go7007-usb.c use pr_*() instead of dev_*() before 'go' initialized in go7007_usb_probe()

[Greg KH]

On Wed, Nov 27, 2013 at 11:48:08AM +0800, Chen Gang wrote:
> dev_*() assumes 'go' is already initialized, so need use pr_*() instead
> of before 'go' initialized. Related warning (with allmodconfig under
> hexagon):
> 
>     CC [M]  drivers/staging/media/go7007/go7007-usb.o
>   drivers/staging/media/go7007/go7007-usb.c: In function 'go7007_usb_probe':
>   drivers/staging/media/go7007/go7007-usb.c:1060:2: warning: 'go' may be used uninitialized in this function [-Wuninitialized]
> 
> Also remove useless code after 'return' statement.

This should all be fixed in my staging-linus branch already, right?  No
need for this anymore from what I can tell, sorry.

greg k-h

Re: [PATCH v2] drivers: staging: media: go7007: go7007-usb.c use pr_*() instead of dev_*() before 'go' initialized in go7007_usb_probe()

[Chen Gang]

On 11/27/2013 12:03 PM, Greg KH wrote:
> On Wed, Nov 27, 2013 at 11:48:08AM +0800, Chen Gang wrote:
>> dev_*() assumes 'go' is already initialized, so need use pr_*() instead
>> of before 'go' initialized. Related warning (with allmodconfig under
>> hexagon):
>>
>>     CC [M]  drivers/staging/media/go7007/go7007-usb.o
>>   drivers/staging/media/go7007/go7007-usb.c: In function 'go7007_usb_probe':
>>   drivers/staging/media/go7007/go7007-usb.c:1060:2: warning: 'go' may be used uninitialized in this function [-Wuninitialized]
>>
>> Also remove useless code after 'return' statement.
> 
> This should all be fixed in my staging-linus branch already, right?  No
> need for this anymore from what I can tell, sorry.
> 

That's all right (in fact don't need sorry).  :-)

And excuse me, I am not quite familiar upstream kernel version merging
and branches. Is it still better/suitable/possible to sync some bug fix
patches from staging brach to next brach?


Thanks.
-- 
Chen Gang

Re: [PATCH v2] drivers: staging: media: go7007: go7007-usb.c use pr_*() instead of dev_*() before 'go' initialized in go7007_usb_probe()

[Dan Carpenter]

On Wed, Nov 27, 2013 at 12:24:22PM +0800, Chen Gang wrote:
> On 11/27/2013 12:03 PM, Greg KH wrote:
> > On Wed, Nov 27, 2013 at 11:48:08AM +0800, Chen Gang wrote:
> >> dev_*() assumes 'go' is already initialized, so need use pr_*() instead
> >> of before 'go' initialized. Related warning (with allmodconfig under
> >> hexagon):
> >>
> >>     CC [M]  drivers/staging/media/go7007/go7007-usb.o
> >>   drivers/staging/media/go7007/go7007-usb.c: In function 'go7007_usb_probe':
> >>   drivers/staging/media/go7007/go7007-usb.c:1060:2: warning: 'go' may be used uninitialized in this function [-Wuninitialized]
> >>
> >> Also remove useless code after 'return' statement.
> > 
> > This should all be fixed in my staging-linus branch already, right?  No
> > need for this anymore from what I can tell, sorry.
> > 
> 
> That's all right (in fact don't need sorry).  :-)
> 
> And excuse me, I am not quite familiar upstream kernel version merging
> and branches. Is it still better/suitable/possible to sync some bug fix
> patches from staging brach to next brach?

next syncs with everyone once a day.

regards,
dan carpenter

Re: [PATCH v2] drivers: staging: media: go7007: go7007-usb.c use pr_*() instead of dev_*() before 'go' initialized in go7007_usb_probe()

[Chen Gang]

On 11/27/2013 06:43 PM, Dan Carpenter wrote:
> On Wed, Nov 27, 2013 at 12:24:22PM +0800, Chen Gang wrote:
>> On 11/27/2013 12:03 PM, Greg KH wrote:
>>> On Wed, Nov 27, 2013 at 11:48:08AM +0800, Chen Gang wrote:
>>>> dev_*() assumes 'go' is already initialized, so need use pr_*() instead
>>>> of before 'go' initialized. Related warning (with allmodconfig under
>>>> hexagon):
>>>>
>>>>     CC [M]  drivers/staging/media/go7007/go7007-usb.o
>>>>   drivers/staging/media/go7007/go7007-usb.c: In function 'go7007_usb_probe':
>>>>   drivers/staging/media/go7007/go7007-usb.c:1060:2: warning: 'go' may be used uninitialized in this function [-Wuninitialized]
>>>>
>>>> Also remove useless code after 'return' statement.
>>>
>>> This should all be fixed in my staging-linus branch already, right?  No
>>> need for this anymore from what I can tell, sorry.
>>>
>>
>> That's all right (in fact don't need sorry).  :-)
>>
>> And excuse me, I am not quite familiar upstream kernel version merging
>> and branches. Is it still better/suitable/possible to sync some bug fix
>> patches from staging brach to next brach?
> 
> next syncs with everyone once a day.
> 

OK, thanks.  :-)

-- 
Chen Gang

[PATCH] drivers: staging: ft1000: ft1000-usb: ft1000_debug.c: check return value of get_user() in ft1000_ioctl()

[Chen Gang]

get_user() may fail and cause 'msgsz' uninitialized, so need give a
check. The related warning (with allmodconfig under hexagon):

    CC [M]  drivers/staging/ft1000/ft1000-usb/ft1000_debug.o
  drivers/staging/ft1000/ft1000-usb/ft1000_debug.c: In function 'ft1000_ioctl':
  include/uapi/linux/swab.h:53:9: warning: 'msgsz' may be used uninitialized in this function [-Wuninitialized]
  drivers/staging/ft1000/ft1000-usb/ft1000_debug.c:533:17: note: 'msgsz' was declared here
 

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/staging/ft1000/ft1000-usb/ft1000_debug.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c b/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c
index 68a55ce..ffdc7f5 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c
@@ -560,6 +560,8 @@ static long ft1000_ioctl(struct file *file, unsigned int command,
 
                 /* Get the length field to see how many bytes to copy */
                 result = get_user(msgsz, (__u16 __user *)argp);
+		if (result)
+			break;
                 msgsz = ntohs(msgsz);
                 /* DEBUG("FT1000:ft1000_ioctl: length of message = %d\n", msgsz); */
 
-- 
1.7.7.6

[PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

In next-20131122 tree, if "sdata->vif.type != NL80211_IFTYPE_AP",
'chanctx_conf' will be not initialized, so need check it. Related
warning (with allmodconfig under hexagon):

    CC [M]  net/mac80211/tx.o
  net/mac80211/tx.c: In function 'ieee80211_subif_start_xmit':
  net/mac80211/tx.c:1827:27: warning: 'chanctx_conf' may be used uninitialized in this function [-Wuninitialized]


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 net/mac80211/tx.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index c558b24..f3245d6 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
 			break;
 		/* fall through */
 	case NL80211_IFTYPE_AP:
-		if (sdata->vif.type == NL80211_IFTYPE_AP)
-			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
+		if (sdata->vif.type != NL80211_IFTYPE_AP)
+			goto fail_rcu;
+		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
 		if (!chanctx_conf)
 			goto fail_rcu;
 		fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS);
-- 
1.7.7.6

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

> +++ b/net/mac80211/tx.c
> @@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>  			break;
>  		/* fall through */
>  	case NL80211_IFTYPE_AP:
> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> +		if (sdata->vif.type != NL80211_IFTYPE_AP)
> +			goto fail_rcu;
> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);

This change is completely wrong.

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 11/29/2013 11:38 PM, Johannes Berg wrote:
> 
>> +++ b/net/mac80211/tx.c
>> @@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>  			break;
>>  		/* fall through */
>>  	case NL80211_IFTYPE_AP:
>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>> +		if (sdata->vif.type != NL80211_IFTYPE_AP)
>> +			goto fail_rcu;
>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> 
> This change is completely wrong.
> 

Oh, it is.

Hmm... for me, this work flow still can be implemented with a little
clearer way (at least it will avoid related warning):

-------------------------diff begin------------------------------

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index c558b24..7076128 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1810,14 +1810,14 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
 		if (!chanctx_conf)
 			goto fail_rcu;
 		band = chanctx_conf->def.chan->band;
-		if (sta)
-			break;
-		/* fall through */
+		if (!sta)
+			goto try_next;
+		break;
 	case NL80211_IFTYPE_AP:
-		if (sdata->vif.type == NL80211_IFTYPE_AP)
-			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
+		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
 		if (!chanctx_conf)
 			goto fail_rcu;
+try_next:
 		fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS);
 		/* DA BSSID SA */
 		memcpy(hdr.addr1, skb->data, ETH_ALEN);



-------------------------diff end--------------------------------


Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Sat, 2013-11-30 at 19:59 +0800, Chen Gang wrote:
> On 11/29/2013 11:38 PM, Johannes Berg wrote:
> > 
> >> +++ b/net/mac80211/tx.c
> >> @@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
> >>  			break;
> >>  		/* fall through */
> >>  	case NL80211_IFTYPE_AP:
> >> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> >> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> >> +		if (sdata->vif.type != NL80211_IFTYPE_AP)
> >> +			goto fail_rcu;
> >> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> > 
> > This change is completely wrong.
> > 
> 
> Oh, it is.
> 
> Hmm... for me, this work flow still can be implemented with a little
> clearer way (at least it will avoid related warning):
> 
> -------------------------diff begin------------------------------
> 
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index c558b24..7076128 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1810,14 +1810,14 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>  		if (!chanctx_conf)
>  			goto fail_rcu;
>  		band = chanctx_conf->def.chan->band;
> -		if (sta)
> -			break;
> -		/* fall through */
> +		if (!sta)
> +			goto try_next;
> +		break;
>  	case NL80211_IFTYPE_AP:
> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>  		if (!chanctx_conf)
>  			goto fail_rcu;
> +try_next:

I don't think that's better than the (fairly obvious) fall-through, and
has a pretty odd goto. Also, depending on the compiler, it still knows
the previous case label and doesn't warn.

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 11/30/2013 08:53 PM, Johannes Berg wrote:
> On Sat, 2013-11-30 at 19:59 +0800, Chen Gang wrote:
>> On 11/29/2013 11:38 PM, Johannes Berg wrote:
>>>
>>>> +++ b/net/mac80211/tx.c
>>>> @@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>>>  			break;
>>>>  		/* fall through */
>>>>  	case NL80211_IFTYPE_AP:
>>>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>>>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>>> +		if (sdata->vif.type != NL80211_IFTYPE_AP)
>>>> +			goto fail_rcu;
>>>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>>
>>> This change is completely wrong.
>>>
>>
>> Oh, it is.
>>
>> Hmm... for me, this work flow still can be implemented with a little
>> clearer way (at least it will avoid related warning):
>>
>> -------------------------diff begin------------------------------
>>
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index c558b24..7076128 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -1810,14 +1810,14 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>  		if (!chanctx_conf)
>>  			goto fail_rcu;
>>  		band = chanctx_conf->def.chan->band;
>> -		if (sta)
>> -			break;
>> -		/* fall through */
>> +		if (!sta)
>> +			goto try_next;
>> +		break;
>>  	case NL80211_IFTYPE_AP:
>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>  		if (!chanctx_conf)
>>  			goto fail_rcu;
>> +try_next:
> 
> I don't think that's better than the (fairly obvious) fall-through, and
> has a pretty odd goto. Also, depending on the compiler, it still knows
> the previous case label and doesn't warn.
> 

Yeah, fall-through is obvious. But check 'A' again just near by "case A"
seems a little strange, and some of compilers (or some of versions) are
really not quit smart enough to know it is not a warning.

Hmm... for me, if the code (implementation) can express real logical
work flow as much as directly and simply, the code (implementation) is
clear enough (don't mind whether use 'goto' or not).


And originally, at first, I am really not quite careful enough, and sent
an incorrect patch after noticed the related compiler's warning. :-)


Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 11/30/2013 09:50 PM, Chen Gang wrote:
> On 11/30/2013 08:53 PM, Johannes Berg wrote:
>> On Sat, 2013-11-30 at 19:59 +0800, Chen Gang wrote:
>>> On 11/29/2013 11:38 PM, Johannes Berg wrote:
>>>>
>>>>> +++ b/net/mac80211/tx.c
>>>>> @@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>>>>  			break;
>>>>>  		/* fall through */
>>>>>  	case NL80211_IFTYPE_AP:
>>>>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>>>>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>>>> +		if (sdata->vif.type != NL80211_IFTYPE_AP)
>>>>> +			goto fail_rcu;
>>>>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>>>
>>>> This change is completely wrong.
>>>>
>>>
>>> Oh, it is.
>>>
>>> Hmm... for me, this work flow still can be implemented with a little
>>> clearer way (at least it will avoid related warning):
>>>
>>> -------------------------diff begin------------------------------
>>>
>>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>>> index c558b24..7076128 100644
>>> --- a/net/mac80211/tx.c
>>> +++ b/net/mac80211/tx.c
>>> @@ -1810,14 +1810,14 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>>  		if (!chanctx_conf)
>>>  			goto fail_rcu;
>>>  		band = chanctx_conf->def.chan->band;
>>> -		if (sta)
>>> -			break;
>>> -		/* fall through */
>>> +		if (!sta)
>>> +			goto try_next;
>>> +		break;
>>>  	case NL80211_IFTYPE_AP:
>>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>>  		if (!chanctx_conf)
>>>  			goto fail_rcu;
>>> +try_next:
>>
>> I don't think that's better than the (fairly obvious) fall-through, and
>> has a pretty odd goto. Also, depending on the compiler, it still knows
>> the previous case label and doesn't warn.
>>
> 
> Yeah, fall-through is obvious. But check 'A' again just near by "case A"
> seems a little strange, and some of compilers (or some of versions) are
> really not quit smart enough to know it is not a warning.
> 

Sorry, the paragraph above may lead misunderstanding, I repeated again:

 - fall-through is obvious (although I did not notice it, originally).

 - Check 'A' again just near by "case A" seems a little strange.

 - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK.


Thanks.

> Hmm... for me, if the code (implementation) can express real logical
> work flow as much as directly and simply, the code (implementation) is
> clear enough (don't mind whether use 'goto' or not).
> 
> 
> And originally, at first, I am really not quite careful enough, and sent
> an incorrect patch after noticed the related compiler's warning. :-)
> 
> 
> Thanks.
> 


-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Sat, 2013-11-30 at 22:02 +0800, Chen Gang wrote:

> >>>  	case NL80211_IFTYPE_AP:
> >>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> >>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> >>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> >>>  		if (!chanctx_conf)
> >>>  			goto fail_rcu;
> >>> +try_next:
> >>
> >> I don't think that's better than the (fairly obvious) fall-through, and
> >> has a pretty odd goto. Also, depending on the compiler, it still knows
> >> the previous case label and doesn't warn.
> >>
> > 
> > Yeah, fall-through is obvious. But check 'A' again just near by "case A"
> > seems a little strange, and some of compilers (or some of versions) are
> > really not quit smart enough to know it is not a warning.
> > 
> 
> Sorry, the paragraph above may lead misunderstanding, I repeated again:
> 
>  - fall-through is obvious (although I did not notice it, originally).
> 
>  - Check 'A' again just near by "case A" seems a little strange.
> 
>  - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK.

I know. If you have any good ideas of how to make it more obvious to the
compiler, I'm all ears, I just don't like any of the solutions offered
so far (and you aren't the first to do so either) :-)

FWIW, I find the label to be odd because if you're familiar with the
code then AP/AP_VLAN *should* be identical except for two special things
that are now linearly & neatly handled in the code (the first being the
4-addr station, the second the chanctx assignment which always has to be
done regardless of 4-addr). IMHO the == check after case should be
enough to make a human reader take a closer look. I understand that you
didn't and that's OK since you were just trying to squelch compile
warnings, but I don't see that this one warrants much attention.

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Joe Perches]

On Sat, 2013-11-30 at 21:08 +0100, Johannes Berg wrote:
> On Sat, 2013-11-30 at 22:02 +0800, Chen Gang wrote:
> 
> > >>>  	case NL80211_IFTYPE_AP:
> > >>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> > >>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> > >>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> > >>>  		if (!chanctx_conf)
> > >>>  			goto fail_rcu;
> > >>> +try_next:
> > >>
> > >> I don't think that's better than the (fairly obvious) fall-through, and
> > >> has a pretty odd goto. Also, depending on the compiler, it still knows
> > >> the previous case label and doesn't warn.
> > >>
> > > 
> > > Yeah, fall-through is obvious. But check 'A' again just near by "case A"
> > > seems a little strange, and some of compilers (or some of versions) are
> > > really not quit smart enough to know it is not a warning.
> > > 
> > 
> > Sorry, the paragraph above may lead misunderstanding, I repeated again:
> > 
> >  - fall-through is obvious (although I did not notice it, originally).
> > 
> >  - Check 'A' again just near by "case A" seems a little strange.
> > 
> >  - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK.
> 
> I know. If you have any good ideas of how to make it more obvious to the
> compiler, I'm all ears, I just don't like any of the solutions offered
> so far (and you aren't the first to do so either) :-)
> 
> FWIW, I find the label to be odd because if you're familiar with the
> code then AP/AP_VLAN *should* be identical except for two special things
> that are now linearly & neatly handled in the code (the first being the
> 4-addr station, the second the chanctx assignment which always has to be
> done regardless of 4-addr). IMHO the == check after case should be
> enough to make a human reader take a closer look. I understand that you
> didn't and that's OK since you were just trying to squelch compile
> warnings, but I don't see that this one warrants much attention.

The label/test could be moved to save a couple of lines
of duplicated code.

Maybe:

 net/mac80211/tx.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 70b5a05..b2160f4 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
 		}
 		ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data,
 					u.ap);
-		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
-		if (!chanctx_conf)
-			goto fail_rcu;
-		band = chanctx_conf->def.chan->band;
-		if (sta)
-			break;
 		/* fall through */
 	case NL80211_IFTYPE_AP:
-		if (sdata->vif.type == NL80211_IFTYPE_AP)
-			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
+		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
 		if (!chanctx_conf)
 			goto fail_rcu;
+		if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) {
+			band = chanctx_conf->def.chan->band;
+			if (sta)
+				break;
+		}
 		fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS);
 		/* DA BSSID SA */
 		memcpy(hdr.addr1, skb->data, ETH_ALEN);

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/01/2013 04:39 AM, Joe Perches wrote:
> On Sat, 2013-11-30 at 21:08 +0100, Johannes Berg wrote:
>> On Sat, 2013-11-30 at 22:02 +0800, Chen Gang wrote:
>>>  - fall-through is obvious (although I did not notice it, originally).
>>>
>>>  - Check 'A' again just near by "case A" seems a little strange.
>>>
>>>  - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK.
>>
>> I know. If you have any good ideas of how to make it more obvious to the
>> compiler, I'm all ears, I just don't like any of the solutions offered
>> so far (and you aren't the first to do so either) :-)
>>

OK, thanks.

>> FWIW, I find the label to be odd because if you're familiar with the
>> code then AP/AP_VLAN *should* be identical except for two special things
>> that are now linearly & neatly handled in the code (the first being the
>> 4-addr station, the second the chanctx assignment which always has to be
>> done regardless of 4-addr). IMHO the == check after case should be
>> enough to make a human reader take a closer look. I understand that you
>> didn't and that's OK since you were just trying to squelch compile
>> warnings, but I don't see that this one warrants much attention.
> 
> The label/test could be moved to save a couple of lines
> of duplicated code.
> 

Hmm... for me, it must be an implementation which can satisfy all of us.

If ieee80211_subif_start_xmit() is not performance sensitive (I guess
so), we can use some short static functions instead of some code blocks
within ieee80211_subif_start_xmit().

 - ieee80211_subif_start_xmit() is a long function (600+ lines).

 - use short static function can share some code.

 - if code can be shared, the work flow can be more clearer too (don't
   need fall-through or goto).


And I guess, the implementation below can cause panic when
"sdata->vif.type == NL80211_IFTYPE_AP_VLAN".  :-(

> Maybe:
> 
>  net/mac80211/tx.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 70b5a05..b2160f4 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>  		}
>  		ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data,
>  					u.ap);
> -		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
> -		if (!chanctx_conf)
> -			goto fail_rcu;
> -		band = chanctx_conf->def.chan->band;
> -		if (sta)
> -			break;
>  		/* fall through */
>  	case NL80211_IFTYPE_AP:
> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> +		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
>  		if (!chanctx_conf)
>  			goto fail_rcu;
> +		if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) {
> +			band = chanctx_conf->def.chan->band;
> +			if (sta)
> +				break;
> +		}
>  		fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS);
>  		/* DA BSSID SA */
>  		memcpy(hdr.addr1, skb->data, ETH_ALEN);
> 
> 


-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/01/2013 07:48 AM, Chen Gang wrote:
> On 12/01/2013 04:39 AM, Joe Perches wrote:
>> On Sat, 2013-11-30 at 21:08 +0100, Johannes Berg wrote:
>>> On Sat, 2013-11-30 at 22:02 +0800, Chen Gang wrote:
>>>>  - fall-through is obvious (although I did not notice it, originally).
>>>>
>>>>  - Check 'A' again just near by "case A" seems a little strange.
>>>>
>>>>  - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK.
>>>
>>> I know. If you have any good ideas of how to make it more obvious to the
>>> compiler, I'm all ears, I just don't like any of the solutions offered
>>> so far (and you aren't the first to do so either) :-)
>>>
> 
> OK, thanks.
> 
>>> FWIW, I find the label to be odd because if you're familiar with the
>>> code then AP/AP_VLAN *should* be identical except for two special things
>>> that are now linearly & neatly handled in the code (the first being the
>>> 4-addr station, the second the chanctx assignment which always has to be
>>> done regardless of 4-addr). IMHO the == check after case should be
>>> enough to make a human reader take a closer look. I understand that you
>>> didn't and that's OK since you were just trying to squelch compile
>>> warnings, but I don't see that this one warrants much attention.
>>
>> The label/test could be moved to save a couple of lines
>> of duplicated code.
>>
> 
> Hmm... for me, it must be an implementation which can satisfy all of us.
> 
> If ieee80211_subif_start_xmit() is not performance sensitive (I guess
> so), we can use some short static functions instead of some code blocks
> within ieee80211_subif_start_xmit().
> 
>  - ieee80211_subif_start_xmit() is a long function (600+ lines).
> 

Oh, one typo issue (need use 400+ instead of 600+), it is a long
function (although not quite long).

>  - use short static function can share some code.
> 
>  - if code can be shared, the work flow can be more clearer too (don't
>    need fall-through or goto).
> 
> 
> And I guess, the implementation below can cause panic when
> "sdata->vif.type == NL80211_IFTYPE_AP_VLAN".  :-(
> 
>> Maybe:
>>
>>  net/mac80211/tx.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index 70b5a05..b2160f4 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>  		}
>>  		ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data,
>>  					u.ap);
>> -		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
>> -		if (!chanctx_conf)
>> -			goto fail_rcu;
>> -		band = chanctx_conf->def.chan->band;
>> -		if (sta)
>> -			break;
>>  		/* fall through */
>>  	case NL80211_IFTYPE_AP:
>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>> +		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
>>  		if (!chanctx_conf)
>>  			goto fail_rcu;
>> +		if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) {
>> +			band = chanctx_conf->def.chan->band;
>> +			if (sta)
>> +				break;
>> +		}
>>  		fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS);
>>  		/* DA BSSID SA */
>>  		memcpy(hdr.addr1, skb->data, ETH_ALEN);
>>
>>
> 
> 


-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Sun, 2013-12-01 at 07:48 +0800, Chen Gang wrote:

> If ieee80211_subif_start_xmit() is not performance sensitive (I guess
> so), we can use some short static functions instead of some code blocks
> within ieee80211_subif_start_xmit().
> 
>  - ieee80211_subif_start_xmit() is a long function (600+ lines).
> 
>  - use short static function can share some code.
> 
>  - if code can be shared, the work flow can be more clearer too (don't
>    need fall-through or goto).

Frankly, I'm getting tired of discussing this. Please don't try to
rewrite this code until you've understood it. You suggesting that
"start_xmit()" isn't a performance sensitive function makes me realize
you haven't even tried.

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/01/2013 05:37 PM, Johannes Berg wrote:
> On Sun, 2013-12-01 at 07:48 +0800, Chen Gang wrote:
> 
>> If ieee80211_subif_start_xmit() is not performance sensitive (I guess
>> so), we can use some short static functions instead of some code blocks
>> within ieee80211_subif_start_xmit().
>>
>>  - ieee80211_subif_start_xmit() is a long function (600+ lines).
>>
>>  - use short static function can share some code.
>>
>>  - if code can be shared, the work flow can be more clearer too (don't
>>    need fall-through or goto).
> 
> Frankly, I'm getting tired of discussing this. Please don't try to
> rewrite this code until you've understood it. You suggesting that
> "start_xmit()" isn't a performance sensitive function makes me realize
> you haven't even tried.
> 

OK, thank you for "you are getting tired ...". Please help try when you
have time.  :-)

No I didn't try -- so use 'if' and 'guess' for discussing.

Hmm... if it is performance sensitive:

 - use static function instead of some code block and try to share them.
   it adds additional instructions, but can shrink binary code size.
   if the performance is acceptable, it is the best way to me.

 - else (1st way not acceptable), use macro instead of static function.
   it expends binary code size, but can save some instructions.
   if the performance is acceptable, it is the acceptable way to me.

 - If neither of static function nor macro are acceptable,
   I still prefer to use 'goto' instead of 'fall-through'.
   that can let all compilers feel well, and can not feel any strange.

     normally, when prev case uses fall-through,
     it need be sure of next case need not notice about it (prev case).
     or it will make a little strange for readers when read next case.


Welcome any suggestions or completions.

Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Sat, 2013-11-30 at 12:39 -0800, Joe Perches wrote:

> +++ b/net/mac80211/tx.c
> @@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>  		}
>  		ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data,
>  					u.ap);
> -		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
> -		if (!chanctx_conf)
> -			goto fail_rcu;
> -		band = chanctx_conf->def.chan->band;
> -		if (sta)
> -			break;
>  		/* fall through */
>  	case NL80211_IFTYPE_AP:
> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> +		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);

Good try, but no, now ap_sdata isn't even assigned. :)

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Joe Perches]

On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote:
> Good try, but no, now ap_sdata isn't even assigned. :)

Right.  Oh well.  There's no improving this without
significant rewrite.  Even then, there may not be much
overall improvement.

btw: Chen, I think fall-throughs are fine as long as
they are commented appropriately.

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/02/2013 06:38 AM, Joe Perches wrote:
> On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote:
>> Good try, but no, now ap_sdata isn't even assigned. :)
> 
> Right.  Oh well.  There's no improving this without
> significant rewrite.  Even then, there may not be much
> overall improvement.
> 
> btw: Chen, I think fall-throughs are fine as long as
> they are commented appropriately.
> 

Hmm... for me, when use fall-through, need 2 things:

 - need a comment.

 - next case need not notice about prev case which fall-through.


If we can not fit the 2 things above, either can not use sub functions
or macros for it because of performance reason, 'goto' is better than
'fall-through'.

For me, in most cases, when a function becomes a long function, it is
always better to use sub-fuctions or macros instead of some blocks (but
it is really not urgent :-) ).


Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Sun, 2013-12-01 at 14:38 -0800, Joe Perches wrote:
> On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote:
> > Good try, but no, now ap_sdata isn't even assigned. :)
> 
> Right.  Oh well.  There's no improving this without
> significant rewrite.  Even then, there may not be much
> overall improvement.

I could see an improvement if we were to actually make changes to assign
the pointer based on the iftype, when that changes, so that each
function only has to handle the right type and we don't need the switch
statement at all...

But that's tricky to get right and I'm between vacations and holidays
and all that ...

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/02/2013 10:48 PM, Johannes Berg wrote:
> On Sun, 2013-12-01 at 14:38 -0800, Joe Perches wrote:
>> On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote:
>>> Good try, but no, now ap_sdata isn't even assigned. :)
>>
>> Right.  Oh well.  There's no improving this without
>> significant rewrite.  Even then, there may not be much
>> overall improvement.
> 
> I could see an improvement if we were to actually make changes to assign
> the pointer based on the iftype, when that changes, so that each
> function only has to handle the right type and we don't need the switch
> statement at all...
> 

OK, thanks, it is one of good ideas to me (which I can not find).  :-)


> But that's tricky to get right and I'm between vacations and holidays
> and all that ...
> 

It is really not urgent, and for keeping quality, it is necessary to
spend suitable time resource (e.g 1 hour or more) to  make, review and
test this kind of patch carefully by oneself.

So could you please help improve it when you have suitable related time
resources?


Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Wed, 2013-12-04 at 10:12 +0800, Chen Gang wrote:

> It is really not urgent, and for keeping quality, it is necessary to
> spend suitable time resource (e.g 1 hour or more) to  make, review and
> test this kind of patch carefully by oneself.
> 
> So could you please help improve it when you have suitable related time
> resources?

No. Who are you to suggest what I need to work on?

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/04/2013 04:04 PM, Johannes Berg wrote:
> On Wed, 2013-12-04 at 10:12 +0800, Chen Gang wrote:
> 
>> It is really not urgent, and for keeping quality, it is necessary to
>> spend suitable time resource (e.g 1 hour or more) to  make, review and
>> test this kind of patch carefully by oneself.
>>
>> So could you please help improve it when you have suitable related time
>> resources?
> 
> No. Who are you to suggest what I need to work on?
> 
> johannes
> 

I am one of volunteers for Open Source, and I don't care about who I am.

Since this thread is started by me, I have duty to try my best to let it
finish (but may fail).

According to our original discussion, it seems we agree that I am not
the suitable member to finish it, so I suggest you or another members to
try.


Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Wed, 2013-12-04 at 16:41 +0800, Chen Gang wrote:

> According to our original discussion, it seems we agree that I am not
> the suitable member to finish it, so I suggest you or another members to
> try.

There's nothing to finish here. The code is fine. The compiler is wrong,
but we haven't found a way to shut up the compiler without breaking the
code. Please let's just let it rest.

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/04/2013 04:49 PM, Johannes Berg wrote:
> On Wed, 2013-12-04 at 16:41 +0800, Chen Gang wrote:
> 
>> According to our original discussion, it seems we agree that I am not
>> the suitable member to finish it, so I suggest you or another members to
>> try.
> 
> There's nothing to finish here. The code is fine. The compiler is wrong,
> but we haven't found a way to shut up the compiler without breaking the
> code. Please let's just let it rest.
> 

For me, I still stick to the code can be improved, although it is not
urgent.

But just like you said, we can just stop discussing -- every members can
save their own opinions.

And I am not the related maintainer, so if no additional suggestions,
discussions or completions, I will/should stop here.


Thanks.
-- 
Chen Gang

[PATCH] arch: hexagon: include: uapi: asm: setup.h add swith macro __KERNEL__

[Chen Gang]

Define dummy '__init' instead of include "linux/init.h" if !__KERNEL__,
or can not pass checking. The related error (with allmodconfig under
hexagon):

    CHECK   include/asm (34 files)
  usr/include/asm/setup.h:22: included file 'linux/init.h' is not exported


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 arch/hexagon/include/uapi/asm/setup.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/hexagon/include/uapi/asm/setup.h b/arch/hexagon/include/uapi/asm/setup.h
index e48285e4a..7e3952d 100644
--- a/arch/hexagon/include/uapi/asm/setup.h
+++ b/arch/hexagon/include/uapi/asm/setup.h
@@ -19,7 +19,12 @@
 #ifndef _ASM_SETUP_H
 #define _ASM_SETUP_H
 
+#ifdef __KERNEL__
 #include <linux/init.h>
+#else
+#define __init
+#endif
+
 #include <asm-generic/setup.h>
 
 extern char external_cmdline_buffer;
-- 
1.7.7.6

Re: [PATCH] arch: hexagon: include: uapi: asm: setup.h add swith macro __KERNEL__

[rkuo]

On Wed, Nov 27, 2013 at 01:28:36PM +0800, Chen Gang wrote:
> Define dummy '__init' instead of include "linux/init.h" if !__KERNEL__,
> or can not pass checking. The related error (with allmodconfig under
> hexagon):
> 
>     CHECK   include/asm (34 files)
>   usr/include/asm/setup.h:22: included file 'linux/init.h' is not exported
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  arch/hexagon/include/uapi/asm/setup.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 

Acked-by: Richard Kuo <rkuo@codeaurora.org>

-- 

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation