[PATCH net-next 0/3] r8152: configuration setting

[PATCH net-next 0/3] r8152: configuration setting

[Hayes Wang]

Some people prefer to use ECM mode rather than vendor mode. Therefore, I add
CONFIG_RTL8152_CONFIG_VALUE in Kconfig. Then, the users could choose the USB
configuration value which they want. The default is to support vendor mode
only.

Hayes Wang (3):
  r8152: check hw version first
  r8152: support ECM mode
  r8152: add CONFIG_RTL8152_CONFIG_VALUE

 drivers/net/usb/Kconfig |  13 ++
 drivers/net/usb/r8152.c | 383 +++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 345 insertions(+), 51 deletions(-)

-- 
2.7.4

[PATCH net-next 1/3] r8152: check hw version first

[Hayes Wang]

Check hw version first in probe(). Do nothing if the driver doesn't
support the chips.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 9338f58..8468704 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -4142,44 +4142,6 @@ static const struct net_device_ops rtl8152_netdev_ops = {
 	.ndo_features_check	= rtl8152_features_check,
 };
 
-static void r8152b_get_version(struct r8152 *tp)
-{
-	u32	ocp_data;
-	u16	version;
-
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_TCR1);
-	version = (u16)(ocp_data & VERSION_MASK);
-
-	switch (version) {
-	case 0x4c00:
-		tp->version = RTL_VER_01;
-		break;
-	case 0x4c10:
-		tp->version = RTL_VER_02;
-		break;
-	case 0x5c00:
-		tp->version = RTL_VER_03;
-		tp->mii.supports_gmii = 1;
-		break;
-	case 0x5c10:
-		tp->version = RTL_VER_04;
-		tp->mii.supports_gmii = 1;
-		break;
-	case 0x5c20:
-		tp->version = RTL_VER_05;
-		tp->mii.supports_gmii = 1;
-		break;
-	case 0x5c30:
-		tp->version = RTL_VER_06;
-		tp->mii.supports_gmii = 1;
-		break;
-	default:
-		netif_info(tp, probe, tp->netdev,
-			   "Unknown version 0x%04x\n", version);
-		break;
-	}
-}
-
 static void rtl8152_unload(struct r8152 *tp)
 {
 	if (test_bit(RTL8152_UNPLUG, &tp->flags))
@@ -4244,14 +4206,66 @@ static int rtl_ops_init(struct r8152 *tp)
 	return ret;
 }
 
+static u8 rtl_get_version(struct usb_interface *intf)
+{
+	struct usb_device *udev = interface_to_usbdev(intf);
+	u32 ocp_data = 0;
+	__le32 *tmp;
+	u8 version;
+	int ret;
+
+	tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+	if (!tmp)
+		return 0;
+
+	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+			      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
+			      PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500);
+	if (ret > 0)
+		ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;
+
+	kfree(tmp);
+
+	switch (ocp_data) {
+	case 0x4c00:
+		version = RTL_VER_01;
+		break;
+	case 0x4c10:
+		version = RTL_VER_02;
+		break;
+	case 0x5c00:
+		version = RTL_VER_03;
+		break;
+	case 0x5c10:
+		version = RTL_VER_04;
+		break;
+	case 0x5c20:
+		version = RTL_VER_05;
+		break;
+	case 0x5c30:
+		version = RTL_VER_06;
+		break;
+	default:
+		version = RTL_VER_UNKNOWN;
+		dev_info(&intf->dev, "Unknown version 0x%04x\n", ocp_data);
+		break;
+	}
+
+	return 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);
 	struct r8152 *tp;
 	struct net_device *netdev;
 	int ret;
 
