| Message ID | 20220818130042.535762-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 Thu, 18 Aug 2022 15:00:42 +0200 Jiri Pirko wrote: > Allow driver to mark certain version obtained by info_get() op as > "flash update default". Expose this information to user which allows him > to understand what version is going to be affected if he does flash > update without specifying the component. Implement this in netdevsim. My intuition would be that if you specify no component you're flashing the entire device. Is that insufficient? Can you explain the use case? Also Documentation/ needs to be updated.
Fri, Aug 19, 2022 at 04:53:01AM CEST, kuba@kernel.org wrote: >On Thu, 18 Aug 2022 15:00:42 +0200 Jiri Pirko wrote: >> Allow driver to mark certain version obtained by info_get() op as >> "flash update default". Expose this information to user which allows him >> to understand what version is going to be affected if he does flash >> update without specifying the component. Implement this in netdevsim. > >My intuition would be that if you specify no component you're flashing >the entire device. Is that insufficient? Can you explain the use case? I guess that it up to the driver implementation. I can imagine arguments for both ways. Anyway, there is no way to restrict this in kernel, so let that up to the driver. > >Also Documentation/ needs to be updated. Okay.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, August 18, 2022 7:53 PM > To: Jiri Pirko <jiri@resnulli.us> > 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 4/4] net: devlink: expose default flash update target > > On Thu, 18 Aug 2022 15:00:42 +0200 Jiri Pirko wrote: > > Allow driver to mark certain version obtained by info_get() op as > > "flash update default". Expose this information to user which allows him > > to understand what version is going to be affected if he does flash > > update without specifying the component. Implement this in netdevsim. > > My intuition would be that if you specify no component you're flashing > the entire device. Is that insufficient? Can you explain the use case? > > Also Documentation/ needs to be updated. Some of the components in ice include the DDP which has an info version, but which is not part of the flash but is loaded by the driver during initialization. Thanks, Jake
On Fri, 19 Aug 2022 20:59:28 +0000 Keller, Jacob E wrote: > > My intuition would be that if you specify no component you're flashing > > the entire device. Is that insufficient? Can you explain the use case? > > > > Also Documentation/ needs to be updated. > > Some of the components in ice include the DDP which has an info > version, but which is not part of the flash but is loaded by the > driver during initialization. Right "entire device" as in "everything in 'stored'". Runtime loaded stuff should not be listed in "stored" and therefore not be considered "flashable". Correct?
On Fri, 19 Aug 2022 10:12:16 +0200 Jiri Pirko wrote: > Fri, Aug 19, 2022 at 04:53:01AM CEST, kuba@kernel.org wrote: > >On Thu, 18 Aug 2022 15:00:42 +0200 Jiri Pirko wrote: > >> Allow driver to mark certain version obtained by info_get() op as > >> "flash update default". Expose this information to user which allows him > >> to understand what version is going to be affected if he does flash > >> update without specifying the component. Implement this in netdevsim. > > > >My intuition would be that if you specify no component you're flashing > >the entire device. Is that insufficient? Can you explain the use case? > > I guess that it up to the driver implementation. I can imagine arguments > for both ways. Anyway, there is no way to restrict this in kernel, so > let that up to the driver. To be clear - your intent is to impose more structure on the relation between the dev info and dev flash, right? But just "to be safe", there's no immediate need to do this? The entire dev info / dev flash interface was driven by practical needs of the fleet management team @Facebook / Meta. What would make the changes you're making more useful here would be if instead of declaring the "default" component, we declared "overall" component. I.e. the component which is guaranteed to encompass all the other versions in "stored", and coincidentally is also the default flashed one. That way the FW version reporting can be simplified to store only one version.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Friday, August 19, 2022 2:46 PM > 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 4/4] net: devlink: expose default flash update target > > On Fri, 19 Aug 2022 20:59:28 +0000 Keller, Jacob E wrote: > > > My intuition would be that if you specify no component you're flashing > > > the entire device. Is that insufficient? Can you explain the use case? > > > > > > Also Documentation/ needs to be updated. > > > > Some of the components in ice include the DDP which has an info > > version, but which is not part of the flash but is loaded by the > > driver during initialization. > > Right "entire device" as in "everything in 'stored'". Runtime loaded > stuff should not be listed in "stored" and therefore not be considered > "flashable". Correct? Yes I believe we don't list those as stored. We do have some extra version information that is reported through multiple info lines, i.e. we report: fw.mgmt fw.mgmt.api fw.mgmt.build where the .api and .build are sub-version fields of the fw.mgmt and can potentially give further information but are just a part of the fw.mgmt component. They can't be flashed separately. Thanks, Jake
Fri, Aug 19, 2022 at 11:54:59PM CEST, kuba@kernel.org wrote: >On Fri, 19 Aug 2022 10:12:16 +0200 Jiri Pirko wrote: >> Fri, Aug 19, 2022 at 04:53:01AM CEST, kuba@kernel.org wrote: >> >On Thu, 18 Aug 2022 15:00:42 +0200 Jiri Pirko wrote: >> >> Allow driver to mark certain version obtained by info_get() op as >> >> "flash update default". Expose this information to user which allows him >> >> to understand what version is going to be affected if he does flash >> >> update without specifying the component. Implement this in netdevsim. >> > >> >My intuition would be that if you specify no component you're flashing >> >the entire device. Is that insufficient? Can you explain the use case? >> >> I guess that it up to the driver implementation. I can imagine arguments >> for both ways. Anyway, there is no way to restrict this in kernel, so >> let that up to the driver. > >To be clear - your intent is to impose more structure on the relation >between the dev info and dev flash, right? But just "to be safe", Correct. Basically I want to make things clear for the user in terms of what he can flash, what component names he can pass, what happens during flash without component. Also, I want to sanitize drivers so they cannot accept *any* component name. >there's no immediate need to do this? Nope. > >The entire dev info / dev flash interface was driven by practical needs >of the fleet management team @Facebook / Meta. > >What would make the changes you're making more useful here would be if >instead of declaring the "default" component, we declared "overall" >component. I.e. the component which is guaranteed to encompass all the >other versions in "stored", and coincidentally is also the default >flashed one. It is just semantics. Default is what we have now and drivers are using it. How, that is up to the driver. I see no way how to enforce this, do you? But anyway, I can split the patchset in 2: 1) sanitize components 2) default/overall/whatever If that would help. > >That way the FW version reporting can be simplified to store only one >version.
Sat, Aug 20, 2022 at 12:07:41AM CEST, jacob.e.keller@intel.com wrote: > > >> -----Original Message----- >> From: Jakub Kicinski <kuba@kernel.org> >> Sent: Friday, August 19, 2022 2:46 PM >> 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 4/4] net: devlink: expose default flash update target >> >> On Fri, 19 Aug 2022 20:59:28 +0000 Keller, Jacob E wrote: >> > > My intuition would be that if you specify no component you're flashing >> > > the entire device. Is that insufficient? Can you explain the use case? >> > > >> > > Also Documentation/ needs to be updated. >> > >> > Some of the components in ice include the DDP which has an info >> > version, but which is not part of the flash but is loaded by the >> > driver during initialization. >> >> Right "entire device" as in "everything in 'stored'". Runtime loaded >> stuff should not be listed in "stored" and therefore not be considered >> "flashable". Correct? > >Yes I believe we don't list those as stored. > >We do have some extra version information that is reported through multiple info lines, i.e. we report: > >fw.mgmt >fw.mgmt.api >fw.mgmt.build > >where the .api and .build are sub-version fields of the fw.mgmt and can potentially give further information but are just a part of the fw.mgmt component. They can't be flashed separately. Yep, in my patchset, this is accounted for. The driver can say if the "version" is flashable (passed as a compenent name) or not. In this case, it is not and it only tells the user version of some fw part. > >Thanks, >Jake
On Sat, 20 Aug 2022 07:44:41 +0200 Jiri Pirko wrote: > >The entire dev info / dev flash interface was driven by practical needs > >of the fleet management team @Facebook / Meta. > > > >What would make the changes you're making more useful here would be if > >instead of declaring the "default" component, we declared "overall" > >component. I.e. the component which is guaranteed to encompass all the > >other versions in "stored", and coincidentally is also the default > >flashed one. > > It is just semantics. You say that like it's a synonym for irrelevance. Semantics are *everything* for the uAPI :) > Default is what we have now and drivers are using > it. How, that is up to the driver. I see no way how to enforce this, do > you? Not sure I understand. We document the thing clearly as "this is the overall and must include all parts of stored FW", name it appropriately. If someone sneaks in code which abuses the value for something else, nothing we can do, like with every driver API we have. The "default" gives user information but to interpret that information user is presupposed to know the semantics of the components. Of what use is knowing that default is component A if I don't know that it's the component I want to flash. And if so why don't I just say "component A"? > But anyway, I can split the patchset in 2: > 1) sanitize components > 2) default/overall/whatever > If that would help. Sure thing, seems like a practical approach. On second thought the overall may not be practical, there are sometimes critical components on the board you don't really want to flash unless you really have to. CPLDs and stuff. So perhaps we should scratch (2) altogether until we have a clear need...
> -----Original Message----- > From: Jiri Pirko <jiri@resnulli.us> > Sent: Friday, August 19, 2022 10:47 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: Jakub Kicinski <kuba@kernel.org>; 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 4/4] net: devlink: expose default flash update target > > Sat, Aug 20, 2022 at 12:07:41AM CEST, jacob.e.keller@intel.com wrote: > > > > > >> -----Original Message----- > >> From: Jakub Kicinski <kuba@kernel.org> > >> Sent: Friday, August 19, 2022 2:46 PM > >> 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 4/4] net: devlink: expose default flash update > target > >> > >> On Fri, 19 Aug 2022 20:59:28 +0000 Keller, Jacob E wrote: > >> > > My intuition would be that if you specify no component you're flashing > >> > > the entire device. Is that insufficient? Can you explain the use case? > >> > > > >> > > Also Documentation/ needs to be updated. > >> > > >> > Some of the components in ice include the DDP which has an info > >> > version, but which is not part of the flash but is loaded by the > >> > driver during initialization. > >> > >> Right "entire device" as in "everything in 'stored'". Runtime loaded > >> stuff should not be listed in "stored" and therefore not be considered > >> "flashable". Correct? > > > >Yes I believe we don't list those as stored. > > > >We do have some extra version information that is reported through multiple > info lines, i.e. we report: > > > >fw.mgmt > >fw.mgmt.api > >fw.mgmt.build > > > >where the .api and .build are sub-version fields of the fw.mgmt and can > potentially give further information but are just a part of the fw.mgmt > component. They can't be flashed separately. > > Yep, in my patchset, this is accounted for. The driver can say if the > "version" is flashable (passed as a compenent name) or not. In this case, > it is not and it only tells the user version of some fw part. > I think we can just go with "is this flashable or not?" and then document that if no component is flashed, the driver should be flashing all marked components? Then we don't need a "default" since the default without component is to flash everything. > > > >Thanks, > >Jake
Mon, Aug 22, 2022 at 07:09:35PM CEST, jacob.e.keller@intel.com wrote: > > >> -----Original Message----- >> From: Jiri Pirko <jiri@resnulli.us> >> Sent: Friday, August 19, 2022 10:47 PM >> To: Keller, Jacob E <jacob.e.keller@intel.com> >> Cc: Jakub Kicinski <kuba@kernel.org>; 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 4/4] net: devlink: expose default flash update target >> >> Sat, Aug 20, 2022 at 12:07:41AM CEST, jacob.e.keller@intel.com wrote: >> > >> > >> >> -----Original Message----- >> >> From: Jakub Kicinski <kuba@kernel.org> >> >> Sent: Friday, August 19, 2022 2:46 PM >> >> 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 4/4] net: devlink: expose default flash update >> target >> >> >> >> On Fri, 19 Aug 2022 20:59:28 +0000 Keller, Jacob E wrote: >> >> > > My intuition would be that if you specify no component you're flashing >> >> > > the entire device. Is that insufficient? Can you explain the use case? >> >> > > >> >> > > Also Documentation/ needs to be updated. >> >> > >> >> > Some of the components in ice include the DDP which has an info >> >> > version, but which is not part of the flash but is loaded by the >> >> > driver during initialization. >> >> >> >> Right "entire device" as in "everything in 'stored'". Runtime loaded >> >> stuff should not be listed in "stored" and therefore not be considered >> >> "flashable". Correct? >> > >> >Yes I believe we don't list those as stored. >> > >> >We do have some extra version information that is reported through multiple >> info lines, i.e. we report: >> > >> >fw.mgmt >> >fw.mgmt.api >> >fw.mgmt.build >> > >> >where the .api and .build are sub-version fields of the fw.mgmt and can >> potentially give further information but are just a part of the fw.mgmt >> component. They can't be flashed separately. >> >> Yep, in my patchset, this is accounted for. The driver can say if the >> "version" is flashable (passed as a compenent name) or not. In this case, >> it is not and it only tells the user version of some fw part. >> > >I think we can just go with "is this flashable or not?" and then document that if no component is flashed, the driver should be flashing all marked components? > >Then we don't need a "default" since the default without component is to flash everything. I dropped "default" from the patchset. We need to document the semanticts for default at least. Is it always "everything"? Idk. > >> > >> >Thanks, >> >Jake
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index 97281b6aa41f..f999b9477a26 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c @@ -995,7 +995,8 @@ static int nsim_dev_info_get(struct devlink *devlink, if (err) return err; - return devlink_info_version_running_put(req, "fw", "11.22.33"); + return devlink_info_version_running_put_ext(req, "fw", "11.22.33", + DEVLINK_INFO_VERSION_TYPE_FLASH_UPDATE_DEFAULT); } #define NSIM_DEV_FLASH_SIZE 500000 diff --git a/include/net/devlink.h b/include/net/devlink.h index 9bf4f03feca6..bfe988a56067 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1719,6 +1719,9 @@ enum devlink_info_version_type { DEVLINK_INFO_VERSION_TYPE_COMPONENT, /* May be used as flash update * component by name. */ + DEVLINK_INFO_VERSION_TYPE_FLASH_UPDATE_DEFAULT, /* Is default flash + * update target. + */ }; int devlink_info_version_fixed_put(struct devlink_info_req *req, diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index 7f2874189188..01e348177f60 100644 --- a/include/uapi/linux/devlink.h +++ b/include/uapi/linux/devlink.h @@ -608,6 +608,7 @@ enum devlink_attr { DEVLINK_ATTR_SELFTESTS, /* nested */ DEVLINK_ATTR_INFO_VERSION_IS_COMPONENT, /* u8 0 or 1 */ + DEVLINK_ATTR_INFO_VERSION_IS_FLASH_UPDATE_DEFAULT, /* u8 0 or 1 */ /* add new attributes above here, update the policy in devlink.c */ diff --git a/net/core/devlink.c b/net/core/devlink.c index 23a5fd92ecaa..de583fb2ed12 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -4748,6 +4748,7 @@ struct devlink_info_req { enum devlink_info_version_type version_type, void *version_cb_priv); void *version_cb_priv; + unsigned int flash_update_default_count; }; struct devlink_flash_component_lookup_ctx { @@ -6656,6 +6657,12 @@ static int devlink_info_version_put(struct devlink_info_req *req, int attr, if (!req->msg) return 0; + if (version_type == DEVLINK_INFO_VERSION_TYPE_FLASH_UPDATE_DEFAULT) { + if (WARN_ON(req->flash_update_default_count++)) + /* Max one flash update default is allowed. */ + return -EINVAL; + } + nest = nla_nest_start_noflag(req->msg, attr); if (!nest) return -EMSGSIZE; @@ -6675,6 +6682,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_FLASH_UPDATE_DEFAULT, + version_type == DEVLINK_INFO_VERSION_TYPE_FLASH_UPDATE_DEFAULT); + if (err) + goto nla_put_failure; + nla_nest_end(req->msg, nest); return 0;