[PATCH net-next 0/4] r8152: firmware support

[PATCH net-next 0/4] r8152: firmware support

[Hayes Wang]

Parsing, checking, and writing the firmware.

Hayes Wang (4):
  r8152: check code with checkpatch.pl
  r8152: replace strncpy with strlcpy
  r8152: remove clear_bp function
  r8152: support firmware files

 drivers/net/usb/r8152.c | 961 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 903 insertions(+), 58 deletions(-)

-- 
1.9.3

[PATCH net-next 1/4] r8152: check code with checkpatch.pl

[Hayes Wang]

 626: CHECK: Alignment should match open parenthesis
 646: CHECK: Alignment should match open parenthesis
 655: CHECK: Alignment should match open parenthesis
 695: CHECK: Alignment should match open parenthesis
 729: CHECK: Alignment should match open parenthesis
 739: CHECK: Alignment should match open parenthesis
 976: WARNING: externs should be avoided in .c files
 1314: CHECK: Alignment should match open parenthesis
 1358: WARNING: networking block comments don't use an empty /* line, use /* Comment...
 1402: WARNING: networking block comments don't use an empty /* line, use /* Comment...
 1521: CHECK: multiple assignments should be avoided
 1775: CHECK: Alignment should match open parenthesis
 1838: CHECK: multiple assignments should be avoided
 1843: CHECK: multiple assignments should be avoided
 1847: CHECK: multiple assignments should be avoided
 1850: WARNING: Missing a blank line after declarations
 1864: CHECK: Alignment should match open parenthesis
 1872: CHECK: braces {} should be used on all arms of this statement
 1906: CHECK: usleep_range is preferred over udelay
 2865: WARNING: networking block comments don't use an empty /* line, use /* Comment...
 3088: CHECK: Alignment should match open parenthesis
 total: 0 errors, 5 warnings, 16 checks, 3567 lines checked

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 66 ++++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 87f7104..2470d9c 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -623,8 +623,8 @@ int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 		return -ENOMEM;
 
 	ret = usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
-			       RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
-			       value, index, tmp, size, 500);
+			      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
+			      value, index, tmp, size, 500);
 
 	memcpy(data, tmp, size);
 	kfree(tmp);
@@ -643,8 +643,8 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 		return -ENOMEM;
 
 	ret = usb_control_msg(tp->udev, usb_sndctrlpipe(tp->udev, 0),
-			       RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
-			       value, index, tmp, size, 500);
+			      RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
+			      value, index, tmp, size, 500);
 
 	kfree(tmp);
 
@@ -652,7 +652,7 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 }
 
 static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
-				void *data, u16 type)
+			    void *data, u16 type)
 {
 	u16 limit = 64;
 	int ret = 0;
@@ -692,7 +692,7 @@ static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
 }
 
 static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
-				u16 size, void *data, u16 type)
+			     u16 size, void *data, u16 type)
 {
 	int ret;
 	u16 byteen_start, byteen_end, byen;
@@ -726,8 +726,8 @@ static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
 		while (size) {
 			if (size > limit) {
 				ret = set_registers(tp, index,
-					type | BYTE_EN_DWORD,
-					limit, data);
+						    type | BYTE_EN_DWORD,
+						    limit, data);
 				if (ret < 0)
 					goto error1;
 
@@ -736,8 +736,8 @@ static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
 				size -= limit;
 			} else {
 				ret = set_registers(tp, index,
-					type | BYTE_EN_DWORD,
-					size, data);
+						    type | BYTE_EN_DWORD,
+						    size, data);
 				if (ret < 0)
 					goto error1;
 
@@ -972,8 +972,8 @@ void write_mii_word(struct net_device *netdev, int phy_id, int reg, int val)
 	usb_autopm_put_interface(tp->intf);
 }
 
-static
-int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags);
+static int
+r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags);
 
 static inline void set_ethernet_addr(struct r8152 *tp)
 {
@@ -1311,8 +1311,8 @@ static int alloc_all_mem(struct r8152 *tp)
 
 	tp->intr_interval = (int)ep_intr->desc.bInterval;
 	usb_fill_int_urb(tp->intr_urb, tp->udev, usb_rcvintpipe(tp->udev, 3),
-		     tp->intr_buff, INTBUFSIZE, intr_callback,
-		     tp, tp->intr_interval);
+			 tp->intr_buff, INTBUFSIZE, intr_callback,
+			 tp, tp->intr_interval);
 
 	return 0;
 
@@ -1354,8 +1354,7 @@ static inline __be16 get_protocol(struct sk_buff *skb)
 	return protocol;
 }
 
-/*
- * r8152_csum_workaround()
+/* r8152_csum_workaround()
  * The hw limites the value the transport offset. When the offset is out of the
  * range, calculate the checksum by sw.
  */
@@ -1398,8 +1397,7 @@ drop:
 	}
 }
 
-/*
- * msdn_giant_send_check()
+/* msdn_giant_send_check()
  * According to the document of microsoft, the TCP Pseudo Header excludes the
  * packet length for IPv6 TCP large packets.
  */
@@ -1518,7 +1516,8 @@ static int r8152_tx_agg_fill(struct r8152 *tp, struct tx_agg *agg)
 	spin_unlock(&tx_queue->lock);
 
 	tx_data = agg->head;
-	agg->skb_num = agg->skb_len = 0;
+	agg->skb_num = 0;
+	agg->skb_len = 0;
 	remain = rx_buf_sz;
 
 	while (remain >= ETH_ZLEN + sizeof(struct tx_desc)) {
@@ -1772,8 +1771,8 @@ static
 int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags)
 {
 	usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1),
-		      agg->head, rx_buf_sz,
-		      (usb_complete_t)read_bulk_callback, agg);
+			  agg->head, rx_buf_sz,
+			  (usb_complete_t)read_bulk_callback, agg);
 
 	return usb_submit_urb(agg->urb, mem_flags);
 }
@@ -1835,18 +1834,22 @@ static void _rtl8152_set_rx_mode(struct net_device *netdev)
 		/* Unconditionally log net taps. */
 		netif_notice(tp, link, netdev, "Promiscuous mode enabled\n");
 		ocp_data |= RCR_AM | RCR_AAP;
-		mc_filter[1] = mc_filter[0] = 0xffffffff;
+		mc_filter[1] = 0xffffffff;
+		mc_filter[0] = 0xffffffff;
 	} else if ((netdev_mc_count(netdev) > multicast_filter_limit) ||
 		   (netdev->flags & IFF_ALLMULTI)) {
 		/* Too many to filter perfectly -- accept all multicasts. */
 		ocp_data |= RCR_AM;
-		mc_filter[1] = mc_filter[0] = 0xffffffff;
+		mc_filter[1] = 0xffffffff;
+		mc_filter[0] = 0xffffffff;
 	} else {
 		struct netdev_hw_addr *ha;
 
-		mc_filter[1] = mc_filter[0] = 0;
+		mc_filter[1] = 0;
+		mc_filter[0] = 0;
 		netdev_for_each_mc_addr(ha, netdev) {
 			int bit_nr = ether_crc(ETH_ALEN, ha->addr) >> 26;
+
 			mc_filter[bit_nr >> 5] |= 1 << (bit_nr & 31);
 			ocp_data |= RCR_AM;
 		}
@@ -1861,7 +1864,7 @@ static void _rtl8152_set_rx_mode(struct net_device *netdev)
 }
 
 static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
-					struct net_device *netdev)
+				      struct net_device *netdev)
 {
 	struct r8152 *tp = netdev_priv(netdev);
 
@@ -1877,8 +1880,9 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
 			usb_mark_last_busy(tp->udev);
 			tasklet_schedule(&tp->tl);
 		}
