Re: [PATCH] mwifiex: Use put_unaligned_le32

2017-10-06 Thread kbuild test robot 
Hi Himanshu,

[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on v4.14-rc3 next-20170929]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Himanshu-Jha/mwifiex-Use-put_unaligned_le32/20171007-095017
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git 
master
config: openrisc-allyesconfig (attached as .config)
compiler: or1k-linux-gcc (GCC) 5.4.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=openrisc 

All errors (new ones prefixed by >>):

   In file included from arch/openrisc/include/asm/unaligned.h:42:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from drivers/net//wireless/marvell/mwifiex/decl.h:28,
from drivers/net//wireless/marvell/mwifiex/cmdevt.c:21:
>> include/linux/unaligned/be_memmove.h:6:19: error: redefinition of 
>> 'get_unaligned_be16'
static inline u16 get_unaligned_be16(const void *p)
  ^
   In file included from drivers/net//wireless/marvell/mwifiex/cmdevt.c:20:0:
   include/linux/unaligned/access_ok.h:22:28: note: previous definition of 
'get_unaligned_be16' was here
static __always_inline u16 get_unaligned_be16(const void *p)
   ^
   In file included from arch/openrisc/include/asm/unaligned.h:42:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from drivers/net//wireless/marvell/mwifiex/decl.h:28,
from drivers/net//wireless/marvell/mwifiex/cmdevt.c:21:
>> include/linux/unaligned/be_memmove.h:11:19: error: redefinition of 
>> 'get_unaligned_be32'
static inline u32 get_unaligned_be32(const void *p)
  ^
   In file included from drivers/net//wireless/marvell/mwifiex/cmdevt.c:20:0:
   include/linux/unaligned/access_ok.h:27:28: note: previous definition of 
'get_unaligned_be32' was here
static __always_inline u32 get_unaligned_be32(const void *p)
   ^
   In file included from arch/openrisc/include/asm/unaligned.h:42:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from drivers/net//wireless/marvell/mwifiex/decl.h:28,
from drivers/net//wireless/marvell/mwifiex/cmdevt.c:21:
>> include/linux/unaligned/be_memmove.h:16:19: error: redefinition of 
>> 'get_unaligned_be64'
static inline u64 get_unaligned_be64(const void *p)
  ^
   In file included from drivers/net//wireless/marvell/mwifiex/cmdevt.c:20:0:
   include/linux/unaligned/access_ok.h:32:28: note: previous definition of 
'get_unaligned_be64' was here
static __always_inline u64 get_unaligned_be64(const void *p)
   ^
   In file included from arch/openrisc/include/asm/unaligned.h:42:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from drivers/net//wireless/marvell/mwifiex/decl.h:28,
from drivers/net//wireless/marvell/mwifiex/cmdevt.c:21:
>> include/linux/unaligned/be_memmove.h:21:20: error: redefinition of 
>> 'put_unaligned_be16'
static inline void put_unaligned_be16(u16 val, void *p)
   ^
   In file included from drivers/net//wireless/marvell/mwifiex/cmdevt.c:20:0:
   include/linux/unaligned/access_ok.h:52:29: note: previous definition of 
'put_unaligned_be16' was here
static __always_inline void put_unaligned_be16(u16 val, void *p)
^
   In file included from arch/openrisc/include/asm/unaligned.h:42:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from drivers/net//wireless/marvell/mwifiex/decl.h:28,
from drivers/net//wireless/marvell/mwifiex/cmdevt.c:21:
>> include/linux/unaligned/be_memmove.h:26:20: error: redefinition of 
>> 'put_unaligned_be32'
static inline void put_unaligned_be32(u32 val, void *p)
   ^
   In file included from drivers/net//wireless/marvell/mwifiex/cmdevt.c:20:0:
   include/linux/unaligned/access_ok.h:57:29: note: previous definition of 
'put_unaligned_be32' was here
static __always_inline void put_unaligned_be32(u32 val, void *p)
^
   In file included from arch/openrisc/include/asm/unaligned.h:42:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from

Re: [PATCH] mwifiex: Use put_unaligned_le32

2017-10-06 Thread kbuild test robot 
Hi Himanshu,

