Re: [PATCH net-next 01/11] devlink: add macro for "drv.spec"

[Jacob Keller <jacob.e.keller@intel.com>]

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