-	} else if (skb_queue_len(&tp->tx_queue) > tp->tx_qlen)
+	} else if (skb_queue_len(&tp->tx_queue) > tp->tx_qlen) {
 		netif_stop_queue(netdev);
+	}
 
 	return NETDEV_TX_OK;
 }
@@ -1903,7 +1907,7 @@ static void rtl8152_nic_reset(struct r8152 *tp)
 	for (i = 0; i < 1000; i++) {
 		if (!(ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CR) & CR_RST))
 			break;
-		udelay(100);
+		usleep_range(100, 400);
 	}
 }
 
@@ -2861,8 +2865,7 @@ static int rtl8152_close(struct net_device *netdev)
 	if (res < 0) {
 		rtl_drop_queued_tx(tp);
 	} else {
-		/*
-		 * The autosuspend may have been enabled and wouldn't
+		/* The autosuspend may have been enabled and wouldn't
 		 * be disable when autoresume occurs, because the
 		 * netif_running() would be false.
 		 */
@@ -3085,8 +3088,9 @@ static int rtl8152_resume(struct usb_interface *intf)
 		} else {
 			tp->rtl_ops.up(tp);
 			rtl8152_set_speed(tp, AUTONEG_ENABLE,
-				tp->mii.supports_gmii ? SPEED_1000 : SPEED_100,
-				DUPLEX_FULL);
+					  tp->mii.supports_gmii ?
+					  SPEED_1000 : SPEED_100,
+					  DUPLEX_FULL);
 		}
 		tp->speed = 0;
 		netif_carrier_off(tp->netdev);
-- 
1.9.3

[PATCH net-next 2/4] r8152: replace strncpy with strlcpy

[Hayes Wang]

Replace the strncpy with strlcpy, and use sizeof to determine the
length.

Signed-off-by: Hayes Wang <hayeswang-Rasf1IRRPZFBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/usb/r8152.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 2470d9c..33dcc97 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3151,8 +3151,8 @@ static void rtl8152_get_drvinfo(struct net_device *netdev,
 {
 	struct r8152 *tp = netdev_priv(netdev);
 
-	strncpy(info->driver, MODULENAME, ETHTOOL_BUSINFO_LEN);
-	strncpy(info->version, DRIVER_VERSION, ETHTOOL_BUSINFO_LEN);
+	strlcpy(info->driver, MODULENAME, sizeof(info->driver));
+	strlcpy(info->version, DRIVER_VERSION, sizeof(info->version));
 	usb_make_path(tp->udev, info->bus_info, sizeof(info->bus_info));
 }
 
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 3/4] r8152: remove clear_bp function

[Hayes Wang]

The functions are used to update the firmware. Move the actions into
the firmware files.

Signed-off-by: Hayes Wang <hayeswang-Rasf1IRRPZFBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/usb/r8152.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 33dcc97..937d132 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -2189,28 +2189,6 @@ static void rtl_phy_reset(struct r8152 *tp)
 	}
 }
 
-static void rtl_clear_bp(struct r8152 *tp)
-{
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_BP_0, 0);
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_BP_2, 0);
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_BP_4, 0);
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_BP_6, 0);
-	ocp_write_dword(tp, MCU_TYPE_USB, USB_BP_0, 0);
-	ocp_write_dword(tp, MCU_TYPE_USB, USB_BP_2, 0);
-	ocp_write_dword(tp, MCU_TYPE_USB, USB_BP_4, 0);
-	ocp_write_dword(tp, MCU_TYPE_USB, USB_BP_6, 0);
-	mdelay(3);
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_BP_BA, 0);
-	ocp_write_word(tp, MCU_TYPE_USB, USB_BP_BA, 0);
-}
-
-static void r8153_clear_bp(struct r8152 *tp)
-{
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_BP_EN, 0);
-	ocp_write_byte(tp, MCU_TYPE_USB, USB_BP_EN, 0);
-	rtl_clear_bp(tp);
-}

Re: [PATCH net-next 3/4] r8152: remove clear_bp function

[Sergei Shtylyov]

Hello.

On 8/20/2014 12:58 PM, Hayes Wang wrote:

> The functions are used to update the firmware. Move the actions into
> the firmware files.

> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
>   drivers/net/usb/r8152.c | 24 ------------------------
>   1 file changed, 24 deletions(-)

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 33dcc97..937d132 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
[...]
> @@ -2248,7 +2226,6 @@ static void r8152b_hw_phy_cfg(struct r8152 *tp)
>
>   	r8152b_disable_aldps(tp);
>
> -	rtl_clear_bp(tp);
>

    Why leave 2 empty lines? One is enough.

>   	r8152b_enable_aldps(tp);
>   	set_bit(PHY_RESET, &tp->flags);
> @@ -2404,7 +2381,6 @@ static void r8153_hw_phy_cfg(struct r8152 *tp)
>   		r8152_mdio_write(tp, MII_BMCR, data);
>   	}
>
> -	r8153_clear_bp(tp);
>

    Ditto.

>   	if (tp->version == RTL_VER_03) {
>   		data = ocp_reg_read(tp, OCP_EEE_CFG);
>

WBR, Sergei

RE: [PATCH net-next 3/4] r8152: remove clear_bp function

[Hayes Wang]

: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com] 
> Sent: Wednesday, August 20, 2014 8:01 PM
> To: Hayes Wang; netdev@vger.kernel.org
> Cc: nic_swsd; linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org
> Subject: Re: [PATCH net-next 3/4] r8152: remove clear_bp function
[...]
> >   	r8152b_disable_aldps(tp);
> >
> > -	rtl_clear_bp(tp);
> >
> 
>     Why leave 2 empty lines? One is enough.

The next patch would use another fucntion at the
same location. I skip removing the empty line and
re-adding it again. Is that better to do so? I would
resend the patches if the answer is yes.
 
Best Regards,
Hayes

Re: [PATCH net-next 3/4] r8152: remove clear_bp function

[Sergei Shtylyov]

On 08/21/2014 06:12 AM, Hayes Wang wrote:

> [...]
>>>    	r8152b_disable_aldps(tp);
>>>
>>> -	rtl_clear_bp(tp);
>>>

>>      Why leave 2 empty lines? One is enough.

> The next patch would use another fucntion at the
> same location. I skip removing the empty line and
> re-adding it again. Is that better to do so? I would
> resend the patches if the answer is yes.

    Sorry, I haven't looked at your next patch, too big for me. :-)

> Best Regards,
> Hayes

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH net-next 3/4] r8152: remove clear_bp function

[Hayes Wang]

 Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com] 
[...]
> >>      Why leave 2 empty lines? One is enough.
> 
> > The next patch would use another fucntion at the
> > same location. I skip removing the empty line and
> > re-adding it again. Is that better to do so? I would
> > resend the patches if the answer is yes.
> 
>     Sorry, I haven't looked at your next patch, too big for me. :-)

It's my mistake. I would avoid it next time. Thanks for your notice.
 
Best Regards,
Hayes

[PATCH net-next 4/4] r8152: support firmware files

[Hayes Wang]

The firmware file is composed of the fw header and the commands. Each
command has the following type.

	cmd(2 bytes) + length(2 bytes) + data(variable bytes)

Before applying the firmware, the driver would check the fw header and
each command.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 867 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 866 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 937d132..63542cc 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -21,10 +21,11 @@
 #include <linux/list.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/firmware.h>
 #include <net/ip6_checksum.h>
 
 /* Version Information */
