[PATCH net-next 0/2] devlink: add params FW_BANK and ENABLE_MIGRATION

[PATCH net-next 0/2] devlink: add params FW_BANK and ENABLE_MIGRATION

[Shannon Nelson]

Some discussions of a recent new driver RFC [1] suggested that these
new parameters would be a good addition to the generic devlink list.
If accepted, they will be used in the next version of the discussed
driver patchset.

[1] https://lore.kernel.org/netdev/20221118225656.48309-1-snelson@pensando.io/

Shannon Nelson (2):
  devlink: add fw bank select parameter
  devlink: add enable_migration parameter

 Documentation/networking/devlink/devlink-params.rst |  8 ++++++++
 include/net/devlink.h                               |  8 ++++++++
 net/core/devlink.c                                  | 10 ++++++++++
 3 files changed, 26 insertions(+)

-- 
2.17.1

[PATCH net-next 1/2] devlink: add fw bank select parameter

[Shannon Nelson]

Some devices have multiple memory banks that can be used to
hold various firmware versions that can be chosen for booting.
This can be used in addition to or along with the FW_LOAD_POLICY
parameter, depending on the capabilities of the particular
device.

This is a parameter suggested by Jake in
https://lore.kernel.org/netdev/CO1PR11MB508942BE965E63893DE9B86AD6129@CO1PR11MB5089.namprd11.prod.outlook.com/

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 Documentation/networking/devlink/devlink-params.rst | 4 ++++
 include/net/devlink.h                               | 4 ++++
 net/core/devlink.c                                  | 5 +++++
 3 files changed, 13 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
index 4e01dc32bc08..ed62c8a92f17 100644
--- a/Documentation/networking/devlink/devlink-params.rst
+++ b/Documentation/networking/devlink/devlink-params.rst
@@ -137,3 +137,7 @@ own name.
    * - ``event_eq_size``
      - u32
      - Control the size of asynchronous control events EQ.
+   * - ``fw_bank``
+     - u8
+     - In a multi-bank flash device, select the FW memory bank to be
+       loaded from on the next device boot/reset.
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 074a79b8933f..8a1430196980 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -510,6 +510,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_ENABLE_IWARP,
 	DEVLINK_PARAM_GENERIC_ID_IO_EQ_SIZE,
 	DEVLINK_PARAM_GENERIC_ID_EVENT_EQ_SIZE,
+	DEVLINK_PARAM_GENERIC_ID_FW_BANK,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -568,6 +569,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_NAME "event_eq_size"
 #define DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_TYPE DEVLINK_PARAM_TYPE_U32
 
