Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net-next] cxgb4: Add support to flash firmware config image
@ 2020-07-30 15:11 Ganji Aravind
  2020-07-30 23:23 ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Ganji Aravind @ 2020-07-30 15:11 UTC (permalink / raw)
  To: netdev; +Cc: davem, vishal, rahul.lakkireddy

Update set_flash to flash firmware configuration image
to flash region.

Signed-off-by: Ganji Aravind <ganji.aravind@chelsio.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h    |  3 +-
 .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    | 93 +++++++++++++++++++
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c    | 13 ++-
 3 files changed, 105 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index adbc0d088070..081f94c539a9 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -143,7 +143,8 @@ enum {
 	CXGB4_ETHTOOL_FLASH_FW = 1,
 	CXGB4_ETHTOOL_FLASH_PHY = 2,
 	CXGB4_ETHTOOL_FLASH_BOOT = 3,
-	CXGB4_ETHTOOL_FLASH_BOOTCFG = 4
+	CXGB4_ETHTOOL_FLASH_BOOTCFG = 4,
+	CXGB4_ETHTOOL_FLASH_FWCFG = 5
 };
 
 struct cxgb4_bootcfg_data {
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
index 12ef9ddd1e54..42e2cf5c33f3 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
@@ -40,6 +40,7 @@ static const char * const flash_region_strings[] = {
 	"PHY Firmware",
 	"Boot",
 	"Boot CFG",
+	"Firmware CFG",
 };
 
 static const char stats_strings[][ETH_GSTRING_LEN] = {
@@ -1351,6 +1352,93 @@ static int cxgb4_ethtool_flash_fw(struct net_device *netdev,
 	return ret;
 }
 
+static const u8 *cxgb4_token_match(const u8 *input, const u8 *token)
+{
+	const u8 *token_match;
+
+	token_match = strstr(input, token);
+	if (token_match)
+		return (token_match + strlen(token));
+
+	return NULL;
+}
+
+static u32 cxgb4_compute_fwcfg_csum(const u8 *data, size_t st_size)
+{
+	unsigned int n, rem;
+	const __be32 *uip;
+	u32 csum;
+
+	uip = (const __be32 *)data;
+	for (csum = 0, n = st_size >> 2; n; n--)
+		csum += be32_to_cpu(*uip++);
+
+	rem = st_size & 0x3;
+	if (rem) {
+		union {
+			char buf[4];
+			__be32 u;
+		} last;
+		char *cp;
+
+		last.u = *uip;
+		for (cp = &last.buf[rem], n = 4 - rem; n; n--)
+			*cp++ = 0;
+		csum += be32_to_cpu(last.u);
+	}
+
+	return csum;
+}
+
+static int cxgb4_validate_fwcfg_image(const u8 *data)
+{
+	const u8 *fwcfg_fini, *version, *checksum;
+	u32 calculated_csum = 0;
+	u8 fw_csum[16] = { 0 };
+	u8 fw_ver[8] = { 0 };
+	size_t st_size;
+
+	fwcfg_fini = cxgb4_token_match(data, "[fini]\n");
+	if (!fwcfg_fini)
+		return -EINVAL;
+
+	version = cxgb4_token_match(fwcfg_fini, "version = ");
+	if (!version)
+		return -EINVAL;
+
+	checksum = cxgb4_token_match(fwcfg_fini, "checksum = ");
+	if (!checksum)
+		return -EINVAL;
+
+	st_size = fwcfg_fini - data;
+	calculated_csum = cxgb4_compute_fwcfg_csum(data, st_size);
+
+	snprintf(fw_csum, sizeof(fw_csum), "0x%x", calculated_csum);
+	snprintf(fw_ver, sizeof(fw_ver), "0x%x", PCI_VENDOR_ID_CHELSIO);
+
+	if (strncmp(fw_csum, checksum, strlen(fw_csum)) &&
+	    strncmp(fw_ver, version, strlen(fw_ver)))
+		return -EPERM;
+
+	return 0;
+}
+
+static int cxgb4_ethtool_flash_fwcfg(struct net_device *netdev,
+				     const u8 *data, u32 size)
+{
+	struct adapter *adap = netdev2adap(netdev);
+	int ret;
+
+	ret = cxgb4_validate_fwcfg_image(data);
+	if (ret) {
+		dev_err(adap->pdev_dev,
+			"Firmware config validation error: %d\n", ret);
+		return ret;
+	}
+
+	return t4_load_cfg(adap, data, size);
+}
+
 static int cxgb4_ethtool_flash_region(struct net_device *netdev,
 				      const u8 *data, u32 size, u32 region)
 {
@@ -1370,6 +1458,9 @@ static int cxgb4_ethtool_flash_region(struct net_device *netdev,
 	case CXGB4_ETHTOOL_FLASH_BOOTCFG:
 		ret = cxgb4_ethtool_flash_bootcfg(netdev, data, size);
 		break;
+	case CXGB4_ETHTOOL_FLASH_FWCFG:
+		ret = cxgb4_ethtool_flash_fwcfg(netdev, data, size);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -1448,6 +1539,8 @@ static int cxgb4_ethtool_get_flash_region(const u8 *data, u32 *size)
 		return CXGB4_ETHTOOL_FLASH_PHY;
 	if (!cxgb4_validate_bootcfg_image(data, size))
 		return CXGB4_ETHTOOL_FLASH_BOOTCFG;
+	if (!cxgb4_validate_fwcfg_image(data))
+		return CXGB4_ETHTOOL_FLASH_FWCFG;
 
 	return -EOPNOTSUPP;
 }
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
index 8a56491bb034..bf3eea91a2cc 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
@@ -10208,11 +10208,18 @@ int t4_load_cfg(struct adapter *adap, const u8 *cfg_data, unsigned int size)
 
 	/* this will write to the flash up to SF_PAGE_SIZE at a time */
 	for (i = 0; i < size; i += SF_PAGE_SIZE) {
-		if ((size - i) <  SF_PAGE_SIZE)
+		if ((size - i) <  SF_PAGE_SIZE) {
+			u8 buf[SF_PAGE_SIZE] = { 0 };
+			u8 npad;
+
 			n = size - i;
-		else
+			npad = ((n + 4 - 1) & ~3) - n;
+			memcpy(buf, cfg_data, n);
+			ret = t4_write_flash(adap, addr, n + npad, buf);
+		} else {
 			n = SF_PAGE_SIZE;
-		ret = t4_write_flash(adap, addr, n, cfg_data);
+			ret = t4_write_flash(adap, addr, n, cfg_data);
+		}
 		if (ret)
 			goto out;
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] cxgb4: Add support to flash firmware config image
  2020-07-30 15:11 [PATCH net-next] cxgb4: Add support to flash firmware config image Ganji Aravind
@ 2020-07-30 23:23 ` Jakub Kicinski
  2020-07-31 11:09   ` Ganji Aravind
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2020-07-30 23:23 UTC (permalink / raw)
  To: Ganji Aravind; +Cc: netdev, davem, vishal, rahul.lakkireddy

On Thu, 30 Jul 2020 20:41:38 +0530 Ganji Aravind wrote:
> Update set_flash to flash firmware configuration image
> to flash region.

And the reason why you need to flash some .ini files separately is?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] cxgb4: Add support to flash firmware config image
  2020-07-30 23:23 ` Jakub Kicinski
@ 2020-07-31 11:09   ` Ganji Aravind
  2020-07-31 18:00     ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Ganji Aravind @ 2020-07-31 11:09 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, davem, vishal, rahul.lakkireddy

On Thursday, July 07/30/20, 2020 at 16:23:35 -0700, Jakub Kicinski wrote:
> On Thu, 30 Jul 2020 20:41:38 +0530 Ganji Aravind wrote:
> > Update set_flash to flash firmware configuration image
> > to flash region.
> 
> And the reason why you need to flash some .ini files separately is?

Hi Jakub,

The firmware config file contains information on how the firmware
should distribute the hardware resources among NIC and
Upper Layer Drivers(ULD), like iWARP, crypto, filtering, etc.

The firmware image comes with an in-built default config file that
distributes resources among the NIC and all the ULDs. However, in
some cases, where we don't want to run a particular ULD, or if we
want to redistribute the resources, then we'd modify the firmware
config file and then firmware will redistribute those resources
according to the new configuration. So, if firmware finds this
custom config file in flash, it reads this first. Otherwise, it'll
continue initializing the adapter with its own in-built default
config file.

Thanks
-Ganji Aravind.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] cxgb4: Add support to flash firmware config image
  2020-07-31 11:09   ` Ganji Aravind
@ 2020-07-31 18:00     ` Jakub Kicinski
  2020-07-31 21:17       ` Rahul Lakkireddy
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2020-07-31 18:00 UTC (permalink / raw)
  To: Ganji Aravind; +Cc: netdev, davem, vishal, rahul.lakkireddy

On Fri, 31 Jul 2020 16:39:04 +0530 Ganji Aravind wrote:
> On Thursday, July 07/30/20, 2020 at 16:23:35 -0700, Jakub Kicinski wrote:
> > On Thu, 30 Jul 2020 20:41:38 +0530 Ganji Aravind wrote:  
> > > Update set_flash to flash firmware configuration image
> > > to flash region.  
> > 
> > And the reason why you need to flash some .ini files separately is?  
> 
> Hi Jakub,
> 
> The firmware config file contains information on how the firmware
> should distribute the hardware resources among NIC and
> Upper Layer Drivers(ULD), like iWARP, crypto, filtering, etc.
> 
> The firmware image comes with an in-built default config file that
> distributes resources among the NIC and all the ULDs. However, in
> some cases, where we don't want to run a particular ULD, or if we
> want to redistribute the resources, then we'd modify the firmware
> config file and then firmware will redistribute those resources
> according to the new configuration. So, if firmware finds this
> custom config file in flash, it reads this first. Otherwise, it'll
> continue initializing the adapter with its own in-built default
> config file.

Sounds like something devlink could be extended to do.

Firmware update interface is not for configuration.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] cxgb4: Add support to flash firmware config image
  2020-07-31 18:00     ` Jakub Kicinski
@ 2020-07-31 21:17       ` Rahul Lakkireddy
  2020-08-02  4:22         ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Rahul Lakkireddy @ 2020-07-31 21:17 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: Ganji Aravind, netdev, davem, vishal

On Friday, July 07/31/20, 2020 at 11:00:08 -0700, Jakub Kicinski wrote:
> On Fri, 31 Jul 2020 16:39:04 +0530 Ganji Aravind wrote:
> > On Thursday, July 07/30/20, 2020 at 16:23:35 -0700, Jakub Kicinski wrote:
> > > On Thu, 30 Jul 2020 20:41:38 +0530 Ganji Aravind wrote:  
> > > > Update set_flash to flash firmware configuration image
> > > > to flash region.  
> > > 
> > > And the reason why you need to flash some .ini files separately is?  
> > 
> > Hi Jakub,
> > 
> > The firmware config file contains information on how the firmware
> > should distribute the hardware resources among NIC and
> > Upper Layer Drivers(ULD), like iWARP, crypto, filtering, etc.
> > 
> > The firmware image comes with an in-built default config file that
> > distributes resources among the NIC and all the ULDs. However, in
> > some cases, where we don't want to run a particular ULD, or if we
> > want to redistribute the resources, then we'd modify the firmware
> > config file and then firmware will redistribute those resources
> > according to the new configuration. So, if firmware finds this
> > custom config file in flash, it reads this first. Otherwise, it'll
> > continue initializing the adapter with its own in-built default
> > config file.
> 
> Sounds like something devlink could be extended to do.
> 
> Firmware update interface is not for configuration.

I thought /lib/firmware is where firmware related files need to be
placed and ethtool --flash needs to be used to program them to
their respective locations on adapter's flash.

Note that we don't have devlink support in our driver yet. And, we're
a bit confused here on why this already existing ethtool feature needs
to be duplicated to devlink.

Thanks,
Rahul

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] cxgb4: Add support to flash firmware config image
  2020-07-31 21:17       ` Rahul Lakkireddy
@ 2020-08-02  4:22         ` Jakub Kicinski
  2020-08-02 17:12           ` Rahul Lakkireddy
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2020-08-02  4:22 UTC (permalink / raw)
  To: Rahul Lakkireddy; +Cc: Ganji Aravind, netdev, davem, vishal

On Sat, 1 Aug 2020 02:47:38 +0530 Rahul Lakkireddy wrote:
> I thought /lib/firmware is where firmware related files need to be
> placed and ethtool --flash needs to be used to program them to
> their respective locations on adapter's flash.

Our goal is to provide solid, common interfaces for Linux users to rely
on. Not give way to vendor specific "solutions" like uploading ini files
to perform device configuration.

> Note that we don't have devlink support in our driver yet. And,
> we're a bit confused here on why this already existing ethtool
> feature needs to be duplicated to devlink.

To be clear - I'm suggesting the creation of a more targeted APIs 
to control the settings you have encoded _inside_ the ini file. 
Not a new interface for an whole sale config upload.

Worst case scenario - if the settings are really device specific 
you can try to use devlink device parameters.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] cxgb4: Add support to flash firmware config image
  2020-08-02  4:22         ` Jakub Kicinski
@ 2020-08-02 17:12           ` Rahul Lakkireddy
  2020-08-03 21:13             ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Rahul Lakkireddy @ 2020-08-02 17:12 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: Ganji Aravind, netdev, davem, vishal

