MIME-Version: 1.0
References: <20220110231255.v2.1.Ie4dcc45b0bf365077303c596891d460d716bb4c5@changeid>
 <CAD=FV=VVzQFK_PwnCXkJ6OK3DcgBnoxmvNG3WXFn30ncdY-E_w@mail.gmail.com>
In-Reply-To: <CAD=FV=VVzQFK_PwnCXkJ6OK3DcgBnoxmvNG3WXFn30ncdY-E_w@mail.gmail.com>
From:   Abhishek Kumar <kuabhs@chromium.org>
Date:   Thu, 13 Jan 2022 22:50:45 -0800
Message-ID: <CACTWRwsT8dEPiVkvk_=FPSAwuN2oti27p8Vb-mVjWE7iJuTNrw@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] ath10k: search for default BDF name provided in DT
To:     Doug Anderson <dianders@chromium.org>
Cc:     Kalle Valo <kvalo@codeaurora.org>,
        ath10k <ath10k@lists.infradead.org>,
        Rakesh Pillai <pillair@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Kalle Valo <kvalo@kernel.org>, netdev <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jan 10, 2022 at 4:51 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Jan 10, 2022 at 3:15 PM Abhishek Kumar <kuabhs@chromium.org> wrote:
> >
> > +int ath10k_core_parse_default_bdf_dt(struct ath10k *ar)
> > +{
> > +       struct device_node *node;
> > +       const char *board_name = NULL;
> > +
> > +       ar->id.default_bdf[0] = '\0';
> > +
> > +       node = ar->dev->of_node;
> > +       if (!node)
> > +               return -ENOENT;
> > +
> > +       of_property_read_string(node, "qcom,ath10k-default-bdf",
> > +                               &board_name);
> > +       if (!board_name)
> > +               return -ENODATA;
> > +
> > +       if (strscpy(ar->id.default_bdf,
> > +                   board_name, sizeof(ar->id.default_bdf)) < 0)
> > +               ath10k_warn(ar,
> > +                           "default board name is longer than allocated buffer, board_name: %s; allocated size: %ld\n",
> > +                           board_name, sizeof(ar->id.default_bdf));
>
> I suspect, but don't know for sure, that you're going to get another
> builder splat here. Just like sizeof() isn't guaranteed to return an
> "unsigned int", it's also not guaranteed to return an "unsigned long".
> I believe you want %zu. See Documentation/core-api/printk-formats.rst
Thanks for the tip, I will make this fix in V3.
>
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(ath10k_core_parse_default_bdf_dt);
>
> Boy, that function seems like overkill for something that you need
> once at init time. ...and I also suspect that the lifetime of the
> string returned by of_property_read_string() is valid for as long as
> your "of_node" is held and thus probably you could use it directly (it
> likely has a longer lifetime than the location you're storing it).
>
> ...but I guess it matches the ath10k_core_check_dt() function above
> it, so I guess it's fine?
Ya, that was my idea to match it with ath10k_core_check_dt, initially,
I was planning to remodify ath10k_core_check_dt to parse the new
property, but looks it is used it multiple places, so I thought having
a separate parser function would be cleaner, however, I am open to new
ideas.

- Abhishek