+#define DEVLINK_PARAM_GENERIC_FW_BANK_NAME "fw_bank"
+#define DEVLINK_PARAM_GENERIC_FW_BANK_TYPE DEVLINK_PARAM_TYPE_U8
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 0e10a8a68c5e..6872d678be5b 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -5231,6 +5231,11 @@ static const struct devlink_param devlink_param_generic[] = {
 		.name = DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_NAME,
 		.type = DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_FW_BANK,
+		.name = DEVLINK_PARAM_GENERIC_FW_BANK_NAME,
+		.type = DEVLINK_PARAM_GENERIC_FW_BANK_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
2.17.1

[PATCH net-next 2/2] devlink: add enable_migration parameter

[Shannon Nelson]

To go along with existing enable_eth, enable_roce,
enable_vnet, etc., we add an enable_migration parameter.

This follows from the discussion of this RFC patch
https://lore.kernel.org/netdev/20221118225656.48309-11-snelson@pensando.io/

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 Documentation/networking/devlink/devlink-params.rst | 4 ++++
 include/net/devlink.h                               | 4 ++++
 net/core/devlink.c                                  | 5 +++++
 3 files changed, 13 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
index ed62c8a92f17..c56caad32a7c 100644
--- a/Documentation/networking/devlink/devlink-params.rst
+++ b/Documentation/networking/devlink/devlink-params.rst
@@ -141,3 +141,7 @@ own name.
      - u8
      - In a multi-bank flash device, select the FW memory bank to be
        loaded from on the next device boot/reset.
+   * - ``enable_migration``
+     - Boolean
+     - When enabled, the device driver will instantiate a live migration
+       specific auxiliary device of the devlink device.
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 8a1430196980..1d35056a558d 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -511,6 +511,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_IO_EQ_SIZE,
 	DEVLINK_PARAM_GENERIC_ID_EVENT_EQ_SIZE,
 	DEVLINK_PARAM_GENERIC_ID_FW_BANK,
+	DEVLINK_PARAM_GENERIC_ID_ENABLE_MIGRATION,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -572,6 +573,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_FW_BANK_NAME "fw_bank"
 #define DEVLINK_PARAM_GENERIC_FW_BANK_TYPE DEVLINK_PARAM_TYPE_U8
 
+#define DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_NAME "enable_migration"
+#define DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_TYPE DEVLINK_PARAM_TYPE_BOOL
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6872d678be5b..0e32a4fe7a66 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -5236,6 +5236,11 @@ static const struct devlink_param devlink_param_generic[] = {
 		.name = DEVLINK_PARAM_GENERIC_FW_BANK_NAME,
 		.type = DEVLINK_PARAM_GENERIC_FW_BANK_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_ENABLE_MIGRATION,
+		.name = DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_NAME,
+		.type = DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
2.17.1

Re: [PATCH net-next 0/2] devlink: add params FW_BANK and ENABLE_MIGRATION

[Leon Romanovsky]

On Mon, Dec 05, 2022 at 09:26:25AM -0800, Shannon Nelson wrote:
> Some discussions of a recent new driver RFC [1] suggested that these
> new parameters would be a good addition to the generic devlink list.
> If accepted, they will be used in the next version of the discussed
> driver patchset.
> 
> [1] https://lore.kernel.org/netdev/20221118225656.48309-1-snelson@pensando.io/
> 
> Shannon Nelson (2):
>   devlink: add fw bank select parameter
>   devlink: add enable_migration parameter

You was CCed on this more mature version, but didn't express any opinion.
https://lore.kernel.org/netdev/20221204141632.201932-8-shayd@nvidia.com/

Thanks

> 
>  Documentation/networking/devlink/devlink-params.rst |  8 ++++++++
>  include/net/devlink.h                               |  8 ++++++++
>  net/core/devlink.c                                  | 10 ++++++++++
>  3 files changed, 26 insertions(+)
> 
> -- 
> 2.17.1
> 

Re: [PATCH net-next 0/2] devlink: add params FW_BANK and ENABLE_MIGRATION

[Shannon Nelson]

On 12/5/22 10:22 AM, Leon Romanovsky wrote:
> On Mon, Dec 05, 2022 at 09:26:25AM -0800, Shannon Nelson wrote:
>> Some discussions of a recent new driver RFC [1] suggested that these
>> new parameters would be a good addition to the generic devlink list.
>> If accepted, they will be used in the next version of the discussed
>> driver patchset.
>>
>> [1] https://lore.kernel.org/netdev/20221118225656.48309-1-snelson@pensando.io/
>>
>> Shannon Nelson (2):
>>    devlink: add fw bank select parameter
>>    devlink: add enable_migration parameter
> 
> You was CCed on this more mature version, but didn't express any opinion.
> https://lore.kernel.org/netdev/20221204141632.201932-8-shayd@nvidia.com/

Yes, and thank you for that Cc.  I wanted to get my follow-up work done 
and sent before I finished thinking about that patch.  I expect to have 
a chance later today.

Basically, this follows the existing example for enabling a feature in 
the primary device, whether or not additional ports are involved, while 
Shay's patch enables a feature for a specific port.  I think there's 
room for both answers.

sln

Re: [PATCH net-next 0/2] devlink: add params FW_BANK and ENABLE_MIGRATION

[Leon Romanovsky]

On Mon, Dec 05, 2022 at 10:55:16AM -0800, Shannon Nelson wrote:
> On 12/5/22 10:22 AM, Leon Romanovsky wrote:
> > On Mon, Dec 05, 2022 at 09:26:25AM -0800, Shannon Nelson wrote:
> > > Some discussions of a recent new driver RFC [1] suggested that these
> > > new parameters would be a good addition to the generic devlink list.
> > > If accepted, they will be used in the next version of the discussed
> > > driver patchset.
> > > 
> > > [1] https://lore.kernel.org/netdev/20221118225656.48309-1-snelson@pensando.io/
> > > 
> > > Shannon Nelson (2):
> > >    devlink: add fw bank select parameter
> > >    devlink: add enable_migration parameter
> > 
> > You was CCed on this more mature version, but didn't express any opinion.
> > https://lore.kernel.org/netdev/20221204141632.201932-8-shayd@nvidia.com/
> 
> Yes, and thank you for that Cc.  I wanted to get my follow-up work done and
> sent before I finished thinking about that patch.  I expect to have a chance
> later today.
> 
> Basically, this follows the existing example for enabling a feature in the
> primary device, whether or not additional ports are involved, while Shay's
> patch enables a feature for a specific port.  I think there's room for both
> answers.

I suggest to continue this discussion in Shay's series.

Thanks

> 
> sln

Re: [PATCH net-next 0/2] devlink: add params FW_BANK and ENABLE_MIGRATION

[Jiri Pirko]

Mon, Dec 05, 2022 at 06:26:25PM CET, shannon.nelson@amd.com wrote:
>Some discussions of a recent new driver RFC [1] suggested that these
>new parameters would be a good addition to the generic devlink list.
>If accepted, they will be used in the next version of the discussed
>driver patchset.
>
>[1] https://lore.kernel.org/netdev/20221118225656.48309-1-snelson@pensando.io/
>
>Shannon Nelson (2):
>  devlink: add fw bank select parameter
>  devlink: add enable_migration parameter

Where's the user? You need to introduce it alongside in this patchset.

>
> Documentation/networking/devlink/devlink-params.rst |  8 ++++++++
> include/net/devlink.h                               |  8 ++++++++
> net/core/devlink.c                                  | 10 ++++++++++
> 3 files changed, 26 insertions(+)
>
>-- 
>2.17.1
>

Re: [PATCH net-next 2/2] devlink: add enable_migration parameter

[Jiri Pirko]

Mon, Dec 05, 2022 at 06:26:27PM CET, shannon.nelson@amd.com wrote:
>To go along with existing enable_eth, enable_roce,
>enable_vnet, etc., we add an enable_migration parameter.

In the patch description, you should be alwyas imperative to the
codebase. Tell it what to do, don't describe what you (plural) do :)


>
>This follows from the discussion of this RFC patch
>https://lore.kernel.org/netdev/20221118225656.48309-11-snelson@pensando.io/
>
>Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>---
> Documentation/networking/devlink/devlink-params.rst | 4 ++++
> include/net/devlink.h                               | 4 ++++
> net/core/devlink.c                                  | 5 +++++
> 3 files changed, 13 insertions(+)
>
>diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>index ed62c8a92f17..c56caad32a7c 100644
>--- a/Documentation/networking/devlink/devlink-params.rst
>+++ b/Documentation/networking/devlink/devlink-params.rst
>@@ -141,3 +141,7 @@ own name.
>      - u8
>      - In a multi-bank flash device, select the FW memory bank to be
>        loaded from on the next device boot/reset.
>+   * - ``enable_migration``
>+     - Boolean
>+     - When enabled, the device driver will instantiate a live migration
>+       specific auxiliary device of the devlink device.

Devlink has not notion of auxdev. Use objects and terms relevant to
devlink please.

I don't really understand what is the semantics of this param at all.


>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 8a1430196980..1d35056a558d 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -511,6 +511,7 @@ enum devlink_param_generic_id {
> 	DEVLINK_PARAM_GENERIC_ID_IO_EQ_SIZE,
> 	DEVLINK_PARAM_GENERIC_ID_EVENT_EQ_SIZE,
> 	DEVLINK_PARAM_GENERIC_ID_FW_BANK,
>+	DEVLINK_PARAM_GENERIC_ID_ENABLE_MIGRATION,
> 
> 	/* add new param generic ids above here*/
> 	__DEVLINK_PARAM_GENERIC_ID_MAX,
>@@ -572,6 +573,9 @@ enum devlink_param_generic_id {
> #define DEVLINK_PARAM_GENERIC_FW_BANK_NAME "fw_bank"
> #define DEVLINK_PARAM_GENERIC_FW_BANK_TYPE DEVLINK_PARAM_TYPE_U8
> 
>+#define DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_NAME "enable_migration"
>+#define DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_TYPE DEVLINK_PARAM_TYPE_BOOL
>+
> #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
> {									\
> 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 6872d678be5b..0e32a4fe7a66 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -5236,6 +5236,11 @@ static const struct devlink_param devlink_param_generic[] = {
> 		.name = DEVLINK_PARAM_GENERIC_FW_BANK_NAME,
> 		.type = DEVLINK_PARAM_GENERIC_FW_BANK_TYPE,
> 	},
>+	{
>+		.id = DEVLINK_PARAM_GENERIC_ID_ENABLE_MIGRATION,
>+		.name = DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_NAME,
>+		.type = DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_TYPE,
>+	},
> };
> 
> static int devlink_param_generic_verify(const struct devlink_param *param)
>-- 
>2.17.1
>

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Jiri Pirko]

Mon, Dec 05, 2022 at 06:26:26PM CET, shannon.nelson@amd.com wrote:
>Some devices have multiple memory banks that can be used to
>hold various firmware versions that can be chosen for booting.
>This can be used in addition to or along with the FW_LOAD_POLICY
>parameter, depending on the capabilities of the particular
>device.
>
>This is a parameter suggested by Jake in
>https://lore.kernel.org/netdev/CO1PR11MB508942BE965E63893DE9B86AD6129@CO1PR11MB5089.namprd11.prod.outlook.com/
>
>Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>---
> Documentation/networking/devlink/devlink-params.rst | 4 ++++
> include/net/devlink.h                               | 4 ++++
> net/core/devlink.c                                  | 5 +++++
> 3 files changed, 13 insertions(+)
>
>diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>index 4e01dc32bc08..ed62c8a92f17 100644
>--- a/Documentation/networking/devlink/devlink-params.rst
>+++ b/Documentation/networking/devlink/devlink-params.rst
>@@ -137,3 +137,7 @@ own name.
>    * - ``event_eq_size``
>      - u32
>      - Control the size of asynchronous control events EQ.
>+   * - ``fw_bank``
>+     - u8
>+     - In a multi-bank flash device, select the FW memory bank to be
>+       loaded from on the next device boot/reset.

Just the next one or any in the future? Please define this precisely.


>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 074a79b8933f..8a1430196980 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -510,6 +510,7 @@ enum devlink_param_generic_id {
> 	DEVLINK_PARAM_GENERIC_ID_ENABLE_IWARP,
> 	DEVLINK_PARAM_GENERIC_ID_IO_EQ_SIZE,
> 	DEVLINK_PARAM_GENERIC_ID_EVENT_EQ_SIZE,
>+	DEVLINK_PARAM_GENERIC_ID_FW_BANK,
> 
> 	/* add new param generic ids above here*/
> 	__DEVLINK_PARAM_GENERIC_ID_MAX,
>@@ -568,6 +569,9 @@ enum devlink_param_generic_id {
> #define DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_NAME "event_eq_size"
> #define DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_TYPE DEVLINK_PARAM_TYPE_U32
> 
>+#define DEVLINK_PARAM_GENERIC_FW_BANK_NAME "fw_bank"
>+#define DEVLINK_PARAM_GENERIC_FW_BANK_TYPE DEVLINK_PARAM_TYPE_U8
>+
> #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
> {									\
> 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 0e10a8a68c5e..6872d678be5b 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -5231,6 +5231,11 @@ static const struct devlink_param devlink_param_generic[] = {
> 		.name = DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_NAME,
> 		.type = DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_TYPE,
> 	},
>+	{
>+		.id = DEVLINK_PARAM_GENERIC_ID_FW_BANK,
>+		.name = DEVLINK_PARAM_GENERIC_FW_BANK_NAME,
>+		.type = DEVLINK_PARAM_GENERIC_FW_BANK_TYPE,
>+	},
> };
> 
> static int devlink_param_generic_verify(const struct devlink_param *param)
>-- 
>2.17.1
>

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Shannon Nelson]

On 12/6/22 1:07 AM, Jiri Pirko wrote:
> Mon, Dec 05, 2022 at 06:26:26PM CET, shannon.nelson@amd.com wrote:
>> Some devices have multiple memory banks that can be used to
>> hold various firmware versions that can be chosen for booting.
>> This can be used in addition to or along with the FW_LOAD_POLICY
>> parameter, depending on the capabilities of the particular
>> device.
>>
>> This is a parameter suggested by Jake in
>> https://lore.kernel.org/netdev/CO1PR11MB508942BE965E63893DE9B86AD6129@CO1PR11MB5089.namprd11.prod.outlook.com/
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>> Documentation/networking/devlink/devlink-params.rst | 4 ++++
>> include/net/devlink.h                               | 4 ++++
>> net/core/devlink.c                                  | 5 +++++
>> 3 files changed, 13 insertions(+)
>>
>> diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>> index 4e01dc32bc08..ed62c8a92f17 100644
>> --- a/Documentation/networking/devlink/devlink-params.rst
>> +++ b/Documentation/networking/devlink/devlink-params.rst
>> @@ -137,3 +137,7 @@ own name.
>>     * - ``event_eq_size``
>>       - u32
>>       - Control the size of asynchronous control events EQ.
>> +   * - ``fw_bank``
>> +     - u8
>> +     - In a multi-bank flash device, select the FW memory bank to be
>> +       loaded from on the next device boot/reset.
> 
> Just the next one or any in the future? Please define this precisely.

I suspect it will depend upon the actual device that uses this.  In our 
case, all future resets until changed again by this or by a devlink dev 
flash command.  I'll tweak the wording a bit to something like
     "... to be loaded from on future device boot/resets."

> 
> 
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index 074a79b8933f..8a1430196980 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -510,6 +510,7 @@ enum devlink_param_generic_id {
>>        DEVLINK_PARAM_GENERIC_ID_ENABLE_IWARP,
>>        DEVLINK_PARAM_GENERIC_ID_IO_EQ_SIZE,
>>        DEVLINK_PARAM_GENERIC_ID_EVENT_EQ_SIZE,
>> +      DEVLINK_PARAM_GENERIC_ID_FW_BANK,
>>
>>        /* add new param generic ids above here*/
>>        __DEVLINK_PARAM_GENERIC_ID_MAX,
>> @@ -568,6 +569,9 @@ enum devlink_param_generic_id {
>> #define DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_NAME "event_eq_size"
>> #define DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_TYPE DEVLINK_PARAM_TYPE_U32
>>
>> +#define DEVLINK_PARAM_GENERIC_FW_BANK_NAME "fw_bank"
>> +#define DEVLINK_PARAM_GENERIC_FW_BANK_TYPE DEVLINK_PARAM_TYPE_U8
>> +
>> #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)    \
>> {                                                                     \
>>        .id = DEVLINK_PARAM_GENERIC_ID_##_id,                           \
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 0e10a8a68c5e..6872d678be5b 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -5231,6 +5231,11 @@ static const struct devlink_param devlink_param_generic[] = {
>>                .name = DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_NAME,
>>                .type = DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_TYPE,
>>        },
>> +      {
>> +              .id = DEVLINK_PARAM_GENERIC_ID_FW_BANK,
>> +              .name = DEVLINK_PARAM_GENERIC_FW_BANK_NAME,
>> +              .type = DEVLINK_PARAM_GENERIC_FW_BANK_TYPE,
>> +      },
>> };
>>
>> static int devlink_param_generic_verify(const struct devlink_param *param)
>> --
>> 2.17.1
>>

Re: [PATCH net-next 0/2] devlink: add params FW_BANK and ENABLE_MIGRATION

[Shannon Nelson]

On 12/6/22 1:00 AM, Jiri Pirko wrote:
> Mon, Dec 05, 2022 at 06:26:25PM CET, shannon.nelson@amd.com wrote:
>> Some discussions of a recent new driver RFC [1] suggested that these
>> new parameters would be a good addition to the generic devlink list.
>> If accepted, they will be used in the next version of the discussed
>> driver patchset.
>>
>> [1] https://lore.kernel.org/netdev/20221118225656.48309-11-snelson@pensando.io/
>>
>> Shannon Nelson (2):
>>   devlink: add fw bank select parameter
>>   devlink: add enable_migration parameter
> 
> Where's the user? You need to introduce it alongside in this patchset.

I'll put them at the beginning of the next version of the pds_core patchset.


> 
>>
>> Documentation/networking/devlink/devlink-params.rst |  8 ++++++++
>> include/net/devlink.h                               |  8 ++++++++
>> net/core/devlink.c                                  | 10 ++++++++++
>> 3 files changed, 26 insertions(+)
>>
>> --
>> 2.17.1
>>

Re: [PATCH net-next 2/2] devlink: add enable_migration parameter

[Shannon Nelson]

On 12/6/22 1:04 AM, Jiri Pirko wrote:
> Mon, Dec 05, 2022 at 06:26:27PM CET, shannon.nelson@amd.com wrote:
>> To go along with existing enable_eth, enable_roce,
>> enable_vnet, etc., we add an enable_migration parameter.
> 
> In the patch description, you should be alwyas imperative to the
> codebase. Tell it what to do, don't describe what you (plural) do :)

This will be better described when rolled up in the pds_core patchset.

> 
> 
>>
>> This follows from the discussion of this RFC patch
>> https://lore.kernel.org/netdev/20221118225656.48309-11-snelson@pensando.io/
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>> Documentation/networking/devlink/devlink-params.rst | 4 ++++
>> include/net/devlink.h                               | 4 ++++
>> net/core/devlink.c                                  | 5 +++++
>> 3 files changed, 13 insertions(+)
>>
>> diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>> index ed62c8a92f17..c56caad32a7c 100644
>> --- a/Documentation/networking/devlink/devlink-params.rst
>> +++ b/Documentation/networking/devlink/devlink-params.rst
>> @@ -141,3 +141,7 @@ own name.
>>       - u8
>>       - In a multi-bank flash device, select the FW memory bank to be
>>         loaded from on the next device boot/reset.
>> +   * - ``enable_migration``
>> +     - Boolean
>> +     - When enabled, the device driver will instantiate a live migration
>> +       specific auxiliary device of the devlink device.
> 
> Devlink has not notion of auxdev. Use objects and terms relevant to
> devlink please.
> 
> I don't really understand what is the semantics of this param at all.

Perhaps we need to update the existing descriptions for enable_eth, 
enable_vnet, etc, as well?  Probably none of them should mention the aux 
device, tho' I know they all came in together after a long discussion.

I'll work this to be more generic to the result and not the underlying 
specifics of how.

sln

> 
> 
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index 8a1430196980..1d35056a558d 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -511,6 +511,7 @@ enum devlink_param_generic_id {
>>        DEVLINK_PARAM_GENERIC_ID_IO_EQ_SIZE,
>>        DEVLINK_PARAM_GENERIC_ID_EVENT_EQ_SIZE,
>>        DEVLINK_PARAM_GENERIC_ID_FW_BANK,
>> +      DEVLINK_PARAM_GENERIC_ID_ENABLE_MIGRATION,
>>
>>        /* add new param generic ids above here*/
>>        __DEVLINK_PARAM_GENERIC_ID_MAX,
>> @@ -572,6 +573,9 @@ enum devlink_param_generic_id {
>> #define DEVLINK_PARAM_GENERIC_FW_BANK_NAME "fw_bank"
>> #define DEVLINK_PARAM_GENERIC_FW_BANK_TYPE DEVLINK_PARAM_TYPE_U8
>>
>> +#define DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_NAME "enable_migration"
>> +#define DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_TYPE DEVLINK_PARAM_TYPE_BOOL
>> +
>> #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)    \
>> {                                                                     \
>>        .id = DEVLINK_PARAM_GENERIC_ID_##_id,                           \
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 6872d678be5b..0e32a4fe7a66 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -5236,6 +5236,11 @@ static const struct devlink_param devlink_param_generic[] = {
>>                .name = DEVLINK_PARAM_GENERIC_FW_BANK_NAME,
>>                .type = DEVLINK_PARAM_GENERIC_FW_BANK_TYPE,
>>        },
>> +      {
>> +              .id = DEVLINK_PARAM_GENERIC_ID_ENABLE_MIGRATION,
>> +              .name = DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_NAME,
>> +              .type = DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_TYPE,
>> +      },
>> };
>>
>> static int devlink_param_generic_verify(const struct devlink_param *param)
>> --
>> 2.17.1
>>

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Jakub Kicinski]

On Mon, 5 Dec 2022 09:26:26 -0800 Shannon Nelson wrote:
> Some devices have multiple memory banks that can be used to
> hold various firmware versions that can be chosen for booting.
> This can be used in addition to or along with the FW_LOAD_POLICY
> parameter, depending on the capabilities of the particular
> device.
> 
> This is a parameter suggested by Jake in
> https://lore.kernel.org/netdev/CO1PR11MB508942BE965E63893DE9B86AD6129@CO1PR11MB5089.namprd11.prod.outlook.com/

Can we make this netlink attributes? 

What is the flow that you have in mind end to end (user actions)?
I think we should document that, by which I mean extend the pseudo 
code here:

https://docs.kernel.org/next/networking/devlink/devlink-flash.html#firmware-version-management

I expect we need to define the behavior such that the user can ignore
the banks by default and get the right behavior.

Let's define
 - current bank - the bank from which the currently running image has
   been loaded
 - active bank - the bank selected for next boot
 - next bank - current bank + 1 mod count

If we want to keep backward compat - if no bank specified for flashing:
 - we flash to "next bank"
 - if flashing is successful we switch "active bank" to "next bank"
not that multiple flashing operations without activation/reboot will
result in overwriting the same "next bank" preventing us from flashing
multiple banks without trying if they work..

"stored" versions in devlink info display the versions for "active bank"
while running display running (i.e. in RAM, not in the banks!)

In terms of modifications to the algo in documentation:
 - the check for "stored" versions check should be changed to an while
   loop that iterates over all banks
 - flashing can actually depend on the defaults as described above so
   no change

We can expose the "current" and "active" bank as netlink attrs in dev
info.

Re: [PATCH net-next 0/2] devlink: add params FW_BANK and ENABLE_MIGRATION

[Jiri Pirko]

Tue, Dec 06, 2022 at 07:21:24PM CET, shannon.nelson@amd.com wrote:
>On 12/6/22 1:00 AM, Jiri Pirko wrote:
>> Mon, Dec 05, 2022 at 06:26:25PM CET, shannon.nelson@amd.com wrote:
>> > Some discussions of a recent new driver RFC [1] suggested that these
>> > new parameters would be a good addition to the generic devlink list.
>> > If accepted, they will be used in the next version of the discussed
>> > driver patchset.
>> > 
>> > [1] https://lore.kernel.org/netdev/20221118225656.48309-11-snelson@pensando.io/
>> > 
>> > Shannon Nelson (2):
>> >   devlink: add fw bank select parameter
>> >   devlink: add enable_migration parameter
>> 
>> Where's the user? You need to introduce it alongside in this patchset.
>
>I'll put them at the beginning of the next version of the pds_core patchset.

You need to do it in a single patchset, if possible. Here, I believe it
is possible easily.


>
>
>> 
>> > 
>> > Documentation/networking/devlink/devlink-params.rst |  8 ++++++++
>> > include/net/devlink.h                               |  8 ++++++++
>> > net/core/devlink.c                                  | 10 ++++++++++
>> > 3 files changed, 26 insertions(+)
>> > 
>> > --
>> > 2.17.1
>> > 

Re: [PATCH net-next 2/2] devlink: add enable_migration parameter

[Jiri Pirko]

Tue, Dec 06, 2022 at 07:28:33PM CET, shannon.nelson@amd.com wrote:
>On 12/6/22 1:04 AM, Jiri Pirko wrote:
>> Mon, Dec 05, 2022 at 06:26:27PM CET, shannon.nelson@amd.com wrote:
>> > To go along with existing enable_eth, enable_roce,
>> > enable_vnet, etc., we add an enable_migration parameter.
>> 
>> In the patch description, you should be alwyas imperative to the
>> codebase. Tell it what to do, don't describe what you (plural) do :)
>
>This will be better described when rolled up in the pds_core patchset.
>
>> 
>> 
>> > 
>> > This follows from the discussion of this RFC patch
>> > https://lore.kernel.org/netdev/20221118225656.48309-11-snelson@pensando.io/
>> > 
>> > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> > ---
>> > Documentation/networking/devlink/devlink-params.rst | 4 ++++
>> > include/net/devlink.h                               | 4 ++++
>> > net/core/devlink.c                                  | 5 +++++
>> > 3 files changed, 13 insertions(+)
>> > 
>> > diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>> > index ed62c8a92f17..c56caad32a7c 100644
>> > --- a/Documentation/networking/devlink/devlink-params.rst
>> > +++ b/Documentation/networking/devlink/devlink-params.rst
>> > @@ -141,3 +141,7 @@ own name.
>> >       - u8
>> >       - In a multi-bank flash device, select the FW memory bank to be
>> >         loaded from on the next device boot/reset.
>> > +   * - ``enable_migration``
>> > +     - Boolean
>> > +     - When enabled, the device driver will instantiate a live migration
>> > +       specific auxiliary device of the devlink device.
>> 
>> Devlink has not notion of auxdev. Use objects and terms relevant to
>> devlink please.
>> 
>> I don't really understand what is the semantics of this param at all.
>
>Perhaps we need to update the existing descriptions for enable_eth,
>enable_vnet, etc, as well?  Probably none of them should mention the aux
>device, tho' I know they all came in together after a long discussion.

Yep, I think so.


>
>I'll work this to be more generic to the result and not the underlying
>specifics of how.

Thanks!

>
>sln
>
>> 
>> 
>> > diff --git a/include/net/devlink.h b/include/net/devlink.h
>> > index 8a1430196980..1d35056a558d 100644
>> > --- a/include/net/devlink.h
>> > +++ b/include/net/devlink.h
>> > @@ -511,6 +511,7 @@ enum devlink_param_generic_id {
>> >        DEVLINK_PARAM_GENERIC_ID_IO_EQ_SIZE,
>> >        DEVLINK_PARAM_GENERIC_ID_EVENT_EQ_SIZE,
>> >        DEVLINK_PARAM_GENERIC_ID_FW_BANK,
>> > +      DEVLINK_PARAM_GENERIC_ID_ENABLE_MIGRATION,
>> > 
>> >        /* add new param generic ids above here*/
>> >        __DEVLINK_PARAM_GENERIC_ID_MAX,
>> > @@ -572,6 +573,9 @@ enum devlink_param_generic_id {
>> > #define DEVLINK_PARAM_GENERIC_FW_BANK_NAME "fw_bank"
>> > #define DEVLINK_PARAM_GENERIC_FW_BANK_TYPE DEVLINK_PARAM_TYPE_U8
>> > 
>> > +#define DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_NAME "enable_migration"
>> > +#define DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_TYPE DEVLINK_PARAM_TYPE_BOOL
>> > +
>> > #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)    \
>> > {                                                                     \
>> >        .id = DEVLINK_PARAM_GENERIC_ID_##_id,                           \
>> > diff --git a/net/core/devlink.c b/net/core/devlink.c
>> > index 6872d678be5b..0e32a4fe7a66 100644
>> > --- a/net/core/devlink.c
>> > +++ b/net/core/devlink.c
>> > @@ -5236,6 +5236,11 @@ static const struct devlink_param devlink_param_generic[] = {
>> >                .name = DEVLINK_PARAM_GENERIC_FW_BANK_NAME,
>> >                .type = DEVLINK_PARAM_GENERIC_FW_BANK_TYPE,
>> >        },
>> > +      {
>> > +              .id = DEVLINK_PARAM_GENERIC_ID_ENABLE_MIGRATION,
>> > +              .name = DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_NAME,
>> > +              .type = DEVLINK_PARAM_GENERIC_ENABLE_MIGRATION_TYPE,
>> > +      },
>> > };
>> > 
>> > static int devlink_param_generic_verify(const struct devlink_param *param)
>> > --
>> > 2.17.1
>> > 

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Jiri Pirko]

Tue, Dec 06, 2022 at 07:18:00PM CET, shannon.nelson@amd.com wrote:
>On 12/6/22 1:07 AM, Jiri Pirko wrote:
>> Mon, Dec 05, 2022 at 06:26:26PM CET, shannon.nelson@amd.com wrote:
>> > Some devices have multiple memory banks that can be used to
>> > hold various firmware versions that can be chosen for booting.
>> > This can be used in addition to or along with the FW_LOAD_POLICY
>> > parameter, depending on the capabilities of the particular
>> > device.
>> > 
>> > This is a parameter suggested by Jake in
>> > https://lore.kernel.org/netdev/CO1PR11MB508942BE965E63893DE9B86AD6129@CO1PR11MB5089.namprd11.prod.outlook.com/
>> > 
>> > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> > ---
>> > Documentation/networking/devlink/devlink-params.rst | 4 ++++
>> > include/net/devlink.h                               | 4 ++++
>> > net/core/devlink.c                                  | 5 +++++
>> > 3 files changed, 13 insertions(+)
>> > 
>> > diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>> > index 4e01dc32bc08..ed62c8a92f17 100644
>> > --- a/Documentation/networking/devlink/devlink-params.rst
>> > +++ b/Documentation/networking/devlink/devlink-params.rst
>> > @@ -137,3 +137,7 @@ own name.
>> >     * - ``event_eq_size``
>> >       - u32
>> >       - Control the size of asynchronous control events EQ.
>> > +   * - ``fw_bank``
>> > +     - u8
>> > +     - In a multi-bank flash device, select the FW memory bank to be
>> > +       loaded from on the next device boot/reset.
>> 
>> Just the next one or any in the future? Please define this precisely.
>
>I suspect it will depend upon the actual device that uses this.  In our case,

It should not. The behaviour should be predictable for the user.


>all future resets until changed again by this or by a devlink dev flash
>command.  I'll tweak the wording a bit to something like
>    "... to be loaded from on future device boot/resets."
>
>> 
>> 
>> > diff --git a/include/net/devlink.h b/include/net/devlink.h
>> > index 074a79b8933f..8a1430196980 100644
>> > --- a/include/net/devlink.h
>> > +++ b/include/net/devlink.h
>> > @@ -510,6 +510,7 @@ enum devlink_param_generic_id {
>> >        DEVLINK_PARAM_GENERIC_ID_ENABLE_IWARP,
>> >        DEVLINK_PARAM_GENERIC_ID_IO_EQ_SIZE,
>> >        DEVLINK_PARAM_GENERIC_ID_EVENT_EQ_SIZE,
>> > +      DEVLINK_PARAM_GENERIC_ID_FW_BANK,
>> > 
>> >        /* add new param generic ids above here*/
>> >        __DEVLINK_PARAM_GENERIC_ID_MAX,
>> > @@ -568,6 +569,9 @@ enum devlink_param_generic_id {
>> > #define DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_NAME "event_eq_size"
>> > #define DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_TYPE DEVLINK_PARAM_TYPE_U32
>> > 
>> > +#define DEVLINK_PARAM_GENERIC_FW_BANK_NAME "fw_bank"
>> > +#define DEVLINK_PARAM_GENERIC_FW_BANK_TYPE DEVLINK_PARAM_TYPE_U8
>> > +
>> > #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)    \
>> > {                                                                     \
>> >        .id = DEVLINK_PARAM_GENERIC_ID_##_id,                           \
>> > diff --git a/net/core/devlink.c b/net/core/devlink.c
>> > index 0e10a8a68c5e..6872d678be5b 100644
>> > --- a/net/core/devlink.c
>> > +++ b/net/core/devlink.c
>> > @@ -5231,6 +5231,11 @@ static const struct devlink_param devlink_param_generic[] = {
>> >                .name = DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_NAME,
>> >                .type = DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_TYPE,
>> >        },
>> > +      {
>> > +              .id = DEVLINK_PARAM_GENERIC_ID_FW_BANK,
>> > +              .name = DEVLINK_PARAM_GENERIC_FW_BANK_NAME,
>> > +              .type = DEVLINK_PARAM_GENERIC_FW_BANK_TYPE,
>> > +      },
>> > };
>> > 
>> > static int devlink_param_generic_verify(const struct devlink_param *param)
>> > --
>> > 2.17.1
>> > 

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Shannon Nelson]

On 12/6/22 5:41 PM, Jakub Kicinski wrote:
 > On Mon, 5 Dec 2022 09:26:26 -0800 Shannon Nelson wrote:
 >> Some devices have multiple memory banks that can be used to
 >> hold various firmware versions that can be chosen for booting.
 >> This can be used in addition to or along with the FW_LOAD_POLICY
 >> parameter, depending on the capabilities of the particular
 >> device.
 >>
 >> This is a parameter suggested by Jake in
 >> 
https://lore.kernel.org/netdev/CO1PR11MB508942BE965E63893DE9B86AD6129@CO1PR11MB5089.namprd11.prod.outlook.com/
 >
 > Can we make this netlink attributes?
To be sure, you are talking about defining new values in enum 
devlink_attr, right?  Perhaps something like
     DEVLINK_ATTR_INFO_VERSION_BANK   /* u32 */
to go along with _VERSION_NAME and _VERSION_VALUE for each item under 
running and stored?

Does u32 make sense here or should it be a string?

Or do we really need another value here, perhaps we should use the 
existing _VERSION_NAME to display the bank?  This is what is essentially 
happening in the current ionic and this proposed pds_core output, but 
without the concept of bank numbers:
       running:
         fw 1.58.0-6
       stored:
         fw.goldfw 1.51.0-3
         fw.mainfwa 1.58.0-6
         fw.mainfwb 1.56.0-47-24-g651edb94cbe

With (optional?) bank numbers, it might look like
       running:
         1 fw 1.58.0-6
       stored:
         0 fw.goldfw 1.51.0-3
         1 fw.mainfwa 1.58.0-6
         2 fw.mainfwb 1.56.0-47-24-g651edb94cbe

Is this reasonable?

 >
 > What is the flow that you have in mind end to end (user actions)?
 > I think we should document that, by which I mean extend the pseudo
 > code here:
 >
 > 
https://docs.kernel.org/next/networking/devlink/devlink-flash.html#firmware-version-management
 >
 > I expect we need to define the behavior such that the user can ignore
 > the banks by default and get the right behavior.
 >
 > Let's define
 >   - current bank - the bank from which the currently running image has
 >     been loaded
I'm not sure this is any more information than what we already have as 
"running" if we add the bank prefix.

 >   - active bank - the bank selected for next boot
Can there be multiple active banks?  I can imagine a device that has FW 
partitioned into multiple banks, and brings in a small set of them for a 
full runtime.

 >   - next bank - current bank + 1 mod count
Next bank for what?  This seems easy to confuse between next bank to 
boot and next bank to flash.  Is this something that needs to be 
displayed to the user?

 >
 > If we want to keep backward compat - if no bank specified for flashing:
 >   - we flash to "next bank"
 >   - if flashing is successful we switch "active bank" to "next bank"
 > not that multiple flashing operations without activation/reboot will
 > result in overwriting the same "next bank" preventing us from flashing
 > multiple banks without trying if they work..
I think this is a nice guideline, but I'm not sure all physical devices 
will work this way.

 >
 > "stored" versions in devlink info display the versions for "active bank"
 > while running display running (i.e. in RAM, not in the banks!)>
 > In terms of modifications to the algo in documentation:
 >   - the check for "stored" versions check should be changed to an while
 >     loop that iterates over all banks
 >   - flashing can actually depend on the defaults as described above so
 >     no change
 >
 > We can expose the "current" and "active" bank as netlink attrs in dev
 > info.
How about a new info item
     DEVLINK_ATTR_INFO_ACTIVE_BANK
which would need a new api function something like
     devlink_info_active_bank_put()

Again, with the existing "running" attribute, maybe we don't need to add 
a "current"?

sln

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Jakub Kicinski]

On Wed, 7 Dec 2022 11:29:58 -0800 Shannon Nelson wrote:
> On 12/6/22 5:41 PM, Jakub Kicinski wrote:
>  > On Mon, 5 Dec 2022 09:26:26 -0800 Shannon Nelson wrote:  
>  >> Some devices have multiple memory banks that can be used to
>  >> hold various firmware versions that can be chosen for booting.
>  >> This can be used in addition to or along with the FW_LOAD_POLICY
>  >> parameter, depending on the capabilities of the particular
>  >> device.
>  >>
>  >> This is a parameter suggested by Jake in
>  >>   
> https://lore.kernel.org/netdev/CO1PR11MB508942BE965E63893DE9B86AD6129@CO1PR11MB5089.namprd11.prod.outlook.com/
>  >
>  > Can we make this netlink attributes?  
> To be sure, you are talking about defining new values in enum 
> devlink_attr, right?  Perhaps something like
>      DEVLINK_ATTR_INFO_VERSION_BANK   /* u32 */
> to go along with _VERSION_NAME and _VERSION_VALUE for each item under 
> running and stored?

Yes.

> Does u32 make sense here or should it be a string?

I'd go with u32, I don't think the banks could have any special meaning?
That'd need to be communicated? If so we can add that as a separate
mapping later (so it doesn't have to be repeated for each version).

> Or do we really need another value here, perhaps we should use the 
> existing _VERSION_NAME to display the bank?  This is what is essentially 
> happening in the current ionic and this proposed pds_core output, but 
> without the concept of bank numbers:
>        running:
>          fw 1.58.0-6
>        stored:
>          fw.goldfw 1.51.0-3
>          fw.mainfwa 1.58.0-6
>          fw.mainfwb 1.56.0-47-24-g651edb94cbe

To a human that makes sense but standardizing this naming scheme cross
vendors, and parsing this in code will be much harder than adding the
attr, IMO.

> With (optional?) bank numbers, it might look like
>        running:
>          1 fw 1.58.0-6
>        stored:
>          0 fw.goldfw 1.51.0-3
>          1 fw.mainfwa 1.58.0-6
>          2 fw.mainfwb 1.56.0-47-24-g651edb94cbe
> 
> Is this reasonable?

Well, the point of the multiple versions was that vendors can expose
components. Let's take the simplest example of management FW vs option
rom/UNDI:

	stored:
	  fw		1.2.3
	  fw.bundle	March 123
	  fw.undi	0.5.6

What I had in mind was to add bank'ed sections:

	stored (bank 0, active, current):
	  fw		1.2.3
	  fw.bundle	March 123
	  fw.undi	0.5.6
	stored (bank 1):
	  fw		1.4.0
	  fw.bundle	May 123
	  fw.undi	0.6.0

>  > What is the flow that you have in mind end to end (user actions)?
>  > I think we should document that, by which I mean extend the pseudo
>  > code here:
>  >
>  >   
> https://docs.kernel.org/next/networking/devlink/devlink-flash.html#firmware-version-management
>  >
>  > I expect we need to define the behavior such that the user can ignore
>  > the banks by default and get the right behavior.
>  >
>  > Let's define
>  >   - current bank - the bank from which the currently running image has
>  >     been loaded  
> I'm not sure this is any more information than what we already have as 
> "running" if we add the bank prefix.

Running is what's running, current let's you decide where the next
image will be flash. We can render "next" in the CLI if that's more
intuitive.

>  >   - active bank - the bank selected for next boot  
> Can there be multiple active banks?  I can imagine a device that has FW 
> partitioned into multiple banks, and brings in a small set of them for a 
> full runtime.

I'm not aware of any such cases, but can't prove they don't exist :S

>  >   - next bank - current bank + 1 mod count  
> Next bank for what? 

Flashing, basically.

> This seems easy to confuse between next bank to 
> boot and next bank to flash.  Is this something that needs to be 
> displayed to the user?

It's gonna decide which bank is getting overwrite.
I was just defining the terms for the benefit of the description below,
not much thought went into them. We can put flash-next or write-target
or whatever seems most obvious in CLI.

>  > If we want to keep backward compat - if no bank specified for flashing:
>  >   - we flash to "next bank"
>  >   - if flashing is successful we switch "active bank" to "next bank"
>  > not that multiple flashing operations without activation/reboot will
>  > result in overwriting the same "next bank" preventing us from flashing
>  > multiple banks without trying if they work..  
> I think this is a nice guideline, but I'm not sure all physical devices 
> will work this way.

Shouldn't it be entirely in SW control? (possibly "FW" category of SW)

I think this is important to get right. Once automation gets unleashed
on many machines, rare conditions and endless loops inevitably happen.
The update of stored flash can happen without taking the machine
offline to lower the downtime. If the update daemon runs at a 15min
interval we can write the flash 100 times a day, easily.

>  > "stored" versions in devlink info display the versions for "active bank"
>  > while running display running (i.e. in RAM, not in the banks!)>
>  > In terms of modifications to the algo in documentation:
>  >   - the check for "stored" versions check should be changed to an while
>  >     loop that iterates over all banks
>  >   - flashing can actually depend on the defaults as described above so
>  >     no change
>  >
>  > We can expose the "current" and "active" bank as netlink attrs in dev
>  > info.  
> How about a new info item
>      DEVLINK_ATTR_INFO_ACTIVE_BANK
> which would need a new api function something like
>      devlink_info_active_bank_put()

Yes, definitely. But I think the next-to-write is also needed, because
we will need to use the next-to-write bank to populate the JSON for
stored FW to keep backward compat.

In CLI we can be more loose but the algo in the docs must work and not
risk overwriting all the banks if machine gets multiple update cycles
before getting drained.

> Again, with the existing "running" attribute, maybe we don't need to add 
> a "current"?

Normal NICs have FW on the flash and FW in the RAM. The one in the RAM
is running, the one in the flash is stored. The stored can be updated
back, forth and nothing happens until reboot (or explicit activation
/reset). There is no service impact of updating the stored live.

Also note that running is a category not a version. With the components
I gave above running would be:

	  fw		1.2.3
	  fw.bundle	March 123
	  fw.undi	0.5.6

So all those versions are running...

Current (in my WIP nomenclature) was just to identify the bank that
running was loaded from. But bank is a single u32, and running versions
can be multiple and arbitrary strings.

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Shannon Nelson]

On 12/7/22 4:36 PM, Jakub Kicinski wrote:
> On Wed, 7 Dec 2022 11:29:58 -0800 Shannon Nelson wrote:
>> On 12/6/22 5:41 PM, Jakub Kicinski wrote:
>>   > On Mon, 5 Dec 2022 09:26:26 -0800 Shannon Nelson wrote:
>>   >> Some devices have multiple memory banks that can be used to
>>   >> hold various firmware versions that can be chosen for booting.
>>   >> This can be used in addition to or along with the FW_LOAD_POLICY
>>   >> parameter, depending on the capabilities of the particular
>>   >> device.
>>   >>

>>   > Can we make this netlink attributes?
>> To be sure, you are talking about defining new values in enum
>> devlink_attr, right?  Perhaps something like
>>       DEVLINK_ATTR_INFO_VERSION_BANK   /* u32 */
>> to go along with _VERSION_NAME and _VERSION_VALUE for each item under
>> running and stored?
> 
> Yes.
> 
>> Does u32 make sense here or should it be a string?
> 
> I'd go with u32, I don't think the banks could have any special meaning?
> That'd need to be communicated? If so we can add that as a separate
> mapping later (so it doesn't have to be repeated for each version).

Works for me.

> 
>> Or do we really need another value here, perhaps we should use the
>> existing _VERSION_NAME to display the bank?  This is what is essentially
>> happening in the current ionic and this proposed pds_core output, but
>> without the concept of bank numbers:
>>         running:
>>           fw 1.58.0-6
>>         stored:
>>           fw.goldfw 1.51.0-3
>>           fw.mainfwa 1.58.0-6
>>           fw.mainfwb 1.56.0-47-24-g651edb94cbe
> 
> To a human that makes sense but standardizing this naming scheme cross
> vendors, and parsing this in code will be much harder than adding the
> attr, IMO.
> 
>> With (optional?) bank numbers, it might look like
>>         running:
>>           1 fw 1.58.0-6
>>         stored:
>>           0 fw.goldfw 1.51.0-3
>>           1 fw.mainfwa 1.58.0-6
>>           2 fw.mainfwb 1.56.0-47-24-g651edb94cbe
>>
>> Is this reasonable?
> 
> Well, the point of the multiple versions was that vendors can expose
> components. Let's take the simplest example of management FW vs option
> rom/UNDI:
> 
>          stored:
>            fw            1.2.3
>            fw.bundle     March 123
>            fw.undi       0.5.6
> 
> What I had in mind was to add bank'ed sections:
> 
>          stored (bank 0, active, current):
>            fw            1.2.3
>            fw.bundle     March 123
>            fw.undi       0.5.6
>          stored (bank 1):
>            fw            1.4.0
>            fw.bundle     May 123
>            fw.undi       0.6.0

Seems reasonable at first glance...



> 
>>   > What is the flow that you have in mind end to end (user actions)?
>>   > I think we should document that, by which I mean extend the pseudo
>>   > code here:
>>   >
>>   >
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.kernel.org%2Fnext%2Fnetworking%2Fdevlink%2Fdevlink-flash.html%23firmware-version-management&data=05%7C01%7Cshannon.nelson%40amd.com%7Ce9ecb748ecab4e58305f08dad8b44e43%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638060566193141649%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=0vhs4ErnQcPoXxkdT8ltnqbJGpiydrpIj5zS0N08uYo%3D&reserved=0
>>   >
>>   > I expect we need to define the behavior such that the user can ignore
>>   > the banks by default and get the right behavior.
>>   >
>>   > Let's define
>>   >   - current bank - the bank from which the currently running image has
>>   >     been loaded
>> I'm not sure this is any more information than what we already have as
>> "running" if we add the bank prefix.
> 
> Running is what's running, current let's you decide where the next
> image will be flash. We can render "next" in the CLI if that's more
> intuitive.
> 
>>   >   - active bank - the bank selected for next boot
>> Can there be multiple active banks?  I can imagine a device that has FW
>> partitioned into multiple banks, and brings in a small set of them for a
>> full runtime.
> 
> I'm not aware of any such cases, but can't prove they don't exist :S

I think your banked sections example above satisfies this question.


> 
>>   >   - next bank - current bank + 1 mod count
>> Next bank for what?
> 
> Flashing, basically.
> 
>> This seems easy to confuse between next bank to
>> boot and next bank to flash.  Is this something that needs to be
>> displayed to the user?
> 
> It's gonna decide which bank is getting overwrite.
> I was just defining the terms for the benefit of the description below,
> not much thought went into them. We can put flash-next or write-target
> or whatever seems most obvious in CLI.

Maybe "flash-target"?

> 
>>   > If we want to keep backward compat - if no bank specified for flashing:
>>   >   - we flash to "next bank"
>>   >   - if flashing is successful we switch "active bank" to "next bank"
>>   > not that multiple flashing operations without activation/reboot will
>>   > result in overwriting the same "next bank" preventing us from flashing
>>   > multiple banks without trying if they work..
>> I think this is a nice guideline, but I'm not sure all physical devices
>> will work this way.
> 
> Shouldn't it be entirely in SW control? (possibly "FW" category of SW)

Sadly, not all HW/FW works the way driver writers would like, nor gives 
us all the features options we want.  Especially that FW that was built 
before we driver writers had an opinion about how this should work.

My comment here mainly is that we need to be able to manage the older FW 
as well as the newer, and be able to make allowances for FW that doesn't 
play along as well.

> 
> I think this is important to get right. Once automation gets unleashed
> on many machines, rare conditions and endless loops inevitably happen.
> The update of stored flash can happen without taking the machine
> offline to lower the downtime. If the update daemon runs at a 15min
> interval we can write the flash 100 times a day, easily.
> 
>>   > "stored" versions in devlink info display the versions for "active bank"
>>   > while running display running (i.e. in RAM, not in the banks!)>
>>   > In terms of modifications to the algo in documentation:
>>   >   - the check for "stored" versions check should be changed to an while
>>   >     loop that iterates over all banks
>>   >   - flashing can actually depend on the defaults as described above so
>>   >     no change
>>   >
>>   > We can expose the "current" and "active" bank as netlink attrs in dev
>>   > info.
>> How about a new info item
>>       DEVLINK_ATTR_INFO_ACTIVE_BANK
>> which would need a new api function something like
>>       devlink_info_active_bank_put()
> 
> Yes, definitely. But I think the next-to-write is also needed, because
> we will need to use the next-to-write bank to populate the JSON for
> stored FW to keep backward compat.
> 
> In CLI we can be more loose but the algo in the docs must work and not
> risk overwriting all the banks if machine gets multiple update cycles
> before getting drained.

If we are going to have multiple "stored" (banks) sections, then we need 
an api that allows for signifying which stored section are we adding a 
fw version to, and to be able to add the "active" and "flash-target" and 
whatever other attributes can get added onto the stored bank.

One option is to assume a bank context gets set by a call to something 
like devlink_info_stored_bank_put(), and add a bitmask of attributes 
(ACTIVE, FLASH_TARGET, CURRENT, ...) that can be added to in the future 
as needed.
     int devlink_info_stored_bank_put(struct devlink_info_req *req,
                                      uint bank_id,
                                      u32 option_mask)



> 
>> Again, with the existing "running" attribute, maybe we don't need to add
>> a "current"?
> 
> Normal NICs have FW on the flash and FW in the RAM. The one in the RAM
> is running, the one in the flash is stored. The stored can be updated
> back, forth and nothing happens until reboot (or explicit activation
> /reset). There is no service impact of updating the stored live.
> 
> Also note that running is a category not a version. With the components
> I gave above running would be:
> 
>            fw            1.2.3
>            fw.bundle     March 123
>            fw.undi       0.5.6
> 
> So all those versions are running...
> 
> Current (in my WIP nomenclature) was just to identify the bank that
> running was loaded from. But bank is a single u32, and running versions
> can be multiple and arbitrary strings.

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Jacob Keller]

On 12/8/2022 10:44 AM, Shannon Nelson wrote:
> On 12/7/22 4:36 PM, Jakub Kicinski wrote:
>> On Wed, 7 Dec 2022 11:29:58 -0800 Shannon Nelson wrote:
>>> Is this reasonable?
>>
>> Well, the point of the multiple versions was that vendors can expose
>> components. Let's take the simplest example of management FW vs option
>> rom/UNDI:
>>
>>          stored:
>>            fw            1.2.3
>>            fw.bundle     March 123
>>            fw.undi       0.5.6
>>
>> What I had in mind was to add bank'ed sections:
>>
>>          stored (bank 0, active, current):
>>            fw            1.2.3
>>            fw.bundle     March 123
>>            fw.undi       0.5.6
>>          stored (bank 1):
>>            fw            1.4.0
>>            fw.bundle     May 123
>>            fw.undi       0.6.0
> 
> Seems reasonable at first glance...
> 
> 

This is what I was thinking of and looks good to me. As for how to add 
attributes to get us from the current netlink API to this, I'm not 100% 
sure.

I think we can mostly just add the bank ID and flags to indicate which 
one is active and which one will be programmed next.

I think we could also add a new attribute to both reload and flash which 
specify which bank to use. For flash, this would be which bank to 
program, and for update this would be which bank to load the firmware 
from when doing a "fw_activate".

Is that reasonable? Do you still need a permanent "use this bank by 
default" parameter as well?

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Jakub Kicinski]

On Thu, 8 Dec 2022 10:44:50 -0800 Shannon Nelson wrote:
> >> I think this is a nice guideline, but I'm not sure all physical devices
> >> will work this way.  
> > 
> > Shouldn't it be entirely in SW control? (possibly "FW" category of SW)  
> 
> Sadly, not all HW/FW works the way driver writers would like, nor gives 
> us all the features options we want.  Especially that FW that was built 
> before we driver writers had an opinion about how this should work.
> 
> My comment here mainly is that we need to be able to manage the older FW 
> as well as the newer, and be able to make allowances for FW that doesn't 
> play along as well.

How do we steer new folks towards this design, tho? 

The only idea I have would break backward compat for you - we keep what
I described as default, and for devices which can't do that we require
sort of a manual opt out, for example user must request "don't set to
active" if the driver can't auto-change the active. And explicitly
select the bank if the driver can't provide the stable next-flash
semantics?

IDK what exact pieces of info you're working with and how much of the
semantics you can "fake" in the driver?

> >> How about a new info item
> >>       DEVLINK_ATTR_INFO_ACTIVE_BANK
> >> which would need a new api function something like
> >>       devlink_info_active_bank_put()  
> > 
> > Yes, definitely. But I think the next-to-write is also needed, because
> > we will need to use the next-to-write bank to populate the JSON for
> > stored FW to keep backward compat.
> > 
> > In CLI we can be more loose but the algo in the docs must work and not
> > risk overwriting all the banks if machine gets multiple update cycles
> > before getting drained.  
> 
> If we are going to have multiple "stored" (banks) sections, then we need 
> an api that allows for signifying which stored section are we adding a 
> fw version to, and to be able to add the "active" and "flash-target" and 
> whatever other attributes can get added onto the stored bank.
> 
> One option is to assume a bank context gets set by a call to something 
> like devlink_info_stored_bank_put(), and add a bitmask of attributes 
> (ACTIVE, FLASH_TARGET, CURRENT, ...) that can be added to in the future 
> as needed.
>      int devlink_info_stored_bank_put(struct devlink_info_req *req,
>                                       uint bank_id,
>                                       u32 option_mask)

Yup, that's an option. Dunno if the mask is easier to use than just
separate call per attribute, but I guess you'll be the one to test
this API so you'll find out :)

At the netlink level we'd have a separate nla for active, target,
current banks, so no masks there.. right?

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Jakub Kicinski]

On Thu, 8 Dec 2022 16:47:31 -0800 Jacob Keller wrote:
> This is what I was thinking of and looks good to me. As for how to add 
> attributes to get us from the current netlink API to this, I'm not 100% 
> sure.
> 
> I think we can mostly just add the bank ID and flags to indicate which 
> one is active and which one will be programmed next.

Why flags, tho?

The current nesting is:

  DEVLINK_ATTR_INFO_DRIVER_NAME		[str]
  DEVLINK_ATTR_INFO_SERIAL_NUMBER	[str]
  DEVLINK_ATTR_INFO_BOARD_SERIAL_NUMBER	[str]

  DEVLINK_ATTR_INFO_VERSION_FIXED	[nest] // multiple VERSION_* nests follow
    DEVLINK_ATTR_INFO_VERSION_NAME	[str]
    DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
  DEVLINK_ATTR_INFO_VERSION_FIXED	[nest]
    DEVLINK_ATTR_INFO_VERSION_NAME	[str]
    DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
  DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
    DEVLINK_ATTR_INFO_VERSION_NAME	[str]
    DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
  DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
    DEVLINK_ATTR_INFO_VERSION_NAME	[str]
    DEVLINK_ATTR_INFO_VERSION_VALUE	[str]


Now we'd throw the bank into the nests, and add root attrs for the
current / flash / active as top level attrs:

  DEVLINK_ATTR_INFO_DRIVER_NAME		[str]
  DEVLINK_ATTR_INFO_SERIAL_NUMBER	[str]
  DEVLINK_ATTR_INFO_BOARD_SERIAL_NUMBER	[str]
  DEVLINK_ATTR_INFO_BANK_ACTIVE		[u32] // << optional
  DEVLINK_ATTR_INFO_BANK_UPDATE_TGT	[u32] // << optional

  DEVLINK_ATTR_INFO_VERSION_FIXED	[nest] // multiple VERSION_* nests follow
    DEVLINK_ATTR_INFO_VERSION_NAME	[str]
    DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
  DEVLINK_ATTR_INFO_VERSION_FIXED	[nest]
    DEVLINK_ATTR_INFO_VERSION_NAME	[str]
    DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
  DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
    DEVLINK_ATTR_INFO_VERSION_NAME	[str]
    DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
  DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
    DEVLINK_ATTR_INFO_VERSION_NAME	[str]
    DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
  DEVLINK_ATTR_INFO_VERSION_STORED	[nest]
    DEVLINK_ATTR_INFO_VERSION_NAME	[str]
    DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
    DEVLINK_ATTR_INFO_VERSION_BANK	[u32] // << optional
  DEVLINK_ATTR_INFO_VERSION_STORED	[nest]
    DEVLINK_ATTR_INFO_VERSION_NAME	[str]
    DEVLINK_ATTR_INFO_VERSION_VALUE     [str]
    DEVLINK_ATTR_INFO_VERSION_BANK	[u32] // << optional

> I think we could also add a new attribute to both reload and flash which 
> specify which bank to use. For flash, this would be which bank to 
> program, and for update this would be which bank to load the firmware 
> from when doing a "fw_activate".

SG!

> Is that reasonable? Do you still need a permanent "use this bank by 
> default" parameter as well?

I hope we cover all cases, so no param needed?

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Jacob Keller]

On 12/8/2022 5:24 PM, Jakub Kicinski wrote:
> On Thu, 8 Dec 2022 16:47:31 -0800 Jacob Keller wrote:
>> This is what I was thinking of and looks good to me. As for how to add
>> attributes to get us from the current netlink API to this, I'm not 100%
>> sure.
>>
>> I think we can mostly just add the bank ID and flags to indicate which
>> one is active and which one will be programmed next.
> 
> Why flags, tho?
> 
> The current nesting is:
> 
>    DEVLINK_ATTR_INFO_DRIVER_NAME		[str]
>    DEVLINK_ATTR_INFO_SERIAL_NUMBER	[str]
>    DEVLINK_ATTR_INFO_BOARD_SERIAL_NUMBER	[str]
> 
>    DEVLINK_ATTR_INFO_VERSION_FIXED	[nest] // multiple VERSION_* nests follow
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_FIXED	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
> 
> 
> Now we'd throw the bank into the nests, and add root attrs for the
> current / flash / active as top level attrs:
> 
>    DEVLINK_ATTR_INFO_DRIVER_NAME		[str]
>    DEVLINK_ATTR_INFO_SERIAL_NUMBER	[str]
>    DEVLINK_ATTR_INFO_BOARD_SERIAL_NUMBER	[str]
>    DEVLINK_ATTR_INFO_BANK_ACTIVE		[u32] // << optional
>    DEVLINK_ATTR_INFO_BANK_UPDATE_TGT	[u32] // << optional
> 
>    DEVLINK_ATTR_INFO_VERSION_FIXED	[nest] // multiple VERSION_* nests follow
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_FIXED	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_STORED	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>      DEVLINK_ATTR_INFO_VERSION_BANK	[u32] // << optional
>    DEVLINK_ATTR_INFO_VERSION_STORED	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE     [str]
>      DEVLINK_ATTR_INFO_VERSION_BANK	[u32] // << optional
> 


Yea this is what I was thinking. With this change we have:

old kernel, old devlink - behaves as today
old kernel, new devlink - prints "unknown bank"
new kernel, old devlink - old devlink should ignore the attribute
new kernel, new devlink - prints bank info along with version

So I don't see any issue with adding these attributes getting confused 
when working with old or new userspace.

>> I think we could also add a new attribute to both reload and flash which
>> specify which bank to use. For flash, this would be which bank to
>> program, and for update this would be which bank to load the firmware
>> from when doing a "fw_activate".
> 
> SG!
> 
>> Is that reasonable? Do you still need a permanent "use this bank by
>> default" parameter as well?
> 
> I hope we cover all cases, so no param needed?

The only reason one might want a parameter is if we want to change some 
default. For example I think I saw some devices load firmware during 
resets or initialization.

But I think that is something we can cross if the extra attributes for 
reload and flash are not sufficient. We can always add a parameter 
later. We can't easily take them away once added.

Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

[Jakub Kicinski]

On Mon, 12 Dec 2022 10:04:37 -0800 Jacob Keller wrote:
> >    DEVLINK_ATTR_INFO_VERSION_STORED	[nest]
> >      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
> >      DEVLINK_ATTR_INFO_VERSION_VALUE     [str]
> >      DEVLINK_ATTR_INFO_VERSION_BANK	[u32] // << optional
> >   
> 
> 
> Yea this is what I was thinking. With this change we have:
> 
> old kernel, old devlink - behaves as today
> old kernel, new devlink - prints "unknown bank"

Ah, unintentionally I put bank in all nests.
For existing single-image devices I think we can continue to skip 
the bank attr. So old kernel new devlink should behave the same as
old/old.

> new kernel, old devlink - old devlink should ignore the attribute
> new kernel, new devlink - prints bank info along with version
> 
> So I don't see any issue with adding these attributes getting confused 
> when working with old or new userspace.
> 
> >> I think we could also add a new attribute to both reload and flash which
> >> specify which bank to use. For flash, this would be which bank to
> >> program, and for update this would be which bank to load the firmware
> >> from when doing a "fw_activate".  
> > 
> > SG!
> >   
> >> Is that reasonable? Do you still need a permanent "use this bank by
> >> default" parameter as well?  
> > 
> > I hope we cover all cases, so no param needed?  
> 
> The only reason one might want a parameter is if we want to change some 
> default. For example I think I saw some devices load firmware during 
> resets or initialization.

Any reset/activation should happen from the active bank, right?
We should have a way to set the active bank but I reckon that's
more of a normal command than a param thing?

> But I think that is something we can cross if the extra attributes for 
> reload and flash are not sufficient. We can always add a parameter 
> later. We can't easily take them away once added.