-#define DRIVER_VERSION "v1.06.0 (2014/03/03)"
+#define DRIVER_VERSION "v1.07.0 (2014/08/20)"
 #define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_swsd@realtek.com>"
 #define DRIVER_DESC "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
 #define MODULENAME "r8152"
@@ -577,6 +578,16 @@ struct r8152 {
 		void (*unload)(struct r8152 *);
 	} rtl_ops;
 
+	struct rtl_fw {
+		const struct firmware *fw;
+
+#define RTL_VER_SIZE		32
+
+		char version[RTL_VER_SIZE];
+		u8 *code;
+		size_t code_size;
+	} rtl_fw;
+
 	int intr_interval;
 	u32 saved_wolopts;
 	u32 msg_enable;
@@ -1321,6 +1332,852 @@ err1:
 	return -ENOMEM;
 }
 
+#define FW_SIGNATURE	0x0bda8152
+
+enum fw_cmd {
+	FW_CMD_INVALID = 0,
+
+	FW_CMD_GENERIC_WRITE,
+	FW_CMD_WRITE_BYTE,
+	FW_CMD_WRITE_WORD,
+	FW_CMD_WRITE_DWORD,
+	FW_CMD_READ_BYTE,
+	FW_CMD_READ_WORD,
+	FW_CMD_READ_DWORD,
+	FW_CMD_W0W1_BYTE,
+	FW_CMD_W0W1_WORD,
+	FW_CMD_W0W1_DWORD,
+	FW_CMD_W0W1_CURRENT,
+	FW_CMD_WRITE_CURRENT_BYTE,
+	FW_CMD_WRITE_CURRENT_WORD,
+	FW_CMD_WRITE_CURRENT_DWORD,
+	FW_CMD_CMP,
+	FW_CMD_JMP,
+	FW_CMD_JE,
+	FW_CMD_JNE,
+	FW_CMD_JA,
+	FW_CMD_JAE,
+	FW_CMD_JB,
+	FW_CMD_JBE,
+	FW_CMD_CX,
+	FW_CMD_LOOP,
+	FW_CMD_LOOPE,
+	FW_CMD_LOOPNE,
+	FW_CMD_USLEEP,
+
+	FW_CMD_END,
+	FW_CMD_MAX
+};
+
+struct fw_cmd_generic {
+	__le16 cmd;
+	__le16 length;
+} __packed;
+
+struct fw_cmd_most_used {
+	__le16 type;
+	__le16 addr;
+} __packed;
+
+struct fw_header {
+	__le32	signature;
+	char	version[RTL_VER_SIZE];
+	__le32	fw_start;
+	__le32	fw_len;
+} __packed;
+
+static bool rtl_fw_format_ok(struct rtl_fw *rtl_fw)
+{
+	const struct firmware *fw = rtl_fw->fw;
+	struct fw_header *fw_header = (struct fw_header *)fw->data;
+	char *version = rtl_fw->version;
+	size_t i, size, start;
+	u8 checksum = 0;
+	bool rc = false;
+
+	if (fw->size < sizeof(*fw_header))
+		goto out;
+
+	if (__le32_to_cpu(fw_header->signature) != FW_SIGNATURE)
+		goto out;
+
+	start = le32_to_cpu(fw_header->fw_start);
+	if (start > fw->size)
+		goto out;
+
+	size = le32_to_cpu(fw_header->fw_len);
+	if (size > (fw->size - start))
+		goto out;
+
+	for (i = 0; i < fw->size; i++)
+		checksum += fw->data[i];
+	if (checksum != 0)
+		goto out;
+
+	memcpy(version, fw_header->version, RTL_VER_SIZE);
+
+	rtl_fw->code = (u8 *)(fw->data + start);
+	rtl_fw->code_size = size;
+
+	version[RTL_VER_SIZE - 1] = 0;
+
+	rc = true;
+out:
+	return rc;
+}
+
+static void rtl_fw_get_info2(u8 *d2, u16 *ptype, u16 *paddr)
+{
+	struct fw_cmd_most_used info2;
+
+	memcpy(&info2, d2, sizeof(info2));
+	*ptype = __le16_to_cpu(info2.type);
+	*paddr = __le16_to_cpu(info2.addr);
+}
+
+static bool rtl_fw_data_ok(u8 *d, size_t total)
+{
+	u16 cmd, len, type, addr, byteen, size, cx = 0;
+	struct fw_cmd_generic op;
+	bool result = false;
+	__le16 le16_data;
+	__le32 le32_data;
+	size_t i = 0, j;
+
+	while (i < total) {
+		if (i + sizeof(op) > total)
+			goto result_return;
+
+		memcpy(&op, &d[i], sizeof(op));
+		cmd = __le16_to_cpu(op.cmd);
+		len = __le16_to_cpu(op.length);
+		j = i + sizeof(op);
+
+		switch (cmd) {
+		case FW_CMD_GENERIC_WRITE:
+			/* struct fw_cmd_generic_write {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	__le16 data_length;
+			 * };
+			 */
+
+			if ((j + sizeof(struct fw_cmd_most_used)) > total)
+				goto result_return;
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			j += sizeof(struct fw_cmd_most_used);
+
+			/* addr size must be the multiple of 4 */
+			if (addr & 3)
+				goto result_return;
+
+			byteen = type & 0xff;
+			type = type & ~0xff;
+
+			memcpy(&le16_data, &d[j], sizeof(le16_data));
+			j += sizeof(le16_data);
+			size = __le16_to_cpu(le16_data);
+
+			/* data size must be the multiple of 4 */
+			if (size & 3)
+				goto result_return;
+
+			size += sizeof(struct fw_cmd_most_used) + sizeof(size);
+
+			break;
+
+		case FW_CMD_WRITE_BYTE:
+			/* struct fw_cmd_generic_write {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	u8 data;
+			 * };
+			 */
+
+			if ((j + sizeof(struct fw_cmd_most_used)) > total)
+				goto result_return;
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+
+			if (type & 0xff)
+				goto result_return;
+
+			size = sizeof(struct fw_cmd_most_used) + 1;
+			break;
+
+		case FW_CMD_WRITE_WORD:
+			/* struct fw_cmd_ocp_write_word {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	__le16 data;
+			 * };
+			 */
+
+			if ((j + sizeof(struct fw_cmd_most_used)) > total)
+				goto result_return;
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+
+			if ((type & 0xff) || (addr & 1))
+				goto result_return;
+
+			size = sizeof(struct fw_cmd_most_used) +
+			       sizeof(le16_data);
+			break;
+
+		case FW_CMD_WRITE_DWORD:
+			/* struct fw_cmd_ocp_write_dword {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	__le32 data;
+			 * };
+			 */
+
+			if ((j + sizeof(struct fw_cmd_most_used)) > total)
+				goto result_return;
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+
+			if ((type & 0xff) || (addr & 3))
+				goto result_return;
+
+			size = sizeof(struct fw_cmd_most_used) +
+			       sizeof(le32_data);
+			break;
+
+		case FW_CMD_READ_BYTE:
+		case FW_CMD_WRITE_CURRENT_BYTE:
+			/* struct fw_cmd_write_current {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 * };
+			 */
+
+			if ((j + sizeof(struct fw_cmd_most_used)) > total)
+				goto result_return;
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+
+			if (type & 0xff)
+				goto result_return;
+
+			size = sizeof(struct fw_cmd_most_used);
+			break;
+
+		case FW_CMD_READ_WORD:
+		case FW_CMD_WRITE_CURRENT_WORD:
+			/* struct fw_cmd_write_current {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 * };
+			 */
+
+			if ((j + sizeof(struct fw_cmd_most_used)) > total)
+				goto result_return;
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+
+			if ((type & 0xff) || (addr & 1))
+				goto result_return;
+
+			size = sizeof(struct fw_cmd_most_used);
+			break;
+
+		case FW_CMD_READ_DWORD:
+		case FW_CMD_WRITE_CURRENT_DWORD:
+			/* struct fw_cmd_write_current {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 * };
+			 */
+
+			if ((j + sizeof(struct fw_cmd_most_used)) > total)
+				goto result_return;
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+
+			if ((type & 0xff) || (addr & 3))
+				goto result_return;
+
+			size = sizeof(struct fw_cmd_most_used);
+			break;
+
+		case FW_CMD_W0W1_BYTE:
+			/* struct fw_cmd_ocp_w0w1_byte {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	u8 w0;
+			 *	u8 w1;
+			 * };
+			 */
+
+			if ((j + sizeof(struct fw_cmd_most_used)) > total)
+				goto result_return;
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+
+			if (type & 0xff)
+				goto result_return;
+
+			size = sizeof(struct fw_cmd_most_used) + 2;
+			break;
+
+		case FW_CMD_W0W1_WORD:
+			/* struct fw_cmd_ocp_w0w1_word {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	__le16 w0;
+			 *	__le16 w1;
+			 * };
+			 */
+
+			if ((j + sizeof(struct fw_cmd_most_used)) > total)
+				goto result_return;
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+
+			if ((type & 0xff) || (addr & 1))
+				goto result_return;
+
+			size = sizeof(struct fw_cmd_most_used) +
+			       sizeof(le16_data) * 2;
+			break;
+
+		case FW_CMD_W0W1_DWORD:
+			/* struct fw_cmd_ocp_w0w1_dword {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	__le32 w0;
+			 *	__le32 w1;
+			 * };
+			 */
+
+			if ((j + sizeof(struct fw_cmd_most_used)) > total)
+				goto result_return;
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+
+			if ((type & 0xff) || (addr & 3))
+				goto result_return;
+
+			size = sizeof(struct fw_cmd_most_used) +
+			       sizeof(le32_data) * 2;
+			break;
+
+		case FW_CMD_CMP:
+			/* struct fw_cmd_compare {
+			 *	struct fw_cmd_generic op;
+			 *	__le32 data;
+			 * };
+			 */
+
+			size = sizeof(le32_data);
+			break;
+
+		case FW_CMD_JMP:
+		case FW_CMD_JE:
+		case FW_CMD_JNE:
+		case FW_CMD_JA:
+		case FW_CMD_JAE:
+		case FW_CMD_JB:
+		case FW_CMD_JBE:
+		case FW_CMD_LOOP:
+		case FW_CMD_LOOPE:
+		case FW_CMD_LOOPNE:
+			/* struct fw_cmd_jump {
+			 *	struct fw_cmd_generic op;
+			 *	__le16 offset;
+			 * };
+			 */
+
+			if (j + sizeof(le16_data) > total)
+				goto result_return;
+
+			memcpy(&le16_data, &d[j], sizeof(le16_data));
+			j = (short)__le16_to_cpu(le16_data);
+
+			j += sizeof(op) + len + i;
+			if (j < 0 || j > total)
+				goto result_return;
+
+			size = sizeof(le16_data);
+			break;
+
+		case FW_CMD_CX:
+		case FW_CMD_USLEEP:
+			/* struct fw_cmd_cx {
+			 *	struct fw_cmd_generic op;
+			 *	__le16 cx;
+			 * };
+			 */
+
+			if (j + sizeof(le16_data) > total)
+				goto result_return;
+
+			memcpy(&le16_data, &d[j], sizeof(le16_data));
+			cx = __le16_to_cpu(le16_data);
+			if (cx == 0)
+				goto result_return;
+
+			size = sizeof(le16_data);
+			break;
+
+		case FW_CMD_W0W1_CURRENT:
+			/* struct fw_cmd_ocp_w0w1_current {
+			 *	struct fw_cmd_generic op;
+			 *	__le32 w0;
+			 *	__le32 w1;
+			 * };
+			 */
+			size = sizeof(le32_data) * 2;
+			break;
+
+		case FW_CMD_END:
+			size = 0;
+			goto result_return;
+
+		default:
+			goto result_return;
+		}
+
+		if (len != size)
+			goto result_return;
+
+		i += sizeof(op) + len;
+	}
+
+	if (i <= total)
+		result = true;
+
+result_return:
+	return result;
+}
+
+static bool rtl_check_firmware(struct r8152 *tp, struct rtl_fw *fw)
+{
+	bool fw_ok = false;
+
+	if (!rtl_fw_format_ok(&tp->rtl_fw))
+		goto out;
+
+	if (rtl_fw_data_ok(tp->rtl_fw.code, tp->rtl_fw.code_size))
+		fw_ok = true;
+
+out:
+	return fw_ok;
+}
+
+static void rtl_fw_write(struct r8152 *tp, u8 *d, size_t total)
+{
+	u16 cmd, len, type, addr, byteen, size, cx = 0, us;
+	u32 ocp_data = 0, compare = 0;
+	struct fw_cmd_generic op;
+	__le16 le16_data;
+	__le32 le32_data;
+	size_t i = 0, j;
+
+	while (i < total) {
+		memcpy(&op, &d[i], sizeof(op));
+		cmd = __le16_to_cpu(op.cmd);
+		len = __le16_to_cpu(op.length);
+		j = i + sizeof(op);
+
+		switch (cmd) {
+		case FW_CMD_GENERIC_WRITE:
+			/* struct fw_cmd_generic_write {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	__le16 data_length;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			j += sizeof(struct fw_cmd_most_used);
+
+			byteen = type & 0xff;
+			type = type & ~0xff;
+
+			memcpy(&le16_data, &d[j], sizeof(le16_data));
+			j += sizeof(le16_data);
+			size = __le16_to_cpu(le16_data);
+
+			generic_ocp_write(tp, addr, byteen, size, &d[j], type);
+			break;
+
+		case FW_CMD_WRITE_BYTE:
+			/* struct fw_cmd_generic_write {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	u8 data;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			j += sizeof(struct fw_cmd_most_used);
+
+			ocp_data = d[j];
+			ocp_write_byte(tp, type, addr, ocp_data);
+			break;
+
+		case FW_CMD_WRITE_WORD:
+			/* struct fw_cmd_ocp_write_word {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	__le16 data;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			j += sizeof(struct fw_cmd_most_used);
+
+			memcpy(&le16_data, &d[j], sizeof(le16_data));
+			ocp_data = __le16_to_cpu(le16_data);
+
+			ocp_write_word(tp, type, addr, ocp_data);
+			break;
+
+		case FW_CMD_WRITE_DWORD:
+			/* struct fw_cmd_ocp_write_dword {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	__le32 data;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			j += sizeof(struct fw_cmd_most_used);
+
+			memcpy(&le32_data, &d[j], sizeof(le32_data));
+			ocp_data = __le32_to_cpu(le32_data);
+
+			ocp_write_dword(tp, type, addr, ocp_data);
+			break;
+
+		case FW_CMD_READ_BYTE:
+			/* struct fw_cmd_ocp_read {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			ocp_data = ocp_read_byte(tp, type, addr);
+			break;
+
+		case FW_CMD_READ_WORD:
+			/* struct fw_cmd_ocp_read {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			ocp_data = ocp_read_word(tp, type, addr);
+			break;
+
+		case FW_CMD_READ_DWORD:
+			/* struct fw_cmd_ocp_read {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			ocp_data = ocp_read_dword(tp, type, addr);
+			break;
+
+		case FW_CMD_W0W1_BYTE:
+			/* struct fw_cmd_ocp_w0w1_byte {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	u8 w0;
+			 *	u8 w1;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			j += sizeof(struct fw_cmd_most_used);
+
+			ocp_data = ocp_read_byte(tp, type, addr);
+			ocp_data &= ~d[j++];
+			ocp_data |= d[j++];
+			ocp_write_byte(tp, type, addr, ocp_data);
+			break;
+
+		case FW_CMD_W0W1_WORD:
+			/* struct fw_cmd_ocp_w0w1_word {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	__le16 w0;
+			 *	__le16 w1;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			j += sizeof(struct fw_cmd_most_used);
+
+			ocp_data = ocp_read_word(tp, type, addr);
+
+			memcpy(&le16_data, &d[j], sizeof(le16_data));
+			j += sizeof(le16_data);
+			ocp_data &= ~__le16_to_cpu(le16_data);
+
+			memcpy(&le16_data, &d[j], sizeof(le16_data));
+			j += sizeof(le16_data);
+			ocp_data |= __le16_to_cpu(le16_data);
+
+			ocp_write_word(tp, type, addr, ocp_data);
+			break;
+
+		case FW_CMD_W0W1_DWORD:
+			/* struct fw_cmd_ocp_w0w1_dword {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 *	__le32 w0;
+			 *	__le32 w1;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			j += sizeof(struct fw_cmd_most_used);
+
+			ocp_data = ocp_read_dword(tp, type, addr);
+
+			memcpy(&le32_data, &d[j], sizeof(le32_data));
+			j += sizeof(le32_data);
+			ocp_data &= ~__le32_to_cpu(le32_data);
+
+			memcpy(&le32_data, &d[j], sizeof(le32_data));
+			j += sizeof(le32_data);
+			ocp_data |= __le32_to_cpu(le32_data);
+
+			ocp_write_dword(tp, type, addr, ocp_data);
+			break;
+
+		case FW_CMD_W0W1_CURRENT:
+			/* struct fw_cmd_ocp_w0w1_current {
+			 *	struct fw_cmd_generic op;
+			 *	__le32 w0;
+			 *	__le32 w1;
+			 * };
+			 */
+
+			memcpy(&le32_data, &d[j], sizeof(le32_data));
+			j += sizeof(le32_data);
+			ocp_data &= ~__le32_to_cpu(le32_data);
+
+			memcpy(&le32_data, &d[j], sizeof(le32_data));
+			j += sizeof(le32_data);
+			ocp_data |= __le32_to_cpu(le32_data);
+			break;
+
+		case FW_CMD_WRITE_CURRENT_BYTE:
+			/* struct fw_cmd_write_current {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			ocp_write_byte(tp, type, addr, ocp_data);
+			break;
+
+		case FW_CMD_WRITE_CURRENT_WORD:
+			/* struct fw_cmd_write_current {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			ocp_write_word(tp, type, addr, ocp_data);
+			break;
+
+		case FW_CMD_WRITE_CURRENT_DWORD:
+			/* struct fw_cmd_write_current {
+			 *	struct fw_cmd_generic op;
+			 *	struct fw_cmd_most_used info2;
+			 * };
+			 */
+
+			rtl_fw_get_info2(&d[j], &type, &addr);
+			ocp_write_dword(tp, type, addr, ocp_data);
+			break;
+
+		case FW_CMD_CMP:
+			/* struct fw_cmd_compare {
+			 *	struct fw_cmd_generic op;
+			 *	__le32 data;
+			 * };
+			 */
+
+			memcpy(&le32_data, &d[j], sizeof(le32_data));
+			compare = __le32_to_cpu(le32_data);
+			break;
+
+		case FW_CMD_JMP:
+			/* struct fw_cmd_jump {
+			 *	struct fw_cmd_generic op;
+			 *	__le16 offset;
+			 * };
+			 */
+do_jump:
+			memcpy(&le16_data, &d[j], sizeof(le16_data));
+			j = (short)__le16_to_cpu(le16_data);
+
+			i += sizeof(op) + len + j;
+			continue;
+
+		case FW_CMD_JE:
+			if (ocp_data == compare)
+				goto do_jump;
+
+			break;
+
+		case FW_CMD_JNE:
+			if (ocp_data != compare)
+				goto do_jump;
+
+			break;
+
+		case FW_CMD_JA:
+			if (ocp_data > compare)
+				goto do_jump;
+
+			break;
+
+		case FW_CMD_JAE:
+			if (ocp_data >= compare)
+				goto do_jump;
+
+			break;
+
+		case FW_CMD_JB:
+			if (ocp_data < compare)
+				goto do_jump;
+
+			break;
+
+		case FW_CMD_JBE:
+			if (ocp_data <= compare)
+				goto do_jump;
+
+			break;
+
+		case FW_CMD_CX:
+			/* struct fw_cmd_cx {
+			 *	struct fw_cmd_generic op;
+			 *	__le16 cx;
+			 * };
+			 */
+
+			memcpy(&le16_data, &d[j], sizeof(le16_data));
+			cx = __le16_to_cpu(le16_data);
+			break;
+
+		case FW_CMD_LOOP:
+			if (--cx > 0)
+				goto do_jump;
+
+			break;
+
+		case FW_CMD_LOOPE:
+			if (--cx > 0 && ocp_data == compare)
+				goto do_jump;
+
+			break;
+
+		case FW_CMD_LOOPNE:
+			if (--cx > 0 && ocp_data != compare)
+				goto do_jump;
+
+			break;
+
+		case FW_CMD_USLEEP:
+			/* struct fw_cmd_usleep {
+			 *	struct fw_cmd_generic op;
+			 *	__le16 us;
+			 * };
+			 */
+
+			memcpy(&le16_data, &d[j], sizeof(le16_data));
+			us = __le16_to_cpu(le16_data);
+			usleep_range(us , us * 4);
+			break;
+
+		case FW_CMD_END:
+		default:
+			return;
+		}
+
+		i += sizeof(op) + len;
+	}
+}
+
+static void rtl_release_firmware(struct r8152 *tp)
+{
+	if (!IS_ERR_OR_NULL(tp->rtl_fw.fw)) {
+		release_firmware(tp->rtl_fw.fw);
+		tp->rtl_fw.fw = NULL;
+	}
+}
+
+static void rtl_request_firmware(struct r8152 *tp)
+{
+	char *fw_name = NULL;
+
+	if (tp->rtl_fw.fw)
+		goto out_request;
+
+	switch (tp->version) {
+	case RTL_VER_01:
+		fw_name = "rtl_nic/rtl8152-1.fw";
+		break;
+	case RTL_VER_02:
+		fw_name = "rtl_nic/rtl8152-2.fw";
+		break;
+	case RTL_VER_03:
+		fw_name = "rtl_nic/rtl8153-1.fw";
+		break;
+	case RTL_VER_04:
+		fw_name = "rtl_nic/rtl8153-2.fw";
+		break;
+	case RTL_VER_05:
+		fw_name = "rtl_nic/rtl8153-3.fw";
+		break;
+	default:
+		goto out_request;
+	}
+
+	if (request_firmware(&tp->rtl_fw.fw, fw_name, &tp->netdev->dev) < 0)
+		goto err_warn;
+
+	if (!rtl_check_firmware(tp, &tp->rtl_fw)) {
+		netif_err(tp, ifup, tp->netdev, "invalid firwmare\n");
+		goto err_release_firmware;
+	}
+
+out_request:
+	return;
+
+err_release_firmware:
+	release_firmware(tp->rtl_fw.fw);
+err_warn:
+	netif_warn(tp, ifup, tp->netdev, "unable to load firmware patch %s\n",
+		   fw_name);
+	tp->rtl_fw.fw = ERR_PTR(-ENOENT);
+	goto out_request;
+}
+
+static void rtl_apply_firmware(struct r8152 *tp)
+{
+	if (!IS_ERR_OR_NULL(tp->rtl_fw.fw)) {
+		rtl_fw_write(tp, tp->rtl_fw.code, tp->rtl_fw.code_size);
+		tp->ocp_base = 0;
+	}
+}
+
 static struct tx_agg *r8152_get_tx_agg(struct r8152 *tp)
 {
 	struct tx_agg *agg = NULL;
@@ -2226,6 +3083,7 @@ static void r8152b_hw_phy_cfg(struct r8152 *tp)
 
 	r8152b_disable_aldps(tp);
 
+	rtl_apply_firmware(tp);
 
 	r8152b_enable_aldps(tp);
 	set_bit(PHY_RESET, &tp->flags);
@@ -2381,6 +3239,7 @@ static void r8153_hw_phy_cfg(struct r8152 *tp)
 		r8152_mdio_write(tp, MII_BMCR, data);
 	}
 
+	rtl_apply_firmware(tp);
 
 	if (tp->version == RTL_VER_03) {
 		data = ocp_reg_read(tp, OCP_EEE_CFG);
@@ -2802,6 +3661,7 @@ static int rtl8152_open(struct net_device *netdev)
 			tp->rtl_ops.disable(tp);
 	}
 
+	rtl_request_firmware(tp);
 	tp->rtl_ops.up(tp);
 
 	rtl8152_set_speed(tp, AUTONEG_ENABLE,
@@ -3130,6 +3990,10 @@ static void rtl8152_get_drvinfo(struct net_device *netdev,
 	strlcpy(info->driver, MODULENAME, sizeof(info->driver));
 	strlcpy(info->version, DRIVER_VERSION, sizeof(info->version));
 	usb_make_path(tp->udev, info->bus_info, sizeof(info->bus_info));
+	BUILD_BUG_ON(sizeof(info->fw_version) < sizeof(tp->rtl_fw.version));
+	if (!IS_ERR_OR_NULL(tp->rtl_fw.fw))
+		strlcpy(info->fw_version, tp->rtl_fw.version,
+			sizeof(info->fw_version));
 }
 
 static
@@ -3514,6 +4378,7 @@ static void rtl8152_disconnect(struct usb_interface *intf)
 		tasklet_kill(&tp->tl);
 		unregister_netdev(tp->netdev);
 		tp->rtl_ops.unload(tp);
+		rtl_release_firmware(tp);
 		free_netdev(tp->netdev);
 	}
 }
