[PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Wu Yunchuan]

No need cast (void *) to (struct ar9170 *), (u8 *) or (void*).

Signed-off-by: Wu Yunchuan <yunchuan@nfschina.com>
---
 drivers/net/wireless/ath/carl9170/usb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c
index e4eb666c6eea..c4edf8355941 100644
--- a/drivers/net/wireless/ath/carl9170/usb.c
+++ b/drivers/net/wireless/ath/carl9170/usb.c
@@ -178,7 +178,7 @@ static void carl9170_usb_tx_data_complete(struct urb *urb)
 	switch (urb->status) {
 	/* everything is fine */
 	case 0:
-		carl9170_tx_callback(ar, (void *)urb->context);
+		carl9170_tx_callback(ar, urb->context);
 		break;
 
 	/* disconnect */
@@ -369,7 +369,7 @@ void carl9170_usb_handle_tx_err(struct ar9170 *ar)
 	struct urb *urb;
 
 	while ((urb = usb_get_from_anchor(&ar->tx_err))) {
-		struct sk_buff *skb = (void *)urb->context;
+		struct sk_buff *skb = urb->context;
 
 		carl9170_tx_drop(ar, skb);
 		carl9170_tx_callback(ar, skb);
@@ -397,7 +397,7 @@ static void carl9170_usb_tasklet(struct tasklet_struct *t)
 
 static void carl9170_usb_rx_complete(struct urb *urb)
 {
-	struct ar9170 *ar = (struct ar9170 *)urb->context;
+	struct ar9170 *ar = urb->context;
 	int err;
 
 	if (WARN_ON_ONCE(!ar))
@@ -559,7 +559,7 @@ static int carl9170_usb_flush(struct ar9170 *ar)
 	int ret, err = 0;
 
 	while ((urb = usb_get_from_anchor(&ar->tx_wait))) {
-		struct sk_buff *skb = (void *)urb->context;
+		struct sk_buff *skb = urb->context;
 		carl9170_tx_drop(ar, skb);
 		carl9170_tx_callback(ar, skb);
 		usb_free_urb(urb);
@@ -668,7 +668,7 @@ int carl9170_exec_cmd(struct ar9170 *ar, const enum carl9170_cmd_oids cmd,
 		memcpy(ar->cmd.data, payload, plen);
 
 	spin_lock_bh(&ar->cmd_lock);
-	ar->readbuf = (u8 *)out;
+	ar->readbuf = out;
 	ar->readlen = outlen;
 	spin_unlock_bh(&ar->cmd_lock);
 
-- 
2.30.2

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Jeff Johnson]

On 9/18/2023 9:49 PM, Wu Yunchuan wrote:
> No need cast (void *) to (struct ar9170 *), (u8 *) or (void*).
> 
> Signed-off-by: Wu Yunchuan <yunchuan@nfschina.com>

Subject should have wifi: prefix added. Kalle can do that when he merges.

Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Christian Lamparter]

On 9/19/23 06:49, Wu Yunchuan wrote:
> No need cast (void *) to (struct ar9170 *), (u8 *) or (void*).

hmm, your mail went into the spam folder. Good thing I checked.

 From what I remember: The reason why these casts were added in
carl9170 was because of compiler warnings/complaints.
Current gcc compilers should be OK (given that the kernel-bot
didn't react, or went your Mail to their spam-folder as well?)
but have you checked these older versions?

(In 6.5.0 Documentation/admin-guide/README.rst states that one
should have at least gcc 5.1 - could you run with those and
see if C=2 W=1 passes?)

Regards,
Christian

> Signed-off-by: Wu Yunchuan <yunchuan@nfschina.com>
> ---
>   drivers/net/wireless/ath/carl9170/usb.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c
> index e4eb666c6eea..c4edf8355941 100644
> --- a/drivers/net/wireless/ath/carl9170/usb.c
> +++ b/drivers/net/wireless/ath/carl9170/usb.c
> @@ -178,7 +178,7 @@ static void carl9170_usb_tx_data_complete(struct urb *urb)
>   	switch (urb->status) {
>   	/* everything is fine */
>   	case 0:
> -		carl9170_tx_callback(ar, (void *)urb->context);
> +		carl9170_tx_callback(ar, urb->context);
>   		break;
>   
>   	/* disconnect */
> @@ -369,7 +369,7 @@ void carl9170_usb_handle_tx_err(struct ar9170 *ar)
>   	struct urb *urb;
>   
>   	while ((urb = usb_get_from_anchor(&ar->tx_err))) {
> -		struct sk_buff *skb = (void *)urb->context;
> +		struct sk_buff *skb = urb->context;
>   
>   		carl9170_tx_drop(ar, skb);
>   		carl9170_tx_callback(ar, skb);
> @@ -397,7 +397,7 @@ static void carl9170_usb_tasklet(struct tasklet_struct *t)
>   
>   static void carl9170_usb_rx_complete(struct urb *urb)
>   {
> -	struct ar9170 *ar = (struct ar9170 *)urb->context;
> +	struct ar9170 *ar = urb->context;
>   	int err;
>   
>   	if (WARN_ON_ONCE(!ar))
> @@ -559,7 +559,7 @@ static int carl9170_usb_flush(struct ar9170 *ar)
>   	int ret, err = 0;
>   
>   	while ((urb = usb_get_from_anchor(&ar->tx_wait))) {
> -		struct sk_buff *skb = (void *)urb->context;
> +		struct sk_buff *skb = urb->context;
>   		carl9170_tx_drop(ar, skb);
>   		carl9170_tx_callback(ar, skb);
>   		usb_free_urb(urb);
> @@ -668,7 +668,7 @@ int carl9170_exec_cmd(struct ar9170 *ar, const enum carl9170_cmd_oids cmd,
>   		memcpy(ar->cmd.data, payload, plen);
>   
>   	spin_lock_bh(&ar->cmd_lock);
> -	ar->readbuf = (u8 *)out;
> +	ar->readbuf = out;
>   	ar->readlen = outlen;
>   	spin_unlock_bh(&ar->cmd_lock);
>   

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[yunchuan]

Hi, Christian

On 2023/9/21 03:00, Christian Lamparter wrote:

> On 9/19/23 06:49, Wu Yunchuan wrote:
>> No need cast (void *) to (struct ar9170 *), (u8 *) or (void*).
>
> hmm, your mail went into the spam folder. Good thing I checked.
>
Sometimes mail didn't work :(.
> From what I remember: The reason why these casts were added in
> carl9170 was because of compiler warnings/complaints.
> Current gcc compilers should be OK (given that the kernel-bot
> didn't react, or went your Mail to their spam-folder as well?)
> but have you checked these older versions?
>
My gcc version is 10.2.1. And this seems work when gcc version is 
5.4(ubuntu 16.04) or 4.84(Ubuntu 14.04）.
I can't  compile Linux under Ubuntu 16.04 because of some compatibility 
problems.

So I test a simple demo like this:


void *p_void = NULL;
long var_long = 5;
long *p_long = &var_long;
p_void = p_long;

This works fine.

> (In 6.5.0 Documentation/admin-guide/README.rst states that one
> should have at least gcc 5.1 - could you run with those and
> see if C=2 W=1 passes?)
Oh, I want to do this, but I can't compile or install gcc 5.1 in my 
computer.
There are some compatibility problems. I hope I can deal this problem 
next week.

Wu Yunchan

>
> Regards,
> Christian
>
>> Signed-off-by: Wu Yunchuan <yunchuan@nfschina.com>
>> ---
>>   drivers/net/wireless/ath/carl9170/usb.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/carl9170/usb.c 
>> b/drivers/net/wireless/ath/carl9170/usb.c
>> index e4eb666c6eea..c4edf8355941 100644
>> --- a/drivers/net/wireless/ath/carl9170/usb.c
>> +++ b/drivers/net/wireless/ath/carl9170/usb.c
>> @@ -178,7 +178,7 @@ static void carl9170_usb_tx_data_complete(struct 
>> urb *urb)
>>       switch (urb->status) {
>>       /* everything is fine */
>>       case 0:
>> -        carl9170_tx_callback(ar, (void *)urb->context);
>> +        carl9170_tx_callback(ar, urb->context);
>>           break;
>>         /* disconnect */
>> @@ -369,7 +369,7 @@ void carl9170_usb_handle_tx_err(struct ar9170 *ar)
>>       struct urb *urb;
>>         while ((urb = usb_get_from_anchor(&ar->tx_err))) {
>> -        struct sk_buff *skb = (void *)urb->context;
>> +        struct sk_buff *skb = urb->context;
>>             carl9170_tx_drop(ar, skb);
>>           carl9170_tx_callback(ar, skb);
>> @@ -397,7 +397,7 @@ static void carl9170_usb_tasklet(struct 
>> tasklet_struct *t)
>>     static void carl9170_usb_rx_complete(struct urb *urb)
>>   {
>> -    struct ar9170 *ar = (struct ar9170 *)urb->context;
>> +    struct ar9170 *ar = urb->context;
>>       int err;
>>         if (WARN_ON_ONCE(!ar))
>> @@ -559,7 +559,7 @@ static int carl9170_usb_flush(struct ar9170 *ar)
>>       int ret, err = 0;
>>         while ((urb = usb_get_from_anchor(&ar->tx_wait))) {
>> -        struct sk_buff *skb = (void *)urb->context;
>> +        struct sk_buff *skb = urb->context;
>>           carl9170_tx_drop(ar, skb);
>>           carl9170_tx_callback(ar, skb);
>>           usb_free_urb(urb);
>> @@ -668,7 +668,7 @@ int carl9170_exec_cmd(struct ar9170 *ar, const 
>> enum carl9170_cmd_oids cmd,
>>           memcpy(ar->cmd.data, payload, plen);
>>         spin_lock_bh(&ar->cmd_lock);
>> -    ar->readbuf = (u8 *)out;
>> +    ar->readbuf = out;
>>       ar->readlen = outlen;
>>       spin_unlock_bh(&ar->cmd_lock);
>

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Kalle Valo]

Christian Lamparter <chunkeey@gmail.com> writes:

> On 9/19/23 06:49, Wu Yunchuan wrote:
>> No need cast (void *) to (struct ar9170 *), (u8 *) or (void*).
>
> hmm, your mail went into the spam folder. Good thing I checked.
>
> From what I remember: The reason why these casts were added in
> carl9170 was because of compiler warnings/complaints.
> Current gcc compilers should be OK (given that the kernel-bot
> didn't react, or went your Mail to their spam-folder as well?)
> but have you checked these older versions?

Do you remember anything more about these warnings? I tried to check the
git history and at least quickly couldn't find anything related to this.

The changes look very safe to me, struct urb::context field and the out
variable are both of type 'void *' so removing the explicit casts should
change anything. I cannot really come up a reason why would this patch
cause new warnings so I am inclined towards taking this patch. What do
you think?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Jeff Johnson]

On 9/28/2023 8:31 AM, Kalle Valo wrote:
> Christian Lamparter <chunkeey@gmail.com> writes:
> 
>> On 9/19/23 06:49, Wu Yunchuan wrote:
>>> No need cast (void *) to (struct ar9170 *), (u8 *) or (void*).
>>
>> hmm, your mail went into the spam folder. Good thing I checked.
>>
>>  From what I remember: The reason why these casts were added in
>> carl9170 was because of compiler warnings/complaints.
>> Current gcc compilers should be OK (given that the kernel-bot
>> didn't react, or went your Mail to their spam-folder as well?)
>> but have you checked these older versions?
> 
> Do you remember anything more about these warnings? I tried to check the
> git history and at least quickly couldn't find anything related to this.
> 
> The changes look very safe to me, struct urb::context field and the out
> variable are both of type 'void *' so removing the explicit casts should
> change anything. I cannot really come up a reason why would this patch
> cause new warnings so I am inclined towards taking this patch. What do
> you think?

Anything that would have had issue would have predated C99.
This change is safe.

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Dan Carpenter]

I don't know anything which would warn about this.  Generally, in the
kernel we try to avoid casts but perhaps there was a static checker
which likes casts?

If removing these sorts of casts were an issue we would have known by
now.

regards,
dan carpenter

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Dan Carpenter]

On Fri, Sep 29, 2023 at 09:43:03AM +0300, Dan Carpenter wrote:
> I don't know anything which would warn about this.  Generally, in the
> kernel we try to avoid casts but perhaps there was a static checker
> which likes casts?
> 
> If removing these sorts of casts were an issue we would have known by
> now.

Thinking about it more, if this caused a static checker warning then
probably every kmalloc() would need a cast.

regards,
dan carpenter

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Christian Lamparter]

On 9/29/23 08:49, Dan Carpenter wrote:
> On Fri, Sep 29, 2023 at 09:43:03AM +0300, Dan Carpenter wrote:
>> I don't know anything which would warn about this.  Generally, in the
>> kernel we try to avoid casts but perhaps there was a static checker
>> which likes casts?
>>
>> If removing these sorts of casts were an issue we would have known by
>> now.
> 
> Thinking about it more, if this caused a static checker warning then
> probably every kmalloc() would need a cast.

Oh, we do have our fair share of static checker noise in:
drivers/net/wireless/ath/ (this is where carl9170 is located)!

I would like to take the chance to again point to this beauty:
<https://lore.kernel.org/linux-wireless/TYAP286MB03154F9AAFD4C35BEEDE4A99BC4CA@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM/T/#mf1b8919a000fe661803c17073f48b3c410888541>
@Dan, @Jeff can you please comment on that too?

As for this patch: If Wu ran some compiles with
what GCC version he has available and nothing turned up:

Acked-by: Christian Lamparter <chunkeey@gmail.com>

Regards,
Christian

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Dan Carpenter]

On Fri, Sep 29, 2023 at 09:23:26AM +0200, Christian Lamparter wrote:
> I would like to take the chance to again point to this beauty:
> <https://lore.kernel.org/linux-wireless/TYAP286MB03154F9AAFD4C35BEEDE4A99BC4CA@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM/T/#mf1b8919a000fe661803c17073f48b3c410888541>
> @Dan, @Jeff can you please comment on that too?

I don't know how Shiji Yang generated this warning.  The warning doesn't
make sense and I don't see how the patch helps.  I tested with GCC (v12)
and Clang (random from git) and neither one generates a warning.  What's
the point of having all the struct members in a group when struct itself
already forms a group?

#confused

regards,
dan carpenter

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Dan Carpenter]

On Fri, Sep 29, 2023 at 03:26:17PM +0300, Dan Carpenter wrote:
> On Fri, Sep 29, 2023 at 09:23:26AM +0200, Christian Lamparter wrote:
> > I would like to take the chance to again point to this beauty:
> > <https://lore.kernel.org/linux-wireless/TYAP286MB03154F9AAFD4C35BEEDE4A99BC4CA@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM/T/#mf1b8919a000fe661803c17073f48b3c410888541>
> > @Dan, @Jeff can you please comment on that too?
> 
> I don't know how Shiji Yang generated this warning.  The warning doesn't
> make sense and I don't see how the patch helps.  I tested with GCC (v12)
> and Clang (random from git) and neither one generates a warning.  What's
> the point of having all the struct members in a group when struct itself
> already forms a group?
> 
> #confused

Wait, all this was in the email thread from June but I didn't scroll
down beyond the end of the patch...  It was just a compiler bug in a
GCC dot release.

regards,
dan carpenter

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Jeff Johnson]

On 9/29/2023 12:23 AM, Christian Lamparter wrote:
> I would like to take the chance to again point to this beauty:
> <https://lore.kernel.org/linux-wireless/TYAP286MB03154F9AAFD4C35BEEDE4A99BC4CA@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM/T/#mf1b8919a000fe661803c17073f48b3c410888541>
> @Dan, @Jeff can you please comment on that too?

I had not seen that patch since it was posted while I was transitioning 
roles. It looks like a reasonable patch to me to handle FORTIFY_SOURCE 
restrictions.

Can it (any any other ath folder patches) be reposted for review?

/jeff

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Jeff Johnson]

On 9/29/2023 9:10 AM, Jeff Johnson wrote:
> On 9/29/2023 12:23 AM, Christian Lamparter wrote:
>> I would like to take the chance to again point to this beauty:
>> <https://lore.kernel.org/linux-wireless/TYAP286MB03154F9AAFD4C35BEEDE4A99BC4CA@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM/T/#mf1b8919a000fe661803c17073f48b3c410888541>
>> @Dan, @Jeff can you please comment on that too?
> 
> I had not seen that patch since it was posted while I was transitioning 
> roles. It looks like a reasonable patch to me to handle FORTIFY_SOURCE 
> restrictions.

Saw Dan's reply, and further looked at the patch and saw this wasn't 
actually a typical FORTIFY_SOURCE patch, so presumably this change is 
NOT needed.

Re: [PATCH v2 wireless-next 2/9] carl9170: remove unnecessary (void*) conversions

[Kalle Valo]

Wu Yunchuan <yunchuan@nfschina.com> wrote:

> No need cast (void *) to (struct ar9170 *), (u8 *) or (void*).
> 
> Signed-off-by: Wu Yunchuan <yunchuan@nfschina.com>
> Acked-by: Christian Lamparter <chunkeey@gmail.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

6c751f1a7bb8 wifi: carl9170: remove unnecessary (void*) conversions

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230919044916.523308-1-yunchuan@nfschina.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches