Harshitha Prem <quic_hprem@xxxxxxxxxxx> writes:
> From: Karthikeyan Periyasamy <quic_periyasa@xxxxxxxxxxx>
>
> 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?
> 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@xxxxxxxxxxx>
> Co-developed-by: Harshitha Prem <quic_hprem@xxxxxxxxxxx>
> Signed-off-by: Harshitha Prem <quic_hprem@xxxxxxxxxxx>
> Acked-by: Jeff Johnson <quic_jjohnson@xxxxxxxxxxx>
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.
BTW I will from now on require proper analysis also for additions to
enum ath12k_dev_flags.
> --- 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.
> +/* 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?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
