| Message ID | 20230919044916.523308-1-yunchuan@nfschina.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6c751f1a7bb864bcb93b2148a09d476706427144 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2,wireless-next,1/9] wifi: ar5523: Remove unnecessary (void*) conversions | expand |
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>
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); >
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); >
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?
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.
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
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
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
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
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
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
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.
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
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);
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(-)