-- 
1.9.3

Re: [PATCH net-next 4/4] r8152: support firmware files

[Daniele Forsi]

2014-08-20 10:58 GMT+02:00 Hayes Wang:

> The firmware file is composed of the fw header and the commands. Each
> command has the following type.
>
>         cmd(2 bytes) + length(2 bytes) + data(variable bytes)

> +static bool rtl_fw_format_ok(struct rtl_fw *rtl_fw)

> +       start = le32_to_cpu(fw_header->fw_start);
> +       if (start > fw->size)
> +               goto out;

since "start" is an offset in an array of size "fw->size" this should
check for ">=" and if a command is at least cmd(2 bytes) + length(2
bytes), shouldn't this check for "start >= fw->size - 4"?

-- 
Daniele Forsi

RE: [PATCH net-next 4/4] r8152: support firmware files

[Hayes Wang]

 Daniele Forsi [mailto:dforsi@gmail.com] 
> Sent: Wednesday, August 20, 2014 8:01 PM
> To: Hayes Wang
> Cc: netdev@vger.kernel.org; nic_swsd; 
> linux-kernel@vger.kernel.org; USB list
> Subject: Re: [PATCH net-next 4/4] r8152: support firmware files
[...]
> > +       start = le32_to_cpu(fw_header->fw_start);
> > +       if (start > fw->size)
> > +               goto out;
> 
> since "start" is an offset in an array of size "fw->size" this should
> check for ">=" and if a command is at least cmd(2 bytes) + length(2
> bytes), shouldn't this check for "start >= fw->size - 4"?

