[PATCH 0/3] staging: wilc1000: rework integer value for x64

[PATCH 0/3] staging: wilc1000: rework integer value for x64

[Johnny Kim]

Hello Greg.

This driver convert pointer address to integer value and use it
as function argument. because the value has a limited 32bit size,
current driver happen many issues on x64 machine.

I have split it up enough sensibliy to allow review. If this patches
are accepted, I will update more patches related with this patch.

Best regards.
Johnny.

Johnny Kim (3):
  staging: wilc1000: wilc_wlan_cfg_commit(): replace integer with
    pointer
  staging: wilc1000: wilc_wlan_cfg_get(): replace integer with void
    pointer
  staging: wilc1000: wilc_wlan_cfg_set(): replace integer with void
    pointer

 drivers/staging/wilc1000/coreconfigurator.c |  4 ++--
 drivers/staging/wilc1000/linux_wlan.c       |  2 +-
 drivers/staging/wilc1000/wilc_wlan.c        | 12 ++++++------
 drivers/staging/wilc1000/wilc_wlan_if.h     |  4 ++--
 4 files changed, 11 insertions(+), 11 deletions(-)

-- 
1.9.1

[PATCH 1/3] staging: wilc1000: wilc_wlan_cfg_commit(): replace integer with pointer

[Johnny Kim]

A argument of wilc_wlan_cfg_commit() is address of structure.
But because the size is restricted to 32bit, it is not correct
for 64bit machine.

So, this changes the interger value to obvious structure pointer.

Signed-off-by: Johnny Kim <johnny.kim@atmel.com>
---
 drivers/staging/wilc1000/wilc_wlan.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
index def72fd..d32e45e 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -1862,13 +1862,13 @@ static void wilc_wlan_cleanup(void)
 
 }
 
