[PATCH v2 0/1] This patch address comments on patch v1

[PATCH v2 0/1] This patch address comments on patch v1

[Abhishek Kumar]

This patch extends ath10k_core_create_board_name function to support chip id
based BDF selection and not add provision for fallback_boardname2, thus
introducing lesser lines of code.

(no changes since v1)

Abhishek Kumar (1):
  ath10k: add option for chip-id based BDF selection

 drivers/net/wireless/ath/ath10k/core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

-- 
2.29.2.222.g5d2a92d10f8-goog

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

[Abhishek Kumar]

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 <kuabhs@chromium.org>
---

(no changes since v1)

 drivers/net/wireless/ath/ath10k/core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index d73ad60b571c..fa9e676b26d9 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1419,12 +1419,13 @@ static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
 }
 
 static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-					 size_t name_len, bool with_variant)
+					 size_t name_len,
+					 bool with_additional_params)
 {
 	/* strlen(',variant=') + strlen(ar->id.bdf_ext) */
 	char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };
 
-	if (with_variant && ar->id.bdf_ext[0] != '\0')
+	if (with_additional_params && ar->id.bdf_ext[0] != '\0')
 		scnprintf(variant, sizeof(variant), ",variant=%s",
 			  ar->id.bdf_ext);
 
@@ -1438,12 +1439,17 @@ static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
 	}
 
 	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);
 		else
 			scnprintf(name, name_len,
 				  "bus=%s,qmi-board-id=%x",
-- 
2.29.2.222.g5d2a92d10f8-goog

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

[Doug Anderson]

Hi,

On Thu, Nov 12, 2020 at 12:09 PM Abhishek Kumar <kuabhs@chromium.org> wrote:
>
> 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 <kuabhs@chromium.org>
> ---
>
> (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.


> @@ -1438,12 +1439,17 @@ static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
>         }
>
>         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

-Doug

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

[Rakesh Pillai]

> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Tuesday, November 24, 2020 6:27 AM
> To: Abhishek Kumar <kuabhs@chromium.org>
> Cc: Kalle Valo <kvalo@codeaurora.org>; Rakesh Pillai
> <pillair@codeaurora.org>; LKML <linux-kernel@vger.kernel.org>; ath10k
> <ath10k@lists.infradead.org>; Brian Norris <briannorris@chromium.org>;
> linux-wireless <linux-wireless@vger.kernel.org>; David S. Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; netdev
> <netdev@vger.kernel.org>
> 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 <kuabhs@chromium.org>
> wrote:
> >
> > 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 <kuabhs@chromium.org>
> > ---
> >
> > (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.
> 
> 
> > @@ -1438,12 +1439,17 @@ static int
> ath10k_core_create_board_name(struct ath10k *ar, char *name,
> >         }
> >
> >         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
> 
> -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.

Thanks,
Rakesh Pillai.

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

[Doug Anderson]

Hi,

On Tue, Nov 24, 2020 at 1:19 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
>
> > -----Original Message-----
> > From: Doug Anderson <dianders@chromium.org>
> > Sent: Tuesday, November 24, 2020 6:27 AM
> > To: Abhishek Kumar <kuabhs@chromium.org>
> > Cc: Kalle Valo <kvalo@codeaurora.org>; Rakesh Pillai
> > <pillair@codeaurora.org>; LKML <linux-kernel@vger.kernel.org>; ath10k
> > <ath10k@lists.infradead.org>; Brian Norris <briannorris@chromium.org>;
> > linux-wireless <linux-wireless@vger.kernel.org>; David S. Miller
> > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; netdev
> > <netdev@vger.kernel.org>
> > 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 <kuabhs@chromium.org>
> > wrote:
> > >
> > > 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 <kuabhs@chromium.org>
> > > ---
> > >
> > > (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.
> >
> >
> > > @@ -1438,12 +1439,17 @@ static int
> > ath10k_core_create_board_name(struct ath10k *ar, char *name,
> > >         }
> > >
> > >         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

> >
> > -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

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

[Rakesh Pillai]

> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Tuesday, November 24, 2020 9:56 PM
> To: Rakesh Pillai <pillair@codeaurora.org>
> Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo
> <kvalo@codeaurora.org>; LKML <linux-kernel@vger.kernel.org>; ath10k
> <ath10k@lists.infradead.org>; Brian Norris <briannorris@chromium.org>;
> linux-wireless <linux-wireless@vger.kernel.org>; David S. Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; netdev
> <netdev@vger.kernel.org>
> Subject: 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 <pillair@codeaurora.org>
> wrote:
> >
> > > -----Original Message-----
> > > From: Doug Anderson <dianders@chromium.org>
> > > Sent: Tuesday, November 24, 2020 6:27 AM
> > > To: Abhishek Kumar <kuabhs@chromium.org>
> > > Cc: Kalle Valo <kvalo@codeaurora.org>; Rakesh Pillai
> > > <pillair@codeaurora.org>; LKML <linux-kernel@vger.kernel.org>; ath10k
> > > <ath10k@lists.infradead.org>; Brian Norris <briannorris@chromium.org>;
> > > linux-wireless <linux-wireless@vger.kernel.org>; David S. Miller
> > > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; netdev
> > > <netdev@vger.kernel.org>
> > > 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
> <kuabhs@chromium.org>
> > > wrote:
> > > >
> > > > 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 <kuabhs@chromium.org>
> > > > ---
> > > >
> > > > (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.
> > >
> > >
> > > > @@ -1438,12 +1439,17 @@ static int
> > > ath10k_core_create_board_name(struct ath10k *ar, char *name,
> > > >         }
> > > >
> > > >         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
> 
> > >
> > > -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.

This can be handled by two calls to ath10k_core_create_board_name
1) ath10k_core_create_board_name(ar, boardname, sizeof(boardname), true)   :  As per my suggestions, this can result in two possible board names
    a) If DT have the "variant" node, it outputs the #1 from your suggestion  (1. We should prefer a BRD entry that has board-id + chip-id + variant)
    b) If DT does not have the "variant" node, it outputs the #2 from your suggestion (2. If #1 isn't there, we should prefer a BRD entry that has board-id + chip-id)

2) ath10k_core_create_board_name(ar, boardname, sizeof(boardname), false)    :  This is the second call to this function and outputs the #3 from your suggestion (3. If #1 and #2 aren't there we fall back to a BRD entry that has board-id)