On Saturday, August 08/01/20, 2020 at 21:22:02 -0700, Jakub Kicinski wrote:
> On Sat, 1 Aug 2020 02:47:38 +0530 Rahul Lakkireddy wrote:
> > I thought /lib/firmware is where firmware related files need to be
> > placed and ethtool --flash needs to be used to program them to
> > their respective locations on adapter's flash.
> 
> Our goal is to provide solid, common interfaces for Linux users to rely
> on. Not give way to vendor specific "solutions" like uploading ini files
> to perform device configuration.
> 
> > Note that we don't have devlink support in our driver yet. And,
> > we're a bit confused here on why this already existing ethtool
> > feature needs to be duplicated to devlink.
> 
> To be clear - I'm suggesting the creation of a more targeted APIs 
> to control the settings you have encoded _inside_ the ini file. 
> Not a new interface for an whole sale config upload.
> 
> Worst case scenario - if the settings are really device specific 
> you can try to use devlink device parameters.

The config file contains very low-level firmware and device specific
params and most of them are dependent on the type of Chelsio NIC.
The params are mostly device dependent register-value pairs.
We don't see users messing around with the params on their own
without consultation. The users only need some mechanism to flash
the custom config file shared by us on to their adapter. After
device restart, the firmware will automatically pick up the flashed
config file and redistribute the resources, as per their requested
use-case.

