| Message ID | 20241212004906.3087425-5-quic_periyasa@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: ath12k: Fix the static checker warning | expand |
Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: > Currently, the uninitialized variable 'ab' is accessed in the > ath12k_mac_allocate() function. Initialize 'ab' with the first radio device > present in the hardware abstraction handle (ah). Additionally, move the > default setting procedure from the pdev mapping iteration to the total > radio calculating iteration for better code readability. Perform the > maximum radio validation check for total_radio to ensure that both num_hw > and radio_per_hw are validated indirectly, as these variables are derived > from total_radio. This also fixes the below Smatch static checker warning. > > Smatch warning: > ath12k_mac_allocate() error: uninitialized symbol 'ab' > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1 > > Fixes: a343d97f27f5 ("wifi: ath12k: move struct ath12k_hw from per device to group") > Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> > --- > drivers/net/wireless/ath/ath12k/mac.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c > index 5cdc1c38b049..98b2f853d243 100644 > --- a/drivers/net/wireless/ath/ath12k/mac.c > +++ b/drivers/net/wireless/ath/ath12k/mac.c > @@ -10962,8 +10962,20 @@ int ath12k_mac_allocate(struct ath12k_hw_group *ag) > u8 radio_per_hw; > > total_radio = 0; > - for (i = 0; i < ag->num_devices; i++) > - total_radio += ag->ab[i]->num_radios; > + for (i = 0; i < ag->num_devices; i++) { > + ab = ag->ab[i]; > + if (!ab) > + continue; > + > + ath12k_mac_set_device_defaults(ab); > + total_radio += ab->num_radios; > + } > + > + if (!total_radio) > + return -EINVAL; 'total_radio == 0' is more readable as it's a counter. Also please add ath12k_warn() > + > + if (WARN_ON(total_radio > ATH12K_GROUP_MAX_RADIO)) > + return -ENOSPC; BTW ath12k_warn() is preferred over WARN_ON(), but this is just for the future as this WARN_ON() was already there before. > > /* All pdev get combined and register as single wiphy based on > * hardware group which participate in multi-link operation else > @@ -10976,14 +10988,16 @@ int ath12k_mac_allocate(struct ath12k_hw_group *ag) > > num_hw = total_radio / radio_per_hw; > > - if (WARN_ON(num_hw >= ATH12K_GROUP_MAX_RADIO)) > - return -ENOSPC; > - > ag->num_hw = 0; > device_id = 0; > mac_id = 0; > for (i = 0; i < num_hw; i++) { > for (j = 0; j < radio_per_hw; j++) { > + if (device_id >= ag->num_devices || !ag->ab[device_id]) { > + ret = -ENOSPC; > + goto err; > + } ath12k_warn()
On 12/12/2024 1:26 PM, Kalle Valo wrote: > Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: > >> Currently, the uninitialized variable 'ab' is accessed in the >> ath12k_mac_allocate() function. Initialize 'ab' with the first radio device >> present in the hardware abstraction handle (ah). Additionally, move the >> default setting procedure from the pdev mapping iteration to the total >> radio calculating iteration for better code readability. Perform the >> maximum radio validation check for total_radio to ensure that both num_hw >> and radio_per_hw are validated indirectly, as these variables are derived >> from total_radio. This also fixes the below Smatch static checker warning. >> >> Smatch warning: >> ath12k_mac_allocate() error: uninitialized symbol 'ab' >> >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1 >> >> Fixes: a343d97f27f5 ("wifi: ath12k: move struct ath12k_hw from per device to group") >> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> >> --- >> drivers/net/wireless/ath/ath12k/mac.c | 27 +++++++++++++++++++++------ >> 1 file changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c >> index 5cdc1c38b049..98b2f853d243 100644 >> --- a/drivers/net/wireless/ath/ath12k/mac.c >> +++ b/drivers/net/wireless/ath/ath12k/mac.c >> @@ -10962,8 +10962,20 @@ int ath12k_mac_allocate(struct ath12k_hw_group *ag) >> u8 radio_per_hw; >> >> total_radio = 0; >> - for (i = 0; i < ag->num_devices; i++) >> - total_radio += ag->ab[i]->num_radios; >> + for (i = 0; i < ag->num_devices; i++) { >> + ab = ag->ab[i]; >> + if (!ab) >> + continue; >> + >> + ath12k_mac_set_device_defaults(ab); >> + total_radio += ab->num_radios; >> + } >> + >> + if (!total_radio) >> + return -EINVAL; > > 'total_radio == 0' is more readable as it's a counter. Also please add ath12k_warn() > This condition came due to no device handle (ab) present in the ath12k_hw_group (ag). ath12k_warn() cannot be used since no device (ab) present here. Additionally, ath12k_hw_warn() also cannot be used here since ath12k_hw (ah) also not created. In future, we can introduce device (ab) and hardware (ah) agnostic warning helper function ath12k*dbg() for this usecase. >> + >> + if (WARN_ON(total_radio > ATH12K_GROUP_MAX_RADIO)) >> + return -ENOSPC; > > BTW ath12k_warn() is preferred over WARN_ON(), but this is just for the > future as this WARN_ON() was already there before. > Sure >> >> /* All pdev get combined and register as single wiphy based on >> * hardware group which participate in multi-link operation else >> @@ -10976,14 +10988,16 @@ int ath12k_mac_allocate(struct ath12k_hw_group *ag) >> >> num_hw = total_radio / radio_per_hw; >> >> - if (WARN_ON(num_hw >= ATH12K_GROUP_MAX_RADIO)) >> - return -ENOSPC; >> - >> ag->num_hw = 0; >> device_id = 0; >> mac_id = 0; >> for (i = 0; i < num_hw; i++) { >> for (j = 0; j < radio_per_hw; j++) { >> + if (device_id >= ag->num_devices || !ag->ab[device_id]) { >> + ret = -ENOSPC; >> + goto err; >> + } > > ath12k_warn() > same here.
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c index 5cdc1c38b049..98b2f853d243 100644 --- a/drivers/net/wireless/ath/ath12k/mac.c +++ b/drivers/net/wireless/ath/ath12k/mac.c @@ -10962,8 +10962,20 @@ int ath12k_mac_allocate(struct ath12k_hw_group *ag) u8 radio_per_hw; total_radio = 0; - for (i = 0; i < ag->num_devices; i++) - total_radio += ag->ab[i]->num_radios; + for (i = 0; i < ag->num_devices; i++) { + ab = ag->ab[i]; + if (!ab) + continue; + + ath12k_mac_set_device_defaults(ab); + total_radio += ab->num_radios; + } + + if (!total_radio) + return -EINVAL; + + if (WARN_ON(total_radio > ATH12K_GROUP_MAX_RADIO)) + return -ENOSPC; /* All pdev get combined and register as single wiphy based on * hardware group which participate in multi-link operation else @@ -10976,14 +10988,16 @@ int ath12k_mac_allocate(struct ath12k_hw_group *ag) num_hw = total_radio / radio_per_hw; - if (WARN_ON(num_hw >= ATH12K_GROUP_MAX_RADIO)) - return -ENOSPC; - ag->num_hw = 0; device_id = 0; mac_id = 0; for (i = 0; i < num_hw; i++) { for (j = 0; j < radio_per_hw; j++) { + if (device_id >= ag->num_devices || !ag->ab[device_id]) { + ret = -ENOSPC; + goto err; + } + ab = ag->ab[device_id]; pdev_map[j].ab = ab; pdev_map[j].pdev_idx = mac_id; @@ -10995,10 +11009,11 @@ int ath12k_mac_allocate(struct ath12k_hw_group *ag) if (mac_id >= ab->num_radios) { mac_id = 0; device_id++; - ath12k_mac_set_device_defaults(ab); } } + ab = pdev_map->ab; + ah = ath12k_mac_hw_allocate(ag, pdev_map, radio_per_hw); if (!ah) { ath12k_warn(ab, "failed to allocate mac80211 hw device for hw_idx %d\n",
Currently, the uninitialized variable 'ab' is accessed in the ath12k_mac_allocate() function. Initialize 'ab' with the first radio device present in the hardware abstraction handle (ah). Additionally, move the default setting procedure from the pdev mapping iteration to the total radio calculating iteration for better code readability. Perform the maximum radio validation check for total_radio to ensure that both num_hw and radio_per_hw are validated indirectly, as these variables are derived from total_radio. This also fixes the below Smatch static checker warning. Smatch warning: ath12k_mac_allocate() error: uninitialized symbol 'ab' Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1 Fixes: a343d97f27f5 ("wifi: ath12k: move struct ath12k_hw from per device to group") Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> --- drivers/net/wireless/ath/ath12k/mac.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)