Is this necessary? Besides the check of the "start",
there are checks of the "size" and rtl_fw_data_ok().
I think they cover the situations which you indicate.
 
Best Regards,
Hayes

Re: [PATCH net-next 4/4] r8152: support firmware files

[Daniele Forsi]

2014-08-20 14:35 GMT+02:00 Hayes Wang:

> Is this necessary? Besides the check of the "start",
> there are checks of the "size" and rtl_fw_data_ok().
> I think they cover the situations which you indicate.

it's not necessary and it's better checked later as you did!

-- 
Daniele Forsi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 4/4] r8152: support firmware files

[Francois Romieu]

Hayes Wang <hayeswang@realtek.com> :
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 937d132..63542cc 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
[...]
> +static void rtl_request_firmware(struct r8152 *tp)
> +{
> +	char *fw_name = NULL;
> +
> +	if (tp->rtl_fw.fw)
> +		goto out_request;
> +
> +	switch (tp->version) {
> +	case RTL_VER_01:
> +		fw_name = "rtl_nic/rtl8152-1.fw";
> +		break;
> +	case RTL_VER_02:
> +		fw_name = "rtl_nic/rtl8152-2.fw";
> +		break;
> +	case RTL_VER_03:
> +		fw_name = "rtl_nic/rtl8153-1.fw";
> +		break;
> +	case RTL_VER_04:
> +		fw_name = "rtl_nic/rtl8153-2.fw";
> +		break;
> +	case RTL_VER_05:
> +		fw_name = "rtl_nic/rtl8153-3.fw";
> +		break;

The driver should use MODULE_FIRMWARE() for these files.

-- 
Ueimor

RE: [PATCH net-next 4/4] r8152: support firmware files

[Hayes Wang]

Francois Romieu [mailto:romieu@fr.zoreil.com] 
[...]
> > +static void rtl_request_firmware(struct r8152 *tp)
> > +{
> > +	char *fw_name = NULL;
> > +
> > +	if (tp->rtl_fw.fw)
> > +		goto out_request;
> > +
> > +	switch (tp->version) {
> > +	case RTL_VER_01:
> > +		fw_name = "rtl_nic/rtl8152-1.fw";
> > +		break;
> > +	case RTL_VER_02:
> > +		fw_name = "rtl_nic/rtl8152-2.fw";
> > +		break;
> > +	case RTL_VER_03:
> > +		fw_name = "rtl_nic/rtl8153-1.fw";
> > +		break;
> > +	case RTL_VER_04:
> > +		fw_name = "rtl_nic/rtl8153-2.fw";
> > +		break;
> > +	case RTL_VER_05:
> > +		fw_name = "rtl_nic/rtl8153-3.fw";
> > +		break;
> 
> The driver should use MODULE_FIRMWARE() for these files.

Oops. I would fix this. Thanks.
 
Best Regards,
Hayes

Re: [PATCH net-next 0/4] r8152: firmware support

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Wed, 20 Aug 2014 16:58:35 +0800

> Parsing, checking, and writing the firmware.

You haven't told us why you need to do this.

These are just programming registers in the chip, and I see no reason
to not keep these in the driver with real code.

I'm not applying this series, you haven't explained what is happening
here and the reason for doing so.  Ironically, that's exactly what you
are supposed to provide in this 0/4 header email.

RE: [PATCH net-next 0/4] r8152: firmware support

[Hayes Wang]

 From: David Miller [mailto:davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org] 
[...]
> You haven't told us why you need to do this.
> 
> These are just programming registers in the chip, and I see no reason
> to not keep these in the driver with real code.
> 
> I'm not applying this series, you haven't explained what is happening
> here and the reason for doing so.  Ironically, that's exactly what you
> are supposed to provide in this 0/4 header email.

The nic has the MCU inside which is used to fix the PHY,
MAC, and some behavior of the USB device. Each parts have
different methods of updating the firmware by accessing the
registers. The firmware files are used to deal with the
processes, so I need some functions to parse the firmware
files to update the fimrware code.

I would resend these. Sorry.
 
Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 0/4] r8152: firmware support

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Mon, 25 Aug 2014 03:43:04 +0000

>  From: David Miller [mailto:davem@davemloft.net] 
> [...]
>> You haven't told us why you need to do this.
>> 
>> These are just programming registers in the chip, and I see no reason
>> to not keep these in the driver with real code.
>> 
>> I'm not applying this series, you haven't explained what is happening
>> here and the reason for doing so.  Ironically, that's exactly what you
>> are supposed to provide in this 0/4 header email.
> 
> The nic has the MCU inside which is used to fix the PHY,
> MAC, and some behavior of the USB device. Each parts have
> different methods of updating the firmware by accessing the
> registers. The firmware files are used to deal with the
> processes, so I need some functions to parse the firmware
> files to update the fimrware code.

That still doesn't convince me.

The functions I see you removing are just programming a set of
registers in some way.

And the firmware that is replacing those functions is just going to be
causing the same register writes, just even more obfuscated than it is
now.

You should keep the C functions which document and show clearly what
is being programmed in each chip.

Don't hide register programming behind firmware files, please.

RE: [PATCH net-next 0/4] r8152: firmware support

[Hayes Wang]

 From: David Miller [mailto:davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org] 
[...]
> That still doesn't convince me.
> 
> The functions I see you removing are just programming a set of
> registers in some way.

That is to clear the break point of the firmware. If a firmware exists,
you should clear it before updating a new one.

> And the firmware that is replacing those functions is just going to be
> causing the same register writes, just even more obfuscated than it is
> now.
> 
> You should keep the C functions which document and show clearly what
> is being programmed in each chip.
> 
> Don't hide register programming behind firmware files, please.

Excuse me. Some settings are relative the content of the firmware.
How should I deal with that parts. Take 8153 (RTL_VER_03) for
example.

	1. wait PLA 0xb800 bit 6 = 1.
	2. set patch key = 0x7000.
	3. update the PHY firmware.
	4. enable the firmware.
	5. set USB 0xcfca bit 14 = 0.
	6. clear break point (That is r8153_clear_bp()).
	7. load the firmware about PLA and USB parts.
	8. set the break point of the firmware.
	9. set USB 0xcfca bit 14 = 1.

Except the step 3, 4, 6 and 7, the other steps depend on the
context of the firmware. That is, for different firmware, some
actions would be removed or added, and some settings would be
different. Especially the step 8, it often different for
different firmwares. Should I add some firmware version check
in the source code? Such as

	if (fw_version == v1) {
		...
		load firmware
		set break point of the firmware
		...
	} else if (fw_version == v2) {
	...--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 0/4] r8152: firmware support

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Mon, 25 Aug 2014 06:43:02 +0000

> Except the step 3, 4, 6 and 7, the other steps depend on the
> context of the firmware. That is, for different firmware, some
> actions would be removed or added, and some settings would be
> different. Especially the step 8, it often different for
> different firmwares. Should I add some firmware version check
> in the source code?

This is extremely poor design of the firmware, adding such constantly
changing dependencies and constantly changing programming sequences
just to get the firmare executing is a terrible idea.

You really need to sanitize this in some way, because what you have
posted is totally unacceptable to me.

[PATCH net-next] r8152: check code with checkpatch.pl

[Hayes Wang]

 626: CHECK: Alignment should match open parenthesis
 646: CHECK: Alignment should match open parenthesis
 655: CHECK: Alignment should match open parenthesis
 695: CHECK: Alignment should match open parenthesis
 729: CHECK: Alignment should match open parenthesis
 739: CHECK: Alignment should match open parenthesis
 976: WARNING: externs should be avoided in .c files
 1314: CHECK: Alignment should match open parenthesis
 1358: WARNING: networking block comments don't use an empty /* line, use /* Comment...
 1402: WARNING: networking block comments don't use an empty /* line, use /* Comment...
 1521: CHECK: multiple assignments should be avoided
 1775: CHECK: Alignment should match open parenthesis
 1838: CHECK: multiple assignments should be avoided
 1843: CHECK: multiple assignments should be avoided
 1847: CHECK: multiple assignments should be avoided
 1850: WARNING: Missing a blank line after declarations
 1864: CHECK: Alignment should match open parenthesis
 1872: CHECK: braces {} should be used on all arms of this statement
 1906: CHECK: usleep_range is preferred over udelay
 2865: WARNING: networking block comments don't use an empty /* line, use /* Comment...
 3088: CHECK: Alignment should match open parenthesis
 total: 0 errors, 5 warnings, 16 checks, 3567 lines checked

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 66 ++++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 87f7104..2470d9c 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -623,8 +623,8 @@ int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 		return -ENOMEM;
 
 	ret = usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
-			       RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
-			       value, index, tmp, size, 500);
+			      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
+			      value, index, tmp, size, 500);
 
 	memcpy(data, tmp, size);
 	kfree(tmp);
@@ -643,8 +643,8 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 		return -ENOMEM;
 
 	ret = usb_control_msg(tp->udev, usb_sndctrlpipe(tp->udev, 0),
-			       RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
-			       value, index, tmp, size, 500);
+			      RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
+			      value, index, tmp, size, 500);
 
 	kfree(tmp);
 
