[PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup

From: Petko Manolov <petkan@nucleusys.com>

Moving constant and structure definitions out of rtl8150.c;

Signed-off-by: Petko Manolov <petkan@nucleusys.com>
---
 drivers/net/usb/rtl8150.c |  121 +----------------------------------
 1 file changed, 2 insertions(+), 119 deletions(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index a491d3a..7d1897b 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c

@@ -17,132 +17,15 @@
 #include <linux/usb.h>
 #include <asm/uaccess.h>
 
+#include "rtl8150.h"
+
 /* Version Information */
 #define DRIVER_VERSION "v0.6.2 (2004/08/27)"
 #define DRIVER_AUTHOR "Petko Manolov <petkan@users.sourceforge.net>"
 #define DRIVER_DESC "rtl8150 based usb-ethernet driver"
 
-#define	IDR			0x0120
-#define	MAR			0x0126
-#define	CR			0x012e
-#define	TCR			0x012f
-#define	RCR			0x0130
-#define	TSR			0x0132
-#define	RSR			0x0133
-#define	CON0			0x0135
-#define	CON1			0x0136
-#define	MSR			0x0137
-#define	PHYADD			0x0138
-#define	PHYDAT			0x0139
-#define	PHYCNT			0x013b
-#define	GPPC			0x013d
-#define	BMCR			0x0140
-#define	BMSR			0x0142
-#define	ANAR			0x0144
-#define	ANLP			0x0146
-#define	AER			0x0148
-#define CSCR			0x014C  /* This one has the link status */
-#define CSCR_LINK_STATUS	(1 << 3)
-
-#define	IDR_EEPROM		0x1202
-
-#define	PHY_READ		0
-#define	PHY_WRITE		0x20
-#define	PHY_GO			0x40
-
-#define	MII_TIMEOUT		10
-#define	INTBUFSIZE		8
-
-#define	RTL8150_REQT_READ	0xc0
-#define	RTL8150_REQT_WRITE	0x40
-#define	RTL8150_REQ_GET_REGS	0x05
-#define	RTL8150_REQ_SET_REGS	0x05
-
-
-/* Transmit status register errors */
-#define TSR_ECOL		(1<<5)
-#define TSR_LCOL		(1<<4)
-#define TSR_LOSS_CRS		(1<<3)
-#define TSR_JBR			(1<<2)
-#define TSR_ERRORS		(TSR_ECOL | TSR_LCOL | TSR_LOSS_CRS | TSR_JBR)
-/* Receive status register errors */
-#define RSR_CRC			(1<<2)
-#define RSR_FAE			(1<<1)
-#define RSR_ERRORS		(RSR_CRC | RSR_FAE)
-
-/* Media status register definitions */
-#define MSR_DUPLEX		(1<<4)
-#define MSR_SPEED		(1<<3)
-#define MSR_LINK		(1<<2)
-
-/* Interrupt pipe data */
-#define INT_TSR			0x00
-#define INT_RSR			0x01
-#define INT_MSR			0x02
-#define INT_WAKSR		0x03
-#define INT_TXOK_CNT		0x04
-#define INT_RXLOST_CNT		0x05
-#define INT_CRERR_CNT		0x06
-#define INT_COL_CNT		0x07
-
-
-#define	RTL8150_MTU		1540
-#define	RTL8150_TX_TIMEOUT	(HZ)
-#define	RX_SKB_POOL_SIZE	4
-
-/* rtl8150 flags */
-#define	RTL8150_HW_CRC		0
-#define	RX_REG_SET		1
-#define	RTL8150_UNPLUG		2
-#define	RX_URB_FAIL		3
-
-/* Define these values to match your device */
-#define	VENDOR_ID_REALTEK		0x0bda
-#define	VENDOR_ID_MELCO			0x0411
-#define	VENDOR_ID_MICRONET		0x3980
-#define	VENDOR_ID_LONGSHINE		0x07b8
-#define	VENDOR_ID_OQO			0x1557
-#define	VENDOR_ID_ZYXEL			0x0586
-
-#define PRODUCT_ID_RTL8150		0x8150
-#define	PRODUCT_ID_LUAKTX		0x0012
-#define	PRODUCT_ID_LCS8138TX		0x401a
-#define PRODUCT_ID_SP128AR		0x0003
-#define	PRODUCT_ID_PRESTIGE		0x401a
-
-#undef	EEPROM_WRITE
-
-/* table of devices that work with this driver */
-static struct usb_device_id rtl8150_table[] = {
-	{USB_DEVICE(VENDOR_ID_REALTEK, PRODUCT_ID_RTL8150)},
-	{USB_DEVICE(VENDOR_ID_MELCO, PRODUCT_ID_LUAKTX)},
-	{USB_DEVICE(VENDOR_ID_MICRONET, PRODUCT_ID_SP128AR)},
-	{USB_DEVICE(VENDOR_ID_LONGSHINE, PRODUCT_ID_LCS8138TX)},
-	{USB_DEVICE(VENDOR_ID_OQO, PRODUCT_ID_RTL8150)},
-	{USB_DEVICE(VENDOR_ID_ZYXEL, PRODUCT_ID_PRESTIGE)},
-	{}
-};
-
 MODULE_DEVICE_TABLE(usb, rtl8150_table);
 
-struct rtl8150 {
-	unsigned long flags;
-	struct usb_device *udev;
-	struct tasklet_struct tl;
-	struct net_device *netdev;
-	struct urb *rx_urb, *tx_urb, *intr_urb, *ctrl_urb;
-	struct sk_buff *tx_skb, *rx_skb;
-	struct sk_buff *rx_skb_pool[RX_SKB_POOL_SIZE];
-	spinlock_t rx_pool_lock;
-	struct usb_ctrlrequest dr;
-	int intr_interval;
-	__le16 rx_creg;
-	u8 *intr_buff;
-	u8 phy;
-};
-
-typedef struct rtl8150 rtl8150_t;
-
 static const char driver_name [] = "rtl8150";
 
 /*

Petko Manolov [off-list ref] :

From: Petko Manolov <petkan@nucleusys.com>

Moving constant and structure definitions out of rtl8150.c;

What's the point ?

[...]

It won't compile. You shouldn't do that.

-- 
Ueimor

On Sat, 18 May 2013, Francois Romieu wrote:

Petko Manolov [off-list ref] :

quoted

From: Petko Manolov <petkan@nucleusys.com>

Moving constant and structure definitions out of rtl8150.c;

What's the point ?

The general logic of having .h files applies.
 

[...]

quoted

---
 drivers/net/usb/rtl8150.c |  121 +----------------------------------
 1 file changed, 2 insertions(+), 119 deletions(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index a491d3a..7d1897b 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c

@@ -17,132 +17,15 @@
 #include <linux/usb.h>
 #include <asm/uaccess.h>
 
+#include "rtl8150.h"

It won't compile. You shouldn't do that.

It does compile.  Both inside and outside of the tree.

If the proper place for rtl8150.h is somewhere in include/linux/... then 
it is different matter.

Petko Manolov [off-list ref] :

On Sat, 18 May 2013, Francois Romieu wrote:

quoted

quoted

From: Petko Manolov <petkan@nucleusys.com>

[...]

quoted

quoted

Moving constant and structure definitions out of rtl8150.c;

What's the point ?

The general logic of having .h files applies.

Which one ?
- share it through the kernel or with userspace
- personal choice .c split

I don't mind the later even if it does not exactly apply to a
WIP driver. I'd still avoid future copycat followers though.

[...]

quoted

quoted

 drivers/net/usb/rtl8150.c |  121 +----------------------------------
 1 file changed, 2 insertions(+), 119 deletions(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index a491d3a..7d1897b 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c

@@ -17,132 +17,15 @@
 #include <linux/usb.h>
 #include <asm/uaccess.h>
 
+#include "rtl8150.h"

It won't compile. You shouldn't do that.

It does compile.  Both inside and outside of the tree.

The rtl8150.h file is created in patch #2. This is patch #1.

So...

-- 
Ueimor

On Sun, 19 May 2013, Francois Romieu wrote:

Which one ?
- share it through the kernel or with userspace
- personal choice .c split

It is obviously not the former.  I think that in general constant and 
other definitions (in their majority) should be in a header file.  I 
definitely like this way better.

Perhaps in this particular case my patch is a bit of an overkill as the 
code lines aren't that many.  If the consensus is in this direction i'll 
revert this part of the series.

I don't mind the later even if it does not exactly apply to a
WIP driver. I'd still avoid future copycat followers though.

This isn't WIP anymore as the W(ork) part is missing.  After so many quiet 
years i assume the experimental tag should be removed.

The rtl8150.h file is created in patch #2. This is patch #1.

So...

So first apply patch #1 and then patch #2.

However, if it is required for the driver to be in build-able form after 
applying any single patch i'll coalesce #1 and #2 before next submission.


David?


		Petko

From: Petko Manolov <petkan@nucleusys.com>
Date: Mon, 20 May 2013 09:58:39 +0300 (EEST)

So first apply patch #1 and then patch #2.

Then nobody can properly GIT bisect through your patch series.

The tree must work and build properly at each and every step
of your patch series.

On Mon, 20 May 2013, David Miller wrote:

From: Petko Manolov <petkan@nucleusys.com>
Date: Mon, 20 May 2013 09:58:39 +0300 (EEST)

quoted

So first apply patch #1 and then patch #2.

Then nobody can properly GIT bisect through your patch series.

The tree must work and build properly at each and every step
of your patch series.

Got it.  What about the .c/.h split?

From: Petko Manolov <petkan@nucleusys.com>
Date: Mon, 20 May 2013 10:09:00 +0300 (EEST)

What about the .c/.h split?

I have no strong opinion either way.

On Mon, 20 May 2013, David Miller wrote:

From: Petko Manolov <petkan@nucleusys.com>
Date: Mon, 20 May 2013 10:09:00 +0300 (EEST)

quoted

What about the .c/.h split?

I have no strong opinion either way.

OK, so i'll prepare new patch series that coalesce my original patch #1 
and #2, and apply the Francois suggestion about using the generic 
netdev_alloc_skb_ip_align() in the interrupt path.

Which tree would you want me to diff against?

From: Petko Manolov <petkan@nucleusys.com>
Date: Mon, 20 May 2013 10:18:17 +0300 (EEST)

On Mon, 20 May 2013, David Miller wrote:

quoted

From: Petko Manolov <petkan@nucleusys.com>
Date: Mon, 20 May 2013 10:09:00 +0300 (EEST)

quoted

What about the .c/.h split?

I have no strong opinion either way.

OK, so i'll prepare new patch series that coalesce my original patch #1 
and #2, and apply the Francois suggestion about using the generic 
netdev_alloc_skb_ip_align() in the interrupt path.

Which tree would you want me to diff against?

As has been explained to you already, cleanups belong in 'net-next',
bug fixes belong in 'net'.

If you series has both, you have to submit them separately.  Submit
the bug fixes to 'net', then the next time I merge 'net' into 'net-next'
you can submit the cleanups on top against 'net-next'.

On Mon, 20 May 2013, David Miller wrote:

From: Petko Manolov <petkan@nucleusys.com>
Date: Mon, 20 May 2013 10:18:17 +0300 (EEST)

quoted

On Mon, 20 May 2013, David Miller wrote:

quoted

From: Petko Manolov <petkan@nucleusys.com>
Date: Mon, 20 May 2013 10:09:00 +0300 (EEST)

quoted

What about the .c/.h split?

I have no strong opinion either way.

OK, so i'll prepare new patch series that coalesce my original patch #1 
and #2, and apply the Francois suggestion about using the generic 
netdev_alloc_skb_ip_align() in the interrupt path.

Which tree would you want me to diff against?

As has been explained to you already, cleanups belong in 'net-next',
bug fixes belong in 'net'.

If you series has both, you have to submit them separately.  Submit
the bug fixes to 'net', then the next time I merge 'net' into 'net-next'
you can submit the cleanups on top against 'net-next'.

Got it.  Thanks.