+	if (version == RTL_VER_UNKNOWN)
+		return -ENODEV;
+
 	if (udev->actconfig->desc.bConfigurationValue != 1) {
 		usb_driver_set_configuration(udev, 1);
 		return -ENODEV;
@@ -4271,8 +4285,18 @@ static int rtl8152_probe(struct usb_interface *intf,
 	tp->udev = udev;
 	tp->netdev = netdev;
 	tp->intf = intf;
+	tp->version = version;
+
+	switch (version) {
+	case RTL_VER_01:
+	case RTL_VER_02:
+		tp->mii.supports_gmii = 0;
+		break;
+	default:
+		tp->mii.supports_gmii = 1;
+		break;
+	}
 
-	r8152b_get_version(tp);
 	ret = rtl_ops_init(tp);
 	if (ret)
 		goto out;
-- 
2.7.4

[PATCH net-next 2/3] r8152: support ECM mode

[Hayes Wang]

If CONFIG_USB_NET_CDCETHER is enabled, support ECM mode through cdc_ether
driver.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 8468704..c4e8339 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -27,6 +27,7 @@
 #include <linux/usb/cdc.h>
 #include <linux/suspend.h>
 #include <linux/acpi.h>
+#include <linux/usb/usbnet.h>
 
 /* Information for net-next */
 #define NETNEXT_VERSION		"08"
@@ -4402,6 +4403,243 @@ static void rtl8152_disconnect(struct usb_interface *intf)
 	}
 }
 