@@ -652,7 +652,7 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 }
 
 static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
-				void *data, u16 type)
+			    void *data, u16 type)
 {
 	u16 limit = 64;
 	int ret = 0;
@@ -692,7 +692,7 @@ static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
 }
 
 static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
-				u16 size, void *data, u16 type)
+			     u16 size, void *data, u16 type)
 {
 	int ret;
 	u16 byteen_start, byteen_end, byen;
@@ -726,8 +726,8 @@ static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
 		while (size) {
 			if (size > limit) {
 				ret = set_registers(tp, index,
-					type | BYTE_EN_DWORD,
-					limit, data);
+						    type | BYTE_EN_DWORD,
+						    limit, data);
 				if (ret < 0)
 					goto error1;
 
@@ -736,8 +736,8 @@ static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
 				size -= limit;
 			} else {
 				ret = set_registers(tp, index,
-					type | BYTE_EN_DWORD,
-					size, data);
+						    type | BYTE_EN_DWORD,
+						    size, data);
 				if (ret < 0)
 					goto error1;
 
@@ -972,8 +972,8 @@ void write_mii_word(struct net_device *netdev, int phy_id, int reg, int val)
 	usb_autopm_put_interface(tp->intf);
 }
 
-static
-int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags);
+static int
+r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags);
 
 static inline void set_ethernet_addr(struct r8152 *tp)
 {
@@ -1311,8 +1311,8 @@ static int alloc_all_mem(struct r8152 *tp)
 
 	tp->intr_interval = (int)ep_intr->desc.bInterval;
 	usb_fill_int_urb(tp->intr_urb, tp->udev, usb_rcvintpipe(tp->udev, 3),
-		     tp->intr_buff, INTBUFSIZE, intr_callback,
-		     tp, tp->intr_interval);
+			 tp->intr_buff, INTBUFSIZE, intr_callback,
+			 tp, tp->intr_interval);
 
 	return 0;
 