Below is the snippet of code change I am suggesting. 

 static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-                                        size_t name_len, bool with_variant)
+                                        size_t name_len, bool with_extra_params)
 {
        /* strlen(',variant=') + strlen(ar->id.bdf_ext) */
        char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };

-       if (with_variant && ar->id.bdf_ext[0] != '\0')
+       if (ar->id.bdf_ext[0] != '\0')
                scnprintf(variant, sizeof(variant), ",variant=%s",
                          ar->id.bdf_ext);

@@ -1493,7 +1493,7 @@ static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
        }

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


Thanks,
Rakesh Pillai.

> 
> 
> 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

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

[Doug Anderson]

Hi,

On Tue, Nov 24, 2020 at 7:44 PM Rakesh Pillai <pillair@codeaurora.org> wrote:
>
> > > 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.
>
> This can be handled by two calls to ath10k_core_create_board_name
> 1) ath10k_core_create_board_name(ar, boardname, sizeof(boardname), true)   :  As per my suggestions, this can result in two possible board names
>     a) If DT have the "variant" node, it outputs the #1 from your suggestion  (1. We should prefer a BRD entry that has board-id + chip-id + variant)
>     b) If DT does not have the "variant" node, it outputs the #2 from your suggestion (2. If #1 isn't there, we should prefer a BRD entry that has board-id + chip-id)
>
> 2) ath10k_core_create_board_name(ar, boardname, sizeof(boardname), false)    :  This is the second call to this function and outputs the #3 from your suggestion (3. If #1 and #2 aren't there we fall back to a BRD entry that has board-id)

What I'm trying to say is this.  Imagine that:

a) the device tree has the "variant" property.

b) the BRD file has two entries, one for "board-id" (1) and one for
"board-id + chip-id" (2).  It doesn't have one for "board-id + chip-id
+ variant" (3).

With your suggestion we'll see the "variant" property in the device
tree.  That means we'll search for (1) and (3).  (3) isn't there, so
we'll pick (1).  ...but we really should have picked (2), right?

-Doug

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

[Rakesh Pillai]

> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Tuesday, December 1, 2020 12:49 AM
> To: Rakesh Pillai <pillair@codeaurora.org>
> Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo
> <kvalo@codeaurora.org>; LKML <linux-kernel@vger.kernel.org>; ath10k
> <ath10k@lists.infradead.org>; Brian Norris <briannorris@chromium.org>;
> linux-wireless <linux-wireless@vger.kernel.org>; David S. Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; netdev
> <netdev@vger.kernel.org>
> Subject: Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF
> selection
> 
> Hi,
> 
> On Tue, Nov 24, 2020 at 7:44 PM Rakesh Pillai <pillair@codeaurora.org>
> wrote:
> >
> > > > 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.
> >
> > This can be handled by two calls to ath10k_core_create_board_name
> > 1) ath10k_core_create_board_name(ar, boardname, sizeof(boardname),
> true)   :  As per my suggestions, this can result in two possible board names
> >     a) If DT have the "variant" node, it outputs the #1 from your suggestion
> (1. We should prefer a BRD entry that has board-id + chip-id + variant)
> >     b) If DT does not have the "variant" node, it outputs the #2 from your
> suggestion (2. If #1 isn't there, we should prefer a BRD entry that has board-
> id + chip-id)
> >
> > 2) ath10k_core_create_board_name(ar, boardname, sizeof(boardname),
> false)    :  This is the second call to this function and outputs the #3 from your
> suggestion (3. If #1 and #2 aren't there we fall back to a BRD entry that has
> board-id)
> 
> What I'm trying to say is this.  Imagine that:
> 
> a) the device tree has the "variant" property.
> 
> b) the BRD file has two entries, one for "board-id" (1) and one for
> "board-id + chip-id" (2).  It doesn't have one for "board-id + chip-id
> + variant" (3).
> 
> With your suggestion we'll see the "variant" property in the device
> tree.  That means we'll search for (1) and (3).  (3) isn't there, so
> we'll pick (1).  ...but we really should have picked (2), right?


Do we expect board-2.bin to not be populated with the bdf with variant field (if its necessary ?)
Seems fine for me, if we have 2 fallback names if that is needed.

> 
> -Doug

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

[Doug Anderson]

Hi,

On Thu, Dec 3, 2020 at 3:33 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
>
> > What I'm trying to say is this.  Imagine that:
> >
> > a) the device tree has the "variant" property.
> >
> > b) the BRD file has two entries, one for "board-id" (1) and one for
> > "board-id + chip-id" (2).  It doesn't have one for "board-id + chip-id
> > + variant" (3).
> >
> > With your suggestion we'll see the "variant" property in the device
> > tree.  That means we'll search for (1) and (3).  (3) isn't there, so
> > we'll pick (1).  ...but we really should have picked (2), right?
>
> Do we expect board-2.bin to not be populated with the bdf with variant field (if its necessary ?)

The whole fact that there is a fallback to begin with implies that
there can be a mismatch between the board-2.bin and the device tree
file.  Once we accept that there can be a mismatch, it seems good to
try all 3 fallbacks in order.


> Seems fine for me, if we have 2 fallback names if that is needed.

OK, sounds good.  So hopefully Abhishek can post a v3 based on what's
in <https://crrev.com/c/2556437> and you can confirm you're good with
it there?

-Doug

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

[Abhishek Kumar]

Hi,

> > (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.
I agree, this was not intentional, I will fix this in the upcoming patches.

On Thu, Dec 3, 2020 at 7:34 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Dec 3, 2020 at 3:33 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
> >
> > > What I'm trying to say is this.  Imagine that:
> > >
> > > a) the device tree has the "variant" property.
> > >
> > > b) the BRD file has two entries, one for "board-id" (1) and one for
> > > "board-id + chip-id" (2).  It doesn't have one for "board-id + chip-id
> > > + variant" (3).
> > >
> > > With your suggestion we'll see the "variant" property in the device
> > > tree.  That means we'll search for (1) and (3).  (3) isn't there, so
> > > we'll pick (1).  ...but we really should have picked (2), right?
> >
> > Do we expect board-2.bin to not be populated with the bdf with variant field (if its necessary ?)
>
> The whole fact that there is a fallback to begin with implies that
> there can be a mismatch between the board-2.bin and the device tree
> file.  Once we accept that there can be a mismatch, it seems good to
> try all 3 fallbacks in order.
>
> > Seems fine for me, if we have 2 fallback names if that is needed.
> OK, sounds good.  So hopefully Abhishek can post a v3 based on what's
> in <https://crrev.com/c/2556437> and you can confirm you're good with
> it there?
I agree, with this patch there can be mismatch between what's provided
in the Board file and what required board name we are generating, so
three calls are needed. So in a sense, we want to keep the V1 patch
with fix to reuse the same BDF.

I am making V3 changes and will address and push that out.

Thanks
Abhishek