+static int rtl_usbnet_probe(struct usb_interface *intf,
+			    const struct usb_device_id *id)
+{
+	switch (id->bInterfaceClass) {
+	case USB_CLASS_VENDOR_SPEC:
+		return rtl8152_probe(intf, id);
+#if IS_ENABLED(CONFIG_USB_NET_CDCETHER)
+	case USB_CLASS_COMM:
+		if (id->bInterfaceSubClass == USB_CDC_SUBCLASS_ETHERNET &&
+		    id->bInterfaceProtocol == USB_CDC_PROTO_NONE)
+			return usbnet_probe(intf, id);
+#endif
+	default:
+		return -ENODEV;
+	}
+}
+
+static void rtl_usbnet_disconnect(struct usb_interface *intf)
+{
+	struct usb_host_interface *alt = intf->cur_altsetting;
+
+	switch (alt->desc.bInterfaceClass) {
+	case USB_CLASS_VENDOR_SPEC:
+		rtl8152_disconnect(intf);
+		break;
+#if IS_ENABLED(CONFIG_USB_NET_CDCETHER)
+	case USB_CLASS_COMM:
+		if (alt->desc.bInterfaceSubClass == USB_CDC_SUBCLASS_ETHERNET &&
+		    alt->desc.bInterfaceProtocol == USB_CDC_PROTO_NONE) {
+			usbnet_disconnect(intf);
+			break;
+		}
+#endif
+	default:
+		break;
+	}
+}
+
+static int rtl_usbnet_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct usb_host_interface *alt = intf->cur_altsetting;
+
+	switch (alt->desc.bInterfaceClass) {
+	case USB_CLASS_VENDOR_SPEC:
+		return rtl8152_suspend(intf, message);
+#if IS_ENABLED(CONFIG_USB_NET_CDCETHER)
+	case USB_CLASS_COMM:
+		if (alt->desc.bInterfaceSubClass == USB_CDC_SUBCLASS_ETHERNET &&
+		    alt->desc.bInterfaceProtocol == USB_CDC_PROTO_NONE)
+			return usbnet_suspend(intf, message);
+#endif
+	default:
+		return -ENODEV;
+	}
+}
+
+static int rtl_usbnet_resume(struct usb_interface *intf)
+{
+	struct usb_host_interface *alt = intf->cur_altsetting;
+
+	switch (alt->desc.bInterfaceClass) {
+	case USB_CLASS_VENDOR_SPEC:
+		return rtl8152_resume(intf);
+#if IS_ENABLED(CONFIG_USB_NET_CDCETHER)
+	case USB_CLASS_COMM:
+		if (alt->desc.bInterfaceSubClass == USB_CDC_SUBCLASS_ETHERNET &&
+		    alt->desc.bInterfaceProtocol == USB_CDC_PROTO_NONE)
+			return usbnet_resume(intf);
+#endif
+	default:
+		return -ENODEV;
+	}
+}
+
+static int rtl_usbnet_reset_resume(struct usb_interface *intf)
+{
+	struct usb_host_interface *alt = intf->cur_altsetting;
+
+	switch (alt->desc.bInterfaceClass) {
+	case USB_CLASS_VENDOR_SPEC:
+		return rtl8152_reset_resume(intf);
+#if IS_ENABLED(CONFIG_USB_NET_CDCETHER)
+	case USB_CLASS_COMM:
+		if (alt->desc.bInterfaceSubClass == USB_CDC_SUBCLASS_ETHERNET &&
+		    alt->desc.bInterfaceProtocol == USB_CDC_PROTO_NONE)
+			return usbnet_resume(intf);
+#endif
+	default:
+		return -ENODEV;
+	}
+}
+
+static int rtl_usbnet_pre_reset(struct usb_interface *intf)
+{
+	struct usb_host_interface *alt = intf->cur_altsetting;
+
+	if (!usb_get_intfdata(intf))
+		return 0;
+
+	switch (alt->desc.bInterfaceClass) {
+	case USB_CLASS_VENDOR_SPEC:
+		return rtl8152_pre_reset(intf);
+	default:
+		return 1;
+	}
+}
+
+static int rtl_usbnet_post_reset(struct usb_interface *intf)
+{
+	struct usb_host_interface *alt = intf->cur_altsetting;
+
+	if (!usb_get_intfdata(intf))
+		return 0;
+
+	switch (alt->desc.bInterfaceClass) {
+	case USB_CLASS_VENDOR_SPEC:
+		return rtl8152_post_reset(intf);
+	default:
+		return 1;
+	}
+}
+
+#if IS_ENABLED(CONFIG_USB_NET_CDCETHER)
+
+static int r815x_mdio_read(struct net_device *netdev, int phy_id, int reg)
+{
+	struct usbnet *dev = netdev_priv(netdev);
+	__le32 tmp;
+	u16 index;
+	u8 shift;
+	int err;
+
+	if (phy_id != R8152_PHY_ID)
+		return -EINVAL;
+
+	index = 0xB400 + reg * 2;
+	shift = index & 2;
+	index &= ~3;
+
+	err = usbnet_read_cmd(dev, RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
+			      index, MCU_TYPE_PLA, &tmp, sizeof(tmp));
+
+	if (err > 0) {
+		err = __le32_to_cpu(tmp);
+		err >>= (shift * 8);
+		err &= 0xffff;
+	}
+
+	return err;
+}
+
+static
+void r815x_mdio_write(struct net_device *netdev, int phy_id, int reg, int val)
+{
+	struct usbnet *dev = netdev_priv(netdev);
+	__le32 tmp;
+	u16 index;
+	u16 byen = BYTE_EN_WORD;
+	u8 shift;
+
+	if (phy_id != R8152_PHY_ID)
+		return;
+
+	val &= 0xffff;
+	index = 0xB400 + reg * 2;
+	shift = index & 2;
+	index &= ~3;
+
+	if (shift) {
+		byen <<= shift;
+		val <<= (shift * 8);
+	}
+
+	tmp = __cpu_to_le32(val);
+
+	usbnet_write_cmd(dev, RTL8152_REQ_GET_REGS, RTL8152_REQT_WRITE, index,
+			 MCU_TYPE_PLA | byen, &tmp, sizeof(tmp));
+}
+
+static int rtl_ecm_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	__le32 tmp;
+	int err;
+
+	err = usbnet_cdc_bind(dev, intf);
+	if (err < 0)
+		goto err1;
+
+	tmp = __cpu_to_le32(0xa000);
+	err = usbnet_write_cmd(dev, RTL8152_REQ_GET_REGS, RTL8152_REQT_WRITE,
+			       PLA_OCP_GPHY_BASE, MCU_TYPE_PLA | BYTE_EN_WORD,
+			       &tmp, sizeof(tmp));
+	if (err < 0)
+		goto err2;
+
+	dev->mii.dev = dev->net;
+	dev->mii.mdio_read = r815x_mdio_read;
+	dev->mii.mdio_write = r815x_mdio_write;
+	dev->mii.phy_id_mask = 0x3f;
+	dev->mii.reg_num_mask = 0x1f;
+	dev->mii.phy_id = R8152_PHY_ID;
+
+	switch (rtl_get_version(intf)) {
+	case RTL_VER_01:
+	case RTL_VER_02:
+		dev->mii.supports_gmii = 0;
+		break;
+	default:
+		dev->mii.supports_gmii = 1;
+		break;
+	}
+
+	return 0;
+
+err2:
+	usbnet_cdc_unbind(dev, intf);
+err1:
+	return err;
+}
+
+static const struct driver_info rtl_ecm_info = {
+	.description =	"RTL8152/RTL8153 ECM Device",
+	.flags =	FLAG_ETHER | FLAG_POINTTOPOINT,
+	.bind =		rtl_ecm_bind,
+	.unbind =	usbnet_cdc_unbind,
+	.status =	usbnet_cdc_status,
+	.manage_power =	usbnet_manage_power,
+};
+
+#define RTL_ECM_INFO	((unsigned long)&rtl_ecm_info)
+
+#else
+
+#define RTL_ECM_INFO	0
+
+#endif
+
 #define REALTEK_USB_DEVICE(vend, prod)	\
 	.match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
 		       USB_DEVICE_ID_MATCH_INT_CLASS, \
