| Message ID | 20220818130042.535762-1-jiri@resnulli.us (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: devlink: sync flash and dev info commands | expand |
> -----Original Message----- > From: Jiri Pirko <jiri@resnulli.us> > Sent: Thursday, August 18, 2022 6:01 AM > To: netdev@vger.kernel.org > Cc: davem@davemloft.net; kuba@kernel.org; idosch@nvidia.com; > pabeni@redhat.com; edumazet@google.com; saeedm@nvidia.com; Keller, Jacob > E <jacob.e.keller@intel.com>; vikas.gupta@broadcom.com; > gospo@broadcom.com > Subject: [patch net-next 0/4] net: devlink: sync flash and dev info commands > > From: Jiri Pirko <jiri@nvidia.com> > > Purpose of this patchset is to introduce consistency between two devlink > commands: > devlink dev info > Shows versions of running default flash target and components. > devlink dev flash > Flashes default flash target or component name (if specified > on cmdline). > > Currently it is up to the driver what versions to expose and what flash > update component names to accept. This is inconsistent. Thankfully, only > netdevsim is currently using components, so it is a good time > to sanitize this. > > This patchset makes sure, that devlink.c calls into driver for > component flash update only in case the driver exposes the same version > name. Makes sense. > > Also there are two flags exposed to the use over netlink for versions: > > 1) if driver considers the version represents flashable component, > DEVLINK_ATTR_INFO_VERSION_IS_COMPONENT is set. > This provides a list of component names for the user. > > 2) if driver considers the version represents default flash target (w/o > component name specified) > DEVLINK_ATTR_INFO_VERSION_IS_FLASH_UPDATE_DEFAULT is set. > This tells the user which version is going to be affected by flash > command when no component name is passed. > This is great. I've been meaning to get around to adding single component update to the ice driver, so this would be a good time to do so along with implementing support for this. > Example: > $ devlink dev info > netdevsim/netdevsim10: > driver netdevsim > versions: > running: > fw.mgmt 10.20.30 > fw 11.22.33 > flash_components: > fw.mgmt > flash_update_default fw > $ devlink dev flash netdevsim/netdevsim10 file somefile.bin > [fw.mgmt] Preparing to flash > [fw.mgmt] Flashing 100% > [fw.mgmt] Flash select > [fw.mgmt] Flashing done > $ devlink dev flash netdevsim/netdevsim10 file somefile.bin component fw.mgmt > [fw.mgmt] Preparing to flash > [fw.mgmt] Flashing 100% > [fw.mgmt] Flash select > [fw.mgmt] Flashing done > $ devlink dev flash netdevsim/netdevsim10 file somefile.bin component dummy > Error: selected component is not supported by this device. > > Jiri Pirko (4): > net: devlink: extend info_get() version put to indicate a flash > component > net: devlink: expose the info about version representing a component > netdevsim: expose version of default flash target > net: devlink: expose default flash update target > > drivers/net/netdevsim/dev.c | 17 +++- > include/net/devlink.h | 18 ++++- > include/uapi/linux/devlink.h | 3 + > net/core/devlink.c | 145 ++++++++++++++++++++++++++++++----- > 4 files changed, 157 insertions(+), 26 deletions(-) > > -- > 2.37.1
On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote: > Currently it is up to the driver what versions to expose and what flash > update component names to accept. This is inconsistent. Thankfully, only > netdevsim is currently using components, so it is a good time > to sanitize this. Please take a look at recently merged code - 5417197dd516 ("Merge branch 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions there so I think you're gonna break them?
Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote: >On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote: >> Currently it is up to the driver what versions to expose and what flash >> update component names to accept. This is inconsistent. Thankfully, only >> netdevsim is currently using components, so it is a good time >> to sanitize this. > >Please take a look at recently merged code - 5417197dd516 ("Merge branch >'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions >there so I think you're gonna break them? Ah, crap. Too late :/ They are passing the string to FW (cmd is the component name here): static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port) { char flash_command[T7XX_FB_COMMAND_SIZE]; snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd); return t7xx_devlink_fb_raw_command(flash_command, port, NULL); } This breaks the pairing with info.versions assumption. Any possibility to revert this and let them redo? Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com, soumya.prakash.mishra@intel.com Guys, could you expose one version for component you are flashing? We need 1:1 mapping here. Thanks!
On 8/19/2022 1:55 PM, Jiri Pirko wrote: > Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote: >> On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote: >>> Currently it is up to the driver what versions to expose and what flash >>> update component names to accept. This is inconsistent. Thankfully, only >>> netdevsim is currently using components, so it is a good time >>> to sanitize this. >> >> Please take a look at recently merged code - 5417197dd516 ("Merge branch >> 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions >> there so I think you're gonna break them? > > Ah, crap. Too late :/ They are passing the string to FW (cmd is > the component name here): > static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port) > { > char flash_command[T7XX_FB_COMMAND_SIZE]; > > snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd); > return t7xx_devlink_fb_raw_command(flash_command, port, NULL); > } > > This breaks the pairing with info.versions assumption. Any possibility > to revert this and let them redo? > > Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com, > soumya.prakash.mishra@intel.com > > Guys, could you expose one version for component you are flashing? We > need 1:1 mapping here. Thanks for the heads-up. I had a look at the patch & my understanding is driver is supposed to expose flash update component name & version details via devlink_info_version_running_put_ext(). Is version value a must ? Internally version value is not used for making any decision so in case driver/device doesn't support it should be ok to pass empty string ? Ex: devlink_info_version_running_put_ext(req, "fw", "", DEVLINK_INFO_VERSION_TYPE_COMPONENT); One observation:- While testing I noticed "flash_components:" is not getting displayed as mentioned in cover note. Below is the snapshot for mtk_t7xx driver. Am I missing something here ? # devlink dev info pci/0000:55:00.0: driver mtk_t7xx versions: running: boot
Tue, Aug 23, 2022 at 12:09:06PM CEST, m.chetan.kumar@linux.intel.com wrote: >On 8/19/2022 1:55 PM, Jiri Pirko wrote: >> Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote: >> > On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote: >> > > Currently it is up to the driver what versions to expose and what flash >> > > update component names to accept. This is inconsistent. Thankfully, only >> > > netdevsim is currently using components, so it is a good time >> > > to sanitize this. >> > >> > Please take a look at recently merged code - 5417197dd516 ("Merge branch >> > 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions >> > there so I think you're gonna break them? >> >> Ah, crap. Too late :/ They are passing the string to FW (cmd is >> the component name here): >> static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port) >> { >> char flash_command[T7XX_FB_COMMAND_SIZE]; >> >> snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd); >> return t7xx_devlink_fb_raw_command(flash_command, port, NULL); >> } >> >> This breaks the pairing with info.versions assumption. Any possibility >> to revert this and let them redo? >> >> Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com, >> soumya.prakash.mishra@intel.com >> >> Guys, could you expose one version for component you are flashing? We >> need 1:1 mapping here. > >Thanks for the heads-up. >I had a look at the patch & my understanding is driver is supposed >to expose flash update component name & version details via >devlink_info_version_running_put_ext(). Yes. > >Is version value a must ? Internally version value is not used for making any >decision so in case driver/device doesn't support it should be ok to pass >empty string ? No. > >Ex: >devlink_info_version_running_put_ext(req, "fw", "", > DEVLINK_INFO_VERSION_TYPE_COMPONENT); > >One observation:- >While testing I noticed "flash_components:" is not getting displayed as >mentioned in cover note. You need iproute2 patch for that which is still in my queue: https://github.com/jpirko/iproute2_mlxsw/commit/e1d36409362257cc42a435f6695d2058ab7ab683 > >Below is the snapshot for mtk_t7xx driver. Am I missing something here ? > ># devlink dev info >pci/0000:55:00.0: >driver mtk_t7xx >versions: > running: > boot > >-- >Chetan
On 8/23/2022 5:50 PM, Jiri Pirko wrote: > Tue, Aug 23, 2022 at 12:09:06PM CEST, m.chetan.kumar@linux.intel.com wrote: >> On 8/19/2022 1:55 PM, Jiri Pirko wrote: >>> Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote: >>>> On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote: >>>>> Currently it is up to the driver what versions to expose and what flash >>>>> update component names to accept. This is inconsistent. Thankfully, only >>>>> netdevsim is currently using components, so it is a good time >>>>> to sanitize this. >>>> >>>> Please take a look at recently merged code - 5417197dd516 ("Merge branch >>>> 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions >>>> there so I think you're gonna break them? >>> >>> Ah, crap. Too late :/ They are passing the string to FW (cmd is >>> the component name here): >>> static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port) >>> { >>> char flash_command[T7XX_FB_COMMAND_SIZE]; >>> >>> snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd); >>> return t7xx_devlink_fb_raw_command(flash_command, port, NULL); >>> } >>> >>> This breaks the pairing with info.versions assumption. Any possibility >>> to revert this and let them redo? >>> >>> Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com, >>> soumya.prakash.mishra@intel.com >>> >>> Guys, could you expose one version for component you are flashing? We >>> need 1:1 mapping here. >> >> Thanks for the heads-up. >> I had a look at the patch & my understanding is driver is supposed >> to expose flash update component name & version details via >> devlink_info_version_running_put_ext(). > > Yes. > >> >> Is version value a must ? Internally version value is not used for making any >> decision so in case driver/device doesn't support it should be ok to pass >> empty string ? > > No. > >> >> Ex: >> devlink_info_version_running_put_ext(req, "fw", "", >> DEVLINK_INFO_VERSION_TYPE_COMPONENT); >> >> One observation:- >> While testing I noticed "flash_components:" is not getting displayed as >> mentioned in cover note. > > You need iproute2 patch for that which is still in my queue: > https://github.com/jpirko/iproute2_mlxsw/commit/e1d36409362257cc42a435f6695d2058ab7ab683 Thanks. After applying this patch "flash_components" details are getting displayed. Another observation is if NULL is passed for version_value there is a crash. Below is the backtrace. 3187.556637] BUG: kernel NULL pointer dereference, address: 0000000000000000 [ 3187.556659] #PF: supervisor read access in kernel mode [ 3187.556666] #PF: error_code(0x0000) - not-present page 3187.556791] Call Trace: [ 3187.556796] <TASK> [ 3187.556801] ? devlink_info_version_put+0x112/0x1d0 [ 3187.556823] ? __nla_put+0x20/0x30 [ 3187.556833] devlink_info_version_running_put_ext+0x1c/0x30 [ 3187.556851] t7xx_devlink_info_get+0x37/0x40 [mtk_t7xx] [ 3187.556880] devlink_nl_info_fill.constprop.0+0xa1/0x120 [ 3187.556892] devlink_nl_cmd_info_get_dumpit+0xa8/0x140 [ 3187.556901] netlink_dump+0x1a3/0x340 [ 3187.556913] __netlink_dump_start+0x1d0/0x290 Is driver expected to set version number along with component name ? mtk_t7xx WWAN driver is using the devlink interface for flashing the fw to WWAN device. If WWAN device is not capable of supporting the versioning for each component how should we handle ? Please suggest.
Tue, Aug 23, 2022 at 06:29:48PM CEST, m.chetan.kumar@linux.intel.com wrote: >On 8/23/2022 5:50 PM, Jiri Pirko wrote: >> Tue, Aug 23, 2022 at 12:09:06PM CEST, m.chetan.kumar@linux.intel.com wrote: >> > On 8/19/2022 1:55 PM, Jiri Pirko wrote: >> > > Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote: >> > > > On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote: >> > > > > Currently it is up to the driver what versions to expose and what flash >> > > > > update component names to accept. This is inconsistent. Thankfully, only >> > > > > netdevsim is currently using components, so it is a good time >> > > > > to sanitize this. >> > > > >> > > > Please take a look at recently merged code - 5417197dd516 ("Merge branch >> > > > 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions >> > > > there so I think you're gonna break them? >> > > >> > > Ah, crap. Too late :/ They are passing the string to FW (cmd is >> > > the component name here): >> > > static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port) >> > > { >> > > char flash_command[T7XX_FB_COMMAND_SIZE]; >> > > >> > > snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd); >> > > return t7xx_devlink_fb_raw_command(flash_command, port, NULL); >> > > } >> > > >> > > This breaks the pairing with info.versions assumption. Any possibility >> > > to revert this and let them redo? >> > > >> > > Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com, >> > > soumya.prakash.mishra@intel.com >> > > >> > > Guys, could you expose one version for component you are flashing? We >> > > need 1:1 mapping here. >> > >> > Thanks for the heads-up. >> > I had a look at the patch & my understanding is driver is supposed >> > to expose flash update component name & version details via >> > devlink_info_version_running_put_ext(). >> >> Yes. >> >> > >> > Is version value a must ? Internally version value is not used for making any >> > decision so in case driver/device doesn't support it should be ok to pass >> > empty string ? >> >> No. >> >> > >> > Ex: >> > devlink_info_version_running_put_ext(req, "fw", "", >> > DEVLINK_INFO_VERSION_TYPE_COMPONENT); >> > >> > One observation:- >> > While testing I noticed "flash_components:" is not getting displayed as >> > mentioned in cover note. >> >> You need iproute2 patch for that which is still in my queue: >> https://github.com/jpirko/iproute2_mlxsw/commit/e1d36409362257cc42a435f6695d2058ab7ab683 > >Thanks. After applying this patch "flash_components" details are getting >displayed. > >Another observation is if NULL is passed for version_value there is a crash. So don't pass NULL :) >Below is the backtrace. > >3187.556637] BUG: kernel NULL pointer dereference, address: 0000000000000000 >[ 3187.556659] #PF: supervisor read access in kernel mode >[ 3187.556666] #PF: error_code(0x0000) - not-present page >3187.556791] Call Trace: >[ 3187.556796] <TASK> >[ 3187.556801] ? devlink_info_version_put+0x112/0x1d0 >[ 3187.556823] ? __nla_put+0x20/0x30 >[ 3187.556833] devlink_info_version_running_put_ext+0x1c/0x30 >[ 3187.556851] t7xx_devlink_info_get+0x37/0x40 [mtk_t7xx] >[ 3187.556880] devlink_nl_info_fill.constprop.0+0xa1/0x120 >[ 3187.556892] devlink_nl_cmd_info_get_dumpit+0xa8/0x140 >[ 3187.556901] netlink_dump+0x1a3/0x340 >[ 3187.556913] __netlink_dump_start+0x1d0/0x290 > >Is driver expected to set version number along with component name ? Of course. > >mtk_t7xx WWAN driver is using the devlink interface for flashing the fw to >WWAN device. If WWAN device is not capable of supporting the versioning for >each component how should we handle ? Please suggest. The user should have a visibility about what version is currently stored/running in the device. You should expose it. > >-- >Chetan
Looped hua.yang@mediatek.com to email. On 8/24/2022 2:17 PM, Jiri Pirko wrote: > Tue, Aug 23, 2022 at 06:29:48PM CEST, m.chetan.kumar@linux.intel.com wrote: >> On 8/23/2022 5:50 PM, Jiri Pirko wrote: >>> Tue, Aug 23, 2022 at 12:09:06PM CEST, m.chetan.kumar@linux.intel.com wrote: >>>> On 8/19/2022 1:55 PM, Jiri Pirko wrote: >>>>> Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote: >>>>>> On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote: >>>>>>> Currently it is up to the driver what versions to expose and what flash >>>>>>> update component names to accept. This is inconsistent. Thankfully, only >>>>>>> netdevsim is currently using components, so it is a good time >>>>>>> to sanitize this. >>>>>> >>>>>> Please take a look at recently merged code - 5417197dd516 ("Merge branch >>>>>> 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions >>>>>> there so I think you're gonna break them? >>>>> >>>>> Ah, crap. Too late :/ They are passing the string to FW (cmd is >>>>> the component name here): >>>>> static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port) >>>>> { >>>>> char flash_command[T7XX_FB_COMMAND_SIZE]; >>>>> >>>>> snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd); >>>>> return t7xx_devlink_fb_raw_command(flash_command, port, NULL); >>>>> } >>>>> >>>>> This breaks the pairing with info.versions assumption. Any possibility >>>>> to revert this and let them redo? >>>>> >>>>> Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com, >>>>> soumya.prakash.mishra@intel.com >>>>> >>>>> Guys, could you expose one version for component you are flashing? We >>>>> need 1:1 mapping here. >>>> >>>> Thanks for the heads-up. >>>> I had a look at the patch & my understanding is driver is supposed >>>> to expose flash update component name & version details via >>>> devlink_info_version_running_put_ext(). >>> >>> Yes. >>> >>>> >>>> Is version value a must ? Internally version value is not used for making any >>>> decision so in case driver/device doesn't support it should be ok to pass >>>> empty string ? >>> >>> No. >>> >>>> >>>> Ex: >>>> devlink_info_version_running_put_ext(req, "fw", "", >>>> DEVLINK_INFO_VERSION_TYPE_COMPONENT); >>>> >>>> One observation:- >>>> While testing I noticed "flash_components:" is not getting displayed as >>>> mentioned in cover note. >>> >>> You need iproute2 patch for that which is still in my queue: >>> https://github.com/jpirko/iproute2_mlxsw/commit/e1d36409362257cc42a435f6695d2058ab7ab683 >> >> Thanks. After applying this patch "flash_components" details are getting >> displayed. >> >> Another observation is if NULL is passed for version_value there is a crash. > > So don't pass NULL :) > > >> Below is the backtrace. >> >> 3187.556637] BUG: kernel NULL pointer dereference, address: 0000000000000000 >> [ 3187.556659] #PF: supervisor read access in kernel mode >> [ 3187.556666] #PF: error_code(0x0000) - not-present page >> 3187.556791] Call Trace: >> [ 3187.556796] <TASK> >> [ 3187.556801] ? devlink_info_version_put+0x112/0x1d0 >> [ 3187.556823] ? __nla_put+0x20/0x30 >> [ 3187.556833] devlink_info_version_running_put_ext+0x1c/0x30 >> [ 3187.556851] t7xx_devlink_info_get+0x37/0x40 [mtk_t7xx] >> [ 3187.556880] devlink_nl_info_fill.constprop.0+0xa1/0x120 >> [ 3187.556892] devlink_nl_cmd_info_get_dumpit+0xa8/0x140 >> [ 3187.556901] netlink_dump+0x1a3/0x340 >> [ 3187.556913] __netlink_dump_start+0x1d0/0x290 >> >> Is driver expected to set version number along with component name ? > > Of course. > > >> >> mtk_t7xx WWAN driver is using the devlink interface for flashing the fw to >> WWAN device. If WWAN device is not capable of supporting the versioning for >> each component how should we handle ? Please suggest. > > The user should have a visibility about what version is currently > stored/running in the device. You should expose it. If the only intention of this component version is to give a visbility to user, the WWAN Driver exposes the AT & MBIM control ports. Applications like Modem Manager uses AT/MBIM commands to obtain fw version info. So would it be ok to keep component version as an optional for WWAN drivers ?
Fri, Aug 26, 2022 at 10:54:52AM CEST, m.chetan.kumar@linux.intel.com wrote: >Looped hua.yang@mediatek.com to email. > >On 8/24/2022 2:17 PM, Jiri Pirko wrote: >> Tue, Aug 23, 2022 at 06:29:48PM CEST, m.chetan.kumar@linux.intel.com wrote: >> > On 8/23/2022 5:50 PM, Jiri Pirko wrote: >> > > Tue, Aug 23, 2022 at 12:09:06PM CEST, m.chetan.kumar@linux.intel.com wrote: >> > > > On 8/19/2022 1:55 PM, Jiri Pirko wrote: >> > > > > Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote: >> > > > > > On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote: >> > > > > > > Currently it is up to the driver what versions to expose and what flash >> > > > > > > update component names to accept. This is inconsistent. Thankfully, only >> > > > > > > netdevsim is currently using components, so it is a good time >> > > > > > > to sanitize this. >> > > > > > >> > > > > > Please take a look at recently merged code - 5417197dd516 ("Merge branch >> > > > > > 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions >> > > > > > there so I think you're gonna break them? >> > > > > >> > > > > Ah, crap. Too late :/ They are passing the string to FW (cmd is >> > > > > the component name here): >> > > > > static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port) >> > > > > { >> > > > > char flash_command[T7XX_FB_COMMAND_SIZE]; >> > > > > >> > > > > snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd); >> > > > > return t7xx_devlink_fb_raw_command(flash_command, port, NULL); >> > > > > } >> > > > > >> > > > > This breaks the pairing with info.versions assumption. Any possibility >> > > > > to revert this and let them redo? >> > > > > >> > > > > Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com, >> > > > > soumya.prakash.mishra@intel.com >> > > > > >> > > > > Guys, could you expose one version for component you are flashing? We >> > > > > need 1:1 mapping here. >> > > > >> > > > Thanks for the heads-up. >> > > > I had a look at the patch & my understanding is driver is supposed >> > > > to expose flash update component name & version details via >> > > > devlink_info_version_running_put_ext(). >> > > >> > > Yes. >> > > >> > > > >> > > > Is version value a must ? Internally version value is not used for making any >> > > > decision so in case driver/device doesn't support it should be ok to pass >> > > > empty string ? >> > > >> > > No. >> > > >> > > > >> > > > Ex: >> > > > devlink_info_version_running_put_ext(req, "fw", "", >> > > > DEVLINK_INFO_VERSION_TYPE_COMPONENT); >> > > > >> > > > One observation:- >> > > > While testing I noticed "flash_components:" is not getting displayed as >> > > > mentioned in cover note. >> > > >> > > You need iproute2 patch for that which is still in my queue: >> > > https://github.com/jpirko/iproute2_mlxsw/commit/e1d36409362257cc42a435f6695d2058ab7ab683 >> > >> > Thanks. After applying this patch "flash_components" details are getting >> > displayed. >> > >> > Another observation is if NULL is passed for version_value there is a crash. >> >> So don't pass NULL :) >> >> >> > Below is the backtrace. >> > >> > 3187.556637] BUG: kernel NULL pointer dereference, address: 0000000000000000 >> > [ 3187.556659] #PF: supervisor read access in kernel mode >> > [ 3187.556666] #PF: error_code(0x0000) - not-present page >> > 3187.556791] Call Trace: >> > [ 3187.556796] <TASK> >> > [ 3187.556801] ? devlink_info_version_put+0x112/0x1d0 >> > [ 3187.556823] ? __nla_put+0x20/0x30 >> > [ 3187.556833] devlink_info_version_running_put_ext+0x1c/0x30 >> > [ 3187.556851] t7xx_devlink_info_get+0x37/0x40 [mtk_t7xx] >> > [ 3187.556880] devlink_nl_info_fill.constprop.0+0xa1/0x120 >> > [ 3187.556892] devlink_nl_cmd_info_get_dumpit+0xa8/0x140 >> > [ 3187.556901] netlink_dump+0x1a3/0x340 >> > [ 3187.556913] __netlink_dump_start+0x1d0/0x290 >> > >> > Is driver expected to set version number along with component name ? >> >> Of course. >> >> >> > >> > mtk_t7xx WWAN driver is using the devlink interface for flashing the fw to >> > WWAN device. If WWAN device is not capable of supporting the versioning for >> > each component how should we handle ? Please suggest. >> >> The user should have a visibility about what version is currently >> stored/running in the device. You should expose it. > >If the only intention of this component version is to give a visbility to >user, the WWAN Driver exposes the AT & MBIM control ports. Applications like >Modem Manager uses AT/MBIM commands to obtain fw version info. > >So would it be ok to keep component version as an optional for WWAN drivers ? Nope. It is mandatory now for the devlink flash component name to match version of component reported. If you cannot do that, there is most likely something wrong with your design
From: Jiri Pirko <jiri@nvidia.com> Purpose of this patchset is to introduce consistency between two devlink commands: devlink dev info Shows versions of running default flash target and components. devlink dev flash Flashes default flash target or component name (if specified on cmdline). Currently it is up to the driver what versions to expose and what flash update component names to accept. This is inconsistent. Thankfully, only netdevsim is currently using components, so it is a good time to sanitize this. This patchset makes sure, that devlink.c calls into driver for component flash update only in case the driver exposes the same version name. Also there are two flags exposed to the use over netlink for versions: 1) if driver considers the version represents flashable component, DEVLINK_ATTR_INFO_VERSION_IS_COMPONENT is set. This provides a list of component names for the user. 2) if driver considers the version represents default flash target (w/o component name specified) DEVLINK_ATTR_INFO_VERSION_IS_FLASH_UPDATE_DEFAULT is set. This tells the user which version is going to be affected by flash command when no component name is passed. Example: $ devlink dev info netdevsim/netdevsim10: driver netdevsim versions: running: fw.mgmt 10.20.30 fw 11.22.33 flash_components: fw.mgmt flash_update_default fw $ devlink dev flash netdevsim/netdevsim10 file somefile.bin [fw.mgmt] Preparing to flash [fw.mgmt] Flashing 100% [fw.mgmt] Flash select [fw.mgmt] Flashing done $ devlink dev flash netdevsim/netdevsim10 file somefile.bin component fw.mgmt [fw.mgmt] Preparing to flash [fw.mgmt] Flashing 100% [fw.mgmt] Flash select [fw.mgmt] Flashing done $ devlink dev flash netdevsim/netdevsim10 file somefile.bin component dummy Error: selected component is not supported by this device. Jiri Pirko (4): net: devlink: extend info_get() version put to indicate a flash component net: devlink: expose the info about version representing a component netdevsim: expose version of default flash target net: devlink: expose default flash update target drivers/net/netdevsim/dev.c | 17 +++- include/net/devlink.h | 18 ++++- include/uapi/linux/devlink.h | 3 + net/core/devlink.c | 145 ++++++++++++++++++++++++++++++----- 4 files changed, 157 insertions(+), 26 deletions(-)