-static int wilc_wlan_cfg_commit(int type, uint32_t drvHandler)
+static int wilc_wlan_cfg_commit(int type, tstrWILC_WFIDrv *drvHandler)
 {
 	wilc_wlan_dev_t *p = (wilc_wlan_dev_t *)&g_wlan;
 	wilc_cfg_frame_t *cfg = &p->cfg_frame;
 	int total_len = p->cfg_frame_offset + 4 + DRIVER_HANDLER_SIZE;
 	int seq_no = p->cfg_seq_no % 256;
-	int driver_handler = (u32)drvHandler;
+	uintptr_t driver_handler = (uintptr_t)drvHandler;
 
 
 	/**
@@ -1923,7 +1923,7 @@ static int wilc_wlan_cfg_set(int start, uint32_t wid, uint8_t *buffer, uint32_t
 		p->cfg_frame_in_use = 1;
 
 		/*Edited by Amr - BugID_4720*/
-		if (wilc_wlan_cfg_commit(WILC_CFG_SET, drvHandler))
+		if (wilc_wlan_cfg_commit(WILC_CFG_SET, (tstrWILC_WFIDrv *)drvHandler))
 			ret_size = 0;   /* BugID_5213 */
 
 		if (p->os_func.os_wait(p->cfg_wait, CFG_PKTS_TIMEOUT)) {
@@ -1960,7 +1960,7 @@ static int wilc_wlan_cfg_get(int start, uint32_t wid, int commit, uint32_t drvHa
 		p->cfg_frame_in_use = 1;
 
 		/*Edited by Amr - BugID_4720*/
-		if (wilc_wlan_cfg_commit(WILC_CFG_QUERY, drvHandler))
+		if (wilc_wlan_cfg_commit(WILC_CFG_QUERY, (tstrWILC_WFIDrv *)drvHandler))
 			ret_size = 0;   /* BugID_5213 */
 
 
-- 
1.9.1

[PATCH 2/3] staging: wilc1000: wilc_wlan_cfg_get(): replace integer with void pointer

[Johnny Kim]

Last argument of wilc_wlan_cfg_get function is actually structure's address.
This should be changed to be compatible with 64bit machine.
Because wilc_wlan_cfg_get function is mapped by function pointer later,
wilc_wlan_oup_t.wlan_cfg_get should be changed together.

tstrWILC_WFIDrv structure is defined after wilc_wlan_oup_t.wlan_cfg_get
is defined. So, this patch changes the argument to void type pointer.

Signed-off-by: Johnny Kim <johnny.kim@atmel.com>
---
 drivers/staging/wilc1000/coreconfigurator.c | 2 +-
 drivers/staging/wilc1000/wilc_wlan.c        | 2 +-
 drivers/staging/wilc1000/wilc_wlan_if.h     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
index b069614..141d7b4 100644
--- a/drivers/staging/wilc1000/coreconfigurator.c
+++ b/drivers/staging/wilc1000/coreconfigurator.c
@@ -2094,7 +2094,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
 				   (counter == u32WIDsCount - 1));
 			if (!gpstrWlanOps->wlan_cfg_get(!counter,
 							pstrWIDs[counter].u16WIDid,
-							(counter == u32WIDsCount - 1), drvHandler)) {
+							(counter == u32WIDsCount - 1), (void *)drvHandler)) {
 				ret = -1;
 				printk("[Sendconfigpkt]Get Timed out\n");
 				break;
diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
index d32e45e..c0a8063 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -1938,7 +1938,7 @@ static int wilc_wlan_cfg_set(int start, uint32_t wid, uint8_t *buffer, uint32_t
 
 	return ret_size;
 }
-static int wilc_wlan_cfg_get(int start, uint32_t wid, int commit, uint32_t drvHandler)
+static int wilc_wlan_cfg_get(int start, uint32_t wid, int commit, void* drvHandler)
 {
 	wilc_wlan_dev_t *p = (wilc_wlan_dev_t *)&g_wlan;
 	uint32_t offset;
diff --git a/drivers/staging/wilc1000/wilc_wlan_if.h b/drivers/staging/wilc1000/wilc_wlan_if.h
index 8735a6a..8c293ab 100644
--- a/drivers/staging/wilc1000/wilc_wlan_if.h
+++ b/drivers/staging/wilc1000/wilc_wlan_if.h
@@ -194,7 +194,7 @@ typedef struct {
 	void (*wlan_handle_rx_isr)(void);
 	void (*wlan_cleanup)(void);
 	int (*wlan_cfg_set)(int, uint32_t, uint8_t *, uint32_t, int, uint32_t);
-	int (*wlan_cfg_get)(int, uint32_t, int, uint32_t);
+	int (*wlan_cfg_get)(int, uint32_t, int, void *);
 	int (*wlan_cfg_get_value)(uint32_t, uint8_t *, uint32_t);
 	/*Bug3959: transmitting mgmt frames received from host*/
 	#if defined(WILC_AP_EXTERNAL_MLME) || defined(WILC_P2P)
-- 
1.9.1

[PATCH 3/3] staging: wilc1000: wilc_wlan_cfg_set(): replace integer with void pointer

[Johnny Kim]

Last argument of wilc_wlan_cfg_set function is actually structure's address.
This should be changed to be compatible with 64bit machine.
Because wilc_wlan_cfg_set function is mapped by function pointer later,
wilc_wlan_oup_t.wlan_cfg_set should be changed together.

tstrWILC_WFIDrv structure is defined after wilc_wlan_oup_t.wlan_cfg_set
is defined. So, this patch changes the argument to void type pointer.

Signed-off-by: Johnny Kim <johnny.kim@atmel.com>
---
 drivers/staging/wilc1000/coreconfigurator.c | 2 +-
 drivers/staging/wilc1000/linux_wlan.c       | 2 +-
 drivers/staging/wilc1000/wilc_wlan.c        | 2 +-
 drivers/staging/wilc1000/wilc_wlan_if.h     | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
index 141d7b4..5c1096d 100644
--- a/drivers/staging/wilc1000/coreconfigurator.c
+++ b/drivers/staging/wilc1000/coreconfigurator.c
@@ -2117,7 +2117,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
 			if (!gpstrWlanOps->wlan_cfg_set(!counter,
 							pstrWIDs[counter].u16WIDid, pstrWIDs[counter].ps8WidVal,
 							pstrWIDs[counter].s32ValueSize,
-							(counter == u32WIDsCount - 1), drvHandler)) {
+							(counter == u32WIDsCount - 1), (void *)drvHandler)) {
 				ret = -1;
 				printk("[Sendconfigpkt]Set Timed out\n");
 				break;
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 5a794df..eadc500 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1340,7 +1340,7 @@ static int linux_wlan_init_test_config(struct net_device *dev, linux_wlan_t *p_n
 		goto _fail_;
 
 	c_val[0] = 1; /* Enable N with immediate block ack. */
-	if (!g_linux_wlan->oup.wlan_cfg_set(0, WID_11N_IMMEDIATE_BA_ENABLED, c_val, 1, 1, (u32)pstrWFIDrv))
+	if (!g_linux_wlan->oup.wlan_cfg_set(0, WID_11N_IMMEDIATE_BA_ENABLED, c_val, 1, 1, (void *)pstrWFIDrv))
 		goto _fail_;
 
 	return 0;
diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
index c0a8063..f9dbd40 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -1899,7 +1899,7 @@ static int wilc_wlan_cfg_commit(int type, tstrWILC_WFIDrv *drvHandler)
 	return 0;
 }
 
-static int wilc_wlan_cfg_set(int start, uint32_t wid, uint8_t *buffer, uint32_t buffer_size, int commit, uint32_t drvHandler)
+static int wilc_wlan_cfg_set(int start, uint32_t wid, uint8_t *buffer, uint32_t buffer_size, int commit, void *drvHandler)
 {
 	wilc_wlan_dev_t *p = (wilc_wlan_dev_t *)&g_wlan;
 	uint32_t offset;
diff --git a/drivers/staging/wilc1000/wilc_wlan_if.h b/drivers/staging/wilc1000/wilc_wlan_if.h
index 8c293ab..9d42bd8 100644
--- a/drivers/staging/wilc1000/wilc_wlan_if.h
+++ b/drivers/staging/wilc1000/wilc_wlan_if.h
@@ -193,7 +193,7 @@ typedef struct {
 	void (*wlan_handle_rx_que)(void);
 	void (*wlan_handle_rx_isr)(void);
 	void (*wlan_cleanup)(void);
-	int (*wlan_cfg_set)(int, uint32_t, uint8_t *, uint32_t, int, uint32_t);
+	int (*wlan_cfg_set)(int, uint32_t, uint8_t *, uint32_t, int, void *);
 	int (*wlan_cfg_get)(int, uint32_t, int, void *);
 	int (*wlan_cfg_get_value)(uint32_t, uint8_t *, uint32_t);
 	/*Bug3959: transmitting mgmt frames received from host*/
-- 
1.9.1

Re: [PATCH 1/3] staging: wilc1000: wilc_wlan_cfg_commit(): replace integer with pointer

[Julian Calaby]

Hi Johnny,

On Fri, Jul 10, 2015 at 3:55 PM, Johnny Kim <johnny.kim@atmel.com> wrote:
> A argument of wilc_wlan_cfg_commit() is address of structure.
> But because the size is restricted to 32bit, it is not correct
> for 64bit machine.
>
> So, this changes the interger value to obvious structure pointer.
>
> Signed-off-by: Johnny Kim <johnny.kim@atmel.com>
> ---
>  drivers/staging/wilc1000/wilc_wlan.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
> index def72fd..d32e45e 100644
> --- a/drivers/staging/wilc1000/wilc_wlan.c
> +++ b/drivers/staging/wilc1000/wilc_wlan.c
> @@ -1862,13 +1862,13 @@ static void wilc_wlan_cleanup(void)
>
>  }
>
> -static int wilc_wlan_cfg_commit(int type, uint32_t drvHandler)
> +static int wilc_wlan_cfg_commit(int type, tstrWILC_WFIDrv *drvHandler)
>  {
>         wilc_wlan_dev_t *p = (wilc_wlan_dev_t *)&g_wlan;
>         wilc_cfg_frame_t *cfg = &p->cfg_frame;
>         int total_len = p->cfg_frame_offset + 4 + DRIVER_HANDLER_SIZE;
>         int seq_no = p->cfg_seq_no % 256;
> -       int driver_handler = (u32)drvHandler;
> +       uintptr_t driver_handler = (uintptr_t)drvHandler;

Er, why not just remove this variable and use drvHandler directly?

> @@ -1923,7 +1923,7 @@ static int wilc_wlan_cfg_set(int start, uint32_t wid, uint8_t *buffer, uint32_t
>                 p->cfg_frame_in_use = 1;
>
>                 /*Edited by Amr - BugID_4720*/
> -               if (wilc_wlan_cfg_commit(WILC_CFG_SET, drvHandler))
> +               if (wilc_wlan_cfg_commit(WILC_CFG_SET, (tstrWILC_WFIDrv *)drvHandler))

No need to cast it to it's own type.

>                         ret_size = 0;   /* BugID_5213 */
>
>                 if (p->os_func.os_wait(p->cfg_wait, CFG_PKTS_TIMEOUT)) {
> @@ -1960,7 +1960,7 @@ static int wilc_wlan_cfg_get(int start, uint32_t wid, int commit, uint32_t drvHa
>                 p->cfg_frame_in_use = 1;
>
>                 /*Edited by Amr - BugID_4720*/
> -               if (wilc_wlan_cfg_commit(WILC_CFG_QUERY, drvHandler))
> +               if (wilc_wlan_cfg_commit(WILC_CFG_QUERY, (tstrWILC_WFIDrv *)drvHandler))

Ditto.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 2/3] staging: wilc1000: wilc_wlan_cfg_get(): replace integer with void pointer

[Julian Calaby]

Hi Johnny,

On Fri, Jul 10, 2015 at 3:55 PM, Johnny Kim <johnny.kim@atmel.com> wrote:
> Last argument of wilc_wlan_cfg_get function is actually structure's address.
> This should be changed to be compatible with 64bit machine.
> Because wilc_wlan_cfg_get function is mapped by function pointer later,
> wilc_wlan_oup_t.wlan_cfg_get should be changed together.
>
> tstrWILC_WFIDrv structure is defined after wilc_wlan_oup_t.wlan_cfg_get
> is defined. So, this patch changes the argument to void type pointer.

Why not add a patch moving the structure definition before
wilc_wlan_oup_t.wlan_cfg_get is defined?

> Signed-off-by: Johnny Kim <johnny.kim@atmel.com>
> ---
>  drivers/staging/wilc1000/coreconfigurator.c | 2 +-
>  drivers/staging/wilc1000/wilc_wlan.c        | 2 +-
>  drivers/staging/wilc1000/wilc_wlan_if.h     | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
> index b069614..141d7b4 100644
> --- a/drivers/staging/wilc1000/coreconfigurator.c
> +++ b/drivers/staging/wilc1000/coreconfigurator.c
> @@ -2094,7 +2094,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
>                                    (counter == u32WIDsCount - 1));
>                         if (!gpstrWlanOps->wlan_cfg_get(!counter,
>                                                         pstrWIDs[counter].u16WIDid,
> -                                                       (counter == u32WIDsCount - 1), drvHandler)) {
> +                                                       (counter == u32WIDsCount - 1), (void *)drvHandler)) {

If I recall correctly, you shouldn't need void * casts.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 3/3] staging: wilc1000: wilc_wlan_cfg_set(): replace integer with void pointer

[Julian Calaby]

Hi Johnny,

On Fri, Jul 10, 2015 at 3:55 PM, Johnny Kim <johnny.kim@atmel.com> wrote:
> Last argument of wilc_wlan_cfg_set function is actually structure's address.
> This should be changed to be compatible with 64bit machine.
> Because wilc_wlan_cfg_set function is mapped by function pointer later,
> wilc_wlan_oup_t.wlan_cfg_set should be changed together.
>
> tstrWILC_WFIDrv structure is defined after wilc_wlan_oup_t.wlan_cfg_set
> is defined. So, this patch changes the argument to void type pointer.

Same question, why not move the structure definition before this op is defined?

> Signed-off-by: Johnny Kim <johnny.kim@atmel.com>
> ---
>  drivers/staging/wilc1000/coreconfigurator.c | 2 +-
>  drivers/staging/wilc1000/linux_wlan.c       | 2 +-
>  drivers/staging/wilc1000/wilc_wlan.c        | 2 +-
>  drivers/staging/wilc1000/wilc_wlan_if.h     | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
> index 141d7b4..5c1096d 100644
> --- a/drivers/staging/wilc1000/coreconfigurator.c
> +++ b/drivers/staging/wilc1000/coreconfigurator.c
> @@ -2117,7 +2117,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
>                         if (!gpstrWlanOps->wlan_cfg_set(!counter,
>                                                         pstrWIDs[counter].u16WIDid, pstrWIDs[counter].ps8WidVal,
>                                                         pstrWIDs[counter].s32ValueSize,
> -                                                       (counter == u32WIDsCount - 1), drvHandler)) {
> +                                                       (counter == u32WIDsCount - 1), (void *)drvHandler)) {

Again, you shouldn't need a void * cast.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 1/3] staging: wilc1000: wilc_wlan_cfg_commit(): replace integer with pointer

[Johnny Kim]

On 2015년 07월 10일 15:20, Julian Calaby wrote:
> Hi Johnny,
>
> On Fri, Jul 10, 2015 at 3:55 PM, Johnny Kim <johnny.kim@atmel.com> wrote:
>> A argument of wilc_wlan_cfg_commit() is address of structure.
>> But because the size is restricted to 32bit, it is not correct
>> for 64bit machine.
>>
>> So, this changes the interger value to obvious structure pointer.
>>
>> Signed-off-by: Johnny Kim <johnny.kim@atmel.com>
>> ---
>>   drivers/staging/wilc1000/wilc_wlan.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
>> index def72fd..d32e45e 100644
>> --- a/drivers/staging/wilc1000/wilc_wlan.c
>> +++ b/drivers/staging/wilc1000/wilc_wlan.c
>> @@ -1862,13 +1862,13 @@ static void wilc_wlan_cleanup(void)
>>
>>   }
>>
>> -static int wilc_wlan_cfg_commit(int type, uint32_t drvHandler)
>> +static int wilc_wlan_cfg_commit(int type, tstrWILC_WFIDrv *drvHandler)
>>   {
>>          wilc_wlan_dev_t *p = (wilc_wlan_dev_t *)&g_wlan;
>>          wilc_cfg_frame_t *cfg = &p->cfg_frame;
>>          int total_len = p->cfg_frame_offset + 4 + DRIVER_HANDLER_SIZE;
>>          int seq_no = p->cfg_seq_no % 256;
>> -       int driver_handler = (u32)drvHandler;
>> +       uintptr_t driver_handler = (uintptr_t)drvHandler;
> Er, why not just remove this variable and use drvHandler directly?
A control packet with the address value is sent to chipset.
Namely, it is used to distinguish various packet via the address finally.
>> @@ -1923,7 +1923,7 @@ static int wilc_wlan_cfg_set(int start, uint32_t wid, uint8_t *buffer, uint32_t
>>                  p->cfg_frame_in_use = 1;
>>
>>                  /*Edited by Amr - BugID_4720*/
>> -               if (wilc_wlan_cfg_commit(WILC_CFG_SET, drvHandler))
>> +               if (wilc_wlan_cfg_commit(WILC_CFG_SET, (tstrWILC_WFIDrv *)drvHandler))
> No need to cast it to it's own type.
drvHandler value is u32 integer. So I thought I need to cast it. I will 
remove the patch as your opinion.
>>                          ret_size = 0;   /* BugID_5213 */
>>
>>                  if (p->os_func.os_wait(p->cfg_wait, CFG_PKTS_TIMEOUT)) {
>> @@ -1960,7 +1960,7 @@ static int wilc_wlan_cfg_get(int start, uint32_t wid, int commit, uint32_t drvHa
>>                  p->cfg_frame_in_use = 1;
>>
>>                  /*Edited by Amr - BugID_4720*/
>> -               if (wilc_wlan_cfg_commit(WILC_CFG_QUERY, drvHandler))
>> +               if (wilc_wlan_cfg_commit(WILC_CFG_QUERY, (tstrWILC_WFIDrv *)drvHandler))
> Ditto.
>
> Thanks,
>
Thanks.

Re: [PATCH 2/3] staging: wilc1000: wilc_wlan_cfg_get(): replace integer with void pointer

[Johnny Kim]

On 2015년 07월 10일 15:25, Julian Calaby wrote:
> Hi Johnny,
>
> On Fri, Jul 10, 2015 at 3:55 PM, Johnny Kim <johnny.kim@atmel.com> wrote:
>> Last argument of wilc_wlan_cfg_get function is actually structure's address.
>> This should be changed to be compatible with 64bit machine.
>> Because wilc_wlan_cfg_get function is mapped by function pointer later,
>> wilc_wlan_oup_t.wlan_cfg_get should be changed together.
>>
>> tstrWILC_WFIDrv structure is defined after wilc_wlan_oup_t.wlan_cfg_get
>> is defined. So, this patch changes the argument to void type pointer.
> Why not add a patch moving the structure definition before
> wilc_wlan_oup_t.wlan_cfg_get is defined?
Current patch focus on accessing 64bit address rightly.
The define order you and I mentioned should be repaired with another subject
because of complexity among files.
>> Signed-off-by: Johnny Kim <johnny.kim@atmel.com>
>> ---
>>   drivers/staging/wilc1000/coreconfigurator.c | 2 +-
>>   drivers/staging/wilc1000/wilc_wlan.c        | 2 +-
>>   drivers/staging/wilc1000/wilc_wlan_if.h     | 2 +-
>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
>> index b069614..141d7b4 100644
>> --- a/drivers/staging/wilc1000/coreconfigurator.c
>> +++ b/drivers/staging/wilc1000/coreconfigurator.c
>> @@ -2094,7 +2094,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
>>                                     (counter == u32WIDsCount - 1));
>>                          if (!gpstrWlanOps->wlan_cfg_get(!counter,
>>                                                          pstrWIDs[counter].u16WIDid,
>> -                                                       (counter == u32WIDsCount - 1), drvHandler)) {
>> +                                                       (counter == u32WIDsCount - 1), (void *)drvHandler)) {
> If I recall correctly, you shouldn't need void * casts.
>
> Thanks,
>
Thanks.
Johnny.

Re: [PATCH 1/3] staging: wilc1000: wilc_wlan_cfg_commit(): replace integer with pointer

[Julian Calaby]

Hi Johnny,

On Fri, Jul 10, 2015 at 5:58 PM, Johnny Kim <johnny.kim@atmel.com> wrote:
>
> On 2015년 07월 10일 15:20, Julian Calaby wrote:
>>
>> Hi Johnny,
>>
>> On Fri, Jul 10, 2015 at 3:55 PM, Johnny Kim <johnny.kim@atmel.com> wrote:
>>>
>>> A argument of wilc_wlan_cfg_commit() is address of structure.
>>> But because the size is restricted to 32bit, it is not correct
>>> for 64bit machine.
>>>
>>> So, this changes the interger value to obvious structure pointer.
>>>
>>> Signed-off-by: Johnny Kim <johnny.kim@atmel.com>
>>> ---
>>>   drivers/staging/wilc1000/wilc_wlan.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/staging/wilc1000/wilc_wlan.c
>>> b/drivers/staging/wilc1000/wilc_wlan.c
>>> index def72fd..d32e45e 100644
>>> --- a/drivers/staging/wilc1000/wilc_wlan.c
>>> +++ b/drivers/staging/wilc1000/wilc_wlan.c
>>> @@ -1862,13 +1862,13 @@ static void wilc_wlan_cleanup(void)
>>>
>>>   }
>>>
>>> -static int wilc_wlan_cfg_commit(int type, uint32_t drvHandler)
>>> +static int wilc_wlan_cfg_commit(int type, tstrWILC_WFIDrv *drvHandler)
>>>   {
>>>          wilc_wlan_dev_t *p = (wilc_wlan_dev_t *)&g_wlan;
>>>          wilc_cfg_frame_t *cfg = &p->cfg_frame;
>>>          int total_len = p->cfg_frame_offset + 4 + DRIVER_HANDLER_SIZE;
>>>          int seq_no = p->cfg_seq_no % 256;
>>> -       int driver_handler = (u32)drvHandler;
>>> +       uintptr_t driver_handler = (uintptr_t)drvHandler;
>>
>> Er, why not just remove this variable and use drvHandler directly?
>
> A control packet with the address value is sent to chipset.
> Namely, it is used to distinguish various packet via the address finally.

Yes, and why does this require another variable?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 2/3] staging: wilc1000: wilc_wlan_cfg_get(): replace integer with void pointer

[Julian Calaby]

Hi Johnny,

On Fri, Jul 10, 2015 at 6:11 PM, Johnny Kim <johnny.kim@atmel.com> wrote:
>
> On 2015년 07월 10일 15:25, Julian Calaby wrote:
>>
>> Hi Johnny,
>>
>> On Fri, Jul 10, 2015 at 3:55 PM, Johnny Kim <johnny.kim@atmel.com> wrote:
>>>
>>> Last argument of wilc_wlan_cfg_get function is actually structure's
>>> address.
>>> This should be changed to be compatible with 64bit machine.
>>> Because wilc_wlan_cfg_get function is mapped by function pointer later,
>>> wilc_wlan_oup_t.wlan_cfg_get should be changed together.
>>>
>>> tstrWILC_WFIDrv structure is defined after wilc_wlan_oup_t.wlan_cfg_get
>>> is defined. So, this patch changes the argument to void type pointer.
>>
>> Why not add a patch moving the structure definition before
>> wilc_wlan_oup_t.wlan_cfg_get is defined?
>
> Current patch focus on accessing 64bit address rightly.
> The define order you and I mentioned should be repaired with another subject
> because of complexity among files.

I'm not saying it should be part of this patch, I'm saying that it
should be a patch in this series. Some of the changes you're making
look like you're fixing one bug only to replace it with another
different one.

But back to the whole issue of order:

tstrWILC_WFIDrv is defined in host_interface.h

wlan_cfg_get is defined in wilc_wlan_if.h

A patch ensuring that host_interface.h is included before
wilc_wlan_if.h in all files should be pretty easy to produce, assuming
that host_interface.h doesn't use anything in wilc_wlan_if.h.

If you include this patch, you can avoid patches later to change the
void pointers to typed pointers.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 2/3] staging: wilc1000: wilc_wlan_cfg_get(): replace integer with void pointer

[Greg KH]

On Fri, Jul 10, 2015 at 02:55:56PM +0900, Johnny Kim wrote:
> Last argument of wilc_wlan_cfg_get function is actually structure's address.
> This should be changed to be compatible with 64bit machine.
> Because wilc_wlan_cfg_get function is mapped by function pointer later,
> wilc_wlan_oup_t.wlan_cfg_get should be changed together.
> 
> tstrWILC_WFIDrv structure is defined after wilc_wlan_oup_t.wlan_cfg_get
> is defined. So, this patch changes the argument to void type pointer.

No, you should never use a void pointer, that indicates the code is
designed wrong.

Please reorder the structures in a .h file if needed, there is nothing
preventing you from doing the right thing here.

Please fix up this whole series and resend, I can't take it as-is,
sorry.

greg k-h