[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on v4.14-rc3 next-20170929]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Himanshu-Jha/mwifiex-Use-put_unaligned_le32/20171007-095017
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git 
master
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   In file included from arch/xtensa/include/asm/unaligned.h:22:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from drivers/net//wireless/marvell/mwifiex/decl.h:28,
from drivers/net//wireless/marvell/mwifiex/cmdevt.c:21:
>> include/linux/unaligned/be_struct.h:6:19: error: redefinition of 
>> 'get_unaligned_be16'
static inline u16 get_unaligned_be16(const void *p)
  ^
   In file included from drivers/net//wireless/marvell/mwifiex/cmdevt.c:20:0:
   include/linux/unaligned/access_ok.h:22:28: note: previous definition of 
'get_unaligned_be16' was here
static __always_inline u16 get_unaligned_be16(const void *p)
   ^
   In file included from arch/xtensa/include/asm/unaligned.h:22:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from drivers/net//wireless/marvell/mwifiex/decl.h:28,
from drivers/net//wireless/marvell/mwifiex/cmdevt.c:21:
>> include/linux/unaligned/be_struct.h:11:19: error: redefinition of 
>> 'get_unaligned_be32'
static inline u32 get_unaligned_be32(const void *p)
  ^
   In file included from drivers/net//wireless/marvell/mwifiex/cmdevt.c:20:0:
   include/linux/unaligned/access_ok.h:27:28: note: previous definition of 
'get_unaligned_be32' was here
static __always_inline u32 get_unaligned_be32(const void *p)
   ^
   In file included from arch/xtensa/include/asm/unaligned.h:22:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from drivers/net//wireless/marvell/mwifiex/decl.h:28,
from drivers/net//wireless/marvell/mwifiex/cmdevt.c:21:
>> include/linux/unaligned/be_struct.h:16:19: error: redefinition of 
>> 'get_unaligned_be64'
static inline u64 get_unaligned_be64(const void *p)
  ^
   In file included from drivers/net//wireless/marvell/mwifiex/cmdevt.c:20:0:
   include/linux/unaligned/access_ok.h:32:28: note: previous definition of 
'get_unaligned_be64' was here
static __always_inline u64 get_unaligned_be64(const void *p)
   ^
   In file included from arch/xtensa/include/asm/unaligned.h:22:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from drivers/net//wireless/marvell/mwifiex/decl.h:28,
from drivers/net//wireless/marvell/mwifiex/cmdevt.c:21:
>> include/linux/unaligned/be_struct.h:21:20: error: redefinition of 
>> 'put_unaligned_be16'
static inline void put_unaligned_be16(u16 val, void *p)
   ^
   In file included from drivers/net//wireless/marvell/mwifiex/cmdevt.c:20:0:
   include/linux/unaligned/access_ok.h:52:29: note: previous definition of 
'put_unaligned_be16' was here
static __always_inline void put_unaligned_be16(u16 val, void *p)
^
   In file included from arch/xtensa/include/asm/unaligned.h:22:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from drivers/net//wireless/marvell/mwifiex/decl.h:28,
from drivers/net//wireless/marvell/mwifiex/cmdevt.c:21:
>> include/linux/unaligned/be_struct.h:26:20: error: redefinition of 
>> 'put_unaligned_be32'
static inline void put_unaligned_be32(u32 val, void *p)
   ^
   In file included from drivers/net//wireless/marvell/mwifiex/cmdevt.c:20:0:
   include/linux/unaligned/access_ok.h:57:29: note: previous definition of 
'put_unaligned_be32' was here
static __always_inline void put_unaligned_be32(u32 val, void *p)
^
   In file included from arch/xtensa/include/asm/unaligned.h:22:0,
from include/linux/etherdevice.h:28,
from include/linux/ieee80211.h:22,
from drivers/net//wireless/marvell/mwifiex/decl.h:28,

Re: [PATCH] mwifiex: Use put_unaligned_le32

2017-10-06 Thread Kalle Valo 
Himanshu Jha  writes:

