| Message ID | 1584458082-29207-2-git-send-email-vasundhara-v.volam@broadcom.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | bnxt_en updates to devlink cmd | expand |
On Tue, 17 Mar 2020 20:44:38 +0530 Vasundhara Volam wrote: > Add definition and documentation for the new generic info "drv.spec". > "drv.spec" specifies the version of the software interfaces between > driver and firmware. > > Cc: Jiri Pirko <jiri@mellanox.com> > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> > Signed-off-by: Michael Chan <michael.chan@broadcom.com> > --- > Documentation/networking/devlink/devlink-info.rst | 6 ++++++ > include/net/devlink.h | 3 +++ > 2 files changed, 9 insertions(+) > > diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst > index 70981dd..0765a48 100644 > --- a/Documentation/networking/devlink/devlink-info.rst > +++ b/Documentation/networking/devlink/devlink-info.rst > @@ -59,6 +59,12 @@ board.manufacture > > An identifier of the company or the facility which produced the part. > > +drv.spec > +-------- > + > +Firmware interface specification version of the software interfaces between Why did you call this "drv" if the first sentence of the description says it's a property of the firmware? Upcoming Intel patches call this "fw.mgmt.api". Please use that name, it makes far more sense. > +driver and firmware. This tag displays spec version implemented by driver. > + > fw > -- > > diff --git a/include/net/devlink.h b/include/net/devlink.h > index c9ca86b..9c4d889 100644 > --- a/include/net/devlink.h > +++ b/include/net/devlink.h > @@ -476,6 +476,9 @@ enum devlink_param_generic_id { > /* Revision of asic design */ > #define DEVLINK_INFO_VERSION_GENERIC_ASIC_REV "asic.rev" > > +/* FW interface specification version implemented by driver */ > +#define DEVLINK_INFO_VERSION_GENERIC_DRV_SPEC "drv.spec" > + > /* Overall FW version */ > #define DEVLINK_INFO_VERSION_GENERIC_FW "fw" > /* Control processor FW version */
On 3/17/2020 10:40 AM, Jakub Kicinski wrote: > On Tue, 17 Mar 2020 20:44:38 +0530 Vasundhara Volam wrote: >> Add definition and documentation for the new generic info "drv.spec". >> "drv.spec" specifies the version of the software interfaces between >> driver and firmware. >> >> Cc: Jiri Pirko <jiri@mellanox.com> >> Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> >> Signed-off-by: Michael Chan <michael.chan@broadcom.com> >> --- >> Documentation/networking/devlink/devlink-info.rst | 6 ++++++ >> include/net/devlink.h | 3 +++ >> 2 files changed, 9 insertions(+) >> >> diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst >> index 70981dd..0765a48 100644 >> --- a/Documentation/networking/devlink/devlink-info.rst >> +++ b/Documentation/networking/devlink/devlink-info.rst >> @@ -59,6 +59,12 @@ board.manufacture >> >> An identifier of the company or the facility which produced the part. >> >> +drv.spec >> +-------- >> + >> +Firmware interface specification version of the software interfaces between > > Why did you call this "drv" if the first sentence of the description > says it's a property of the firmware? > > Upcoming Intel patches call this "fw.mgmt.api". Please use that name, > it makes far more sense. > Yep, I think this is equivalent to "fw.mgmt.api" as well. If we want to make this a standard name, I'm fine with that, and we can update the ice driver to use the macro. Thanks, Jake >> +driver and firmware. This tag displays spec version implemented by driver. >> + >> fw >> -- >> >> diff --git a/include/net/devlink.h b/include/net/devlink.h >> index c9ca86b..9c4d889 100644 >> --- a/include/net/devlink.h >> +++ b/include/net/devlink.h >> @@ -476,6 +476,9 @@ enum devlink_param_generic_id { >> /* Revision of asic design */ >> #define DEVLINK_INFO_VERSION_GENERIC_ASIC_REV "asic.rev" >> >> +/* FW interface specification version implemented by driver */ >> +#define DEVLINK_INFO_VERSION_GENERIC_DRV_SPEC "drv.spec" >> + >> /* Overall FW version */ >> #define DEVLINK_INFO_VERSION_GENERIC_FW "fw" >> /* Control processor FW version */ >
On Tue, Mar 17, 2020 at 11:10 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 17 Mar 2020 20:44:38 +0530 Vasundhara Volam wrote: > > Add definition and documentation for the new generic info "drv.spec". > > "drv.spec" specifies the version of the software interfaces between > > driver and firmware. > > > > Cc: Jiri Pirko <jiri@mellanox.com> > > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> > > Signed-off-by: Michael Chan <michael.chan@broadcom.com> > > --- > > Documentation/networking/devlink/devlink-info.rst | 6 ++++++ > > include/net/devlink.h | 3 +++ > > 2 files changed, 9 insertions(+) > > > > diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst > > index 70981dd..0765a48 100644 > > --- a/Documentation/networking/devlink/devlink-info.rst > > +++ b/Documentation/networking/devlink/devlink-info.rst > > @@ -59,6 +59,12 @@ board.manufacture > > > > An identifier of the company or the facility which produced the part. > > > > +drv.spec > > +-------- > > + > > +Firmware interface specification version of the software interfaces between > > Why did you call this "drv" if the first sentence of the description > says it's a property of the firmware? Since it is a version of interface between driver and firmware. Both driver and firmware can support different versions. I intend to display the version implemented in the driver. I can probably add for both driver and firmware. Add it is as drv.spec and fw.spec. Thanks, Vasundhara > > Upcoming Intel patches call this "fw.mgmt.api". Please use that name, > it makes far more sense. > > > +driver and firmware. This tag displays spec version implemented by driver. > > + > > fw > > -- > > > > diff --git a/include/net/devlink.h b/include/net/devlink.h > > index c9ca86b..9c4d889 100644 > > --- a/include/net/devlink.h > > +++ b/include/net/devlink.h > > @@ -476,6 +476,9 @@ enum devlink_param_generic_id { > > /* Revision of asic design */ > > #define DEVLINK_INFO_VERSION_GENERIC_ASIC_REV "asic.rev" > > > > +/* FW interface specification version implemented by driver */ > > +#define DEVLINK_INFO_VERSION_GENERIC_DRV_SPEC "drv.spec" > > + > > /* Overall FW version */ > > #define DEVLINK_INFO_VERSION_GENERIC_FW "fw" > > /* Control processor FW version */ >
On Wed, 18 Mar 2020 09:51:29 +0530 Vasundhara Volam wrote: > On Tue, Mar 17, 2020 at 11:10 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 17 Mar 2020 20:44:38 +0530 Vasundhara Volam wrote: > > > Add definition and documentation for the new generic info "drv.spec". > > > "drv.spec" specifies the version of the software interfaces between > > > driver and firmware. > > > > > > Cc: Jiri Pirko <jiri@mellanox.com> > > > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> > > > Signed-off-by: Michael Chan <michael.chan@broadcom.com> > > > --- > > > Documentation/networking/devlink/devlink-info.rst | 6 ++++++ > > > include/net/devlink.h | 3 +++ > > > 2 files changed, 9 insertions(+) > > > > > > diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst > > > index 70981dd..0765a48 100644 > > > --- a/Documentation/networking/devlink/devlink-info.rst > > > +++ b/Documentation/networking/devlink/devlink-info.rst > > > @@ -59,6 +59,12 @@ board.manufacture > > > > > > An identifier of the company or the facility which produced the part. > > > > > > +drv.spec > > > +-------- > > > + > > > +Firmware interface specification version of the software interfaces between > > > > Why did you call this "drv" if the first sentence of the description > > says it's a property of the firmware? > > Since it is a version of interface between driver and firmware. Both > driver and firmware > can support different versions. I intend to display the version > implemented in the driver. We're just getting rid of driver versions, with significant effort, so starting to extend devlink info with driver stuff seems risky. How is driver information part of device info in the first place? As you said good driver and firmware will be modular and backward compatible, so what's the meaning of the API version? This field is meaningless. > I can probably add for both driver and firmware. Add it is as drv.spec > and fw.spec.
On 3/18/2020 1:04 PM, Jakub Kicinski wrote: > On Wed, 18 Mar 2020 09:51:29 +0530 Vasundhara Volam wrote: >> On Tue, Mar 17, 2020 at 11:10 PM Jakub Kicinski <kuba@kernel.org> wrote: >>> On Tue, 17 Mar 2020 20:44:38 +0530 Vasundhara Volam wrote: >>>> Add definition and documentation for the new generic info "drv.spec". >>>> "drv.spec" specifies the version of the software interfaces between >>>> driver and firmware. >>>> >>>> Cc: Jiri Pirko <jiri@mellanox.com> >>>> Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> >>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com> >>>> --- >>>> Documentation/networking/devlink/devlink-info.rst | 6 ++++++ >>>> include/net/devlink.h | 3 +++ >>>> 2 files changed, 9 insertions(+) >>>> >>>> diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst >>>> index 70981dd..0765a48 100644 >>>> --- a/Documentation/networking/devlink/devlink-info.rst >>>> +++ b/Documentation/networking/devlink/devlink-info.rst >>>> @@ -59,6 +59,12 @@ board.manufacture >>>> >>>> An identifier of the company or the facility which produced the part. >>>> >>>> +drv.spec >>>> +-------- >>>> + >>>> +Firmware interface specification version of the software interfaces between >>> >>> Why did you call this "drv" if the first sentence of the description >>> says it's a property of the firmware? >> >> Since it is a version of interface between driver and firmware. Both >> driver and firmware >> can support different versions. I intend to display the version >> implemented in the driver. > > We're just getting rid of driver versions, with significant effort, > so starting to extend devlink info with driver stuff seems risky. > How is driver information part of device info in the first place? > > As you said good driver and firmware will be modular and backward > compatible, so what's the meaning of the API version? > > This field is meaningless. > I think I agree with Jakub here. I assume, if it's anything like what the ice driver does, the firmware has an API field used to communicate to the driver what it can support. This can be used by the driver to decide if it can load. For example, if the major API number increases, the ice driver then assumes that it must be a very old driver which will not work at all with that firmware. (This is mostly kept as a safety hatch in case no other alternative can be determined). The driver can then use this API number as a way to decide if certain features can be enabled or not. I suppose printing the driver's "expected" API number makes sense, but I think the stronger approach is to make the driver able to interoperate with any previous API version. Newer minor API numbers only mean that new features exist which the driver might not be aware of. (for example, if you're running an old driver). The only reason to care would be in the case where a major breaking increase occurred. This really shouldn't be necessary, especially if the API between firmware and driver is designed well, but could be useful as a last ditch exit in case of some major breaking change that must be done. Even then, your driver *should* be able to tell and then behave differently based on this and do the old v1 or v<whatever> that it knows the firmware CAN support. In practice, I'm not sure how well this is actually done, as there is always some maintenance burden for carrying multiple variations of support, and in the case of a really poorly designed API.. it can be quite a nightmare. Thanks, Jake
On Wed, Mar 18, 2020 at 5:05 PM Jacob Keller <jacob.e.keller@intel.com> wrote: > On 3/18/2020 1:04 PM, Jakub Kicinski wrote: > > We're just getting rid of driver versions, with significant effort, > > so starting to extend devlink info with driver stuff seems risky. > > How is driver information part of device info in the first place? > > > > As you said good driver and firmware will be modular and backward > > compatible, so what's the meaning of the API version? > > > > This field is meaningless. > > > > I think I agree with Jakub here. I assume, if it's anything like what > the ice driver does, the firmware has an API field used to communicate > to the driver what it can support. This can be used by the driver to > decide if it can load. > > For example, if the major API number increases, the ice driver then > assumes that it must be a very old driver which will not work at all > with that firmware. (This is mostly kept as a safety hatch in case no > other alternative can be determined). > > The driver can then use this API number as a way to decide if certain > features can be enabled or not. > > I suppose printing the driver's "expected" API number makes sense, but I > think the stronger approach is to make the driver able to interoperate > with any previous API version. Newer minor API numbers only mean that > new features exist which the driver might not be aware of. (for example, > if you're running an old driver). > Agreed. Our driver is backward and forward compatible with all production firmware for the most part. The idea is that the effective API version number is the minimum of the driver's API and firmware's API. For example, if firmware is at v1.5 and driver is at v1.4, then the effective or operating API is v1.4. The new features after v1.4 are unused because the driver does not understand those new features. Similarly, a newer driver running on older firmware will have the older firmware's API as the effective API. The driver will not use the new features that the firmware doesn't understand. So if there is only one API version to report, reporting the min. makes the most sense to the user in our case. It is similar to a Gen4 PCIe card currently operating in a Gen3 slot. Thanks.
On Wed, 18 Mar 2020 17:47:26 -0700 Michael Chan wrote: > On Wed, Mar 18, 2020 at 5:05 PM Jacob Keller <jacob.e.keller@intel.com> wrote: > > On 3/18/2020 1:04 PM, Jakub Kicinski wrote: > > > We're just getting rid of driver versions, with significant effort, > > > so starting to extend devlink info with driver stuff seems risky. > > > How is driver information part of device info in the first place? > > > > > > As you said good driver and firmware will be modular and backward > > > compatible, so what's the meaning of the API version? > > > > > > This field is meaningless. > > > > I think I agree with Jakub here. I assume, if it's anything like what > > the ice driver does, the firmware has an API field used to communicate > > to the driver what it can support. This can be used by the driver to > > decide if it can load. > > > > For example, if the major API number increases, the ice driver then > > assumes that it must be a very old driver which will not work at all > > with that firmware. (This is mostly kept as a safety hatch in case no > > other alternative can be determined). > > > > The driver can then use this API number as a way to decide if certain > > features can be enabled or not. > > > > I suppose printing the driver's "expected" API number makes sense, but I > > think the stronger approach is to make the driver able to interoperate > > with any previous API version. Newer minor API numbers only mean that > > new features exist which the driver might not be aware of. (for example, > > if you're running an old driver). > > Agreed. Our driver is backward and forward compatible with all > production firmware for the most part. The idea is that the effective > API version number is the minimum of the driver's API and firmware's > API. For example, if firmware is at v1.5 and driver is at v1.4, then > the effective or operating API is v1.4. The new features after v1.4 > are unused because the driver does not understand those new features. > Similarly, a newer driver running on older firmware will have the > older firmware's API as the effective API. The driver will not use > the new features that the firmware doesn't understand. > > So if there is only one API version to report, reporting the min. > makes the most sense to the user in our case. It is similar to a Gen4 > PCIe card currently operating in a Gen3 slot. Sounds reasonable.
diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst index 70981dd..0765a48 100644 --- a/Documentation/networking/devlink/devlink-info.rst +++ b/Documentation/networking/devlink/devlink-info.rst @@ -59,6 +59,12 @@ board.manufacture An identifier of the company or the facility which produced the part. +drv.spec +-------- + +Firmware interface specification version of the software interfaces between +driver and firmware. This tag displays spec version implemented by driver. + fw -- diff --git a/include/net/devlink.h b/include/net/devlink.h index c9ca86b..9c4d889 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -476,6 +476,9 @@ enum devlink_param_generic_id { /* Revision of asic design */ #define DEVLINK_INFO_VERSION_GENERIC_ASIC_REV "asic.rev" +/* FW interface specification version implemented by driver */ +#define DEVLINK_INFO_VERSION_GENERIC_DRV_SPEC "drv.spec" + /* Overall FW version */ #define DEVLINK_INFO_VERSION_GENERIC_FW "fw" /* Control processor FW version */