We're already foreseeing very long awkward list (more than 50 params)
for mapping the config file to devlink-dev params and are hoping this
is fine. Here's a sample on how it would look.

hw_sge_reg_1008=0x40800
hw_sge_reg_100c=0x22222222
hw_sge_reg_10a0=0x01040810
hw_tp_reg_7d04=0x00010000
hw_tp_reg_7dc0=0x0e2f8849

and so on.

Thanks,
Rahul

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] cxgb4: Add support to flash firmware config image
  2020-08-02 17:12           ` Rahul Lakkireddy
@ 2020-08-03 21:13             ` Jakub Kicinski
  2020-08-04  9:43               ` Rahul Lakkireddy
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2020-08-03 21:13 UTC (permalink / raw)
  To: Rahul Lakkireddy; +Cc: Ganji Aravind, netdev, davem, vishal

On Sun, 2 Aug 2020 22:42:28 +0530 Rahul Lakkireddy wrote:
> The config file contains very low-level firmware and device specific
> params and most of them are dependent on the type of Chelsio NIC.
> The params are mostly device dependent register-value pairs.
> We don't see users messing around with the params on their own
> without consultation. The users only need some mechanism to flash
> the custom config file shared by us on to their adapter. After
> device restart, the firmware will automatically pick up the flashed
> config file and redistribute the resources, as per their requested
> use-case.
> 
> We're already foreseeing very long awkward list (more than 50 params)
> for mapping the config file to devlink-dev params and are hoping this
> is fine. Here's a sample on how it would look.
> 
> hw_sge_reg_1008=0x40800
> hw_sge_reg_100c=0x22222222
> hw_sge_reg_10a0=0x01040810
> hw_tp_reg_7d04=0x00010000
> hw_tp_reg_7dc0=0x0e2f8849
> 
> and so on.

I have no details on what you're actually storing in the config, 
and I don't care what your format is.

If it's a configuration parameter - it needs a proper API.

If it's a low level board param or such - it doesn't need a separate
flashable partition and can come with the rest of FW.

I know the firmware flashing interface is a lovely, binary, opaque
interface which vendors love. We'll not entertain this kind of abuse.

Nacked-by: Jakub Kicinski <kuba@kernel.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] cxgb4: Add support to flash firmware config image
  2020-08-03 21:13             ` Jakub Kicinski
