Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection

Hi,

On Tue, Nov 24, 2020 at 1:19 AM Rakesh Pillai [off-list ref] wrote:

quoted

-----Original Message-----
From: Doug Anderson <dianders@chromium.org>
Sent: Tuesday, November 24, 2020 6:27 AM
To: Abhishek Kumar <redacted>
Cc: Kalle Valo <redacted>; Rakesh Pillai
[off-list ref]; LKML [off-list ref]; ath10k
[off-list ref]; Brian Norris [off-list ref];
linux-wireless [off-list ref]; David S. Miller
[off-list ref]; Jakub Kicinski [off-list ref]; netdev
[off-list ref]
Subject: Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF
selection

Hi,

On Thu, Nov 12, 2020 at 12:09 PM Abhishek Kumar [off-list ref]
wrote:

quoted

In some devices difference in chip-id should be enough to pick
the right BDF. Add another support for chip-id based BDF selection.
With this new option, ath10k supports 2 fallback options.

The board name with chip-id as option looks as follows
board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'

Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1
Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1
Signed-off-by: Abhishek Kumar <redacted>
---

(no changes since v1)

I think you need to work on the method you're using to generate your
patches.  There are most definitely changes since v1.  You described
them in your cover letter (which you don't really need for a singleton
patch) instead of here.

quoted

@@ -1438,12 +1439,17 @@ static int

ath10k_core_create_board_name(struct ath10k *ar, char *name,

quoted

        }

        if (ar->id.qmi_ids_valid) {
-               if (with_variant && ar->id.bdf_ext[0] != '\0')
+               if (with_additional_params && ar->id.bdf_ext[0] != '\0')
                        scnprintf(name, name_len,
                                  "bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
                                  ath10k_bus_str(ar->hif.bus),
                                  ar->id.qmi_board_id, ar->id.qmi_chip_id,
                                  variant);
+               else if (with_additional_params)
+                       scnprintf(name, name_len,
+                                 "bus=%s,qmi-board-id=%x,qmi-chip-id=%x",
+                                 ath10k_bus_str(ar->hif.bus),
+                                 ar->id.qmi_board_id, ar->id.qmi_chip_id);

I believe this is exactly opposite of what Rakesh was requesting.
Specifically, he was trying to eliminate the extra scnprintf() but I
think he still agreed that it was a good idea to generate 3 different
strings.  I believe the proper diff to apply to v1 is:

https://crrev.com/c/255643

Wow, I seem to have deleted the last digit from my URL.  Should have been:

https://crrev.com/c/2556437

quoted

-Doug

Hi Abhishek/Doug,

I missed on reviewing this change. Also I agree with Doug that this is not the change I was looking for.

The argument "with_variant" can be renamed to "with_extra_params". There is no need for any new argument to this function.
Case 1: with_extra_params=0,  ar->id.bdf_ext[0] = 0             ->   The default name will be used (bus=snoc,qmi_board_id=0xab)
Case 2: with_extra_params=1,  ar->id.bdf_ext[0] = 0             ->   bus=snoc,qmi_board_id=0xab,qmi_chip_id=0xcd
Case 3: with_extra_params=1,  ar->id.bdf_ext[0] = "xyz"      ->   bus=snoc,qmi_board_id=0xab,qmi_chip_id=0xcd,variant=xyz

ar->id.bdf_ext[0] depends on the DT entry for variant field.

I'm confused about your suggestion.  Maybe you can help clarify.  Are
you suggesting:

a) Only two calls to ath10k_core_create_board_name()

I'm pretty sure this will fail in some cases.  Specifically consider
the case where the device tree has a "variant" defined but the BRD
file only has one entry for (board-id) and one for (board-id +
chip-id) but no entry for (board-id + chip-id + variant).  If you are
only making two calls then I don't think you'll pick the right one.

Said another way...

If the device tree has a variant:
1. We should prefer a BRD entry that has board-id + chip-id + variant
2. If #1 isn't there, we should prefer a BRD entry that has board-id + chip-id
3. If #1 and #2 aren't there we fall back to a BRD entry that has board-id.

...without 3 calls to ath10k_core_create_board_name() we can't handle
all 3 cases.


b) Three calls to ath10k_core_create_board_name() but the caller
manually whacks "ar->id.bdf_ext[0]" for one of the calls

This doesn't look like it's a clean solution, but maybe I'm missing something.


-Doug