@@ -1354,8 +1354,7 @@ static inline __be16 get_protocol(struct sk_buff *skb)
 	return protocol;
 }
 
-/*
- * r8152_csum_workaround()
+/* r8152_csum_workaround()
  * The hw limites the value the transport offset. When the offset is out of the
  * range, calculate the checksum by sw.
  */
@@ -1398,8 +1397,7 @@ drop:
 	}
 }
 
-/*
- * msdn_giant_send_check()
+/* msdn_giant_send_check()
  * According to the document of microsoft, the TCP Pseudo Header excludes the
  * packet length for IPv6 TCP large packets.
  */
@@ -1518,7 +1516,8 @@ static int r8152_tx_agg_fill(struct r8152 *tp, struct tx_agg *agg)
 	spin_unlock(&tx_queue->lock);
 
 	tx_data = agg->head;
-	agg->skb_num = agg->skb_len = 0;
+	agg->skb_num = 0;
+	agg->skb_len = 0;
 	remain = rx_buf_sz;
 
 	while (remain >= ETH_ZLEN + sizeof(struct tx_desc)) {
@@ -1772,8 +1771,8 @@ static
 int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags)
 {
 	usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1),
-		      agg->head, rx_buf_sz,
-		      (usb_complete_t)read_bulk_callback, agg);
+			  agg->head, rx_buf_sz,
+			  (usb_complete_t)read_bulk_callback, agg);
 
 	return usb_submit_urb(agg->urb, mem_flags);
 }
@@ -1835,18 +1834,22 @@ static void _rtl8152_set_rx_mode(struct net_device *netdev)
 		/* Unconditionally log net taps. */
 		netif_notice(tp, link, netdev, "Promiscuous mode enabled\n");
 		ocp_data |= RCR_AM | RCR_AAP;
-		mc_filter[1] = mc_filter[0] = 0xffffffff;
+		mc_filter[1] = 0xffffffff;
+		mc_filter[0] = 0xffffffff;
 	} else if ((netdev_mc_count(netdev) > multicast_filter_limit) ||
 		   (netdev->flags & IFF_ALLMULTI)) {
 		/* Too many to filter perfectly -- accept all multicasts. */
 		ocp_data |= RCR_AM;
-		mc_filter[1] = mc_filter[0] = 0xffffffff;
+		mc_filter[1] = 0xffffffff;
+		mc_filter[0] = 0xffffffff;
 	} else {
 		struct netdev_hw_addr *ha;
 
-		mc_filter[1] = mc_filter[0] = 0;
+		mc_filter[1] = 0;
+		mc_filter[0] = 0;
 		netdev_for_each_mc_addr(ha, netdev) {
 			int bit_nr = ether_crc(ETH_ALEN, ha->addr) >> 26;
+
 			mc_filter[bit_nr >> 5] |= 1 << (bit_nr & 31);
 			ocp_data |= RCR_AM;
 		}
@@ -1861,7 +1864,7 @@ static void _rtl8152_set_rx_mode(struct net_device *netdev)
 }
 
 static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
-					struct net_device *netdev)
+				      struct net_device *netdev)
 {
 	struct r8152 *tp = netdev_priv(netdev);
 
@@ -1877,8 +1880,9 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
 			usb_mark_last_busy(tp->udev);
 			tasklet_schedule(&tp->tl);
 		}
-	} else if (skb_queue_len(&tp->tx_queue) > tp->tx_qlen)
+	} else if (skb_queue_len(&tp->tx_queue) > tp->tx_qlen) {
 		netif_stop_queue(netdev);
+	}
 
 	return NETDEV_TX_OK;
 }
@@ -1903,7 +1907,7 @@ static void rtl8152_nic_reset(struct r8152 *tp)
 	for (i = 0; i < 1000; i++) {
 		if (!(ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CR) & CR_RST))
 			break;
-		udelay(100);
+		usleep_range(100, 400);
 	}
 }
 
@@ -2861,8 +2865,7 @@ static int rtl8152_close(struct net_device *netdev)
 	if (res < 0) {
 		rtl_drop_queued_tx(tp);
 	} else {
-		/*
-		 * The autosuspend may have been enabled and wouldn't
+		/* The autosuspend may have been enabled and wouldn't
 		 * be disable when autoresume occurs, because the
 		 * netif_running() would be false.
 		 */
@@ -3085,8 +3088,9 @@ static int rtl8152_resume(struct usb_interface *intf)
 		} else {
 			tp->rtl_ops.up(tp);
 			rtl8152_set_speed(tp, AUTONEG_ENABLE,
-				tp->mii.supports_gmii ? SPEED_1000 : SPEED_100,
-				DUPLEX_FULL);
+					  tp->mii.supports_gmii ?
+					  SPEED_1000 : SPEED_100,
+					  DUPLEX_FULL);
 		}
 		tp->speed = 0;
 		netif_carrier_off(tp->netdev);
-- 
1.9.3

Re: [PATCH net-next] r8152: check code with checkpatch.pl

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Mon, 25 Aug 2014 15:53:00 +0800

>  626: CHECK: Alignment should match open parenthesis
>  646: CHECK: Alignment should match open parenthesis
>  655: CHECK: Alignment should match open parenthesis
>  695: CHECK: Alignment should match open parenthesis
>  729: CHECK: Alignment should match open parenthesis
>  739: CHECK: Alignment should match open parenthesis
>  976: WARNING: externs should be avoided in .c files
>  1314: CHECK: Alignment should match open parenthesis
>  1358: WARNING: networking block comments don't use an empty /* line, use /* Comment...
>  1402: WARNING: networking block comments don't use an empty /* line, use /* Comment...
>  1521: CHECK: multiple assignments should be avoided
>  1775: CHECK: Alignment should match open parenthesis
>  1838: CHECK: multiple assignments should be avoided
>  1843: CHECK: multiple assignments should be avoided
>  1847: CHECK: multiple assignments should be avoided
>  1850: WARNING: Missing a blank line after declarations
>  1864: CHECK: Alignment should match open parenthesis
>  1872: CHECK: braces {} should be used on all arms of this statement
>  1906: CHECK: usleep_range is preferred over udelay
>  2865: WARNING: networking block comments don't use an empty /* line, use /* Comment...
>  3088: CHECK: Alignment should match open parenthesis
>  total: 0 errors, 5 warnings, 16 checks, 3567 lines checked
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>

Applied, thanks.

[PATCH net-next] r8152: replace strncpy with strlcpy

[Hayes Wang]

Replace the strncpy with strlcpy, and use sizeof to determine the
length.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 2470d9c..33dcc97 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3151,8 +3151,8 @@ static void rtl8152_get_drvinfo(struct net_device *netdev,
 {
 	struct r8152 *tp = netdev_priv(netdev);
 
-	strncpy(info->driver, MODULENAME, ETHTOOL_BUSINFO_LEN);
-	strncpy(info->version, DRIVER_VERSION, ETHTOOL_BUSINFO_LEN);
+	strlcpy(info->driver, MODULENAME, sizeof(info->driver));
+	strlcpy(info->version, DRIVER_VERSION, sizeof(info->version));
 	usb_make_path(tp->udev, info->bus_info, sizeof(info->bus_info));
 }
 
-- 
1.9.3

Re: [PATCH net-next] r8152: replace strncpy with strlcpy

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Tue, 26 Aug 2014 10:08:23 +0800

> Replace the strncpy with strlcpy, and use sizeof to determine the
> length.
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>

Applied, thanks.