Re: [PATCH net-next v2] net/usb/r8153_ecm: support ECM mode for RTL8153
On Wed, 4 Nov 2020 01:39:52 + Hayes Wang wrote: > Jakub Kicinski > > Sent: Wednesday, November 4, 2020 12:16 AM > [...] > > > So no, please do not create such a common file, it is not needed or a > > > good idea. > > > > I wouldn't go that far, PCI subsystem just doesn't want everyone to add > > IDs to the shared file unless there is a reason. > > > > * Do not add new entries to this file unless the definitions > > * are shared between multiple drivers. > > > > Which seems quite reasonable. But it is most certainly your call :) > > Do I have to resend this patch? Yes please, that'd be easiest for me. Also Oliver wasn't CCed on this posting.
RE: [PATCH net-next v2] net/usb/r8153_ecm: support ECM mode for RTL8153
Jakub Kicinski > Sent: Wednesday, November 4, 2020 12:16 AM [...] > > So no, please do not create such a common file, it is not needed or a > > good idea. > > I wouldn't go that far, PCI subsystem just doesn't want everyone to add > IDs to the shared file unless there is a reason. > > *Do not add new entries to this file unless the definitions > *are shared between multiple drivers. > > Which seems quite reasonable. But it is most certainly your call :) Do I have to resend this patch? Best Regards, Hayes
Re: [PATCH net-next v2] net/usb/r8153_ecm: support ECM mode for RTL8153
On Tue, 3 Nov 2020 10:32:41 +0100 Greg Kroah-Hartman wrote: > On Mon, Nov 02, 2020 at 11:47:18AM -0800, Jakub Kicinski wrote: > > On Mon, 2 Nov 2020 07:20:15 + Hayes Wang wrote: > > > Jakub Kicinski > > > > Can you describe the use case in more detail? > > > > > > > > AFAICT r8152 defines a match for the exact same device. > > > > Does it not mean that which driver is used will be somewhat random > > > > if both are built? > > > > > > I export rtl_get_version() from r8152. It would return none zero > > > value if r8152 could support this device. Both r8152 and r8153_ecm > > > would check the return value of rtl_get_version() in porbe(). > > > Therefore, if rtl_get_version() return none zero value, the r8152 > > > is used for the device with vendor mode. Otherwise, the r8153_ecm > > > is used for the device with ECM mode. > > > > Oh, I see, I missed that the rtl_get_version() checking is the inverse > > of r8152. > > > > > > > +/* Define these values to match your device */ > > > > > +#define VENDOR_ID_REALTEK0x0bda > > > > > +#define VENDOR_ID_MICROSOFT 0x045e > > > > > +#define VENDOR_ID_SAMSUNG0x04e8 > > > > > +#define VENDOR_ID_LENOVO 0x17ef > > > > > +#define VENDOR_ID_LINKSYS0x13b1 > > > > > +#define VENDOR_ID_NVIDIA 0x0955 > > > > > +#define VENDOR_ID_TPLINK 0x2357 > > > > > > > > $ git grep 0x2357 | grep -i tplink > > > > drivers/net/usb/cdc_ether.c:#define TPLINK_VENDOR_ID0x2357 > > > > drivers/net/usb/r8152.c:#define VENDOR_ID_TPLINK0x2357 > > > > drivers/usb/serial/option.c:#define TPLINK_VENDOR_ID > > > > 0x2357 > > > > > > > > $ git grep 0x17ef | grep -i lenovo > > > > drivers/hid/hid-ids.h:#define USB_VENDOR_ID_LENOVO 0x17ef > > > > drivers/hid/wacom.h:#define USB_VENDOR_ID_LENOVO0x17ef > > > > drivers/net/usb/cdc_ether.c:#define LENOVO_VENDOR_ID0x17ef > > > > drivers/net/usb/r8152.c:#define VENDOR_ID_LENOVO0x17ef > > > > > > > > Time to consolidate those vendor id defines perhaps? > > > > > > It seems that there is no such header file which I could include > > > or add the new vendor IDs. > > > > Please create one. (Adding Greg KH to the recipients, in case there is > > a reason that USB subsystem doesn't have a common vendor id header.) > > There is a reason, it's a nightmare to maintain and handle merges for, > just don't do it. Ah! Good that we asked :) > Read the comments at the top of the pci_ids.h file if you are curious > why we don't even do this for PCI device ids anymore for the past 10+ > years. > > So no, please do not create such a common file, it is not needed or a > good idea. I wouldn't go that far, PCI subsystem just doesn't want everyone to add IDs to the shared file unless there is a reason. * Do not add new entries to this file unless the definitions * are shared between multiple drivers. Which seems quite reasonable. But it is most certainly your call :)
RE: [PATCH net-next v2] net/usb/r8153_ecm: support ECM mode for RTL8153
Greg Kroah-Hartman > Sent: Tuesday, November 3, 2020 5:33 PM [...] > There is a reason, it's a nightmare to maintain and handle merges for, > just don't do it. > > Read the comments at the top of the pci_ids.h file if you are curious > why we don't even do this for PCI device ids anymore for the past 10+ > years. > > So no, please do not create such a common file, it is not needed or a > good idea. Oops. I have sent it.
Re: [PATCH net-next v2] net/usb/r8153_ecm: support ECM mode for RTL8153
On Mon, Nov 02, 2020 at 11:47:18AM -0800, Jakub Kicinski wrote: > On Mon, 2 Nov 2020 07:20:15 + Hayes Wang wrote: > > Jakub Kicinski > > > Can you describe the use case in more detail? > > > > > > AFAICT r8152 defines a match for the exact same device. > > > Does it not mean that which driver is used will be somewhat random > > > if both are built? > > > > I export rtl_get_version() from r8152. It would return none zero > > value if r8152 could support this device. Both r8152 and r8153_ecm > > would check the return value of rtl_get_version() in porbe(). > > Therefore, if rtl_get_version() return none zero value, the r8152 > > is used for the device with vendor mode. Otherwise, the r8153_ecm > > is used for the device with ECM mode. > > Oh, I see, I missed that the rtl_get_version() checking is the inverse > of r8152. > > > > > +/* Define these values to match your device */ > > > > +#define VENDOR_ID_REALTEK 0x0bda > > > > +#define VENDOR_ID_MICROSOFT0x045e > > > > +#define VENDOR_ID_SAMSUNG 0x04e8 > > > > +#define VENDOR_ID_LENOVO 0x17ef > > > > +#define VENDOR_ID_LINKSYS 0x13b1 > > > > +#define VENDOR_ID_NVIDIA 0x0955 > > > > +#define VENDOR_ID_TPLINK 0x2357 > > > > > > $ git grep 0x2357 | grep -i tplink > > > drivers/net/usb/cdc_ether.c:#define TPLINK_VENDOR_ID 0x2357 > > > drivers/net/usb/r8152.c:#define VENDOR_ID_TPLINK 0x2357 > > > drivers/usb/serial/option.c:#define TPLINK_VENDOR_ID > > > 0x2357 > > > > > > $ git grep 0x17ef | grep -i lenovo > > > drivers/hid/hid-ids.h:#define USB_VENDOR_ID_LENOVO0x17ef > > > drivers/hid/wacom.h:#define USB_VENDOR_ID_LENOVO 0x17ef > > > drivers/net/usb/cdc_ether.c:#define LENOVO_VENDOR_ID 0x17ef > > > drivers/net/usb/r8152.c:#define VENDOR_ID_LENOVO 0x17ef > > > > > > Time to consolidate those vendor id defines perhaps? > > > > It seems that there is no such header file which I could include > > or add the new vendor IDs. > > Please create one. (Adding Greg KH to the recipients, in case there is > a reason that USB subsystem doesn't have a common vendor id header.) There is a reason, it's a nightmare to maintain and handle merges for, just don't do it. Read the comments at the top of the pci_ids.h file if you are curious why we don't even do this for PCI device ids anymore for the past 10+ years. So no, please do not create such a common file, it is not needed or a good idea. thanks, greg k-h
Re: [PATCH net-next v2] net/usb/r8153_ecm: support ECM mode for RTL8153
On Mon, 2 Nov 2020 07:20:15 + Hayes Wang wrote: > Jakub Kicinski > > Can you describe the use case in more detail? > > > > AFAICT r8152 defines a match for the exact same device. > > Does it not mean that which driver is used will be somewhat random > > if both are built? > > I export rtl_get_version() from r8152. It would return none zero > value if r8152 could support this device. Both r8152 and r8153_ecm > would check the return value of rtl_get_version() in porbe(). > Therefore, if rtl_get_version() return none zero value, the r8152 > is used for the device with vendor mode. Otherwise, the r8153_ecm > is used for the device with ECM mode. Oh, I see, I missed that the rtl_get_version() checking is the inverse of r8152. > > > +/* Define these values to match your device */ > > > +#define VENDOR_ID_REALTEK0x0bda > > > +#define VENDOR_ID_MICROSOFT 0x045e > > > +#define VENDOR_ID_SAMSUNG0x04e8 > > > +#define VENDOR_ID_LENOVO 0x17ef > > > +#define VENDOR_ID_LINKSYS0x13b1 > > > +#define VENDOR_ID_NVIDIA 0x0955 > > > +#define VENDOR_ID_TPLINK 0x2357 > > > > $ git grep 0x2357 | grep -i tplink > > drivers/net/usb/cdc_ether.c:#define TPLINK_VENDOR_ID0x2357 > > drivers/net/usb/r8152.c:#define VENDOR_ID_TPLINK0x2357 > > drivers/usb/serial/option.c:#define TPLINK_VENDOR_ID > > 0x2357 > > > > $ git grep 0x17ef | grep -i lenovo > > drivers/hid/hid-ids.h:#define USB_VENDOR_ID_LENOVO 0x17ef > > drivers/hid/wacom.h:#define USB_VENDOR_ID_LENOVO0x17ef > > drivers/net/usb/cdc_ether.c:#define LENOVO_VENDOR_ID0x17ef > > drivers/net/usb/r8152.c:#define VENDOR_ID_LENOVO0x17ef > > > > Time to consolidate those vendor id defines perhaps? > > It seems that there is no such header file which I could include > or add the new vendor IDs. Please create one. (Adding Greg KH to the recipients, in case there is a reason that USB subsystem doesn't have a common vendor id header.) Also please make sure to add Oliver to the CC for v3, to make sure the reuse of CDC_ETHER is okay.
RE: [PATCH net-next v2] net/usb/r8153_ecm: support ECM mode for RTL8153
Jakub Kicinski [...] > Can you describe the use case in more detail? > > AFAICT r8152 defines a match for the exact same device. > Does it not mean that which driver is used will be somewhat random > if both are built? I export rtl_get_version() from r8152. It would return none zero value if r8152 could support this device. Both r8152 and r8153_ecm would check the return value of rtl_get_version() in porbe(). Therefore, if rtl_get_version() return none zero value, the r8152 is used for the device with vendor mode. Otherwise, the r8153_ecm is used for the device with ECM mode. > > +/* Define these values to match your device */ > > +#define VENDOR_ID_REALTEK 0x0bda > > +#define VENDOR_ID_MICROSOFT0x045e > > +#define VENDOR_ID_SAMSUNG 0x04e8 > > +#define VENDOR_ID_LENOVO 0x17ef > > +#define VENDOR_ID_LINKSYS 0x13b1 > > +#define VENDOR_ID_NVIDIA 0x0955 > > +#define VENDOR_ID_TPLINK 0x2357 > > $ git grep 0x2357 | grep -i tplink > drivers/net/usb/cdc_ether.c:#define TPLINK_VENDOR_ID 0x2357 > drivers/net/usb/r8152.c:#define VENDOR_ID_TPLINK 0x2357 > drivers/usb/serial/option.c:#define TPLINK_VENDOR_ID 0x2357 > > $ git grep 0x17ef | grep -i lenovo > drivers/hid/hid-ids.h:#define USB_VENDOR_ID_LENOVO0x17ef > drivers/hid/wacom.h:#define USB_VENDOR_ID_LENOVO 0x17ef > drivers/net/usb/cdc_ether.c:#define LENOVO_VENDOR_ID 0x17ef > drivers/net/usb/r8152.c:#define VENDOR_ID_LENOVO 0x17ef > > Time to consolidate those vendor id defines perhaps? It seems that there is no such header file which I could include or add the new vendor IDs. Best Regards, Hayes
Re: [PATCH net-next v2] net/usb/r8153_ecm: support ECM mode for RTL8153
On Fri, 30 Oct 2020 11:23:08 +0800 Hayes Wang wrote: > Support ECM mode based on cdc_ether with relative mii functions, > when CONFIG_USB_RTL8152 is not set, or the device is not supported > by r8152 driver. > > Signed-off-by: Hayes Wang Can you describe the use case in more detail? AFAICT r8152 defines a match for the exact same device. Does it not mean that which driver is used will be somewhat random if both are built? > +/* Define these values to match your device */ > +#define VENDOR_ID_REALTEK0x0bda > +#define VENDOR_ID_MICROSOFT 0x045e > +#define VENDOR_ID_SAMSUNG0x04e8 > +#define VENDOR_ID_LENOVO 0x17ef > +#define VENDOR_ID_LINKSYS0x13b1 > +#define VENDOR_ID_NVIDIA 0x0955 > +#define VENDOR_ID_TPLINK 0x2357 $ git grep 0x2357 | grep -i tplink drivers/net/usb/cdc_ether.c:#define TPLINK_VENDOR_ID0x2357 drivers/net/usb/r8152.c:#define VENDOR_ID_TPLINK0x2357 drivers/usb/serial/option.c:#define TPLINK_VENDOR_ID0x2357 $ git grep 0x17ef | grep -i lenovo drivers/hid/hid-ids.h:#define USB_VENDOR_ID_LENOVO 0x17ef drivers/hid/wacom.h:#define USB_VENDOR_ID_LENOVO0x17ef drivers/net/usb/cdc_ether.c:#define LENOVO_VENDOR_ID0x17ef drivers/net/usb/r8152.c:#define VENDOR_ID_LENOVO0x17ef Time to consolidate those vendor id defines perhaps?
[PATCH net-next v2] net/usb/r8153_ecm: support ECM mode for RTL8153
Support ECM mode based on cdc_ether with relative mii functions,
when CONFIG_USB_RTL8152 is not set, or the device is not supported
by r8152 driver.
Signed-off-by: Hayes Wang
---
v2:
Add include/linux/usb/r8152.h to avoid the warning about
no previous prototype for rtl8152_get_version.
drivers/net/usb/Makefile| 2 +-
drivers/net/usb/r8152.c | 30 +--
drivers/net/usb/r8153_ecm.c | 162
include/linux/usb/r8152.h | 37
4 files changed, 204 insertions(+), 27 deletions(-)
create mode 100644 drivers/net/usb/r8153_ecm.c
create mode 100644 include/linux/usb/r8152.h
diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
index 99fd12be2111..99381e6bea78 100644
--- a/drivers/net/usb/Makefile
+++ b/drivers/net/usb/Makefile
@@ -13,7 +13,7 @@ obj-$(CONFIG_USB_LAN78XX) += lan78xx.o
obj-$(CONFIG_USB_NET_AX8817X) += asix.o
asix-y := asix_devices.o asix_common.o ax88172a.o
obj-$(CONFIG_USB_NET_AX88179_178A) += ax88179_178a.o
-obj-$(CONFIG_USB_NET_CDCETHER) += cdc_ether.o
+obj-$(CONFIG_USB_NET_CDCETHER) += cdc_ether.o r8153_ecm.o
obj-$(CONFIG_USB_NET_CDC_EEM) += cdc_eem.o
obj-$(CONFIG_USB_NET_DM9601) += dm9601.o
obj-$(CONFIG_USB_NET_SR9700) += sr9700.o
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index b1770489aca5..7d2523d96c51 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
/* Information for net-next */
#define NETNEXT_VERSION"11"
@@ -653,18 +654,6 @@ enum rtl_register_content {
#define INTR_LINK 0x0004
-#define RTL8152_REQT_READ 0xc0
-#define RTL8152_REQT_WRITE 0x40
-#define RTL8152_REQ_GET_REGS 0x05
-#define RTL8152_REQ_SET_REGS 0x05
-
-#define BYTE_EN_DWORD 0xff
-#define BYTE_EN_WORD 0x33
-#define BYTE_EN_BYTE 0x11
-#define BYTE_EN_SIX_BYTES 0x3f
-#define BYTE_EN_START_MASK 0x0f
-#define BYTE_EN_END_MASK 0xf0
-
#define RTL8153_MAX_PACKET 9216 /* 9K */
#define RTL8153_MAX_MTU(RTL8153_MAX_PACKET - VLAN_ETH_HLEN - \
ETH_FCS_LEN)
@@ -689,21 +678,9 @@ enum rtl8152_flags {
LENOVO_MACPASSTHRU,
};
-/* Define these values to match your device */
-#define VENDOR_ID_REALTEK 0x0bda
-#define VENDOR_ID_MICROSOFT0x045e
-#define VENDOR_ID_SAMSUNG 0x04e8
-#define VENDOR_ID_LENOVO 0x17ef
-#define VENDOR_ID_LINKSYS 0x13b1
-#define VENDOR_ID_NVIDIA 0x0955
-#define VENDOR_ID_TPLINK 0x2357
-
#define DEVICE_ID_THINKPAD_THUNDERBOLT3_DOCK_GEN2 0x3082
#define DEVICE_ID_THINKPAD_USB_C_DOCK_GEN2 0xa387
-#define MCU_TYPE_PLA 0x0100
-#define MCU_TYPE_USB 0x
-
struct tally_counter {
__le64 tx_packets;
__le64 rx_packets;
@@ -6615,7 +6592,7 @@ static int rtl_fw_init(struct r8152 *tp)
return 0;
}
-static u8 rtl_get_version(struct usb_interface *intf)
+u8 rtl8152_get_version(struct usb_interface *intf)
{
struct usb_device *udev = interface_to_usbdev(intf);
u32 ocp_data = 0;
@@ -6673,12 +6650,13 @@ static u8 rtl_get_version(struct usb_interface *intf)
return version;
}
+EXPORT_SYMBOL_GPL(rtl8152_get_version);
static int rtl8152_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct usb_device *udev = interface_to_usbdev(intf);
- u8 version = rtl_get_version(intf);
+ u8 version = rtl8152_get_version(intf);
struct r8152 *tp;
struct net_device *netdev;
int ret;
diff --git a/drivers/net/usb/r8153_ecm.c b/drivers/net/usb/r8153_ecm.c
new file mode 100644
index ..2c3fabd38b16
--- /dev/null
+++ b/drivers/net/usb/r8153_ecm.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define OCP_BASE 0xe86c
+
+static int pla_read_word(struct usbnet *dev, u16 index)
+{
+ u16 byen = BYTE_EN_WORD;
+ u8 shift = index & 2;
+ __le32 tmp;
+ int ret;
+
+ if (shift)
+ byen <<= shift;
+
+ index &= ~3;
+
+ ret = usbnet_read_cmd(dev, RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
index,
+ MCU_TYPE_PLA | byen, , sizeof(tmp));
+ if (ret < 0)
+ goto out;
+
+ ret = __le32_to_cpu(tmp);
+ ret >>= (shift * 8);
+ ret &= 0x;
+
+out:
+ return ret;
+}
+
+static int pla_write_word(struct usbnet *dev, u16 index, u32 data)
+{
+ u32 mask = 0x;
+ u16 byen = BYTE_EN_WORD;
+ u8 shift = index & 2;
+ __le32 tmp;
+ int ret;
+
+ data &= mask;
+
+ if (shift) {
+ byen <<= shift;
+ mask <<= (shift * 8);
+ data <<= (shift * 8);
+
