[PATCH v8 6/8] wifi: ath12k: Introduce device group abstraction
Harshitha Prem
quic_hprem at quicinc.com
Fri Jun 7 06:29:09 PDT 2024
On 6/6/2024 6:50 PM, Kalle Valo wrote:
> Harshitha Prem <quic_hprem at quicinc.com> writes:
>
>> From: Karthikeyan Periyasamy <quic_periyasa at quicinc.com>
>>
>> Currently, single device is probed and once firmware is ready, the device
>> is registered to mac80211. For multi-link operation, different bands of
>> different devices or same device would be part of a single wiphy and for
>> this, hardware device group abstraction would be helpful.
>>
>> Hardware device group abstraction - when there are multiple devices (with
>> single radio or dual radio) that are connected by any means of interface
>> for communicating between them, then these devices can be combined
>> together as a single group using a group id to form a group abstraction
>> and register to mac80211.
>>
>> The grouping information of multiple devices would be based on device tree
>> during device probe. If no such information is available then a single
>> device will be part of group abstraction and registered to mac80211 else
>> multiple devices advertised in device tree are combined and then registered
>> to mac80211.
>>
>> For device group abstraction, a base structure named ath12k_hw_group (ag)
>> and the following helpers are introduced:
>> ath12k_core_hw_group_alloc() : allocate ath12k_hw_group (ag)
>> based on group id and number
>> of devices that are going to
>> be part of this group.
>> ath12k_core_hw_group_free() : free ag during deinit.
>> ath12k_core_assign_hw_group() : assign/map the details of group
>> to ath12k_base (ab).
>> ath12k_core_unassign_hw_group() : unassign/unmap the details of ag
>> in ath12k_base (ab).
>> ath12k_core_hw_group_create() : create the devices which are part
>> of group (ag).
>> ath12k_core_hw_group_destroy() : cleanup the devices in ag
>>
>> These helpers are used during device probe and mapping the group to the
>> devices involved.
>>
>> Please find the illustration of how multiple devices might be combined
>> together in future based on group id.
>>
>> Grouping of multiple devices (in future)
>>
>> +------------------------------------------------------------------------+
>> | +-------------------------------------+ +-------------------+ |
>> | | +-----------+ | | +-----------+ | | +-----------+ | |
>> | | | ar (2GHz) | | | | ar (5GHz) | | | | ar (6GHz) | | |
>> | | +-----------+ | | +-----------+ | | +-----------+ | |
>> | | ath12k_base (ab) | | ath12k_base (ab) | |
>> | | (Dual band device) | | | |
>> | +-------------------------------------+ +-------------------+ |
>> | ath12k_hw_group (ag) based on group id |
>> +------------------------------------------------------------------------+
>
> This is a good diagram, thanks for that. But how does struct ath12k_hw
> fit into the diagram?
>
Currently with this base device group series, ath12k_hw sits above
ath12k_base.
Please find the representation below:
+-----------------------------------------------+
| +-------------------+ +-------------------+ |
| | +---------+ | | +---------+ | |
| | | | | | | | | |
| | | mac80211| | | | mac80211| | |
| | | hw->priv| | | | hw->priv| | |
| | | | | | | | | |
| | +---------+ | | +---------+ | |
| | +---------+ | | +---------+ | |
| | | | | | | | | |
| | |ath12k_hw| | | |ath12k_hw| | |
| | | (ah) | | | | (ah) | | |
| | | | | | | | | |
| | +---------+ | | +---------+ | |
| | ^ | | ^ | |
| | +------|--------+ | | +------|--------+ | |
| | | | | | | | | | | |
| | | +----------+ | | | | +----------+ | | |
| | | | ar | | | | | | ar | | | |
| | | | 5 GHz | | | | | | 6 GHz | | | |
| | | | | | | | | | | | | |
| | | +----------+ | | | | +----------+ | | |
| | | ath12k_base | | | | ath12k_base | | |
| | | (ab) | | | | (ab) | | |
| | +---------------+ | | +---------------+ | |
| | ath12k_hw_group | | ath12k_hw_group | |
| +-------------------+ +-------------------+ |
+-----------------------------------------------+
As multiple device grouping is not yet brought in with this patch, this
is how it might look like.
>> In the above representation, two devices are combined into single group
>> based on group id.
>>
>> Add base code changes where single device would be part of a group with an
>> invalid group id forming an group abstraction. Multi device grouping will
>> be introduced in future.
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>> Tested-on: WCN7850 hw2.0 PCI
>> WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4
>>
>> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa at quicinc.com>
>> Co-developed-by: Harshitha Prem <quic_hprem at quicinc.com>
>> Signed-off-by: Harshitha Prem <quic_hprem at quicinc.com>
>> Acked-by: Jeff Johnson <quic_jjohnson at quicinc.com>
>
> Like I have said before, for adding any new locks there needs to be a
> proper analysis for the locking and good justifications why new locks
> are needed. I don't see any of that above.
>
Sure, Kalle. Will add the details for new locks. As grouping has to be
done in synchronous manner lock was introduced.
> BTW I will from now on require proper analysis also for additions to
> enum ath12k_dev_flags.
Sure, Will update the details.
There are few racy conditions with mutiple device grouping which needed
these flags.
One such instance is, if a reboot is triggered between core_init and
core_start, some of the devices in ath12k_hw_group would have already
started and completed till FW ready but some would not even reached QMI
firmware ready event. But, during reboot as part of pci shutdown group
clean up would try to do core stop for all devices in group. This will
lead to crashes as for some devices core start is yet to do but we tried
to invoked core stop.
>
>> --- a/drivers/net/wireless/ath/ath12k/core.c
>> +++ b/drivers/net/wireless/ath/ath12k/core.c
>> @@ -21,6 +21,9 @@ unsigned int ath12k_debug_mask;
>> module_param_named(debug_mask, ath12k_debug_mask, uint, 0644);
>> MODULE_PARM_DESC(debug_mask, "Debugging mask");
>>
>> +static DEFINE_MUTEX(ath12k_hw_lock);
>> +static struct list_head ath12k_hw_groups = LIST_HEAD_INIT(ath12k_hw_groups);
>
> I can somehow understand/guess why this mutex is needed (even though
> there's no documentation) but the naming is not really clear as we
> already have struct ath12k_hw::hw_mutex.
>
This lock is for device group list handling. Will modify the name
accordingly and update the documentation. Thanks.
>> +/* Holds info on the group of devices that are registered as a single wiphy */
>> +struct ath12k_hw_group {
>> + struct list_head list;
>> + u8 id;
>> + u8 num_devices;
>> + u8 num_probed;
>> + struct ath12k_base *ab[ATH12K_MAX_SOCS];
>> + /* To synchronize group create, assign, start, stop */
>> + struct mutex mutex_lock;
>> +};
>
> But why we really need this mutex? And does it really justify the extra
> complexity it creates?
>
This lock is necessary because for some QMI handling and WMI MLO
handling should happen only after all the devices in group are probed
and devices are started. In those WMI & QMI MLO handshake, it requires
the partner details.
QMI firmware ready can come from any device in a group and may not be
synchronous but the group has to be registered only after all the
devices have received QMI firmware ready. Similarly, core stop, firmware
recovery scenarios. To handle this the ag lock is needed. Will update
the details.
Thanks,
Harshitha
More information about the ath12k
mailing list