| Message ID | 20241023133004.2253830-2-kvalo@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jeff Johnson |
| Headers | show |
| Series | wifi: ath12k: MLO support part 2 | expand |
On 10/23/2024 6:29 AM, Kalle Valo wrote: > From: Kalle Valo <quic_kvalo@quicinc.com> > > In commit 477cabfdb776 ("wifi: ath12k: modify link arvif creation and removal > for MLO") I had accidentally left one personal TODO comment about using goto > instead of ret. Switch to use goto to be consistent with the error handling in > the function. > > Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 > > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > --- > drivers/net/wireless/ath/ath12k/mac.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c > index f5f96a8b1d61..f45f32f3b5f6 100644 > --- a/drivers/net/wireless/ath/ath12k/mac.c > +++ b/drivers/net/wireless/ath/ath12k/mac.c > @@ -7047,8 +7047,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) > ret = ath12k_wait_for_peer_delete_done(ar, arvif->vdev_id, > arvif->bssid); > if (ret) > - /* KVALO: why not goto err? */ > - return ret; > + goto err; why does this goto err instead of err_vdev_del? > > ar->num_peers--; > } looking at the context for this patch I have a question about a different part of this function: param_id = WMI_VDEV_PARAM_RTS_THRESHOLD; param_value = hw->wiphy->rts_threshold; ret = ath12k_wmi_vdev_set_param_cmd(ar, arvif->vdev_id, param_id, param_value); if (ret) { ath12k_warn(ar->ab, "failed to set rts threshold for vdev %d: %d\n", arvif->vdev_id, ret); NOTE: no return or goto } ath12k_dp_vdev_tx_attach(ar, arvif); if (vif->type != NL80211_IFTYPE_MONITOR && ar->monitor_conf_enabled) ath12k_mac_monitor_vdev_create(ar); return ret; NOTE: this can return an error if the RTS threshold set fails, but fails without cleaning up (dp vdev still attached and monitor vdev created) Seems either we need error handling if the set param fails, or we should ret 0 at this point
Jeff Johnson <quic_jjohnson@quicinc.com> writes: > On 10/23/2024 6:29 AM, Kalle Valo wrote: >> From: Kalle Valo <quic_kvalo@quicinc.com> >> >> In commit 477cabfdb776 ("wifi: ath12k: modify link arvif creation and removal >> for MLO") I had accidentally left one personal TODO comment about using goto >> instead of ret. Switch to use goto to be consistent with the error handling in >> the function. >> >> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 >> >> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> >> --- >> drivers/net/wireless/ath/ath12k/mac.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c >> index f5f96a8b1d61..f45f32f3b5f6 100644 >> --- a/drivers/net/wireless/ath/ath12k/mac.c >> +++ b/drivers/net/wireless/ath/ath12k/mac.c >> @@ -7047,8 +7047,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) >> ret = ath12k_wait_for_peer_delete_done(ar, arvif->vdev_id, >> arvif->bssid); >> if (ret) >> - /* KVALO: why not goto err? */ >> - return ret; >> + goto err; > > why does this goto err instead of err_vdev_del? Good point. I did this to follow the same as the next command does: ret = ath12k_wait_for_peer_delete_done(ar, arvif->vdev_id, arvif->bssid); if (ret) goto err; But yeah, err_vdev_del looks like more approriate. > >> >> ar->num_peers--; >> } > > looking at the context for this patch I have a question about a different part > of this function: > > param_id = WMI_VDEV_PARAM_RTS_THRESHOLD; > param_value = hw->wiphy->rts_threshold; > ret = ath12k_wmi_vdev_set_param_cmd(ar, arvif->vdev_id, > param_id, param_value); > if (ret) { > ath12k_warn(ar->ab, "failed to set rts threshold for vdev %d: %d\n", > arvif->vdev_id, ret); > > NOTE: no return or goto > > } > > ath12k_dp_vdev_tx_attach(ar, arvif); > if (vif->type != NL80211_IFTYPE_MONITOR && ar->monitor_conf_enabled) > ath12k_mac_monitor_vdev_create(ar); > > return ret; > > NOTE: this can return an error if the RTS threshold set fails, but fails > without cleaning up (dp vdev still attached and monitor vdev created) > > Seems either we need error handling if the set param fails, or we should ret 0 > at this point Yeah, I do not like this kind of vague error handling at all. I think we should have either a proper error handling or a comment explaining why we continue to the execution. An example about the comment: ret = foo(); if (ret) ath12k_warn("foo failed: %d", ret); /* continue function because foo is optional */ I think this should all this should be cleaned up in a separate patch.
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c index f5f96a8b1d61..f45f32f3b5f6 100644 --- a/drivers/net/wireless/ath/ath12k/mac.c +++ b/drivers/net/wireless/ath/ath12k/mac.c @@ -7047,8 +7047,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) ret = ath12k_wait_for_peer_delete_done(ar, arvif->vdev_id, arvif->bssid); if (ret) - /* KVALO: why not goto err? */ - return ret; + goto err; ar->num_peers--; }