> On Thu, Oct 05, 2017 at 11:02:50AM -0700, Brian Norris wrote:
>> On Thu, Oct 05, 2017 at 08:52:33PM +0530, Himanshu Jha wrote:
>> > There are various instances where a function used in file say for eg
>> > int func_align (void* a)
>> > is used and it is defined in align.h
>> > But many files don't *directly* include align.h and rather include
>> > any other header which includes align.h
>> 
>> I believe the general rule is that you should included headers for all
>> symbols you use, and not rely on implicit includes.
>> 
>> The modification to the general rule is that not all headers are
>> intended to be included directly, and in such cases there's likely a
>> parent header that is the more appropriate target.
>> 
>> In this case, the key is CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS. It
>> seems that asm-generic/unaligned.h is set up to include different
>> headers, based on the expected architecture behavior.
>>
> Yes, asm-generic/unaligned.h looks more appopriate and is most generic
> implementation of unaligned accesses and  arc specific.
>
> Let's see what Kalle Valo recommends! And then I will send v2 of the
> patch.

Not sure what you are asking from me. But if you are asking should it
be:

#include 

or:

#include 

I think it should be the former.

-- 
Kalle Valo

Re: [PATCH] mwifiex: Use put_unaligned_le32

2017-10-05 Thread Igor Mitsyanko 

On 10/05/2017 12:07 PM, Himanshu Jha wrote:


In this case, the key is CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS. It
seems that asm-generic/unaligned.h is set up to include different
headers, based on the expected architecture behavior.


Yes, asm-generic/unaligned.h looks more appopriate and is most generic
implementation of unaligned accesses and  arc specific.



Probably the idea is that each ARCH knows exactly what is the best way 
to do unaligned access, so common code (like drivers) should include 
ARCH-specific asm/unaligned.h only. It will then decide how to do that 
and will include asm-generic/unaligned.h itself, if required.

Re: [PATCH] mwifiex: Use put_unaligned_le32

2017-10-05 Thread Himanshu Jha 
On Thu, Oct 05, 2017 at 11:02:50AM -0700, Brian Norris wrote:
> On Thu, Oct 05, 2017 at 08:52:33PM +0530, Himanshu Jha wrote:
> > There are various instances where a function used in file say for eg
> > int func_align (void* a)
> > is used and it is defined in align.h
> > But many files don't *directly* include align.h and rather include
> > any other header which includes align.h
> 
> I believe the general rule is that you should included headers for all
> symbols you use, and not rely on implicit includes.
> 
> The modification to the general rule is that not all headers are
> intended to be included directly, and in such cases there's likely a
> parent header that is the more appropriate target.
> 
> In this case, the key is CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS. It
> seems that asm-generic/unaligned.h is set up to include different
> headers, based on the expected architecture behavior.
>
Yes, asm-generic/unaligned.h looks more appopriate and is most generic
implementation of unaligned accesses and  arc specific.

Let's see what Kalle Valo recommends! And then I will send v2 of the
patch.

Thanks for the information!

Himanshu Jha

> I wonder if include/linux/unaligned/access_ok.h should have a safety
> check (e.g., raise an #error if
> !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?).
> 
> > Is compiling the file the only way to check if apppropriate header is
> > included or is there some other way to check for it.
> 
> I believe it's mostly manual. Implicit includes have been a problem for
> anyone who refactors header files.
> 
> Brian

Re: [PATCH] mwifiex: Use put_unaligned_le32

2017-10-05 Thread Brian Norris 
On Thu, Oct 05, 2017 at 08:52:33PM +0530, Himanshu Jha wrote:
> There are various instances where a function used in file say for eg
> int func_align (void* a)
> is used and it is defined in align.h
> But many files don't *directly* include align.h and rather include
> any other header which includes align.h

I believe the general rule is that you should included headers for all
symbols you use, and not rely on implicit includes.

The modification to the general rule is that not all headers are
intended to be included directly, and in such cases there's likely a
parent header that is the more appropriate target.

In this case, the key is CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS. It
seems that asm-generic/unaligned.h is set up to include different
headers, based on the expected architecture behavior.

I wonder if include/linux/unaligned/access_ok.h should have a safety
check (e.g., raise an #error if
!CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?).

