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

[Jiri Pirko <jiri@resnulli.us>]

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
>> >