| Message ID | 20220822170247.974743-5-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: devlink: sync flash and dev info commands | expand |
On Mon, 22 Aug 2022 19:02:47 +0200 Jiri Pirko wrote: > If certain version exposed by a driver is marked to be representing a > component, expose this info to the user. > > Example: > $ devlink dev info > netdevsim/netdevsim10: > driver netdevsim > versions: > running: > fw.mgmt 10.20.30 > flash_components: > fw.mgmt I don't think we should add API without a clear use, let's drop this as well.
Tue, Aug 23, 2022 at 05:01:16AM CEST, kuba@kernel.org wrote: >On Mon, 22 Aug 2022 19:02:47 +0200 Jiri Pirko wrote: >> If certain version exposed by a driver is marked to be representing a >> component, expose this info to the user. >> >> Example: >> $ devlink dev info >> netdevsim/netdevsim10: >> driver netdevsim >> versions: >> running: >> fw.mgmt 10.20.30 >> flash_components: >> fw.mgmt > >I don't think we should add API without a clear use, let's drop this >as well. What do you mean? This just simply lists components that are possible to use with devlink dev flash. What is not clear? I don't follow.
On Tue, 23 Aug 2022 08:36:01 +0200 Jiri Pirko wrote: > Tue, Aug 23, 2022 at 05:01:16AM CEST, kuba@kernel.org wrote: > >I don't think we should add API without a clear use, let's drop this > >as well. > > What do you mean? This just simply lists components that are possible to > use with devlink dev flash. What is not clear? I don't follow. Dumping random internal bits of the kernel is not how we create uAPIs. Again, what is the scenario in which user space needs to know the flashable component today ?
Tue, Aug 23, 2022 at 09:31:21PM CEST, kuba@kernel.org wrote: >On Tue, 23 Aug 2022 08:36:01 +0200 Jiri Pirko wrote: >> Tue, Aug 23, 2022 at 05:01:16AM CEST, kuba@kernel.org wrote: >> >I don't think we should add API without a clear use, let's drop this >> >as well. >> >> What do you mean? This just simply lists components that are possible to >> use with devlink dev flash. What is not clear? I don't follow. > >Dumping random internal bits of the kernel is not how we create uAPIs. > >Again, what is the scenario in which user space needs to know >the flashable component today ? Well, I thought it would be polite to let the user know what component he can pass to the kernel. Now, it is try-fail/success game. But if you think it is okay to let the user in the doubts, no problem. I will drop the patch.
> -----Original Message----- > From: Jiri Pirko <jiri@resnulli.us> > Sent: Wednesday, August 24, 2022 1:50 AM > To: Jakub Kicinski <kuba@kernel.org> > Cc: netdev@vger.kernel.org; davem@davemloft.net; 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: Re: [patch net-next v2 4/4] net: devlink: expose the info about version > representing a component > > Tue, Aug 23, 2022 at 09:31:21PM CEST, kuba@kernel.org wrote: > >On Tue, 23 Aug 2022 08:36:01 +0200 Jiri Pirko wrote: > >> Tue, Aug 23, 2022 at 05:01:16AM CEST, kuba@kernel.org wrote: > >> >I don't think we should add API without a clear use, let's drop this > >> >as well. > >> > >> What do you mean? This just simply lists components that are possible to > >> use with devlink dev flash. What is not clear? I don't follow. > > > >Dumping random internal bits of the kernel is not how we create uAPIs. > > > >Again, what is the scenario in which user space needs to know > >the flashable component today ? > > Well, I thought it would be polite to let the user know what component > he can pass to the kernel. Now, it is try-fail/success game. But if you > think it is okay to let the user in the doubts, no problem. I will drop > the patch. I would prefer exposing this as well since it lets the user know which names are valid for flashing. I do have some patches for ice to support individual component update as well I can post soon. Thanks, Jake
On Wed, 24 Aug 2022 17:31:46 +0000 Keller, Jacob E wrote: > > Well, I thought it would be polite to let the user know what component > > he can pass to the kernel. Now, it is try-fail/success game. But if you > > think it is okay to let the user in the doubts, no problem. I will drop > > the patch. > > I would prefer exposing this as well since it lets the user know which names are valid for flashing. > > I do have some patches for ice to support individual component update as well I can post soon. Gentlemen, I had multiple false starts myself adding information to device info, flashing and health reporters. Adding APIs which will actually be _useful_ in production is not trivial. I have the advantage of being able to talk to Meta's production team first so none of my patches made it to the list. To be clear I'm not saying (nor believe) that Meta's needs or processes are in any way "the right way to go" or otherwise should dictate the APIs. It's just an example I have direct access to. I don't think I'm out of line asking you for a clear use case. Just knowing something is flashable is not sufficient information, the user needs to know what the component actually describes and what binary to use to update it. Since we have no use of component flashing now it's all cart before the horse. Coincidentally I doubt anyone is making serious use of the health infrastructure.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Wednesday, August 24, 2022 11:12 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org; davem@davemloft.net; > idosch@nvidia.com; pabeni@redhat.com; edumazet@google.com; > saeedm@nvidia.com; vikas.gupta@broadcom.com; gospo@broadcom.com > Subject: Re: [patch net-next v2 4/4] net: devlink: expose the info about version > representing a component > > On Wed, 24 Aug 2022 17:31:46 +0000 Keller, Jacob E wrote: > > > Well, I thought it would be polite to let the user know what component > > > he can pass to the kernel. Now, it is try-fail/success game. But if you > > > think it is okay to let the user in the doubts, no problem. I will drop > > > the patch. > > > > I would prefer exposing this as well since it lets the user know which names are > valid for flashing. > > > > I do have some patches for ice to support individual component update as well > I can post soon. > > Gentlemen, I had multiple false starts myself adding information > to device info, flashing and health reporters. Adding APIs which > will actually be _useful_ in production is not trivial. I have > the advantage of being able to talk to Meta's production team first > so none of my patches made it to the list. > > To be clear I'm not saying (nor believe) that Meta's needs or processes > are in any way "the right way to go" or otherwise should dictate > the APIs. It's just an example I have direct access to. > > I don't think I'm out of line asking you for a clear use case. > Just knowing something is flashable is not sufficient information, > the user needs to know what the component actually describes and > what binary to use to update it. > At least for ice, the same binary would be used for individual component update. the PLDM firmware binary header describes where each component is within it, and is decoded by lib/pldmfw, we just need to translate the PLDM header codes to the userspace names. The old tools which Intel supports do have support for such an individual component update, but the demand wasn't very high, so I never got around to posting the patches to support this. There are some corner cases where it might be helpful to flash (or reflash) a single component, but it seems somewhat less useful for most end-users and mostly would be useful for internal engineering and debugging. Users would still need to know what each component is, and there isn't much the kernel API itself can do here. We do document a short description, but that is going to be limited in usefulness since it likely depends on a lot of related knowledge. Thanks, Jake > Since we have no use of component flashing now it's all cart > before the horse. > > Coincidentally I doubt anyone is making serious use of the health > infrastructure. There haven't been a lot of implementations here, which makes it hard to develop tooling, and then without tooling no one wants to write the implementation....
On Wed, Aug 24, 2022 at 06:46:13PM +0000, Keller, Jacob E wrote: > > > > -----Original Message----- > > From: Jakub Kicinski <kuba@kernel.org> > > Sent: Wednesday, August 24, 2022 11:12 AM > > To: Keller, Jacob E <jacob.e.keller@intel.com> > > Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org; davem@davemloft.net; > > idosch@nvidia.com; pabeni@redhat.com; edumazet@google.com; > > saeedm@nvidia.com; vikas.gupta@broadcom.com; gospo@broadcom.com > > Subject: Re: [patch net-next v2 4/4] net: devlink: expose the info about version > > representing a component > > > > On Wed, 24 Aug 2022 17:31:46 +0000 Keller, Jacob E wrote: > > > > Well, I thought it would be polite to let the user know what component > > > > he can pass to the kernel. Now, it is try-fail/success game. But if you > > > > think it is okay to let the user in the doubts, no problem. I will drop > > > > the patch. > > > > > > I would prefer exposing this as well since it lets the user know which names are > > valid for flashing. > > > > > > I do have some patches for ice to support individual component update as well > > I can post soon. > > > > Gentlemen, I had multiple false starts myself adding information > > to device info, flashing and health reporters. Adding APIs which > > will actually be _useful_ in production is not trivial. I have > > the advantage of being able to talk to Meta's production team first > > so none of my patches made it to the list. > > > > To be clear I'm not saying (nor believe) that Meta's needs or processes > > are in any way "the right way to go" or otherwise should dictate > > the APIs. It's just an example I have direct access to. > > > > I don't think I'm out of line asking you for a clear use case. > > Just knowing something is flashable is not sufficient information, > > the user needs to know what the component actually describes and > > what binary to use to update it. > > > > At least for ice, the same binary would be used for individual component update. the PLDM firmware binary header describes where each component is within it, and is decoded by lib/pldmfw, we just need to translate the PLDM header codes to the userspace names. > > The old tools which Intel supports do have support for such an individual component update, but the demand wasn't very high, so I never got around to posting the patches to support this. There are some corner cases where it might be helpful to flash (or reflash) a single component, but it seems somewhat less useful for most end-users and mostly would be useful for internal engineering and debugging. > > Users would still need to know what each component is, and there isn't much the kernel API itself can do here. We do document a short description, but that is going to be limited in usefulness since it likely depends on a lot of related knowledge. > I agree with this. Individual component updates are most useful in a dev/debug environment rather than in production. I'm not sure there is value in exporting this all the way out to kernel APIs.
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index 2f24b53a87a5..7f2874189188 100644 --- a/include/uapi/linux/devlink.h +++ b/include/uapi/linux/devlink.h @@ -607,6 +607,8 @@ enum devlink_attr { DEVLINK_ATTR_SELFTESTS, /* nested */ + DEVLINK_ATTR_INFO_VERSION_IS_COMPONENT, /* u8 0 or 1 */ + /* add new attributes above here, update the policy in devlink.c */ __DEVLINK_ATTR_MAX, diff --git a/net/core/devlink.c b/net/core/devlink.c index 17b78123ad9d..23a5fd92ecaa 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -6670,6 +6670,11 @@ static int devlink_info_version_put(struct devlink_info_req *req, int attr, if (err) goto nla_put_failure; + err = nla_put_u8(req->msg, DEVLINK_ATTR_INFO_VERSION_IS_COMPONENT, + version_type == DEVLINK_INFO_VERSION_TYPE_COMPONENT); + if (err) + goto nla_put_failure; + nla_nest_end(req->msg, nest); return 0;