[PATCH v2] net/r8169: update the new parser for the new firmware

Subsystems: networking drivers, the rest

STALE5536d

7 messages, 3 authors, 2011-06-17 · open the first message on its own page

[PATCH v2] net/r8169: update the new parser for the new firmware

From: Hayes Wang <hidden>
Date: 2011-06-15 03:51:26

Update the parser for the new firmware which is embedded some information.

Signed-off-by: Hayes Wang <redacted>
---
 drivers/net/r8169.c |   96 +++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 70 insertions(+), 26 deletions(-)
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index ef1ce2e..bd9e78f 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -594,6 +594,14 @@ struct ring_info {
 	u8		__pad[sizeof(void *) - sizeof(u32)];
 };
 
+struct fw_info {
+	u32	magic;
+	char	version[32];
+	u32	fw_start;
+	u32	fw_len;
+	u8	chksum;
+};
+
 enum features {
 	RTL_FEATURE_WOL		= (1 << 0),
 	RTL_FEATURE_MSI		= (1 << 1),
@@ -1740,21 +1748,57 @@ static void rtl_writephy_batch(struct rtl8169_private *tp,
 #define PHY_DELAY_MS		0xe0000000
 #define PHY_WRITE_ERI_WORD	0xf0000000
 
+static int is_firmware_valid(const struct firmware *fw)
+{
+	struct fw_info *f_info = (struct fw_info *)fw->data;
+
+	if (f_info->magic == 0) {
+		size_t i, fw_size;
+		u8 checksum = 0;
+
+		for (i = 0; i < fw->size; i++) {
+			checksum += fw->data[i];
+		}
+
+		if (checksum != 0)
+			return 0;
+
+		fw_size = (fw->size - le32_to_cpu(f_info->fw_start)) / 4;
+		if (fw_size < le32_to_cpu(f_info->fw_len))
+			return 0;
+	} else if (fw->size % 4) {
+		return 0;
+	}
+
+	return 1;
+}
+
 static void
 rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
 {
-	__le32 *phytable = (__le32 *)fw->data;
+	struct fw_info *f_info = (struct fw_info *)fw->data;
 	struct net_device *dev = tp->dev;
-	size_t index, fw_size = fw->size / sizeof(*phytable);
+	__le32 *phytable;
+	size_t i, fw_size;
 	u32 predata, count;
 
-	if (fw->size % sizeof(*phytable)) {
-		netif_err(tp, probe, dev, "odd sized firmware %zd\n", fw->size);
+	if (!is_firmware_valid(fw)) {
+		netif_err(tp, probe, dev, "Invalid firwmare\n");
 		return;
 	}
 
-	for (index = 0; index < fw_size; index++) {
-		u32 action = le32_to_cpu(phytable[index]);
+	if (f_info->magic == 0) {
+		netif_info(tp, probe, dev, "firmware: %s\n", f_info->version);
+
+		phytable = (__le32 *)(fw->data + le32_to_cpu(f_info->fw_start));
+		fw_size = le32_to_cpu(f_info->fw_len);
+	} else {
+		phytable = (__le32 *)fw->data;
+		fw_size = fw->size / sizeof(*phytable);
+	}
+
+	for (i = 0; i < fw_size; i++) {
+		u32 action = le32_to_cpu(phytable[i]);
 		u32 regno = (action & 0x0fff0000) >> 16;
 
 		switch(action & 0xf0000000) {
@@ -1769,14 +1813,14 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
 			break;
 
 		case PHY_BJMPN:
-			if (regno > index) {
+			if (regno > i) {
 				netif_err(tp, probe, tp->dev,
 					  "Out of range of firmware\n");
 				return;
 			}
 			break;
 		case PHY_READCOUNT_EQ_SKIP:
-			if (index + 2 >= fw_size) {
+			if (i + 2 >= fw_size) {
 				netif_err(tp, probe, tp->dev,
 					  "Out of range of firmware\n");
 				return;
@@ -1785,7 +1829,7 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
 		case PHY_COMP_EQ_SKIPN:
 		case PHY_COMP_NEQ_SKIPN:
 		case PHY_SKIPN:
-			if (index + 1 + regno >= fw_size) {
+			if (i + 1 + regno >= fw_size) {
 				netif_err(tp, probe, tp->dev,
 					  "Out of range of firmware\n");
 				return;
@@ -1805,8 +1849,8 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
 	predata = 0;
 	count = 0;
 
-	for (index = 0; index < fw_size; ) {
-		u32 action = le32_to_cpu(phytable[index]);
+	for (i = 0; i < fw_size; ) {
+		u32 action = le32_to_cpu(phytable[i]);
 		u32 data = action & 0x0000ffff;
 		u32 regno = (action & 0x0fff0000) >> 16;
 
@@ -1817,54 +1861,54 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
 		case PHY_READ:
 			predata = rtl_readphy(tp, regno);
 			count++;
-			index++;
+			i++;
 			break;
 		case PHY_DATA_OR:
 			predata |= data;
-			index++;
+			i++;
 			break;
 		case PHY_DATA_AND:
 			predata &= data;
-			index++;
+			i++;
 			break;
 		case PHY_BJMPN:
-			index -= regno;
+			i -= regno;
 			break;
 		case PHY_READ_EFUSE:
 			predata = rtl8168d_efuse_read(tp->mmio_addr, regno);
-			index++;
+			i++;
 			break;
 		case PHY_CLEAR_READCOUNT:
 			count = 0;
-			index++;
+			i++;
 			break;
 		case PHY_WRITE:
 			rtl_writephy(tp, regno, data);
-			index++;
+			i++;
 			break;
 		case PHY_READCOUNT_EQ_SKIP:
-			index += (count == data) ? 2 : 1;
+			i += (count == data) ? 2 : 1;
 			break;
 		case PHY_COMP_EQ_SKIPN:
 			if (predata == data)
-				index += regno;
-			index++;
+				i += regno;
+			i++;
 			break;
 		case PHY_COMP_NEQ_SKIPN:
 			if (predata != data)
-				index += regno;
-			index++;
+				i += regno;
+			i++;
 			break;
 		case PHY_WRITE_PREVIOUS:
 			rtl_writephy(tp, regno, predata);
-			index++;
+			i++;
 			break;
 		case PHY_SKIPN:
-			index += regno + 1;
+			i += regno + 1;
 			break;
 		case PHY_DELAY_MS:
 			mdelay(data);
-			index++;
+			i++;
 			break;
 
 		case PHY_READ_MAC_BYTE:
-- 
1.7.3.2

Re: [PATCH v2] net/r8169: update the new parser for the new firmware

From: Francois Romieu <romieu@fr.zoreil.com>
Date: 2011-06-16 23:11:45

Hayes Wang [off-list ref] :
Update the parser for the new firmware which is embedded some information.
I have modified several things :
- s/u32/__le32/ in fw_info
- fix unsigned (size_t) comparisons
- more size checks before dereferencing fw_info

The new firmware format should be the same. The old r8168d-1.fw firmware
proved usable when prefixed with :

0000000: 0000 0000 3031 0000 0000 0000 0000 0000  ....01..........
0000010: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000020: 0000 0000 3000 0000 7501 0000 a000 0000  ....0...u.......

I realized after testing that netif_err could be abused with non-string
fw_info.version. :o/

Comments ?
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 7310824..5a0a7c3 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -1741,21 +1741,82 @@ static void rtl_writephy_batch(struct rtl8169_private *tp,
 #define PHY_DELAY_MS		0xe0000000
 #define PHY_WRITE_ERI_WORD	0xf0000000
 
+struct fw_info {
+	u32	magic;
+	char	version[32];
+	__le32	fw_start;
+	__le32	fw_len;
+	u8	chksum;
+} __packed;
+
+struct fw_phy_code {
+	__le32 *code;
+	size_t size;
+};
+
+#define FW_OPCODE_SIZE	sizeof(typeof(*((struct fw_phy_code *)0)->code))
+
+static bool rtl_fw_format_ok(struct rtl8169_private *tp, struct net_device *dev,
+			     const struct firmware *fw, struct fw_phy_code *pc)
+{
+	struct fw_info *fw_info = (struct fw_info *)fw->data;
+	bool rc = false;
+
+	if (fw->size < FW_OPCODE_SIZE)
+		goto out;
+
+	if (fw_info->magic == 0) {
+		size_t i, size, start;
+		u8 checksum = 0;
+
+		if (fw->size < sizeof(*fw_info))
+			goto out;
+
+		for (i = 0; i < fw->size; i++)
+			checksum += fw->data[i];
+		if (checksum != 0)
+			goto out;
+
+		start = le32_to_cpu(fw_info->fw_start);
+		if (start > fw->size)
+			goto out;
+
+		size = le32_to_cpu(fw_info->fw_len);
+		if (size > (fw->size - start) / FW_OPCODE_SIZE)
+			goto out;
+
+		netif_info(tp, probe, dev, "firmware: %s\n", fw_info->version);
+		pc->code = (__le32 *)(fw->data + start);
+		pc->size = size;
+	} else {
+		if (fw->size % FW_OPCODE_SIZE)
+			goto out;
+
+		netif_info(tp, probe, dev, "legacy firmware format detected\n");
+		pc->code = (__le32 *)fw->data;
+		pc->size = fw->size / FW_OPCODE_SIZE;
+	}
+
+	rc = true;
+out:
+	return rc;
+}
+
 static void
 rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
 {
-	__le32 *phytable = (__le32 *)fw->data;
 	struct net_device *dev = tp->dev;
-	size_t index, fw_size = fw->size / sizeof(*phytable);
+	struct fw_phy_code phytable;
 	u32 predata, count;
+	size_t i;
 
-	if (fw->size % sizeof(*phytable)) {
-		netif_err(tp, probe, dev, "odd sized firmware %zd\n", fw->size);
+	if (!rtl_fw_format_ok(tp, dev, fw, &phytable)) {
+		netif_err(tp, probe, dev, "invalid firwmare\n");
 		return;
 	}
 
-	for (index = 0; index < fw_size; index++) {
-		u32 action = le32_to_cpu(phytable[index]);
+	for (i = 0; i < phytable.size; i++) {
+		u32 action = le32_to_cpu(phytable.code[i]);
 		u32 regno = (action & 0x0fff0000) >> 16;
 
 		switch(action & 0xf0000000) {
@@ -1770,14 +1831,14 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
 			break;
 
 		case PHY_BJMPN:
-			if (regno > index) {
+			if (regno > i) {
 				netif_err(tp, probe, tp->dev,
 					  "Out of range of firmware\n");
 				return;
 			}
 			break;
 		case PHY_READCOUNT_EQ_SKIP:
-			if (index + 2 >= fw_size) {
+			if (i + 2 >= phytable.size) {
 				netif_err(tp, probe, tp->dev,
 					  "Out of range of firmware\n");
 				return;
@@ -1786,7 +1847,7 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
 		case PHY_COMP_EQ_SKIPN:
 		case PHY_COMP_NEQ_SKIPN:
 		case PHY_SKIPN:
-			if (index + 1 + regno >= fw_size) {
+			if (i + 1 + regno >= phytable.size) {
 				netif_err(tp, probe, tp->dev,
 					  "Out of range of firmware\n");
 				return;
@@ -1806,8 +1867,8 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
 	predata = 0;
 	count = 0;
 
-	for (index = 0; index < fw_size; ) {
-		u32 action = le32_to_cpu(phytable[index]);
+	for (i = 0; i < phytable.size; ) {
+		u32 action = le32_to_cpu(phytable.code[i]);
 		u32 data = action & 0x0000ffff;
 		u32 regno = (action & 0x0fff0000) >> 16;
 
@@ -1818,54 +1879,54 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
 		case PHY_READ:
 			predata = rtl_readphy(tp, regno);
 			count++;
-			index++;
+			i++;
 			break;
 		case PHY_DATA_OR:
 			predata |= data;
-			index++;
+			i++;
 			break;
 		case PHY_DATA_AND:
 			predata &= data;
-			index++;
+			i++;
 			break;
 		case PHY_BJMPN:
-			index -= regno;
+			i -= regno;
 			break;
 		case PHY_READ_EFUSE:
 			predata = rtl8168d_efuse_read(tp->mmio_addr, regno);
-			index++;
+			i++;
 			break;
 		case PHY_CLEAR_READCOUNT:
 			count = 0;
-			index++;
+			i++;
 			break;
 		case PHY_WRITE:
 			rtl_writephy(tp, regno, data);
-			index++;
+			i++;
 			break;
 		case PHY_READCOUNT_EQ_SKIP:
-			index += (count == data) ? 2 : 1;
+			i += (count == data) ? 2 : 1;
 			break;
 		case PHY_COMP_EQ_SKIPN:
 			if (predata == data)
-				index += regno;
-			index++;
+				i += regno;
+			i++;
 			break;
 		case PHY_COMP_NEQ_SKIPN:
 			if (predata != data)
-				index += regno;
-			index++;
+				i += regno;
+			i++;
 			break;
 		case PHY_WRITE_PREVIOUS:
 			rtl_writephy(tp, regno, predata);
-			index++;
+			i++;
 			break;
 		case PHY_SKIPN:
-			index += regno + 1;
+			i += regno + 1;
 			break;
 		case PHY_DELAY_MS:
 			mdelay(data);
-			index++;
+			i++;
 			break;
 
 		case PHY_READ_MAC_BYTE:

RE: [PATCH v2] net/r8169: update the new parser for the new firmware

From: hayeswang <hidden>
Date: 2011-06-17 03:25:23

 
-----Original Message-----
From: Francois Romieu [mailto:romieu@fr.zoreil.com] 
Sent: Friday, June 17, 2011 6:59 AM
To: Hayeswang
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] net/r8169: update the new parser for 
the new firmware

Hayes Wang [off-list ref] :
quoted
Update the parser for the new firmware which is embedded 
some information.

I have modified several things :
- s/u32/__le32/ in fw_info
- fix unsigned (size_t) comparisons
- more size checks before dereferencing fw_info
Thanks. They are fine.
The new firmware format should be the same. The old 
r8168d-1.fw firmware proved usable when prefixed with :

0000000: 0000 0000 3031 0000 0000 0000 0000 0000  ....01..........
0000010: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000020: 0000 0000 3000 0000 7501 0000 a000 0000  ....0...u.......

I realized after testing that netif_err could be abused with 
non-string fw_info.version. :o/
Excuse me. I don't understand what you want to express. Do you mean the
situation of the old paser with the new firmware for checking the firmware? For
the normal situation, the old paser would not use the new firmware. And I put
zero in front of the new firmware to prevent the old paser from running it. That
is all I do.

If you don't mean that, I could promise the new firmware I release would contain
the valid string unless someone modifies it.
Comments ?
 
Best Regards,
Hayes

Re: [PATCH v2] net/r8169: update the new parser for the new firmware

From: Francois Romieu <romieu@fr.zoreil.com>
Date: 2011-06-17 06:52:27

hayeswang [off-list ref] :
Francois Romieu :
[...]
quoted
The new firmware format should be the same. The old 
r8168d-1.fw firmware proved usable when prefixed with :

0000000: 0000 0000 3031 0000 0000 0000 0000 0000  ....01..........
0000010: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000020: 0000 0000 3000 0000 7501 0000 a000 0000  ....0...u.......

I realized after testing that netif_err could be abused with 
non-string fw_info.version. :o/
Excuse me. I don't understand what you want to express. Do you mean the
situation of the old paser with the new firmware for checking the firmware?
No. The code does a printk with a %s specifier on fw_info.version while there
is no evidence that fw_info.version is 0 terminated.
For the normal situation, the old paser would not use the new firmware.
And I put zero in front of the new firmware to prevent the old paser from
running it. That is all I do.
It is fine.

-- 
Ueimor

Re: [PATCH v2] net/r8169: update the new parser for the new firmware

From: Ben Hutchings <hidden>
Date: 2011-06-17 12:22:48

On Fri, 2011-06-17 at 08:39 +0200, Francois Romieu wrote:
hayeswang [off-list ref] :
quoted
Francois Romieu :
[...]
quoted
quoted
The new firmware format should be the same. The old 
r8168d-1.fw firmware proved usable when prefixed with :

0000000: 0000 0000 3031 0000 0000 0000 0000 0000  ....01..........
0000010: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000020: 0000 0000 3000 0000 7501 0000 a000 0000  ....0...u.......

I realized after testing that netif_err could be abused with 
non-string fw_info.version. :o/
Excuse me. I don't understand what you want to express. Do you mean the
situation of the old paser with the new firmware for checking the firmware?
No. The code does a printk with a %s specifier on fw_info.version while there
is no evidence that fw_info.version is 0 terminated.
I thought the idea of embedding a version here was to be able to report
in in get_drvinfo, not to print it on load.

Ben.
quoted
For the normal situation, the old paser would not use the new firmware.
And I put zero in front of the new firmware to prevent the old paser from
running it. That is all I do.
It is fine.
-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH v2] net/r8169: update the new parser for the new firmware

From: Francois Romieu <romieu@fr.zoreil.com>
Date: 2011-06-17 13:05:47

Ben Hutchings [off-list ref] :
[...]
I thought the idea of embedding a version here was to be able to report
in in get_drvinfo, not to print it on load.
Reporting the version for the new firmware format through ethtool is
currently hacked on.

Do you want the load time version / format messages removed ?

I think they can be of some use - at least temporarily - but I won't mind
removing them now.

-- 
Ueimor

Re: [PATCH v2] net/r8169: update the new parser for the new firmware

From: Ben Hutchings <hidden>
Date: 2011-06-17 14:51:28

On Fri, 2011-06-17 at 14:53 +0200, Francois Romieu wrote:
Ben Hutchings [off-list ref] :
[...]
quoted
I thought the idea of embedding a version here was to be able to report
in in get_drvinfo, not to print it on load.
Reporting the version for the new firmware format through ethtool is
currently hacked on.

Do you want the load time version / format messages removed ?

I think they can be of some use - at least temporarily - but I won't mind
removing them now.
I don't care one way or the other.  But it's more important to report
the version through get_drvinfo.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help