| Message ID | 20250209-ath12k-uninit-v1-1-afc8005847be@ethancedwards.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jeff Johnson |
| Headers | show |
| Series | wifi: ath12k: Fix uninitialized variable and remove goto | expand |
On 2/9/2025 8:36 PM, Ethan Carter Edwards wrote: commit subject should be as specific as possible. suggest you at least mention the function > There is a possibility for an uninitialized *ret* variable to be > returned in some code paths. > > This moves *ret* inside the conditional and explicitly returns 0 without > an error. Also removes goto that returned *ret*. ath driver convention is to declare function variables at the beginning of the function -- please do not relocate the definition of 'ret' > > Addresses-Coverity-ID: 1642337 ("Uninitialized scalar variable") Is that an official kernel tag? IMO the proper tag would be Closes: https://scan5.scan.coverity.com/#/project-view/63541/10063?selectedIssue=1642337 (is there a shorter URL?) see https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > Fixes: b716a10d99a28 ("wifi: ath12k: enable MLO setup and teardown from core") > Signed-off-by: Ethan Carter Edwards <ethan@ethancedwards.com> > --- > drivers/net/wireless/ath/ath12k/core.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c > index 0606116d6b9c491b6ede401b2e1aedfb619339a8..ae75cdad3bd6b6eb80a35fee94c18d365d123cbd 100644 > --- a/drivers/net/wireless/ath/ath12k/core.c > +++ b/drivers/net/wireless/ath/ath12k/core.c > @@ -908,7 +908,6 @@ int ath12k_mac_mlo_ready(struct ath12k_hw_group *ag) > { > struct ath12k_hw *ah; > struct ath12k *ar; > - int ret; > int i, j; > > for (i = 0; i < ag->num_hw; i++) { > @@ -918,14 +917,13 @@ int ath12k_mac_mlo_ready(struct ath12k_hw_group *ag) > > for_each_ar(ah, ar, j) { > ar = &ah->radio[j]; > - ret = __ath12k_mac_mlo_ready(ar); > + int ret = __ath12k_mac_mlo_ready(ar); > if (ret) > - goto out; > + return ret; > } > } > > -out: > - return ret; > + return 0; > } > > static int ath12k_core_mlo_setup(struct ath12k_hw_group *ag) > > --- > base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3 > change-id: 20250209-ath12k-uninit-18560fd91c07 > > Best regards, replacing goto out with return ret and unconditional return 0 LGTM. can you respin a v2 with the other comments addressed? /jeff
On 25/02/10 05:08PM, Jeff Johnson wrote: > On 2/9/2025 8:36 PM, Ethan Carter Edwards wrote: > > commit subject should be as specific as possible. > suggest you at least mention the function Will change. Thanks! > > > There is a possibility for an uninitialized *ret* variable to be > > returned in some code paths. > > > > This moves *ret* inside the conditional and explicitly returns 0 without > > an error. Also removes goto that returned *ret*. > > ath driver convention is to declare function variables at the beginning of the > function -- please do not relocate the definition of 'ret' Will fix. > > > > > Addresses-Coverity-ID: 1642337 ("Uninitialized scalar variable") > > Is that an official kernel tag? IMO the proper tag would be So, it isn't "official" as far as I can tell, but it is widely used in other commits, especially by Gustavo Silva. Also: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=778e2478d19574508861bcb1806d6e34d095587c Coverity-IDs: is another option I have found. I have seen Closes: a few times as well. I'm not really sure what the best option is, honestly. I'll use Closes: in v2. LMK if you want me to use something else. > > Closes: https://scan5.scan.coverity.com/#/project-view/63541/10063?selectedIssue=1642337 > > (is there a shorter URL?) Not that I know of. > > see https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > > > Fixes: b716a10d99a28 ("wifi: ath12k: enable MLO setup and teardown from core") > > Signed-off-by: Ethan Carter Edwards <ethan@ethancedwards.com> > > --- > > drivers/net/wireless/ath/ath12k/core.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c > > index 0606116d6b9c491b6ede401b2e1aedfb619339a8..ae75cdad3bd6b6eb80a35fee94c18d365d123cbd 100644 > > --- a/drivers/net/wireless/ath/ath12k/core.c > > +++ b/drivers/net/wireless/ath/ath12k/core.c > > @@ -908,7 +908,6 @@ int ath12k_mac_mlo_ready(struct ath12k_hw_group *ag) > > { > > struct ath12k_hw *ah; > > struct ath12k *ar; > > - int ret; > > int i, j; > > > > for (i = 0; i < ag->num_hw; i++) { > > @@ -918,14 +917,13 @@ int ath12k_mac_mlo_ready(struct ath12k_hw_group *ag) > > > > for_each_ar(ah, ar, j) { > > ar = &ah->radio[j]; > > - ret = __ath12k_mac_mlo_ready(ar); > > + int ret = __ath12k_mac_mlo_ready(ar); > > if (ret) > > - goto out; > > + return ret; > > } > > } > > > > -out: > > - return ret; > > + return 0; > > } > > > > static int ath12k_core_mlo_setup(struct ath12k_hw_group *ag) > > > > --- > > base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3 > > change-id: 20250209-ath12k-uninit-18560fd91c07 > > > > Best regards, > > replacing goto out with return ret and unconditional return 0 LGTM. > > can you respin a v2 with the other comments addressed? Will do! Thank ya much. > > /jeff
> > > > > > Addresses-Coverity-ID: 1642337 ("Uninitialized scalar variable") > > > > Is that an official kernel tag? IMO the proper tag would be > So, it isn't "official" as far as I can tell, but it is widely used in > other commits, especially by Gustavo Silva. > > Also: > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=778e2478d19574508861bcb > 1806d6e34d095587c > > Coverity-IDs: is another option I have found. I have seen Closes: a few > times as well. I'm not really sure what the best option is, honestly. In my patch, I used and treated Addresses-Coverity-ID as a unofficial tag, so additional empty line is added. Days ago I have received Coverity issues sent to mailing list, so I used Closes tag at that time. But recently I have not seen that kind of mails. Instead, I visit Coverity web site to check issues and use Addresses-Coverity-ID tag, since Coverity link is not visible to everyone. That is just my thought.
On 2/10/2025 8:09 PM, Ping-Ke Shih wrote: >>>> >>>> Addresses-Coverity-ID: 1642337 ("Uninitialized scalar variable") >>> >>> Is that an official kernel tag? IMO the proper tag would be >> So, it isn't "official" as far as I can tell, but it is widely used in >> other commits, especially by Gustavo Silva. >> >> Also: >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=778e2478d19574508861bcb >> 1806d6e34d095587c >> >> Coverity-IDs: is another option I have found. I have seen Closes: a few >> times as well. I'm not really sure what the best option is, honestly. > > In my patch, I used and treated Addresses-Coverity-ID as a unofficial tag, > so additional empty line is added. > > Days ago I have received Coverity issues sent to mailing list, so I used > Closes tag at that time. But recently I have not seen that kind of mails. > Instead, I visit Coverity web site to check issues and use > Addresses-Coverity-ID tag, since Coverity link is not visible to everyone. > That is just my thought. The problem I have is that I get Coverity fixes both from the linux and the linux-next projects: https://scan.coverity.com/projects/linux?tab=overview https://scan.coverity.com/projects/linux-next-weekly-scan?tab=overview The Coverity IDs from these projects are allocated independently, so a Coverity ID does not uniquely identify an issue. The URL uniquely identifies an issue, and also utilizes an official tag. That is my thought. /jeff
Jeff Johnson <jeff.johnson@oss.qualcomm.com> wrote: > On 2/10/2025 8:09 PM, Ping-Ke Shih wrote: > >>>> > >>>> Addresses-Coverity-ID: 1642337 ("Uninitialized scalar variable") > >>> > >>> Is that an official kernel tag? IMO the proper tag would be > >> So, it isn't "official" as far as I can tell, but it is widely used in > >> other commits, especially by Gustavo Silva. > >> > >> Also: > >> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=778e2478d19574508861bcb > >> 1806d6e34d095587c > >> > >> Coverity-IDs: is another option I have found. I have seen Closes: a few > >> times as well. I'm not really sure what the best option is, honestly. > > > > In my patch, I used and treated Addresses-Coverity-ID as a unofficial tag, > > so additional empty line is added. > > > > Days ago I have received Coverity issues sent to mailing list, so I used > > Closes tag at that time. But recently I have not seen that kind of mails. > > Instead, I visit Coverity web site to check issues and use > > Addresses-Coverity-ID tag, since Coverity link is not visible to everyone. > > That is just my thought. > > The problem I have is that I get Coverity fixes both from the linux and the > linux-next projects: > > https://scan.coverity.com/projects/linux?tab=overview > https://scan.coverity.com/projects/linux-next-weekly-scan?tab=overview > > The Coverity IDs from these projects are allocated independently, so a > Coverity ID does not uniquely identify an issue. > > The URL uniquely identifies an issue, and also utilizes an official tag. > > That is my thought. Yes, I have the same problem as yours. For me, I only annotate Coverity IDs from linux project, and the linux-next project is as a reference to check if issues are still existing in -next tree.
diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c index 0606116d6b9c491b6ede401b2e1aedfb619339a8..ae75cdad3bd6b6eb80a35fee94c18d365d123cbd 100644 --- a/drivers/net/wireless/ath/ath12k/core.c +++ b/drivers/net/wireless/ath/ath12k/core.c @@ -908,7 +908,6 @@ int ath12k_mac_mlo_ready(struct ath12k_hw_group *ag) { struct ath12k_hw *ah; struct ath12k *ar; - int ret; int i, j; for (i = 0; i < ag->num_hw; i++) { @@ -918,14 +917,13 @@ int ath12k_mac_mlo_ready(struct ath12k_hw_group *ag) for_each_ar(ah, ar, j) { ar = &ah->radio[j]; - ret = __ath12k_mac_mlo_ready(ar); + int ret = __ath12k_mac_mlo_ready(ar); if (ret) - goto out; + return ret; } } -out: - return ret; + return 0; } static int ath12k_core_mlo_setup(struct ath12k_hw_group *ag)
There is a possibility for an uninitialized *ret* variable to be returned in some code paths. This moves *ret* inside the conditional and explicitly returns 0 without an error. Also removes goto that returned *ret*. Addresses-Coverity-ID: 1642337 ("Uninitialized scalar variable") Fixes: b716a10d99a28 ("wifi: ath12k: enable MLO setup and teardown from core") Signed-off-by: Ethan Carter Edwards <ethan@ethancedwards.com> --- drivers/net/wireless/ath/ath12k/core.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) --- base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3 change-id: 20250209-ath12k-uninit-18560fd91c07 Best regards,