Message ID | 1394712342-15778-387-Taiwan-albertk@realtek.com |
---|---|
State | New |
Headers | show |
Series | net/usb/r8153_ecm: support ECM mode for RTL8153 | expand |
Hi Hayes, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on net/master] [also build test WARNING on net-next/master ipvs/master linus/master v5.10-rc1 next-20201029] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Hayes-Wang/net-usb-r8153_ecm-support-ECM-mode-for-RTL8153/20201029-203440 base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git d6535dca28859d8d9ef80894eb287b2ac35a32e8 config: xtensa-allyesconfig (attached as .config) compiler: xtensa-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/e34498795de95fbccb0f2feee72cd8df723f9fd3 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Hayes-Wang/net-usb-r8153_ecm-support-ECM-mode-for-RTL8153/20201029-203440 git checkout e34498795de95fbccb0f2feee72cd8df723f9fd3 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=xtensa If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> drivers/net/usb/r8152.c:6618:4: warning: no previous prototype for 'rtl8152_get_version' [-Wmissing-prototypes] 6618 | u8 rtl8152_get_version(struct usb_interface *intf) | ^~~~~~~~~~~~~~~~~~~ vim +/rtl8152_get_version +6618 drivers/net/usb/r8152.c 6617 > 6618 u8 rtl8152_get_version(struct usb_interface *intf) 6619 { 6620 struct usb_device *udev = interface_to_usbdev(intf); 6621 u32 ocp_data = 0; 6622 __le32 *tmp; 6623 u8 version; 6624 int ret; 6625 6626 tmp = kmalloc(sizeof(*tmp), GFP_KERNEL); 6627 if (!tmp) 6628 return 0; 6629 6630 ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), 6631 RTL8152_REQ_GET_REGS, RTL8152_REQT_READ, 6632 PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500); 6633 if (ret > 0) 6634 ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK; 6635 6636 kfree(tmp); 6637 6638 switch (ocp_data) { 6639 case 0x4c00: 6640 version = RTL_VER_01; 6641 break; 6642 case 0x4c10: 6643 version = RTL_VER_02; 6644 break; 6645 case 0x5c00: 6646 version = RTL_VER_03; 6647 break; 6648 case 0x5c10: 6649 version = RTL_VER_04; 6650 break; 6651 case 0x5c20: 6652 version = RTL_VER_05; 6653 break; 6654 case 0x5c30: 6655 version = RTL_VER_06; 6656 break; 6657 case 0x4800: 6658 version = RTL_VER_07; 6659 break; 6660 case 0x6000: 6661 version = RTL_VER_08; 6662 break; 6663 case 0x6010: 6664 version = RTL_VER_09; 6665 break; 6666 default: 6667 version = RTL_VER_UNKNOWN; 6668 dev_info(&intf->dev, "Unknown version 0x%04x\n", ocp_data); 6669 break; 6670 } 6671 6672 dev_dbg(&intf->dev, "Detected version 0x%04x\n", version); 6673 6674 return version; 6675 } 6676 EXPORT_SYMBOL_GPL(rtl8152_get_version); 6677 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Mon, Nov 02, 2020 at 11:47:18AM -0800, Jakub Kicinski wrote: > On Mon, 2 Nov 2020 07:20:15 +0000 Hayes Wang wrote: > > Jakub Kicinski <kuba@kernel.org> > > > 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_MICROSOFT 0x045e > > > > +#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_LENOVO 0x17ef > > > 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
v3: Move original patch to #2. And add a new patch #1 to consolidate vendor ID of USB devices. v2: Add include/linux/usb/r8152.h to avoid the warning about no previous prototype for rtl8152_get_version. Hayes Wang (2): include/linux/usb: new header file for the vendor ID of USB devices net/usb/r8153_ecm: support ECM mode for RTL8153 drivers/net/usb/Makefile | 2 +- drivers/net/usb/cdc_ether.c | 93 +++++++---------- drivers/net/usb/r8152.c | 68 +++++-------- drivers/net/usb/r8153_ecm.c | 162 ++++++++++++++++++++++++++++++ include/linux/usb/r8152.h | 30 ++++++ include/linux/usb/usb_vendor_id.h | 51 ++++++++++ 6 files changed, 306 insertions(+), 100 deletions(-) create mode 100644 drivers/net/usb/r8153_ecm.c create mode 100644 include/linux/usb/r8152.h create mode 100644 include/linux/usb/usb_vendor_id.h
Greg Kroah-Hartman <gregkh@linuxfoundation.org> > 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.
On Tue, Nov 03, 2020 at 05:46:37PM +0800, Hayes Wang wrote: > diff --git a/include/linux/usb/usb_vendor_id.h b/include/linux/usb/usb_vendor_id.h > new file mode 100644 > index 000000000000..23b6e6849515 > --- /dev/null > +++ b/include/linux/usb/usb_vendor_id.h > @@ -0,0 +1,51 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > + <snip> No, this is not ok, sorry. Please see the top of the pci_ids.h file why we do not do this. There is nothing wrong with putting the individual ids in the different drivers, we don't want one single huge file that is a pain for merges and builds. We learn from our past mistakes, please do not fail to learn from history :) thanks, greg k-h
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 +0000 Hayes Wang wrote: > > > Jakub Kicinski <kuba@kernel.org> > > > > 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_MICROSOFT 0x045e > > > > > +#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_LENOVO 0x17ef > > > > 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. 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 :)
Jakub Kicinski <kuba@kernel.org> > 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
On Wed, 4 Nov 2020 01:39:52 +0000 Hayes Wang wrote: > Jakub Kicinski <kuba@kernel.org> > > 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.
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..1b5d10f1de5f 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -6615,7 +6615,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 +6673,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 000000000000..723841525a6a --- /dev/null +++ b/drivers/net/usb/r8153_ecm.c @@ -0,0 +1,174 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#include <linux/module.h> +#include <linux/netdevice.h> +#include <linux/mii.h> +#include <linux/usb.h> +#include <linux/usb/cdc.h> +#include <linux/usb/usbnet.h> + +#define RTL8153_CMD 0X05 +#define RTL8153_REQT_READ 0xc0 +#define RTL8153_REQT_WRITE 0x40 + +#define MCU_TYPE_PLA 0x0100 +#define OCP_BASE 0xe86c + +#define BYTE_EN_WORD 0x33 + +#define REALTEK_VENDOR_ID 0x0bda + +#if IS_REACHABLE(CONFIG_USB_RTL8152) +extern u8 rtl8152_get_version(struct usb_interface *intf); +#endif + +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, RTL8153_CMD, RTL8153_REQT_READ, index, + MCU_TYPE_PLA | byen, &tmp, sizeof(tmp)); + if (ret < 0) + goto out; + + ret = __le32_to_cpu(tmp); + ret >>= (shift * 8); + ret &= 0xffff; + +out: + return ret; +} + +static int pla_write_word(struct usbnet *dev, u16 index, u32 data) +{ + u32 mask = 0xffff; + 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); + } + + index &= ~3; + + ret = usbnet_read_cmd(dev, RTL8153_CMD, RTL8153_REQT_READ, index, + MCU_TYPE_PLA | byen, &tmp, sizeof(tmp)); + + if (ret < 0) + goto out; + + data |= __le32_to_cpu(tmp) & ~mask; + tmp = __cpu_to_le32(data); + + ret = usbnet_write_cmd(dev, RTL8153_CMD, RTL8153_REQT_WRITE, index, + MCU_TYPE_PLA | byen, &tmp, sizeof(tmp)); + +out: + return ret; +} + +static int r8153_ecm_mdio_read(struct net_device *netdev, int phy_id, int reg) +{ + struct usbnet *dev = netdev_priv(netdev); + int ret; + + ret = pla_write_word(dev, OCP_BASE, 0xa000); + if (ret < 0) + goto out; + + ret = pla_read_word(dev, 0xb400 + reg * 2); + +out: + return ret; +} + +static void r8153_ecm_mdio_write(struct net_device *netdev, int phy_id, int reg, int val) +{ + struct usbnet *dev = netdev_priv(netdev); + int ret; + + ret = pla_write_word(dev, OCP_BASE, 0xa000); + if (ret < 0) + return; + + ret = pla_write_word(dev, 0xb400 + reg * 2, val); +} + +static int r8153_bind(struct usbnet *dev, struct usb_interface *intf) +{ + int status; + + status = usbnet_cdc_bind(dev, intf); + if (status < 0) + return status; + + dev->mii.dev = dev->net; + dev->mii.mdio_read = r8153_ecm_mdio_read; + dev->mii.mdio_write = r8153_ecm_mdio_write; + dev->mii.reg_num_mask = 0x1f; + dev->mii.supports_gmii = 1; + + return status; +} + +static const struct driver_info r8153_info = { + .description = "RTL8153 ECM Device", + .flags = FLAG_ETHER, + .bind = r8153_bind, + .unbind = usbnet_cdc_unbind, + .status = usbnet_cdc_status, + .manage_power = usbnet_manage_power, +}; + +static const struct usb_device_id products[] = { +{ + USB_DEVICE_AND_INTERFACE_INFO(REALTEK_VENDOR_ID, 0x8153, USB_CLASS_COMM, + USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE), + .driver_info = (unsigned long)&r8153_info, +}, + + { }, /* END */ +}; +MODULE_DEVICE_TABLE(usb, products); + +static int rtl8153_ecm_probe(struct usb_interface *intf, + const struct usb_device_id *id) +{ +#if IS_REACHABLE(CONFIG_USB_RTL8152) + if (rtl8152_get_version(intf)) + return -ENODEV; +#endif + + return usbnet_probe(intf, id); +} + +static struct usb_driver r8153_ecm_driver = { + .name = "r8153_ecm", + .id_table = products, + .probe = rtl8153_ecm_probe, + .disconnect = usbnet_disconnect, + .suspend = usbnet_suspend, + .resume = usbnet_resume, + .reset_resume = usbnet_resume, + .supports_autosuspend = 1, + .disable_hub_initiated_lpm = 1, +}; + +module_usb_driver(r8153_ecm_driver); + +MODULE_AUTHOR("Hayes Wang"); +MODULE_DESCRIPTION("Realtek USB ECM device"); +MODULE_LICENSE("GPL");
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 <hayeswang@realtek.com> --- drivers/net/usb/Makefile | 2 +- drivers/net/usb/r8152.c | 5 +- drivers/net/usb/r8153_ecm.c | 174 ++++++++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 3 deletions(-) create mode 100644 drivers/net/usb/r8153_ecm.c