@ 2020-08-04  9:43               ` Rahul Lakkireddy
  0 siblings, 0 replies; 9+ messages in thread
From: Rahul Lakkireddy @ 2020-08-04  9:43 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: Ganji Aravind, netdev, davem, vishal

On Monday, August 08/03/20, 2020 at 14:13:04 -0700, Jakub Kicinski wrote:
> On Sun, 2 Aug 2020 22:42:28 +0530 Rahul Lakkireddy wrote:
> > The config file contains very low-level firmware and device specific
> > params and most of them are dependent on the type of Chelsio NIC.
> > The params are mostly device dependent register-value pairs.
> > We don't see users messing around with the params on their own
> > without consultation. The users only need some mechanism to flash
> > the custom config file shared by us on to their adapter. After
> > device restart, the firmware will automatically pick up the flashed
> > config file and redistribute the resources, as per their requested
> > use-case.
> > 
> > We're already foreseeing very long awkward list (more than 50 params)
> > for mapping the config file to devlink-dev params and are hoping this
> > is fine. Here's a sample on how it would look.
> > 
> > hw_sge_reg_1008=0x40800
> > hw_sge_reg_100c=0x22222222
> > hw_sge_reg_10a0=0x01040810
> > hw_tp_reg_7d04=0x00010000
> > hw_tp_reg_7dc0=0x0e2f8849
> > 
> > and so on.
> 
> I have no details on what you're actually storing in the config, 
> and I don't care what your format is.
> 
> If it's a configuration parameter - it needs a proper API.
> 
> If it's a low level board param or such - it doesn't need a separate
> flashable partition and can come with the rest of FW.
> 
> I know the firmware flashing interface is a lovely, binary, opaque
> interface which vendors love. We'll not entertain this kind of abuse.
> 
> Nacked-by: Jakub Kicinski <kuba@kernel.org>

Sure, will drop the patch for now and revisit again, as part of
devlink or better API.

Thanks,
Rahul

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-30 15:11 [PATCH net-next] cxgb4: Add support to flash firmware config image Ganji Aravind
2020-07-30 23:23 ` Jakub Kicinski
2020-07-31 11:09   ` Ganji Aravind
2020-07-31 18:00     ` Jakub Kicinski
2020-07-31 21:17       ` Rahul Lakkireddy
2020-08-02  4:22         ` Jakub Kicinski
2020-08-02 17:12           ` Rahul Lakkireddy
2020-08-03 21:13             ` Jakub Kicinski
2020-08-04  9:43               ` Rahul Lakkireddy

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git