by Raj Kumar Bhagat

[permalink] [raw]

Subject: Re: [PATCH 02/13] wifi: ath12k: indicate NON MBSSID vdev by default during vdev start

On 10/31/2023 4:42 AM, Jeff Johnson wrote:
> On 10/30/2023 3:26 PM, Raj Kumar Bhagat wrote:
>> From: Sriram R <[email protected]>
>>
>> When any VDEV is started, MBSSID flags are passed to firmware to
>> indicate if its a MBSSID/EMA AP vdev. If the interface is not an AP
>> or if the AP doesn't support MBSSID, the vdev needs to be brought up
>> as a non MBSSID vdev. Set these flags as a non MBSSID AP by default
>> which can be updated as and when MBSSID support is added in ath12k.
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Sriram R <[email protected]>
>> Signed-off-by: Raj Kumar Bhagat <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath12k/mac.c | 5 +++++
>> drivers/net/wireless/ath/ath12k/wmi.c | 1 +
>> drivers/net/wireless/ath/ath12k/wmi.h | 8 ++++++++
>> 3 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>> index fc0d14ea3..594aa18e7 100644
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -5987,6 +5987,11 @@ ath12k_mac_vdev_start_restart(struct ath12k_vif *arvif,
>>       arg.pref_tx_streams = ar->num_tx_chains;
>>       arg.pref_rx_streams = ar->num_rx_chains;
>> +    /* Fill the MBSSID flags to indicate AP is non MBSSID by default
>> +     * Corresponding flags would be updated with MBSSID support.
>> +     */
>> +    arg.mbssid_flags = WMI_VDEV_FLAGS_NON_MBSSID_AP;
>> +
>>       if (arvif->vdev_type == WMI_VDEV_TYPE_AP) {
>>           arg.ssid = arvif->u.ap.ssid;
>>           arg.ssid_len = arvif->u.ap.ssid_len;
>> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
>> index 0e5bf5ce8..88ec77dee 100644
>> --- a/drivers/net/wireless/ath/ath12k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath12k/wmi.c
>> @@ -1024,6 +1024,7 @@ int ath12k_wmi_vdev_start(struct ath12k *ar, struct wmi_vdev_start_req_arg *arg,
>>       cmd->regdomain = cpu_to_le32(arg->regdomain);
>>       cmd->he_ops = cpu_to_le32(arg->he_ops);
>>       cmd->punct_bitmap = cpu_to_le32(arg->punct_bitmap);
>> +    cmd->mbssid_flags = cpu_to_le32(arg->mbssid_flags);
>>       if (!restart) {
>>           if (arg->ssid) {
>> diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
>> index 8e1eda7aa..dfe9eb0cb 100644
>> --- a/drivers/net/wireless/ath/ath12k/wmi.h
>> +++ b/drivers/net/wireless/ath/ath12k/wmi.h
>> @@ -2269,6 +2269,14 @@ struct ath12k_wmi_hal_reg_capabilities_ext_arg {
>>       u32 high_5ghz_chan;
>> };
>> +enum {
>> +    WMI_VDEV_FLAGS_NON_MBSSID_AP       = BIT(0),
>
> only the 1st one is used. do we need the rest at this point?
> perhaps add the others as they are needed?

Sure, will add only one - WMI_VDEV_FLAGS_NON_MBSSID_AP.
>
>> +    WMI_VDEV_FLAGS_TRANSMIT_AP         = BIT(1),
>> +    WMI_VDEV_FLAGS_NON_TRANSMIT_AP     = BIT(2),
>> +    WMI_VDEV_FLAGS_EMA_MODE            = BIT(3),
>> +    WMI_VDEV_FLAGS_SCAN_MODE_VAP       = BIT(4),
>
> note that "vap" is a non-standard term that ideally should not be used in ath12k (although a few references are present)
>
>> +};
>
> these seem to be added at some random place in the file. considering that these are applicable to the mbssid_flags in struct wmi_vdev_start_request_cmd, I'd suggest:
>
> 1) defining the enum just before this struct
> 2) naming the enum, i.e. wmi_vdev_mbssid_flags
> 3) using a prefix that matches the name, i.e. WMI_VDEV_MBSSID_FLAGS_NON_MBSSID_AP
> 4) in struct wmi_vdev_start_request_cmd add a comment to:
>     __le32 mbssid_flags; /* uses enum wmi_vdev_mbssid_flags */
>
> /jeff

Will implement the above suggestions in next version.

2024-01-05 18:42:26

by Raj Kumar Bhagat

[permalink] [raw]

Subject: Re: [PATCH 10/13] wifi: ath12k: subscribe required word mask from rx tlv

On 10/31/2023 6:23 AM, Jeff Johnson wrote:
> On 10/30/2023 3:26 PM, Raj Kumar Bhagat wrote:
>> From: Karthikeyan Kathirvel <[email protected]>
>>
>> Most of the RX descriptors fields are currently not used in the
>> ath12k driver. Hence add support to selectively subscribe to the
>> required quad words (64 bits) within msdu_end and mpdu_start of
>> rx_desc.
>>
>> Add compact rx_desc structures and configure the bit mask for Rx TLVs
>> (msdu_end, mpdu_start, mpdu_end) via registers. With these registers
>> SW can configure to DMA the partial TLV struct to Rx buffer.
>>
>> Each TLV type has its own register to configure the mask value.
>> The mask value configured in register will indicate if a particular
>> QWORD has to be written to rx buffer or not i.e., if Nth bit is enabled
>> in the mask Nth QWORD will be written and it will not be written if the
>> bit is disabled in mask. While 0th bit indicates whether TLV tag will be
>> written or not.
>>
>> Advantages of Qword subscription of TLVs
>> - Avoid multiple cache-line misses as the all the required fields
>> of the TLV are within 128 bytes.
>> - Memory optimization as TLVs + DATA + SHINFO can fit in 2k buffer
>> even for 64 bit kernel.
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Karthikeyan Kathirvel <[email protected]>
>> Co-developed-by: Raj Kumar Bhagat <[email protected]>
>> Signed-off-by: Raj Kumar Bhagat <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath12k/dp.c      |   9 +
>> drivers/net/wireless/ath/ath12k/dp.h      | 13 +
>> drivers/net/wireless/ath/ath12k/dp_rx.c   | 16 +-
>> drivers/net/wireless/ath/ath12k/dp_tx.c   | 20 ++
>> drivers/net/wireless/ath/ath12k/hal.c     | 352 ++++++++++++++++++++++
>> drivers/net/wireless/ath/ath12k/hal.h     |   3 +
>> drivers/net/wireless/ath/ath12k/rx_desc.h | 112 ++++++-
>> drivers/net/wireless/ath/ath12k/wmi.h     |   2 +
>> 8 files changed, 519 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/dp.c b/drivers/net/wireless/ath/ath12k/dp.c
>> index 80d7ce44d..faeef965e 100644
>> --- a/drivers/net/wireless/ath/ath12k/dp.c
>> +++ b/drivers/net/wireless/ath/ath12k/dp.c
>> @@ -1001,6 +1001,15 @@ void ath12k_dp_pdev_pre_alloc(struct ath12k_base *ab)
>> void ath12k_dp_hal_rx_desc_init(struct ath12k_base *ab)
>> {
>> +    if (test_bit(WMI_TLV_SERVICE_WMSK_COMPACTION_RX_TLVS, ab->wmi_ab.svc_map) &&
>> +        ab->hw_params->hal_ops->rxdma_ring_wmask_rx_mpdu_start &&
>> +        ab->hw_params->hal_ops->rxdma_ring_wmask_rx_msdu_end) {
>> +        /* RX TLVS compaction is supported, hence change the hal_rx_ops
>> +         * based on device.
>> +         */
>> +        if (ab->hal_rx_ops == &hal_rx_qcn9274_ops)
>> +            ab->hal_rx_ops = &hal_rx_qcn9274_compact_ops;
>
> I only have one comment on this patch.
>
> in order to avoid chipset-specific logic here suggest that there should be an abstraction.
>
> several ideas come to mind:
> 1) have a hal_ops callback to retrieve it
> 2) have a pointer to the compact ops in the hw_params
>
> since we are already using hal_ops to get the masks, suggest that is where we should get the pointer to the compact ops
>
Thanks Jeff for the suggestion, will implement hal_ops to retrieve
the corresponding compact ops.

>> +    }
>>       ab->hal.hal_desc_sz =
>>           ab->hal_rx_ops->rx_desc_get_desc_size();
>> }
>