| Message ID | 20200717183541.797878-7-jacob.e.keller@intel.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | introduce PLDM firmware update library | expand |
Fri, Jul 17, 2020 at 08:35:41PM CEST, jacob.e.keller@intel.com wrote: >A flash image may contain settings or device identifying information. >When performing a flash update, these settings and information may >conflict with contents already in the flash. Devices may handle this >conflict in multiple ways. > >Add a new attribute to the devlink command, >DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MODE, which specifies how the device >should handle these settings and fields. > >DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING, the default, requests that all >settings and device identifiers within the current flash are kept. That >is, no settings or fields will be overwritten. This is the expected >behavior for most updates, and appears to be how all of the drivers are >implemented today. > >DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS, requests that the device >overwrite any device settings in the flash section with the settings >from the flash image, but to preserve identifiers such as the MAC >address and serial identifier. This may be useful as a way to restore >a device to known-good settings from a new flash image. > >DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING, requests that all content in >the flash image be preserved over content of flash on the device. This >mode requests the device to completely overwrite the flash section, >possibly changing settings and device identifiers. The primary >motivation is to support writing initial device identifiers during >manufacturing. It is not expected to be necessary in normal end-user >flash updates. > >For the ice driver, implement support for the overwrite mode by >selecting the associated preservation level to request from firmware. > >For all other drivers that support flash update, require that the mode >be DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING, which is the expected >default. > >Update the documentation to explain the overwrite mode attribute. This looks odd. You have a single image yet you somehow divide it into "program" and "config" areas. We already have infra in place to take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT. You should have 2 components: 1) "program" 2) "config" Then it is up to the user what he decides to flash.
On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote: > This looks odd. You have a single image yet you somehow divide it > into "program" and "config" areas. We already have infra in place to > take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT. > You should have 2 components: > 1) "program" > 2) "config" > > Then it is up to the user what he decides to flash. 99.9% of the time users want to flash "all". To achieve "don't flash config" with current infra users would have to flash each component one by one and then omit the one(s) which is config (guessing which one that is based on the name). Wouldn't this be quite inconvenient? In case of MLX is PSID considered config?
On 7/20/2020 8:51 AM, Jakub Kicinski wrote: > On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote: >> This looks odd. You have a single image yet you somehow divide it >> into "program" and "config" areas. We already have infra in place to >> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT. >> You should have 2 components: >> 1) "program" >> 2) "config" >> First off, unfortunately at least for ice, the "main" section of NVM contains both the management firmware as well as config settings. I don't really have a way to split it up. This series includes support for updating the main NVM section containing the management firmware (and some config) "fw.mgmt", as well as "fw.undi" which contains the OptionROM, and "fw.netlist" which contains additional configuration TLVs. The firmware interface allows me to separate the three components, but does not let me separate the "fw binary" from the "config settings" that are stored within the main NVM bank. (These fields include other data like the device MAC address and VPD area of the device too, so using "config" is a bit of a misnomer). >> Then it is up to the user what he decides to flash. > > 99.9% of the time users want to flash "all". To achieve "don't flash > config" with current infra users would have to flash each component > one by one and then omit the one(s) which is config (guessing which > one that is based on the name). > > Wouldn't this be quite inconvenient? > I also agree here, I'd like to be able to make the "update with the complete file" just work in the most straight forward way (i.e. without erasing stuff by accident) be the default. The option I'm proposing here is to enable allowing tools to optionally specify handling this type of overwrite. The goal is to support these use cases: a) (default) just update the image, but keep the config and vital data the same as before the update. b) overwrite config fields, but keep vital fields the same. Intended to allow returning configuration to "image defaults". This mostly is intended in case regular update caused some issues like if somehow the config preservation didn't work properly. c) overwrite all fields. The intention here is to allow programming a customized image during initial setup that would contain new IDs etc. It is not expected to be used in general, as this does overwrite vital data like the MAC addresses and such. So the problem is that the vital data, config data, and firmware binaries are stored in the same section, without a good way to separate between them. We program all of these together as one chunk to the "secondary NVM bank" and then ask firmware to update. It reads through and based on our "preservation" setting will update the binaries and merge the configuration sections. > In case of MLX is PSID considered config? >
Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote: >On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote: >> This looks odd. You have a single image yet you somehow divide it >> into "program" and "config" areas. We already have infra in place to >> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT. >> You should have 2 components: >> 1) "program" >> 2) "config" >> >> Then it is up to the user what he decides to flash. > >99.9% of the time users want to flash "all". To achieve "don't flash >config" with current infra users would have to flash each component Well you can have multiple component what would overlap: 1) "program" + "config" (default) 2) "program" 3) "config" >one by one and then omit the one(s) which is config (guessing which >one that is based on the name). > >Wouldn't this be quite inconvenient? I see it as an extra knob that is actually somehow provides degradation of components. > >In case of MLX is PSID considered config? Nope.
Mon, Jul 20, 2020 at 08:52:58PM CEST, jacob.e.keller@intel.com wrote: > > >On 7/20/2020 8:51 AM, Jakub Kicinski wrote: >> On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote: >>> This looks odd. You have a single image yet you somehow divide it >>> into "program" and "config" areas. We already have infra in place to >>> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT. >>> You should have 2 components: >>> 1) "program" >>> 2) "config" >>> > >First off, unfortunately at least for ice, the "main" section of NVM >contains both the management firmware as well as config settings. I >don't really have a way to split it up. You don't have to split it up. Just for component "x" you push binary "A" and flash part of it and for comonent "y" you push the same binary "A" and flash different part of it. Consider the component as a "mask" in your case. > >This series includes support for updating the main NVM section >containing the management firmware (and some config) "fw.mgmt", as well >as "fw.undi" which contains the OptionROM, and "fw.netlist" which >contains additional configuration TLVs. > >The firmware interface allows me to separate the three components, but >does not let me separate the "fw binary" from the "config settings" that >are stored within the main NVM bank. (These fields include other data >like the device MAC address and VPD area of the device too, so using >"config" is a bit of a misnomer). > >>> Then it is up to the user what he decides to flash. >> >> 99.9% of the time users want to flash "all". To achieve "don't flash >> config" with current infra users would have to flash each component >> one by one and then omit the one(s) which is config (guessing which >> one that is based on the name). >> >> Wouldn't this be quite inconvenient? >> > >I also agree here, I'd like to be able to make the "update with the >complete file" just work in the most straight forward way (i.e. without >erasing stuff by accident) be the default. > >The option I'm proposing here is to enable allowing tools to optionally >specify handling this type of overwrite. The goal is to support these >use cases: > >a) (default) just update the image, but keep the config and vital data >the same as before the update. > >b) overwrite config fields, but keep vital fields the same. Intended to >allow returning configuration to "image defaults". This mostly is >intended in case regular update caused some issues like if somehow the >config preservation didn't work properly. > >c) overwrite all fields. The intention here is to allow programming a >customized image during initial setup that would contain new IDs etc. It >is not expected to be used in general, as this does overwrite vital data >like the MAC addresses and such. > >So the problem is that the vital data, config data, and firmware >binaries are stored in the same section, without a good way to separate >between them. We program all of these together as one chunk to the >"secondary NVM bank" and then ask firmware to update. It reads through >and based on our "preservation" setting will update the binaries and >merge the configuration sections. > >> In case of MLX is PSID considered config? >>
On Tue, 21 Jul 2020 15:53:56 +0200 Jiri Pirko wrote: > Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote: > >On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote: > >> This looks odd. You have a single image yet you somehow divide it > >> into "program" and "config" areas. We already have infra in place to > >> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT. > >> You should have 2 components: > >> 1) "program" > >> 2) "config" > >> > >> Then it is up to the user what he decides to flash. > > > >99.9% of the time users want to flash "all". To achieve "don't flash > >config" with current infra users would have to flash each component > > Well you can have multiple component what would overlap: > 1) "program" + "config" (default) > 2) "program" > 3) "config" Say I have FW component and UNDI driver. Now I'll have 4 components? fw.prog, fw.config, undi.prog etc? Are those extra ones visible or just "implied"? If they are visible what version does the config have? Also (3) - flashing config from one firmware version and program from another - makes a very limited amount of sense to me. > >one by one and then omit the one(s) which is config (guessing which > >one that is based on the name). > > > >Wouldn't this be quite inconvenient? > > I see it as an extra knob that is actually somehow provides degradation > of components. Hm. We have the exact opposite view on the matter. To me components currently correspond to separate fw/hw entities, that's a very clear meaning. PHY firmware, management FW, UNDI. Now we would add a completely orthogonal meaning to the same API. Why? In the name of "field reuse"? > >In case of MLX is PSID considered config? > > Nope.
On 7/21/2020 6:56 AM, Jiri Pirko wrote: > Mon, Jul 20, 2020 at 08:52:58PM CEST, jacob.e.keller@intel.com wrote: >> >> >> On 7/20/2020 8:51 AM, Jakub Kicinski wrote: >>> On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote: >>>> This looks odd. You have a single image yet you somehow divide it >>>> into "program" and "config" areas. We already have infra in place to >>>> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT. >>>> You should have 2 components: >>>> 1) "program" >>>> 2) "config" >>>> >> >> First off, unfortunately at least for ice, the "main" section of NVM >> contains both the management firmware as well as config settings. I >> don't really have a way to split it up. > > You don't have to split it up. Just for component "x" you push binary > "A" and flash part of it and for comonent "y" you push the same binary > "A" and flash different part of it. > > Consider the component as a "mask" in your case. > > The driver itself doesn't really know what parts of the image are which. I have to ask the firmware. And it doesn't have a "settings only" flag. I have roughly equivalent to "binary only", "binary + settings" and "binary + settings + vital fields" Plus, this means that every update must be single-component and there's no way to distinguish this when an update is supposed to be for all of the components in the PLDM file.
On 7/21/2020 10:04 AM, Jakub Kicinski wrote: > On Tue, 21 Jul 2020 15:53:56 +0200 Jiri Pirko wrote: >> Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote: >>> On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote: >>>> This looks odd. You have a single image yet you somehow divide it >>>> into "program" and "config" areas. We already have infra in place to >>>> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT. >>>> You should have 2 components: >>>> 1) "program" >>>> 2) "config" >>>> >>>> Then it is up to the user what he decides to flash. >>> >>> 99.9% of the time users want to flash "all". To achieve "don't flash >>> config" with current infra users would have to flash each component >> >> Well you can have multiple component what would overlap: >> 1) "program" + "config" (default) >> 2) "program" >> 3) "config" > > Say I have FW component and UNDI driver. Now I'll have 4 components? > fw.prog, fw.config, undi.prog etc? Are those extra ones visible or just > "implied"? If they are visible what version does the config have? > > Also (3) - flashing config from one firmware version and program from > another - makes a very limited amount of sense to me. > Right, this is actually one of the potential problems I've been told about: if the config doesn't match the firmware it's supposed to work, but the "overwrite config" option is partially there to help have a way out in case the config and firmware aren't in sync and something goes wrong. >>> one by one and then omit the one(s) which is config (guessing which >>> one that is based on the name). >>> >>> Wouldn't this be quite inconvenient? >> >> I see it as an extra knob that is actually somehow provides degradation >> of components. > > Hm. We have the exact opposite view on the matter. To me components > currently correspond to separate fw/hw entities, that's a very clear > meaning. PHY firmware, management FW, UNDI. Now we would add a > completely orthogonal meaning to the same API. > > Why? > > In the name of "field reuse"? > Right. I understand that other hardware works differently and has all config separated to separate distinct components, but I think it would be needlessly confusing to have separate component names. Plus, as I said in another thread: I can't really separate the two components when I update. I have to send the combined block to firmware with the flag indicating how it should do preservation/merging. So I can't really do "just settings" anyways, meaning that it really would be two components which overlap. Plus, I wouldn't really have a separate "info" display. Ultimately it ends up feeling like a significant hack of the component name if I go that route.
Tue, Jul 21, 2020 at 07:04:06PM CEST, kubakici@wp.pl wrote: >On Tue, 21 Jul 2020 15:53:56 +0200 Jiri Pirko wrote: >> Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote: >> >On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote: >> >> This looks odd. You have a single image yet you somehow divide it >> >> into "program" and "config" areas. We already have infra in place to >> >> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT. >> >> You should have 2 components: >> >> 1) "program" >> >> 2) "config" >> >> >> >> Then it is up to the user what he decides to flash. >> > >> >99.9% of the time users want to flash "all". To achieve "don't flash >> >config" with current infra users would have to flash each component >> >> Well you can have multiple component what would overlap: >> 1) "program" + "config" (default) >> 2) "program" >> 3) "config" > >Say I have FW component and UNDI driver. Now I'll have 4 components? >fw.prog, fw.config, undi.prog etc? Are those extra ones visible or just Visible in which sense? We don't show components anywhere if I'm not mistaken. They are currently very rarely used. Basically we just ported it from ethtool without much thinking. >"implied"? If they are visible what version does the config have? Good question. we don't have per-component version so far. I think it would be good to have it alonside with the listing. > >Also (3) - flashing config from one firmware version and program from >another - makes a very limited amount of sense to me. > >> >one by one and then omit the one(s) which is config (guessing which >> >one that is based on the name). >> > >> >Wouldn't this be quite inconvenient? >> >> I see it as an extra knob that is actually somehow provides degradation >> of components. > >Hm. We have the exact opposite view on the matter. To me components >currently correspond to separate fw/hw entities, that's a very clear >meaning. PHY firmware, management FW, UNDI. Now we would add a >completely orthogonal meaning to the same API. I understand. My concern is, we would have a component with some "subparts". Now it is some fuzzy vagely defined "config part", in the future it might be something else. That is what I'm concerned about. Components have clear api. So perhaps we can introduce something like "component mask", which would allow to flash only part of the component. That is basically what Jacob has, I would just like to have it well defined. > >Why? > >In the name of "field reuse"? > >> >In case of MLX is PSID considered config? >> >> Nope. >
> -----Original Message----- > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On > Behalf Of Jiri Pirko > Sent: Wednesday, July 22, 2020 3:52 AM > To: Jakub Kicinski <kubakici@wp.pl> > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; Tom > Herbert <tom@herbertland.com>; Jiri Pirko <jiri@mellanox.com>; Jakub Kicinski > <kuba@kernel.org>; Jonathan Corbet <corbet@lwn.net>; Michael Chan > <michael.chan@broadcom.com>; Bin Luo <luobin9@huawei.com>; Saeed > Mahameed <saeedm@mellanox.com>; Leon Romanovsky <leon@kernel.org>; > Ido Schimmel <idosch@mellanox.com>; Danielle Ratson > <danieller@mellanox.com> > Subject: Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash > update > > Tue, Jul 21, 2020 at 07:04:06PM CEST, kubakici@wp.pl wrote: > >On Tue, 21 Jul 2020 15:53:56 +0200 Jiri Pirko wrote: > >> Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote: > >> >On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote: > >> >> This looks odd. You have a single image yet you somehow divide it > >> >> into "program" and "config" areas. We already have infra in place to > >> >> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT. > >> >> You should have 2 components: > >> >> 1) "program" > >> >> 2) "config" > >> >> > >> >> Then it is up to the user what he decides to flash. > >> > > >> >99.9% of the time users want to flash "all". To achieve "don't flash > >> >config" with current infra users would have to flash each component > >> > >> Well you can have multiple component what would overlap: > >> 1) "program" + "config" (default) > >> 2) "program" > >> 3) "config" > > > >Say I have FW component and UNDI driver. Now I'll have 4 components? > >fw.prog, fw.config, undi.prog etc? Are those extra ones visible or just > > Visible in which sense? We don't show components anywhere if I'm not > mistaken. They are currently very rarely used. Basically we just ported > it from ethtool without much thinking. > Component names are used in devlink info and displayed to end users along with versions, plus they're names passed by the user in devlink flash update. As far as documented, we shouldn't add new components without associated versions in the info report. > > >"implied"? If they are visible what version does the config have? > > Good question. we don't have per-component version so far. I think it > would be good to have it alonside with the listing. > > > > > >Also (3) - flashing config from one firmware version and program from > >another - makes a very limited amount of sense to me. > > > >> >one by one and then omit the one(s) which is config (guessing which > >> >one that is based on the name). > >> > > >> >Wouldn't this be quite inconvenient? > >> > >> I see it as an extra knob that is actually somehow provides degradation > >> of components. > > > >Hm. We have the exact opposite view on the matter. To me components > >currently correspond to separate fw/hw entities, that's a very clear > >meaning. PHY firmware, management FW, UNDI. Now we would add a > >completely orthogonal meaning to the same API. > > I understand. My concern is, we would have a component with some > "subparts". Now it is some fuzzy vagely defined "config part", > in the future it might be something else. That is what I'm concerned > about. Components have clear api. > > So perhaps we can introduce something like "component mask", which would > allow to flash only part of the component. That is basically what Jacob > has, I would just like to have it well defined. > > So, we could make this selection a series of masked bits instead of a single enumeration value.
On Wed, 22 Jul 2020 15:30:05 +0000 Keller, Jacob E wrote: > > >> >one by one and then omit the one(s) which is config (guessing which > > >> >one that is based on the name). > > >> > > > >> >Wouldn't this be quite inconvenient? > > >> > > >> I see it as an extra knob that is actually somehow provides degradation > > >> of components. > > > > > >Hm. We have the exact opposite view on the matter. To me components > > >currently correspond to separate fw/hw entities, that's a very clear > > >meaning. PHY firmware, management FW, UNDI. Now we would add a > > >completely orthogonal meaning to the same API. > > > > I understand. My concern is, we would have a component with some > > "subparts". Now it is some fuzzy vagely defined "config part", > > in the future it might be something else. That is what I'm concerned > > about. Components have clear api. > > > > So perhaps we can introduce something like "component mask", which would > > allow to flash only part of the component. That is basically what Jacob > > has, I would just like to have it well defined. > > So, we could make this selection a series of masked bits instead of a > single enumeration value. I'd still argue that components (as defined in devlink info) and config are pretty orthogonal. In my experience config is stored in its own section of the flash, and some of the knobs are in no obvious way associated with components (used by components). That said, if we rename the "component mask" to "update mask" that's fine with me. Then we'd have bit 0 - don't overwrite config bit 1 - don't overwrite identifiers ? Let's define a bit for "don't update program" when we actually need it.
On 7/22/2020 9:52 AM, Jakub Kicinski wrote: > On Wed, 22 Jul 2020 15:30:05 +0000 Keller, Jacob E wrote: >>> So perhaps we can introduce something like "component mask", which would >>> allow to flash only part of the component. That is basically what Jacob >>> has, I would just like to have it well defined. >> >> So, we could make this selection a series of masked bits instead of a >> single enumeration value. > > I'd still argue that components (as defined in devlink info) and config > are pretty orthogonal. In my experience config is stored in its own > section of the flash, and some of the knobs are in no obvious way > associated with components (used by components). > > That said, if we rename the "component mask" to "update mask" that's > fine with me. > > Then we'd have > > bit 0 - don't overwrite config > bit 1 - don't overwrite identifiers > > ? > > Let's define a bit for "don't update program" when we actually need it. > Ok. And this can be later extended with additional bits with new meanings should the need arise. Additionally, drivers can ensure that the valid combination of bits is set. the drivers can reject requests for combinations that they do not support. I can make that change. My preference is that "0" for a bit means do not overwrite while "1" means overwrite. This way, if/when additional bits are added, drivers won't need to be updated to reject such requests. If we make "1" the "do not overwrite" then we'd have a case where drivers must update to ensure they reject requests which don't set the bit. Thanks, Jake
Wed, Jul 22, 2020 at 05:30:05PM CEST, jacob.e.keller@intel.com wrote: > > >> -----Original Message----- >> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On >> Behalf Of Jiri Pirko >> Sent: Wednesday, July 22, 2020 3:52 AM >> To: Jakub Kicinski <kubakici@wp.pl> >> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; Tom >> Herbert <tom@herbertland.com>; Jiri Pirko <jiri@mellanox.com>; Jakub Kicinski >> <kuba@kernel.org>; Jonathan Corbet <corbet@lwn.net>; Michael Chan >> <michael.chan@broadcom.com>; Bin Luo <luobin9@huawei.com>; Saeed >> Mahameed <saeedm@mellanox.com>; Leon Romanovsky <leon@kernel.org>; >> Ido Schimmel <idosch@mellanox.com>; Danielle Ratson >> <danieller@mellanox.com> >> Subject: Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash >> update >> >> Tue, Jul 21, 2020 at 07:04:06PM CEST, kubakici@wp.pl wrote: >> >On Tue, 21 Jul 2020 15:53:56 +0200 Jiri Pirko wrote: >> >> Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote: >> >> >On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote: >> >> >> This looks odd. You have a single image yet you somehow divide it >> >> >> into "program" and "config" areas. We already have infra in place to >> >> >> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT. >> >> >> You should have 2 components: >> >> >> 1) "program" >> >> >> 2) "config" >> >> >> >> >> >> Then it is up to the user what he decides to flash. >> >> > >> >> >99.9% of the time users want to flash "all". To achieve "don't flash >> >> >config" with current infra users would have to flash each component >> >> >> >> Well you can have multiple component what would overlap: >> >> 1) "program" + "config" (default) >> >> 2) "program" >> >> 3) "config" >> > >> >Say I have FW component and UNDI driver. Now I'll have 4 components? >> >fw.prog, fw.config, undi.prog etc? Are those extra ones visible or just >> >> Visible in which sense? We don't show components anywhere if I'm not >> mistaken. They are currently very rarely used. Basically we just ported >> it from ethtool without much thinking. >> > >Component names are used in devlink info and displayed to end users along with versions, plus they're names passed by the user in devlink flash update. As far as documented, we shouldn't add new components without associated versions in the info report. Okay. So it is loosely coupled. I think it would be nice to tight those 2 togeter so it is not up to the driver how he decides to implement it. > >> >> >"implied"? If they are visible what version does the config have? >> >> Good question. we don't have per-component version so far. I think it >> would be good to have it alonside with the listing. >> >> >> > >> >Also (3) - flashing config from one firmware version and program from >> >another - makes a very limited amount of sense to me. >> > >> >> >one by one and then omit the one(s) which is config (guessing which >> >> >one that is based on the name). >> >> > >> >> >Wouldn't this be quite inconvenient? >> >> >> >> I see it as an extra knob that is actually somehow provides degradation >> >> of components. >> > >> >Hm. We have the exact opposite view on the matter. To me components >> >currently correspond to separate fw/hw entities, that's a very clear >> >meaning. PHY firmware, management FW, UNDI. Now we would add a >> >completely orthogonal meaning to the same API. >> >> I understand. My concern is, we would have a component with some >> "subparts". Now it is some fuzzy vagely defined "config part", >> in the future it might be something else. That is what I'm concerned >> about. Components have clear api. >> >> So perhaps we can introduce something like "component mask", which would >> allow to flash only part of the component. That is basically what Jacob >> has, I would just like to have it well defined. >> >> > >So, we could make this selection a series of masked bits instead of a single enumeration value. Yeah.
Wed, Jul 22, 2020 at 08:21:22PM CEST, jacob.e.keller@intel.com wrote: > > >On 7/22/2020 9:52 AM, Jakub Kicinski wrote: >> On Wed, 22 Jul 2020 15:30:05 +0000 Keller, Jacob E wrote: >>>> So perhaps we can introduce something like "component mask", which would >>>> allow to flash only part of the component. That is basically what Jacob >>>> has, I would just like to have it well defined. >>> >>> So, we could make this selection a series of masked bits instead of a >>> single enumeration value. >> >> I'd still argue that components (as defined in devlink info) and config >> are pretty orthogonal. In my experience config is stored in its own >> section of the flash, and some of the knobs are in no obvious way >> associated with components (used by components). >> >> That said, if we rename the "component mask" to "update mask" that's >> fine with me. >> >> Then we'd have >> >> bit 0 - don't overwrite config >> bit 1 - don't overwrite identifiers >> >> ? >> >> Let's define a bit for "don't update program" when we actually need it. >> > > >Ok. And this can be later extended with additional bits with new >meanings should the need arise. > >Additionally, drivers can ensure that the valid combination of bits is >set. the drivers can reject requests for combinations that they do not >support. Makes sense. > >I can make that change. > >My preference is that "0" for a bit means do not overwrite while "1" >means overwrite. This way, if/when additional bits are added, drivers >won't need to be updated to reject such requests. If we make "1" the "do >not overwrite" then we'd have a case where drivers must update to ensure >they reject requests which don't set the bit. 0 should be default and driver should bahave accordingly. > >Thanks, >Jake
On 7/26/2020 12:18 AM, Jiri Pirko wrote: > Wed, Jul 22, 2020 at 08:21:22PM CEST, jacob.e.keller@intel.com wrote: >> >> >> On 7/22/2020 9:52 AM, Jakub Kicinski wrote: >>> On Wed, 22 Jul 2020 15:30:05 +0000 Keller, Jacob E wrote: >>>>> So perhaps we can introduce something like "component mask", which would >>>>> allow to flash only part of the component. That is basically what Jacob >>>>> has, I would just like to have it well defined. >>>> >>>> So, we could make this selection a series of masked bits instead of a >>>> single enumeration value. >>> >>> I'd still argue that components (as defined in devlink info) and config >>> are pretty orthogonal. In my experience config is stored in its own >>> section of the flash, and some of the knobs are in no obvious way >>> associated with components (used by components). >>> >>> That said, if we rename the "component mask" to "update mask" that's >>> fine with me. >>> >>> Then we'd have >>> >>> bit 0 - don't overwrite config >>> bit 1 - don't overwrite identifiers >>> >>> ? >>> >>> Let's define a bit for "don't update program" when we actually need it. >>> >> >> >> Ok. And this can be later extended with additional bits with new >> meanings should the need arise. >> >> Additionally, drivers can ensure that the valid combination of bits is >> set. the drivers can reject requests for combinations that they do not >> support. > > Makes sense. > >> >> I can make that change. >> >> My preference is that "0" for a bit means do not overwrite while "1" >> means overwrite. This way, if/when additional bits are added, drivers >> won't need to be updated to reject such requests. If we make "1" the "do >> not overwrite" then we'd have a case where drivers must update to ensure >> they reject requests which don't set the bit. > > 0 should be default and driver should bahave accordingly. > Correct, and it's good to spell that out more clearly. Thanks, Jake > >> >> Thanks, >> Jake
On 7/26/2020 12:16 AM, Jiri Pirko wrote: > Wed, Jul 22, 2020 at 05:30:05PM CEST, jacob.e.keller@intel.com wrote: >> >> >>> -----Original Message----- >>> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On >>> Visible in which sense? We don't show components anywhere if I'm not >>> mistaken. They are currently very rarely used. Basically we just ported >>> it from ethtool without much thinking. >>> >> >> Component names are used in devlink info and displayed to end users along with versions, plus they're names passed by the user in devlink flash update. As far as documented, we shouldn't add new components without associated versions in the info report. > > Okay. So it is loosely coupled. I think it would be nice to tight those > 2 togeter so it is not up to the driver how he decides to implement it. > I felt the coupling was quite clear from Jakub's recent documentation improvements in the devlink-flash.rst doc file. Are you thinking find some way to tie these two lists more closely in code? Thanks, Jake
Mon, Jul 27, 2020 at 08:13:12PM CEST, jacob.e.keller@intel.com wrote: > > >On 7/26/2020 12:16 AM, Jiri Pirko wrote: >> Wed, Jul 22, 2020 at 05:30:05PM CEST, jacob.e.keller@intel.com wrote: >>> >>> >>>> -----Original Message----- >>>> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On >>>> Visible in which sense? We don't show components anywhere if I'm not >>>> mistaken. They are currently very rarely used. Basically we just ported >>>> it from ethtool without much thinking. >>>> >>> >>> Component names are used in devlink info and displayed to end users along with versions, plus they're names passed by the user in devlink flash update. As far as documented, we shouldn't add new components without associated versions in the info report. >> >> Okay. So it is loosely coupled. I think it would be nice to tight those >> 2 togeter so it is not up to the driver how he decides to implement it. >> >I felt the coupling was quite clear from Jakub's recent documentation >improvements in the devlink-flash.rst doc file. > >Are you thinking find some way to tie these two lists more closely in code? Yes. Documentation is very easy to ignore unfortunatelly. The driver developer has to be tight up by the core code and api, I believe. > >Thanks, >Jake
On 7/28/2020 4:19 AM, Jiri Pirko wrote: > Yes. Documentation is very easy to ignore unfortunatelly. The driver > developer has to be tight up by the core code and api, I believe. > So I'm not sure what the best proposal here is. We do have a list of generic components, but given that each piece of HW has different elements, it's not always feasible to have fully generic names. Some of the names are driver specific. I guess we could use some system where components are "registered" when loading the devlink, so that they can be verified by the stack when used as a parameter for flash update? Perhaps take something like the table-driven approach used for infos and extend that into devlink core so that drivers basically register a table of the components which includes both a function callback that gets the version string as well as an indication of whether that component can be updated via flash_update? I know it would also be useful for ice to have a sort of "pre-info" callback that generates a context structure that is passed to each of the info callbacks. (that way a single up-front step could be to lookup the relevant information, and this is then forwarded to each of the formatter functions for each component). Am I on the right track here or just over-engineering?
On Tue, 28 Jul 2020 09:58:44 -0700 Jacob Keller wrote: > On 7/28/2020 4:19 AM, Jiri Pirko wrote: > > Yes. Documentation is very easy to ignore unfortunatelly. The driver > > developer has to be tight up by the core code and api, I believe. > > So I'm not sure what the best proposal here is. We do have a list of > generic components, but given that each piece of HW has different > elements, it's not always feasible to have fully generic names. Some of > the names are driver specific. > > I guess we could use some system where components are "registered" when > loading the devlink, so that they can be verified by the stack when used > as a parameter for flash update? Perhaps take something like the > table-driven approach used for infos and extend that into devlink core > so that drivers basically register a table of the components which > includes both a function callback that gets the version string as well > as an indication of whether that component can be updated via flash_update? > > I know it would also be useful for ice to have a sort of "pre-info" > callback that generates a context structure that is passed to each of > the info callbacks. (that way a single up-front step could be to lookup > the relevant information, and this is then forwarded to each of the > formatter functions for each component). > > Am I on the right track here or just over-engineering? I don't understand why we're having this conversation. No driver right now uses the component name. AFAIU we agreed not to use the component name for config vs code. So you may as well remove the component name from the devlink op and add a todo there saying "when adding component back, make sure it's tightly coupled to info".
On 7/28/2020 10:09 AM, Jakub Kicinski wrote: > On Tue, 28 Jul 2020 09:58:44 -0700 Jacob Keller wrote: >> On 7/28/2020 4:19 AM, Jiri Pirko wrote: >>> Yes. Documentation is very easy to ignore unfortunatelly. The driver >>> developer has to be tight up by the core code and api, I believe. >> >> So I'm not sure what the best proposal here is. We do have a list of >> generic components, but given that each piece of HW has different >> elements, it's not always feasible to have fully generic names. Some of >> the names are driver specific. >> >> I guess we could use some system where components are "registered" when >> loading the devlink, so that they can be verified by the stack when used >> as a parameter for flash update? Perhaps take something like the >> table-driven approach used for infos and extend that into devlink core >> so that drivers basically register a table of the components which >> includes both a function callback that gets the version string as well >> as an indication of whether that component can be updated via flash_update? >> >> I know it would also be useful for ice to have a sort of "pre-info" >> callback that generates a context structure that is passed to each of >> the info callbacks. (that way a single up-front step could be to lookup >> the relevant information, and this is then forwarded to each of the >> formatter functions for each component). >> >> Am I on the right track here or just over-engineering? > > I don't understand why we're having this conversation. > > No driver right now uses the component name. > > AFAIU we agreed not to use the component name for config vs code. > > So you may as well remove the component name from the devlink op and > add a todo there saying "when adding component back, make sure it's > tightly coupled to info". > Fair enough yea.
On 7/28/2020 10:09 AM, Jakub Kicinski wrote: > On Tue, 28 Jul 2020 09:58:44 -0700 Jacob Keller wrote: >> On 7/28/2020 4:19 AM, Jiri Pirko wrote: >>> Yes. Documentation is very easy to ignore unfortunatelly. The driver >>> developer has to be tight up by the core code and api, I believe. >> >> So I'm not sure what the best proposal here is. We do have a list of >> generic components, but given that each piece of HW has different >> elements, it's not always feasible to have fully generic names. Some of >> the names are driver specific. >> >> I guess we could use some system where components are "registered" when >> loading the devlink, so that they can be verified by the stack when used >> as a parameter for flash update? Perhaps take something like the >> table-driven approach used for infos and extend that into devlink core >> so that drivers basically register a table of the components which >> includes both a function callback that gets the version string as well >> as an indication of whether that component can be updated via flash_update? >> >> I know it would also be useful for ice to have a sort of "pre-info" >> callback that generates a context structure that is passed to each of >> the info callbacks. (that way a single up-front step could be to lookup >> the relevant information, and this is then forwarded to each of the >> formatter functions for each component). >> >> Am I on the right track here or just over-engineering? > > I don't understand why we're having this conversation. > > No driver right now uses the component name. > > AFAIU we agreed not to use the component name for config vs code. > > So you may as well remove the component name from the devlink op and > add a todo there saying "when adding component back, make sure it's > tightly coupled to info". > Another simpler option would be to just call .info_get and verify that the requested component appeared in that list. Ofcourse we'd be making this call every flash_update...
On 7/22/2020 9:52 AM, Jakub Kicinski wrote: > On Wed, 22 Jul 2020 15:30:05 +0000 Keller, Jacob E wrote: >>>>>> one by one and then omit the one(s) which is config (guessing which >>>>>> one that is based on the name). >>>>>> >>>>>> Wouldn't this be quite inconvenient? >>>>> >>>>> I see it as an extra knob that is actually somehow provides degradation >>>>> of components. >>>> >>>> Hm. We have the exact opposite view on the matter. To me components >>>> currently correspond to separate fw/hw entities, that's a very clear >>>> meaning. PHY firmware, management FW, UNDI. Now we would add a >>>> completely orthogonal meaning to the same API. >>> >>> I understand. My concern is, we would have a component with some >>> "subparts". Now it is some fuzzy vagely defined "config part", >>> in the future it might be something else. That is what I'm concerned >>> about. Components have clear api. >>> >>> So perhaps we can introduce something like "component mask", which would >>> allow to flash only part of the component. That is basically what Jacob >>> has, I would just like to have it well defined. >> >> So, we could make this selection a series of masked bits instead of a >> single enumeration value. > > I'd still argue that components (as defined in devlink info) and config > are pretty orthogonal. In my experience config is stored in its own > section of the flash, and some of the knobs are in no obvious way > associated with components (used by components). > > That said, if we rename the "component mask" to "update mask" that's > fine with me. > > Then we'd have > > bit 0 - don't overwrite config > bit 1 - don't overwrite identifiers > > ? > > Let's define a bit for "don't update program" when we actually need it. > One further wrinkle I was just reminded about. The ice hardware has a section of the flash which defines a "minimum security revision". All NVM images also have a "security revision". The firmware will fail to load if the NVM image's security revision is less than the mimimum security revision. The minimum security revision is not updated automatically. Current tools which had direct access have an optional "opt in to minimum security revision update" which would optionally bump the minimum security revision after an update. The intent is that once an image is tested and verified to be stable, an administrator can opt in to prevent downgrade below that security revision. (Thus preventing potential downgrade to a known insecure image). The folks adjusting our tools would like to continue to support this. I think the best solution would be to have both the security revision and minimum security revision become components, i.e. "fw.mgmt.security_revision" and "fw.mgmt.min_security_revision" (maybe shortened like "secrev or srev?), and then use the fw.mgmt.min_security_revision component name in the flash update request. The security revision is tied into the management firmware image and would always be updated when an image is updated, but the minimum revision is only updated on an explicit request request. In theory this could be done as part of this overwrite, but since I suspect this is somewhat device specific, (not sure other vendors have something similar?), and because there is a valid/known version we can report I think a component makes the most sense.
On Wed, 29 Jul 2020 15:49:05 -0700 Jacob Keller wrote: > The security revision is tied into the management firmware image and > would always be updated when an image is updated, but the minimum > revision is only updated on an explicit request request. Does it have to be updated during FW flashing? Can't it be a devlink param?
On 7/29/2020 4:16 PM, Jakub Kicinski wrote: > On Wed, 29 Jul 2020 15:49:05 -0700 Jacob Keller wrote: >> The security revision is tied into the management firmware image and >> would always be updated when an image is updated, but the minimum >> revision is only updated on an explicit request request. > > Does it have to be updated during FW flashing? Can't it be a devlink > param? > Oh, right. I'd forgotten about that type of parameter. Makes sense. I'll implement the current security revision as a component of flash info (so that it can be reported via devlink info, and can't be changed) but the minimum should be able to be a parameter just fine. Thanks, Jake
diff --git a/Documentation/networking/devlink/devlink-flash.rst b/Documentation/networking/devlink/devlink-flash.rst index 40a87c0222cb..75f7e284a97a 100644 --- a/Documentation/networking/devlink/devlink-flash.rst +++ b/Documentation/networking/devlink/devlink-flash.rst @@ -16,6 +16,37 @@ Note that the file name is a path relative to the firmware loading path (usually ``/lib/firmware/``). Drivers may send status updates to inform user space about the progress of the update operation. +Overwrite Mode +============== + +The ``devlink-flash`` command allows optionally specifying an overwrite mode +indicating how the device should handle static settings and fields in the +device flash when updating. + +.. list-table:: List of overwrite modes + :widths: 5 95 + + * - Name + - Description + * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING`` + - Do not overwrite any settings or fields in the device flash; only + update the associated firmware binaries. This preserves settings + stored in the flash, as well as device identifiers. This is the + default if no overwrite mode is specified. + * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS`` + - Overwrite device settings stored in the flash with settings from the + provided image, but do not overwrite device identifiers such as MAC + addresses or serial identifiers. This may be useful to restore a + device to default settings from a new image. + * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING`` + - Overwrite everything in the device flash including settings and + device identifiers with the contents from the provided flash image. + Unless the provided image has been customized for this device, it + will result in clearing the identifying information in the device + flash. This mode is primarily intended for device manufacturers + performing initial device programming, and is not expected to be + necessary when performing regular flash updates. + Firmware Loading ================ diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst index 237848d56f9b..0f4428d7e693 100644 --- a/Documentation/networking/devlink/ice.rst +++ b/Documentation/networking/devlink/ice.rst @@ -81,6 +81,33 @@ The ``ice`` driver reports the following versions - 0xee16ced7 - The first 4 bytes of the hash of the netlist module contents. +Flash Update +============ + +The ``ice`` driver implements support for flash update using the +``devlink-flash`` interface. It supports updating the device flash using a +combined flash image that contains the ``fw.mgmt``, ``fw.undi``, and +``fw.netlist`` components. + +.. list-table:: List of supported overwrite modes + :widths: 5 95 + + * - Name + - Behavior + * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING`` + - Do not overwrite any settings or device identifying information. + * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS`` + - Overwrite device settings, but not identifying information. This + includes overwriting the port configuration that determines the + number of physical functions the device will initialize with. + * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING`` + - Overwrite everything in the flash with the contents from the provided + flash image. This includes overwriting all settings as well as device + identifying information such as the MAC address and device serial + number. It is expected that this be used with an image customized for + the specific device, and is not necessary or expected in most + circumstances. + Regions ======= diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c index 3a854195d5b0..eec99acd9d39 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c @@ -18,7 +18,9 @@ static int bnxt_dl_flash_update(struct devlink *dl, const char *filename, - const char *region, struct netlink_ext_ack *extack) + const char *region, + enum devlink_flash_update_overwrite_mode mode, + struct netlink_ext_ack *extack) { struct bnxt *bp = bnxt_get_bp_from_dl(dl); int rc; @@ -26,6 +28,9 @@ bnxt_dl_flash_update(struct devlink *dl, const char *filename, if (region) return -EOPNOTSUPP; + if (mode != DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING) + return -EOPNOTSUPP; + if (!BNXT_PF(bp)) { NL_SET_ERR_MSG_MOD(extack, "flash update not supported from a VF"); diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c index a40a10ac1ee9..7994b5615b0e 100644 --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c @@ -283,6 +283,7 @@ static int hinic_firmware_update(struct hinic_devlink_priv *priv, static int hinic_devlink_flash_update(struct devlink *devlink, const char *file_name, const char *component, + enum devlink_flash_update_overwrite_mode mode, struct netlink_ext_ack *extack) { struct hinic_devlink_priv *priv = devlink_priv(devlink); @@ -291,6 +292,8 @@ static int hinic_devlink_flash_update(struct devlink *devlink, if (component) return -EOPNOTSUPP; + if (mode != DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING) + return -EOPNOTSUPP; err = request_firmware_direct(&fw, file_name, &priv->hwdev->hwif->pdev->dev); diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c index dbbd8b6f9d1a..d53e5d86857a 100644 --- a/drivers/net/ethernet/intel/ice/ice_devlink.c +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c @@ -244,7 +244,9 @@ static int ice_devlink_info_get(struct devlink *devlink, */ static int ice_devlink_flash_update(struct devlink *devlink, const char *path, - const char *component, struct netlink_ext_ack *extack) + const char *component, + enum devlink_flash_update_overwrite_mode mode, + struct netlink_ext_ack *extack) { struct ice_pf *pf = devlink_priv(devlink); struct device *dev = &pf->pdev->dev; @@ -274,7 +276,7 @@ ice_devlink_flash_update(struct devlink *devlink, const char *path, devlink_flash_update_begin_notify(devlink); devlink_flash_update_status_notify(devlink, "Preparing to flash", component, 0, 0); - err = ice_flash_pldm_image(pf, fw, extack); + err = ice_flash_pldm_image(pf, fw, mode, extack); devlink_flash_update_end_notify(devlink); release_firmware(fw); diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c b/drivers/net/ethernet/intel/ice/ice_fw_update.c index 51b575ba197d..7d053e4879db 100644 --- a/drivers/net/ethernet/intel/ice/ice_fw_update.c +++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c @@ -630,6 +630,7 @@ static const struct pldmfw_ops ice_fwu_ops = { * ice_flash_pldm_image - Write a PLDM-formatted firmware image to the device * @pf: private device driver structure * @fw: firmware object pointing to the relevant firmware file + * @mode: flash overwrite mode * @extack: netlink extended ACK structure * * Parse the data for a given firmware file, verifying that it is a valid PLDM @@ -643,6 +644,7 @@ static const struct pldmfw_ops ice_fwu_ops = { * Returns: zero on success or a negative error code on failure. */ int ice_flash_pldm_image(struct ice_pf *pf, const struct firmware *fw, + enum devlink_flash_update_overwrite_mode mode, struct netlink_ext_ack *extack) { struct device *dev = ice_pf_to_dev(pf); @@ -657,7 +659,27 @@ int ice_flash_pldm_image(struct ice_pf *pf, const struct firmware *fw, priv.context.dev = dev; priv.extack = extack; priv.pf = pf; - priv.activate_flags = ICE_AQC_NVM_PRESERVE_ALL; + + /* Based on the requested overwrite mode, determine what preservation + * level to specify when activating the NVM banks at the end of the + * update. + */ + switch (mode) { + case DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING: + /* Preserve all settings and fields in the existing flash */ + priv.activate_flags = ICE_AQC_NVM_PRESERVE_ALL; + break; + case DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS: + /* Overwrite settings, but preserve limited fields */ + priv.activate_flags = ICE_AQC_NVM_PRESERVE_SELECTED; + break; + case DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING: + /* Overwrite everything in the flash */ + priv.activate_flags = ICE_AQC_NVM_NO_PRESERVATION; + break; + default: + return -EOPNOTSUPP; + } status = ice_acquire_nvm(hw, ICE_RES_WRITE); if (status) { diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.h b/drivers/net/ethernet/intel/ice/ice_fw_update.h index 79472cc618b4..66d539ae87d6 100644 --- a/drivers/net/ethernet/intel/ice/ice_fw_update.h +++ b/drivers/net/ethernet/intel/ice/ice_fw_update.h @@ -5,6 +5,7 @@ #define _ICE_FW_UPDATE_H_ int ice_flash_pldm_image(struct ice_pf *pf, const struct firmware *fw, + enum devlink_flash_update_overwrite_mode mode, struct netlink_ext_ack *extack); int ice_check_for_pending_update(struct ice_pf *pf, const char *component, struct netlink_ext_ack *extack); diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c index c709e9a385f6..8c7472ff0376 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c @@ -10,6 +10,7 @@ static int mlx5_devlink_flash_update(struct devlink *devlink, const char *file_name, const char *component, + enum devlink_flash_update_overwrite_mode mode, struct netlink_ext_ack *extack) { struct mlx5_core_dev *dev = devlink_priv(devlink); @@ -19,6 +20,9 @@ static int mlx5_devlink_flash_update(struct devlink *devlink, if (component) return -EOPNOTSUPP; + if (mode != DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING) + return -EOPNOTSUPP; + err = request_firmware_direct(&fw, file_name, &dev->pdev->dev); if (err) return err; diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c index 1363168b3c82..310a863f0b69 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/core.c +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c @@ -1139,6 +1139,7 @@ mlxsw_devlink_core_bus_device_reload_up(struct devlink *devlink, static int mlxsw_devlink_flash_update(struct devlink *devlink, const char *file_name, const char *component, + enum devlink_flash_update_overwrite_mode mode, struct netlink_ext_ack *extack) { struct mlxsw_core *mlxsw_core = devlink_priv(devlink); @@ -1147,7 +1148,7 @@ static int mlxsw_devlink_flash_update(struct devlink *devlink, if (!mlxsw_driver->flash_update) return -EOPNOTSUPP; return mlxsw_driver->flash_update(mlxsw_core, file_name, - component, extack); + component, mode, extack); } static int mlxsw_devlink_trap_init(struct devlink *devlink, diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h index c1c1e039323a..d4ec9cb6e5f3 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/core.h +++ b/drivers/net/ethernet/mellanox/mlxsw/core.h @@ -319,6 +319,7 @@ struct mlxsw_driver { u32 *p_cur, u32 *p_max); int (*flash_update)(struct mlxsw_core *mlxsw_core, const char *file_name, const char *component, + enum devlink_flash_update_overwrite_mode mode, struct netlink_ext_ack *extack); int (*trap_init)(struct mlxsw_core *mlxsw_core, const struct devlink_trap *trap, void *trap_ctx); diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c index 519eb44e4097..52f099659827 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c @@ -419,6 +419,7 @@ static int mlxsw_sp_fw_rev_validate(struct mlxsw_sp *mlxsw_sp) static int mlxsw_sp_flash_update(struct mlxsw_core *mlxsw_core, const char *file_name, const char *component, + enum devlink_flash_update_overwrite_mode mode, struct netlink_ext_ack *extack) { struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core); @@ -428,6 +429,9 @@ static int mlxsw_sp_flash_update(struct mlxsw_core *mlxsw_core, if (component) return -EOPNOTSUPP; + if (mode != DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING) + return -EOPNOTSUPP; + err = request_firmware_direct(&firmware, file_name, mlxsw_sp->bus_info->dev); if (err) diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c index be52510d446b..37401cde9fe9 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c @@ -330,10 +330,14 @@ nfp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req, static int nfp_devlink_flash_update(struct devlink *devlink, const char *path, - const char *component, struct netlink_ext_ack *extack) + const char *component, + enum devlink_flash_update_overwrite_mode mode, + struct netlink_ext_ack *extack) { if (component) return -EOPNOTSUPP; + if (mode != DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING) + return -EOPNOTSUPP; return nfp_flash_update_common(devlink_priv(devlink), path, extack); } diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index ce719c830a77..a75b1421d179 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c @@ -742,6 +742,7 @@ static int nsim_dev_info_get(struct devlink *devlink, static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name, const char *component, + enum devlink_flash_update_overwrite_mode mode, struct netlink_ext_ack *extack) { struct nsim_dev *nsim_dev = devlink_priv(devlink); diff --git a/include/net/devlink.h b/include/net/devlink.h index 913e8679ae35..a851209c7145 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1045,6 +1045,7 @@ struct devlink_ops { struct netlink_ext_ack *extack); int (*flash_update)(struct devlink *devlink, const char *file_name, const char *component, + enum devlink_flash_update_overwrite_mode mode, struct netlink_ext_ack *extack); /** * @trap_init: Trap initialization function. diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index cfef4245ea5a..b5341fe5fa61 100644 --- a/include/uapi/linux/devlink.h +++ b/include/uapi/linux/devlink.h @@ -228,6 +228,20 @@ enum { DEVLINK_ATTR_STATS_MAX = __DEVLINK_ATTR_STATS_MAX - 1 }; +/* All flash update overwrite modes must be documented in + * Documentation/networking/devlink/devlink-flash.rst + */ +enum devlink_flash_update_overwrite_mode { + DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING, + DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS, + DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING, + + /* Add new flash overwrite modes above */ + __DEVLINK_FLASH_UPDATE_OVERWRITE_MODE_MAX, + DEVLINK_FLASH_UPDATE_OVERWRITE_MODE_MAX = + __DEVLINK_FLASH_UPDATE_OVERWRITE_MODE_MAX - 1, +}; + /** * enum devlink_trap_action - Packet trap action. * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not @@ -458,6 +472,8 @@ enum devlink_attr { DEVLINK_ATTR_PORT_LANES, /* u32 */ DEVLINK_ATTR_PORT_SPLITTABLE, /* u8 */ + DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MODE, /* u8 */ + /* add new attributes above here, update the policy in devlink.c */ __DEVLINK_ATTR_MAX, diff --git a/net/core/devlink.c b/net/core/devlink.c index 6335e1851088..48b3a5739363 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -3097,9 +3097,10 @@ EXPORT_SYMBOL_GPL(devlink_flash_update_status_notify); static int devlink_nl_cmd_flash_update(struct sk_buff *skb, struct genl_info *info) { + enum devlink_flash_update_overwrite_mode mode; struct devlink *devlink = info->user_ptr[0]; + struct nlattr *nla_component, *nla_mode; const char *file_name, *component; - struct nlattr *nla_component; if (!devlink->ops->flash_update) return -EOPNOTSUPP; @@ -3111,7 +3112,13 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb, nla_component = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT]; component = nla_component ? nla_data(nla_component) : NULL; - return devlink->ops->flash_update(devlink, file_name, component, + nla_mode = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MODE]; + mode = nla_mode ? nla_get_u8(nla_mode) : DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING; + + if (mode > DEVLINK_FLASH_UPDATE_OVERWRITE_MODE_MAX) + return -EINVAL; + + return devlink->ops->flash_update(devlink, file_name, component, mode, info->extack); } @@ -6999,6 +7006,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { [DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER] = { .type = NLA_U8 }, [DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME] = { .type = NLA_NUL_STRING }, [DEVLINK_ATTR_FLASH_UPDATE_COMPONENT] = { .type = NLA_NUL_STRING }, + [DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MODE] = { .type = NLA_U8 }, [DEVLINK_ATTR_TRAP_NAME] = { .type = NLA_NUL_STRING }, [DEVLINK_ATTR_TRAP_ACTION] = { .type = NLA_U8 }, [DEVLINK_ATTR_TRAP_GROUP_NAME] = { .type = NLA_NUL_STRING }, @@ -9564,7 +9572,9 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name) } mutex_lock(&devlink->lock); - ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL); + ret = devlink->ops->flash_update(devlink, file_name, NULL, + DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING, + NULL); mutex_unlock(&devlink->lock); out:
A flash image may contain settings or device identifying information. When performing a flash update, these settings and information may conflict with contents already in the flash. Devices may handle this conflict in multiple ways. Add a new attribute to the devlink command, DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MODE, which specifies how the device should handle these settings and fields. DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING, the default, requests that all settings and device identifiers within the current flash are kept. That is, no settings or fields will be overwritten. This is the expected behavior for most updates, and appears to be how all of the drivers are implemented today. DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS, requests that the device overwrite any device settings in the flash section with the settings from the flash image, but to preserve identifiers such as the MAC address and serial identifier. This may be useful as a way to restore a device to known-good settings from a new flash image. DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING, requests that all content in the flash image be preserved over content of flash on the device. This mode requests the device to completely overwrite the flash section, possibly changing settings and device identifiers. The primary motivation is to support writing initial device identifiers during manufacturing. It is not expected to be necessary in normal end-user flash updates. For the ice driver, implement support for the overwrite mode by selecting the associated preservation level to request from firmware. For all other drivers that support flash update, require that the mode be DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING, which is the expected default. Update the documentation to explain the overwrite mode attribute. Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Cc: Jiri Pirko <jiri@mellanox.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Jonathan Corbet <corbet@lwn.net> Cc: Michael Chan <michael.chan@broadcom.com> Cc: Bin Luo <luobin9@huawei.com> Cc: Saeed Mahameed <saeedm@mellanox.com> Cc: Leon Romanovsky <leon@kernel.org> Cc: Ido Schimmel <idosch@mellanox.com> Cc: Danielle Ratson <danieller@mellanox.com> --- .../networking/devlink/devlink-flash.rst | 31 +++++++++++++++++++ Documentation/networking/devlink/ice.rst | 27 ++++++++++++++++ .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 7 ++++- .../net/ethernet/huawei/hinic/hinic_devlink.c | 3 ++ drivers/net/ethernet/intel/ice/ice_devlink.c | 6 ++-- .../net/ethernet/intel/ice/ice_fw_update.c | 24 +++++++++++++- .../net/ethernet/intel/ice/ice_fw_update.h | 1 + .../net/ethernet/mellanox/mlx5/core/devlink.c | 4 +++ drivers/net/ethernet/mellanox/mlxsw/core.c | 3 +- drivers/net/ethernet/mellanox/mlxsw/core.h | 1 + .../net/ethernet/mellanox/mlxsw/spectrum.c | 4 +++ .../net/ethernet/netronome/nfp/nfp_devlink.c | 6 +++- drivers/net/netdevsim/dev.c | 1 + include/net/devlink.h | 1 + include/uapi/linux/devlink.h | 16 ++++++++++ net/core/devlink.c | 16 ++++++++-- 16 files changed, 142 insertions(+), 9 deletions(-)