@@ -4416,7 +4654,8 @@ static void rtl8152_disconnect(struct usb_interface *intf)
 	.idProduct = (prod), \
 	.bInterfaceClass = USB_CLASS_COMM, \
 	.bInterfaceSubClass = USB_CDC_SUBCLASS_ETHERNET, \
-	.bInterfaceProtocol = USB_CDC_PROTO_NONE
+	.bInterfaceProtocol = USB_CDC_PROTO_NONE, \
+	.driver_info = RTL_ECM_INFO,
 
 /* table of devices that work with this driver */
 static struct usb_device_id rtl8152_table[] = {
@@ -4434,13 +4673,13 @@ MODULE_DEVICE_TABLE(usb, rtl8152_table);
 static struct usb_driver rtl8152_driver = {
 	.name =		MODULENAME,
 	.id_table =	rtl8152_table,
-	.probe =	rtl8152_probe,
-	.disconnect =	rtl8152_disconnect,
-	.suspend =	rtl8152_suspend,
-	.resume =	rtl8152_resume,
-	.reset_resume =	rtl8152_reset_resume,
-	.pre_reset =	rtl8152_pre_reset,
-	.post_reset =	rtl8152_post_reset,
+	.probe =	rtl_usbnet_probe,
+	.disconnect =	rtl_usbnet_disconnect,
+	.suspend =	rtl_usbnet_suspend,
+	.resume =	rtl_usbnet_resume,
+	.reset_resume =	rtl_usbnet_reset_resume,
+	.pre_reset =	rtl_usbnet_pre_reset,
+	.post_reset =	rtl_usbnet_post_reset,
 	.supports_autosuspend = 1,
 	.disable_hub_initiated_lpm = 1,
 };
-- 
2.7.4

[PATCH net-next 3/3] r8152: add CONFIG_RTL8152_CONFIG_VALUE

[Hayes Wang]

According to CONFIG_RTL8152_CONFIG_VALUE, to determine which mode the
driver supports. The value 0 means to support both vendor mode and
ECM mode. The value 1 means to support vendor mode only. The value 2
means to support ECM mode only.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/Kconfig | 13 +++++++++++++
 drivers/net/usb/r8152.c | 30 ++++++++++++++++++++++++------
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index cdde590..3d2fd09 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -106,6 +106,19 @@ config USB_RTL8152
 	  To compile this driver as a module, choose M here: the
 	  module will be called r8152.
 
+config RTL8152_CONFIG_VALUE
+	depends on USB_RTL8152
+	int "RTL8152/RTL8153 USB Configuration Value"
+	default 1
+	help
+		This is used to select the USB configuration value of RTL8152/RTL8153.
+		0: support all configuration values. It means the user could
+		   change the configuration value. However, it may have problem
+		   if you change the configuration value frequently. Don't set
+		   this value unless you really know what you want.
+		1: use vendor mode only. (recommended)
+		2: use ECM mode only.
+
 config USB_LAN78XX
 	tristate "Microchip LAN78XX Based USB Ethernet Adapters"
 	select MII
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index c4e8339..480af78 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -30,7 +30,7 @@
 #include <linux/usb/usbnet.h>
 
 /* Information for net-next */
-#define NETNEXT_VERSION		"08"
+#define NETNEXT_VERSION		"09"
 
 /* Information for net */
 #define NET_VERSION		"5"
@@ -4267,11 +4267,6 @@ static int rtl8152_probe(struct usb_interface *intf,
 	if (version == RTL_VER_UNKNOWN)
 		return -ENODEV;
 
-	if (udev->actconfig->desc.bConfigurationValue != 1) {
-		usb_driver_set_configuration(udev, 1);
-		return -ENODEV;
-	}
-
 	usb_reset_device(udev);
 	netdev = alloc_etherdev(sizeof(struct r8152));
 	if (!netdev) {
@@ -4403,9 +4398,32 @@ static void rtl8152_disconnect(struct usb_interface *intf)
 	}
 }
 
+static bool rtl_change_config(struct usb_interface *intf)
+{
+	struct usb_device *udev = interface_to_usbdev(intf);
+	u8 actconfig = udev->actconfig->desc.bConfigurationValue;
+
+	if (CONFIG_RTL8152_CONFIG_VALUE <= 0 ||
+	    udev->descriptor.bNumConfigurations < CONFIG_RTL8152_CONFIG_VALUE ||
+	    actconfig == CONFIG_RTL8152_CONFIG_VALUE)
+		return false;
+
+	if (CONFIG_RTL8152_CONFIG_VALUE == 1 && !rtl_get_version(intf))
+		return false;
+
+	return true;
+}
+
 static int rtl_usbnet_probe(struct usb_interface *intf,
 			    const struct usb_device_id *id)
 {
+	if (rtl_change_config(intf)) {
+		struct usb_device *udev = interface_to_usbdev(intf);
+
+		usb_driver_set_configuration(udev, CONFIG_RTL8152_CONFIG_VALUE);
+		return -ENODEV;
+	}
+
 	switch (id->bInterfaceClass) {
 	case USB_CLASS_VENDOR_SPEC:
 		return rtl8152_probe(intf, id);
-- 
2.7.4

Re: [PATCH net-next 0/3] r8152: configuration setting

[Bjørn Mork]

[ CCing Oliver, who AFAIK still is the cdc_ether maintainer and should
  have the final word on this ]

Hayes Wang <hayeswang@realtek.com> writes:

> Some people prefer to use ECM mode rather than vendor mode. Therefore, I add
> CONFIG_RTL8152_CONFIG_VALUE in Kconfig. Then, the users could choose the USB
> configuration value which they want. The default is to support vendor mode
> only.
>
> Hayes Wang (3):
>   r8152: check hw version first
>   r8152: support ECM mode
>   r8152: add CONFIG_RTL8152_CONFIG_VALUE
>
>  drivers/net/usb/Kconfig |  13 ++
>  drivers/net/usb/r8152.c | 383 +++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 345 insertions(+), 51 deletions(-)


So this adds a lot of code to work around the issues you introduced by
unnecessarily blacklisting the CDC ECM configuration earlier, and still
makes the r8152 driver handle the device even in ECM mode.

Sorry, but this is a total mess.

Just remove the completely unnecessary blacklist, and let the cdc_ether
driver handle the device if the user selects the ECM configuration.
That't how the USB system works.  There is no need for any code in r8152
to do that.

Ref https://lkml.org/lkml/2014/1/3/57



Bjørn

Re: [PATCH net-next 0/3] r8152: configuration setting

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Wed, 7 Sep 2016 16:12:19 +0800

> Some people prefer to use ECM mode rather than vendor mode. Therefore, I add
> CONFIG_RTL8152_CONFIG_VALUE in Kconfig. Then, the users could choose the USB
> configuration value which they want. The default is to support vendor mode
> only.

By forcing a certain mode via a Kconfig value, you are basically making it
impossible for distributions to do something reasonable here.

This needs to be somehow a runtime thing, and I'm pretty sure we've
had many other situations of USB devices which want to be run by
different "personality" drivers in the past.

Perhaps we can do something generically for USB devices for this kind
of situation and make r8152 use it.

Thanks.

RE: [PATCH net-next 0/3] r8152: configuration setting

[Hayes Wang]

Bjørn Mork [mailto:bjorn@mork.no]
> Sent: Wednesday, September 07, 2016 9:51 PM
[...]
> So this adds a lot of code to work around the issues you introduced by
> unnecessarily blacklisting the CDC ECM configuration earlier, and still
> makes the r8152 driver handle the device even in ECM mode.

I suggest to use vendor mode only, but some people ask me to
submit such patches. If these patches are rejected, I have
enough reasons to tell them it is unacceptable rather than
I don't do it.

> Just remove the completely unnecessary blacklist, and let the cdc_ether
> driver handle the device if the user selects the ECM configuration.
> That't how the USB system works.  There is no need for any code in r8152
> to do that.

The pure cdc_ether driver couldn't change the speed of the
ethernet, because it doesn't know how to access the PHY of
the device. Therefore, I add relative code in r8152 driver.

Best Regards,
Hayes

RE: [PATCH net-next 0/3] r8152: configuration setting

[Hayes Wang]

David Miller [mailto:davem@davemloft.net]
> Sent: Thursday, September 08, 2016 8:38 AM
[...]
> By forcing a certain mode via a Kconfig value, you are basically making it
> impossible for distributions to do something reasonable here.

The request is always from some manufacturers, not end users.
They always ask the driver to control everything. And I don't
think the end user knows or cares about how the device is set.
That is why I try to satisfy them via Kconfig.

I think you have rejected this way. I would think if there
is any other method. Thanks.

Best Regards,
Hayes

Re: [PATCH net-next 0/3] r8152: configuration setting

[Bjørn Mork]

Hayes Wang <hayeswang@realtek.com> writes:

> Bjørn Mork [mailto:bjorn@mork.no]
>> Sent: Wednesday, September 07, 2016 9:51 PM
> [...]
>> So this adds a lot of code to work around the issues you introduced by
>> unnecessarily blacklisting the CDC ECM configuration earlier, and still
>> makes the r8152 driver handle the device even in ECM mode.
>
> I suggest to use vendor mode only, but some people ask me to
> submit such patches. If these patches are rejected, I have
> enough reasons to tell them it is unacceptable rather than
> I don't do it.
>
>> Just remove the completely unnecessary blacklist, and let the cdc_ether
>> driver handle the device if the user selects the ECM configuration.
>> That't how the USB system works.  There is no need for any code in r8152
>> to do that.
>
> The pure cdc_ether driver couldn't change the speed of the
> ethernet, because it doesn't know how to access the PHY of
> the device. Therefore, I add relative code in r8152 driver.

Yes, I see that.  But is that strictly necessary? Couldn't you just say:
"CDC ECM is supported by cdc_ether and therefore limited to the features
implemented by cdc_ether.  If you want feature X, then please use our
vendor specific mode with the r8152 driver?"

As for the dynamic switching of multi-configuration USB devices:  This
is not special for your device.  It is very common for e.g. LTE modems
to provide 2 or more configurations, allowing the user to select between
for example

 1: ECM class + ACM class functions
 2: MBIM class function
 3: vendor specific functions

Each USB configuation comes with a set of descriptors identifying the
functions, and USB interface drivers attach to the functions they
support.  The user can dynamically switch the device from e.g. cfg #1 to
cfg #3 by writing "3" to /sys/bus/usb/devices/<port>/bConfigurationValue
This will cause the ECM and ACM USB interfaces to disappear, and the
associated class drivers will unbind, and new vendor specific USB
interfaces appear instead, causing a matching vendor specific driver to
load and bind.

Naturally, end users will not switch configurations all the time.  They
will select the configuration providing the set of functions they want.
If this is different from the default configuration  selected by the
Linux USB core, then that's a simple udev rule to update the
bConfigurationValue sysfs attribute on device disceovery.

This is how the USB core works by *default*, as long as you do not
deliberately break it by blacklisting any functions or forcing a
specific configuration.

You are overdesigning to solve a non-existing problem.



Bjørn

RE: [PATCH net-next 0/3] r8152: configuration setting

[Hayes Wang]

Bjørn Mork [mailto:bjorn@mork.no]
> Sent: Thursday, September 08, 2016 3:55 PM
[...]
> Yes, I see that.  But is that strictly necessary? Couldn't you just say:
> "CDC ECM is supported by cdc_ether and therefore limited to the features
> implemented by cdc_ether.  If you want feature X, then please use our
> vendor specific mode with the r8152 driver?"

My customs have a case that they must force the speed to 100M
for some reasons. I also wish to implement the driver as simple
as possible, but I don't think I could determine this. I accept
you reject my patches. However, I couldn't deny the requests
from the boss or customs without doing anything. I must prove
the way is unacceptable.

[...]
> Each USB configuation comes with a set of descriptors identifying the
> functions, and USB interface drivers attach to the functions they
> support.  The user can dynamically switch the device from e.g. cfg #1 to
> cfg #3 by writing "3" to /sys/bus/usb/devices/<port>/bConfigurationValue
> This will cause the ECM and ACM USB interfaces to disappear, and the
> associated class drivers will unbind, and new vendor specific USB
> interfaces appear instead, causing a matching vendor specific driver to
> load and bind.
> 
> Naturally, end users will not switch configurations all the time.  They
> will select the configuration providing the set of functions they want.
> If this is different from the default configuration  selected by the
> Linux USB core, then that's a simple udev rule to update the
> bConfigurationValue sysfs attribute on device disceovery.

I tested above method before. And I found that the cdc_ether
was loaded before switching the configuration. The behavior
of loading one driver and changing to another driver has
opportunity to let our some previous chips become abnormal.
To switch configuration is fine. However, it may have problem
to switch driver. That is why the current kernel only supports
vendor mode. If the method works fine, I have no trouble now.

Best Regards,
Hayes

Re: [PATCH net-next 0/3] r8152: configuration setting

[Oliver Neukum]

On Thu, 2016-09-08 at 13:02 +0000, Hayes Wang wrote:
> I tested above method before. And I found that the cdc_ether 
> was loaded before switching the configuration. The behavior 
> of loading one driver and changing to another driver has 
> opportunity to let our some previous chips become abnormal. 
> To switch configuration is fine. However, it may have problem 
> to switch driver. That is why the current kernel only supports 
> vendor mode. If the method works fine, I have no trouble now.

We may talk about creating extensions to cdc-ether, if they
are needed for the phy. What is unacceptable is to override
the configuration mechanism in the kernel.

	Regards
		Oliver

Re: [PATCH net-next 0/3] r8152: configuration setting

[Bjørn Mork]

Hayes Wang <hayeswang@realtek.com> writes:

> Bjørn Mork [mailto:bjorn@mork.no]
>> Sent: Thursday, September 08, 2016 3:55 PM
> [...]
>> Yes, I see that.  But is that strictly necessary? Couldn't you just say:
>> "CDC ECM is supported by cdc_ether and therefore limited to the features
>> implemented by cdc_ether.  If you want feature X, then please use our
>> vendor specific mode with the r8152 driver?"
>
> My customs have a case that they must force the speed to 100M
> for some reasons. I also wish to implement the driver as simple
> as possible, but I don't think I could determine this. I accept
> you reject my patches. However, I couldn't deny the requests
> from the boss or customs without doing anything. I must prove
> the way is unacceptable.

That's an odd combination of requirements, but I know how customers can
be :)

Just to make it clear:  I provide comments, but I am in no position to
reject any of your patches.  Or have any wish to do so.  You maintain
r8152.  Oliver maintains cdc_ether.  I am confident that whatever you
two decide is going to be fine.

> [...]
>> Each USB configuation comes with a set of descriptors identifying the
>> functions, and USB interface drivers attach to the functions they
>> support.  The user can dynamically switch the device from e.g. cfg #1 to
>> cfg #3 by writing "3" to /sys/bus/usb/devices/<port>/bConfigurationValue
>> This will cause the ECM and ACM USB interfaces to disappear, and the
>> associated class drivers will unbind, and new vendor specific USB
>> interfaces appear instead, causing a matching vendor specific driver to
>> load and bind.
>> 
>> Naturally, end users will not switch configurations all the time.  They
>> will select the configuration providing the set of functions they want.
>> If this is different from the default configuration  selected by the
>> Linux USB core, then that's a simple udev rule to update the
>> bConfigurationValue sysfs attribute on device disceovery.
>
> I tested above method before. And I found that the cdc_ether
> was loaded before switching the configuration. The behavior
> of loading one driver and changing to another driver has
> opportunity to let our some previous chips become abnormal.
> To switch configuration is fine. However, it may have problem
> to switch driver. That is why the current kernel only supports
> vendor mode. If the method works fine, I have no trouble now.

Yes, many firmwares/devices are not prepared to do "late" config
switching and can end up in a strange limbo if they are initialized
before switching. An udev rule should still run early enough to prevent
this problem I believe.

But if that doesn't work, then I agree that a blacklist makes more
sense. Just make it runtime configurable so that distros can do
something sane with it.



Bjørn

Re: [PATCH net-next 2/3] r8152: support ECM mode

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

Hi Hayes,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Hayes-Wang/r8152-configuration-setting/20160907-192351
config: i386-randconfig-x0-09182136 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `r815x_mdio_write':
>> r8152.c:(.text+0x19a664): undefined reference to `usbnet_write_cmd'
   drivers/built-in.o: In function `r815x_mdio_read':
>> r8152.c:(.text+0x19a6ae): undefined reference to `usbnet_read_cmd'
   drivers/built-in.o: In function `rtl_usbnet_disconnect':
>> r8152.c:(.text+0x19aaa9): undefined reference to `usbnet_disconnect'
   drivers/built-in.o: In function `rtl_ecm_bind':
>> r8152.c:(.text+0x19c143): undefined reference to `usbnet_cdc_bind'
   r8152.c:(.text+0x19c175): undefined reference to `usbnet_write_cmd'
>> r8152.c:(.text+0x19c1e8): undefined reference to `usbnet_cdc_unbind'
   drivers/built-in.o: In function `rtl_usbnet_suspend':
>> r8152.c:(.text+0x19d3ab): undefined reference to `usbnet_suspend'
   drivers/built-in.o: In function `rtl_usbnet_probe':
>> r8152.c:(.text+0x19d7bd): undefined reference to `usbnet_probe'
   drivers/built-in.o: In function `rtl_usbnet_reset_resume':
>> r8152.c:(.text+0x19ec33): undefined reference to `usbnet_resume'
   drivers/built-in.o: In function `rtl_usbnet_resume':
   r8152.c:(.text+0x19ec68): undefined reference to `usbnet_resume'
   drivers/built-in.o: In function `lkdtm_rodata_do_nothing':
>> (.rodata+0x3538c): undefined reference to `usbnet_cdc_unbind'
   drivers/built-in.o: In function `lkdtm_rodata_do_nothing':
>> (.rodata+0x3539c): undefined reference to `usbnet_manage_power'
   drivers/built-in.o: In function `lkdtm_rodata_do_nothing':
>> (.rodata+0x353a0): undefined reference to `usbnet_cdc_status'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30778 bytes --]

Re: [PATCH net-next 2/3] r8152: support ECM mode

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

Hi Hayes,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Hayes-Wang/r8152-configuration-setting/20160907-192351
config: x86_64-randconfig-s0-09190146 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `rtl_ecm_bind':
   r8152.c:(.text+0x64b43c): undefined reference to `usbnet_cdc_bind'
   r8152.c:(.text+0x64b59e): undefined reference to `usbnet_cdc_unbind'
>> drivers/built-in.o:(.rodata+0x272438): undefined reference to `usbnet_cdc_unbind'
>> drivers/built-in.o:(.rodata+0x272460): undefined reference to `usbnet_cdc_status'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33233 bytes --]