From: Chaoyong He <chaoyong.he@corigine.com>
To: Ferruh Yigit <ferruh.yigit@amd.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: oss-drivers <oss-drivers@corigine.com>
Subject: RE: [PATCH 0/8] optimize the firmware loading process
Date: Fri, 26 Jan 2024 08:25:47 +0000 [thread overview]
Message-ID: <SJ0PR13MB55453D245D1916B475F576639E792@SJ0PR13MB5545.namprd13.prod.outlook.com> (raw)
In-Reply-To: <4d14b304-f20f-4484-bce9-9c3338d37750@amd.com>
> On 1/25/2024 2:06 AM, Chaoyong He wrote:
> >>>>>> On 1/15/2024 2:54 AM, Chaoyong He wrote:
> >>>>>>> This patch series aims to speedup the DPDK application start by
> >>>>>>> optimize the firmware loading process in sereval places.
> >>>>>>> We also simplify the port name in multiple PF firmware case to
> >>>>>>> make the customer happy.
> >>>>>>>
> >>>>>>
> >>>>>> <...>
> >>>>>>
> >>>>>>> net/nfp: add the elf module
> >>>>>>> net/nfp: reload the firmware only when firmware changed
> >>>>>>
> >>>>>> Above commit adds elf parser capability and second one loads
> >>>>>> firmware when build time is different.
> >>>>>>
> >>>>>> I can see this is an optimization effort, to understand FW status
> >>>>>> before loading FW, but relying on build time seems fragile. Does
> >>>>>> it help to add a new section to store version information and
> >>>>>> evaluate based
> >>>> on this information?
> >>>>>>
> >>>>>
> >>>>> We have a branch of firmware (several app type combined with
> >>>>> NFD3/NFDk) with the same version information(monthly publish), so
> >>>>> the
> >>>> version information can't help us, because we can't distinguish
> >>>> among
> >> them.
> >>>>>
> >>>>> But the build time is different for every firmware, and that's the
> >>>>> reason we
> >>>> choose it.
> >>>>>
> >>>>
> >>>> If version is same although FW itself is different, isn't this
> >>>> problem on its
> >> own?
> >>>> Perhaps an additional field is required in version syntax.
> >>>
> >>> Actually, it's just the role the build time which embed in the
> >>> firmware plays,
> >> which is unique for every firmware, and which can't be modified once
> >> the firmware was published.
> >>>
> >>
> >> I see it is already in the elf binary but relying on build time of a
> >> FW to decide to load it or not looks a weak method to me, and fragile as
> stated before.
> >>
> >>> What you said also has its mean, but in practice we can't accept(at
> >>> least can't
> >> do it immediately), which needs to discuss with firmware team.
> >>>
> >>
> >> As it is an optimization I assume it is not urgent, so would you mind
> >> discussing the issue with the FW team, perhaps it can lead to a
> >> better solution, we can proceed after that.
> >
> > After discussing with the FW team, seems we still can't just use version
> because our firmware are published monthly.
> >
> > Between two public version, we also have internal versions like 'rc0, rc1'
> scheme just as DPDK.
> > But the problem is, we may compile and share many firmware in daily
> development.
> > They are maybe only valid for one time and will never publish, so we won't
> assign a version for them.
> >
>
> So is my understanding correct that this effort is not for customers and not for
> published FWs, but mostly for developers with development FWs.
>
> If so perhaps it can be simplified even more, there can be a devarg that force
> loads the FW, for development scenario.
> By default, driver checks the FW version and loads according, but if flag is set it
> loads the FW even with same version. When a new development FW is out,
> developer can run DPDK app with this parameter and it loads the FW to test,
> does this make sense.
Yeah, it's a perfect method, thanks.
>
> I am not trying to be difficult, just the proposed approach didn't satisfy me I
> am trying to understand it and help if I can. And if you please continue
> discussion in the mailing list it enables others to comment and perhaps
> provide a better option.
>
>
> > So the build time is the only information we can distinguish them, if just
> using version, PMD will not reload the firmware which needs to test and
> debug.
> >
> > How about we first using version to check, and then using build time to
> check for the same version?
> >
>
> Nope, I though elf parsing and relying on build time can be removed, if you
> have them anyway, no need to add the version check, can use just the build
> time to check.
>
>
> And another related question, if developers are using daily FWs with exact
> same version, when a developer had an unexpected behavior, how she will
> know exactly which version of the FW (in the device), this maybe required to
> track down the problem. Shouldn't each FW have a unique identifier, except
> the "build time" info which is stored in the elf binary not FW itself?
We will use the devarg and version as your advice, and remove the build time logics.
>
> >>
> >> Meanwhile I will continue with remaining patches, excluding these two
> >> patches.
> >>
> >>> If you insist that we should change the design, maybe we can just
> >>> kick off
> >> two commits, and upstream the other commits?
> >>> - net/nfp: add the elf module
> >>> - net/nfp: reload the firmware only when firmware changed
> >
next prev parent reply other threads:[~2024-01-26 8:25 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-15 2:54 Chaoyong He
2024-01-15 2:54 ` [PATCH 1/8] net/nfp: add the interface for getting the firmware name Chaoyong He
2024-01-15 2:54 ` [PATCH 2/8] net/nfp: speed up the firmware loading process Chaoyong He
2024-01-15 2:54 ` [PATCH 3/8] net/nfp: optimize loading the firmware process Chaoyong He
2024-01-15 2:54 ` [PATCH 4/8] net/nfp: enlarge the range of skipping loading the firmware Chaoyong He
2024-01-15 2:54 ` [PATCH 5/8] net/nfp: add the step of clearing the beat time Chaoyong He
2024-01-15 2:54 ` [PATCH 6/8] net/nfp: add the elf module Chaoyong He
2024-01-15 2:54 ` [PATCH 7/8] net/nfp: reload the firmware only when firmware changed Chaoyong He
2024-01-15 2:54 ` [PATCH 8/8] net/nfp: simplify the port name for multiple PFs Chaoyong He
2024-01-22 15:08 ` [PATCH 0/8] optimize the firmware loading process Ferruh Yigit
2024-01-23 1:46 ` Chaoyong He
2024-01-23 9:14 ` Ferruh Yigit
2024-01-23 11:27 ` Chaoyong He
2024-01-23 11:42 ` Ferruh Yigit
2024-01-26 8:25 ` Chaoyong He [this message]
2024-01-23 15:18 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SJ0PR13MB55453D245D1916B475F576639E792@SJ0PR13MB5545.namprd13.prod.outlook.com \
--to=chaoyong.he@corigine.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=oss-drivers@corigine.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).