> Is compiling the file the only way to check if apppropriate header is
> included or is there some other way to check for it.

I believe it's mostly manual. Implicit includes have been a problem for
anyone who refactors header files.

Brian

Re: [PATCH] mwifiex: Use put_unaligned_le32

2017-10-05 Thread Himanshu Jha 
On Thu, Oct 05, 2017 at 11:41:38AM +0300, Kalle Valo wrote:
> Himanshu Jha  writes:
> 
> >> > --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> >> > +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> >> > @@ -17,6 +17,7 @@
> >> >   * this warranty disclaimer.
> >> >   */
> >> >  
> >> > +#include 
> >> 
> >> I don't think this is correct. Should it be asm/unaligned.h?
> >
> > Would mind explainig me as to why it is incorrect! Also, it defined in
> > both the header files but, why is asm/unaligned.h preferred ?
> 
> asm/unaligned.h seems to be the toplevel header file which includes
> header files based on arch configuration. Also grepping the sources
> support that, nobody from drivers/ include access_ok.h directly.
>
Yes, you are correct!

I will send v2 patch with asm/unaligned.h

> But I can't say that I fully understand how the header files work so
> please do correct me if I have mistaken.
>
It is confusing to me as well.
There are various instances where a function used in file say for eg
int func_align (void* a)
is used and it is defined in align.h
But many files don't *directly* include align.h and rather include
any other header which includes align.h

Is compiling the file the only way to check if apppropriate header is
included or is there some other way to check for it.

Thanks

Re: [PATCH] mwifiex: Use put_unaligned_le32

2017-10-05 Thread Kalle Valo 
Himanshu Jha  writes:

>> > --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
>> > +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
>> > @@ -17,6 +17,7 @@
>> >   * this warranty disclaimer.
>> >   */
>> >  
>> > +#include 
>> 
>> I don't think this is correct. Should it be asm/unaligned.h?
>
> Would mind explainig me as to why it is incorrect! Also, it defined in
> both the header files but, why is asm/unaligned.h preferred ?

asm/unaligned.h seems to be the toplevel header file which includes
header files based on arch configuration. Also grepping the sources
support that, nobody from drivers/ include access_ok.h directly.

But I can't say that I fully understand how the header files work so
please do correct me if I have mistaken.

-- 
Kalle Valo

Re: [PATCH] mwifiex: Use put_unaligned_le32

2017-10-05 Thread Himanshu Jha 
On Thu, Oct 05, 2017 at 10:23:37AM +0300, Kalle Valo wrote:
> Himanshu Jha  writes:
> 
> > Use put_unaligned_le32 rather than using byte ordering function and
> > memcpy which makes code clear.
> > Also, add the header file where it is declared.
> >
> > Done using Coccinelle and semantic patch used is :
> >
> > @ rule1 @
> > identifier tmp; expression ptr,x; type T;
> > @@
> >
> > - tmp = cpu_to_le32(x);
> >
> >   <+... when != tmp
> > - memcpy(ptr, (T), ...);
> > + put_unaligned_le32(x,ptr);
> >   ...+>
> >
> > @ depends on rule1 @
> > type j; identifier tmp;
> > @@
> >
> > - j tmp;
> >   ...when != tmp
> >
> > Signed-off-by: Himanshu Jha 
> > ---
> >  drivers/net/wireless/marvell/mwifiex/cmdevt.c | 10 --
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
> > b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> > index 0edc5d6..e28e119 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> > @@ -17,6 +17,7 @@
> >   * this warranty disclaimer.
> >   */
> >  
> > +#include 
> 
> I don't think this is correct. Should it be asm/unaligned.h?

Would mind explainig me as to why it is incorrect! Also, it defined in
both the header files but, why is asm/unaligned.h preferred ?

Thanks

> -- 
> Kalle Valo

Re: [PATCH] mwifiex: Use put_unaligned_le32

2017-10-05 Thread Kalle Valo 
Himanshu Jha  writes:

> Use put_unaligned_le32 rather than using byte ordering function and
> memcpy which makes code clear.
> Also, add the header file where it is declared.
>
> Done using Coccinelle and semantic patch used is :
>
> @ rule1 @
> identifier tmp; expression ptr,x; type T;
> @@
>
> - tmp = cpu_to_le32(x);
>
>   <+... when != tmp
> - memcpy(ptr, (T), ...);
> + put_unaligned_le32(x,ptr);
>   ...+>
>
> @ depends on rule1 @
> type j; identifier tmp;
> @@
>
> - j tmp;
>   ...when != tmp
>
> Signed-off-by: Himanshu Jha 
> ---
>  drivers/net/wireless/marvell/mwifiex/cmdevt.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
> b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> index 0edc5d6..e28e119 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> @@ -17,6 +17,7 @@
>   * this warranty disclaimer.
>   */
>  
> +#include 

I don't think this is correct. Should it be asm/unaligned.h?

-- 
Kalle Valo

[PATCH] mwifiex: Use put_unaligned_le32

2017-10-04 Thread Himanshu Jha 
Use put_unaligned_le32 rather than using byte ordering function and
memcpy which makes code clear.
Also, add the header file where it is declared.

Done using Coccinelle and semantic patch used is :

@ rule1 @
identifier tmp; expression ptr,x; type T;
@@

- tmp = cpu_to_le32(x);

  <+... when != tmp
- memcpy(ptr, (T), ...);
+ put_unaligned_le32(x,ptr);
  ...+>

@ depends on rule1 @
type j; identifier tmp;
@@

- j tmp;
  ...when != tmp

Signed-off-by: Himanshu Jha 
---
 drivers/net/wireless/marvell/mwifiex/cmdevt.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 0edc5d6..e28e119 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -17,6 +17,7 @@
  * this warranty disclaimer.
  */
 
+#include 
 #include "decl.h"
 #include "ioctl.h"
 #include "util.h"
@@ -183,7 +184,6 @@ static int mwifiex_dnld_cmd_to_fw(struct mwifiex_private 
*priv,
uint16_t cmd_code;
uint16_t cmd_size;
unsigned long flags;
-   __le32 tmp;
 
if (!adapter || !cmd_node)
return -1;
@@ -249,9 +249,9 @@ static int mwifiex_dnld_cmd_to_fw(struct mwifiex_private 
*priv,
mwifiex_dbg_dump(adapter, CMD_D, "cmd buffer:", host_cmd, cmd_size);
 
if (adapter->iface_type == MWIFIEX_USB) {
-   tmp = cpu_to_le32(MWIFIEX_USB_TYPE_CMD);
skb_push(cmd_node->cmd_skb, MWIFIEX_TYPE_LEN);
-   memcpy(cmd_node->cmd_skb->data, , MWIFIEX_TYPE_LEN);
+   put_unaligned_le32(MWIFIEX_USB_TYPE_CMD,
+  cmd_node->cmd_skb->data);
adapter->cmd_sent = true;
ret = adapter->if_ops.host_to_card(adapter,
   MWIFIEX_USB_EP_CMD_EVENT,
@@ -317,7 +317,6 @@ static int mwifiex_dnld_sleep_confirm_cmd(struct 
mwifiex_adapter *adapter)
(struct mwifiex_opt_sleep_confirm *)
adapter->sleep_cfm->data;
struct sk_buff *sleep_cfm_tmp;
-   __le32 tmp;
 
priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
 
@@ -342,8 +341,7 @@ static int mwifiex_dnld_sleep_confirm_cmd(struct 
mwifiex_adapter *adapter)
  + MWIFIEX_TYPE_LEN);
skb_put(sleep_cfm_tmp, sizeof(struct mwifiex_opt_sleep_confirm)
+ MWIFIEX_TYPE_LEN);
-   tmp = cpu_to_le32(MWIFIEX_USB_TYPE_CMD);
-   memcpy(sleep_cfm_tmp->data, , MWIFIEX_TYPE_LEN);
+   put_unaligned_le32(MWIFIEX_USB_TYPE_CMD, sleep_cfm_tmp->data);
memcpy(sleep_cfm_tmp->data + MWIFIEX_TYPE_LEN,
   adapter->sleep_cfm->data,
   sizeof(struct mwifiex_opt_sleep_confirm));
-- 
2.7.4