| Message ID | 20230309083436.6729-1-kangzhen.lou@dell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: cdc_ncm: support ACPI MAC address pass through functionality | expand |
On Thu, Mar 09, 2023 at 04:34:36PM +0800, Kangzhen Lou wrote: > Make cdc_ncm to support ACPI MAC address pass through functionality that > also exists in the Realtek r8152 driver. > > RTL8153DD will load cdc_ncm driver by default with mainline 6.2 kernel. > This is to support Realtek RTL8153DD Ethernet Interface Controller's MAC > pass through function which used in dock, dongle or monitor. > > Although there is patch “ec51fbd1b8a2bca2948dede99c14ec63dc57ff6b” will > make RTL8153DD load r8152-cfgselector instead cdc_ncm driver, would also > submit this patch in case anyone need this feature in cdc_ncm in the > future. Please reference commits in the correct way, as documented: ec51fbd1b8a2 ("r8152: add USB device driver for config selection") But because of that commit, why is your change needed at all? > > Signed-off-by: Kangzhen Lou <kangzhen.lou@dell.com> > --- > drivers/net/usb/cdc_ncm.c | 199 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 197 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c > index 6ce8f4f0c70e..8179516b819c 100644 > --- a/drivers/net/usb/cdc_ncm.c > +++ b/drivers/net/usb/cdc_ncm.c > @@ -53,6 +53,7 @@ > #include <linux/usb/usbnet.h> > #include <linux/usb/cdc.h> > #include <linux/usb/cdc_ncm.h> > +#include <linux/acpi.h> What about non-acpi systems? > > #if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM) > static bool prefer_mbim = true; > @@ -814,6 +815,177 @@ static const struct net_device_ops cdc_ncm_netdev_ops = { > .ndo_validate_addr = eth_validate_addr, > }; > > +int acpi_mac_passthru_invalid(void) > +{ > + acpi_status status; > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + union acpi_object *obj; > + int ret = -EINVAL; > + > + /* returns _AUXMAC_#AABBCCDDEEFF# */ > + status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer); > + obj = (union acpi_object *)buffer.pointer; > + > + if (!ACPI_SUCCESS(status)) > + return -ENODEV; > + if (obj->type != ACPI_TYPE_BUFFER || obj->string.length != 0x17) { > + acpi_info("Invalid buffer for pass-thru MAC addr: (%d, %d)\n", > + obj->type, obj->string.length); Why is that an "info" message? > + goto amacout; > + } > + if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0 || > + strncmp(obj->string.pointer + 0x15, "#", 1) != 0) { > + acpi_info("Invalid header when reading pass-thru MAC addr\n"); Again, info? Shouldn't this be an error? Or just debug as userspace can't do anything about it. > + goto amacout; > + } > + /* return success, otherwise return non-zero if invalid buffer read or > + * MAC pass through is disabled in BIOS > + */ > + ret = 0; > + > +amacout: > + kfree(obj); > + return ret; > +} > + > +int get_acpi_mac_passthru(char *MACAddress) > +{ > + acpi_status status; > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + union acpi_object *obj; > + int ret = -EINVAL; > + unsigned char buf[6]; > + > + /* returns _AUXMAC_#AABBCCDDEEFF# */ > + status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer); > + obj = (union acpi_object *)buffer.pointer; > + > + if (!ACPI_SUCCESS(status)) > + return -ENODEV; > + > + ret = hex2bin(buf, obj->string.pointer + 9, 6); > + if (!(ret == 0 && is_valid_ether_addr(buf))) { > + acpi_info("Invalid MAC for pass-thru MAC addr: %d, %pM\n", > + ret, buf); > + ret = -EINVAL; > + goto amacout; > + } > + memcpy(MACAddress, buf, 6); > + acpi_info("Pass-thru MAC addr %pM\n", MACAddress); > + > +amacout: > + kfree(obj); > + return ret; > +} > + > +/* Provide method to get MAC address from the USB device's ethernet controller. > + * If the device supports CDC_GET_ADDRESS, we should receive just six bytes. > + * Otherwise, use the prior method by asking for the descriptor. > + */ > +static int cdc_ncm_get_ethernet_address(struct usbnet *dev, > + struct cdc_ncm_ctx *ctx) > +{ > + int ret; > + u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; > + > + ret = usbnet_read_cmd(dev, USB_CDC_GET_NET_ADDRESS, > + USB_DIR_IN | USB_TYPE_CLASS > + | USB_RECIP_INTERFACE, 0, > + iface_no, dev->net->dev_addr, ETH_ALEN); > + > + if (ret == ETH_ALEN) { > + ret = 0; /* success */ > + } else { > + ret = usbnet_get_ethernet_addr(dev, > + ctx->ether_desc->iMACAddress); > + } > + > + return ret; > +} > + > +/* Provide method to push MAC address to the USB device's ethernet controller. > + * If the device does not support CDC_SET_ADDRESS, there is no harm and we > + * proceed as before. > + */ > +static int cdc_ncm_set_ethernet_address(struct usbnet *dev, > + struct sockaddr *addr) > +{ > + int ret; > + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; > + u8 iface_no_data = ctx->data->cur_altsetting->desc.bInterfaceNumber; > + u8 iface_no_control = ctx->control->cur_altsetting->desc.bInterfaceNumber; > + int temp; > + > + /* The host shall only send SET_NET_ADDRESS command while > + * the NCM Data Interface is in alternate setting 0. > + */ > + temp = usb_set_interface(dev->udev, iface_no_data, 0); > + if (temp) { > + dev_dbg(&dev->udev->dev, "set interface failed\n"); > + return -ENODEV; > + } Always use checkpatch.pl before submitting patches so you don't get grumpy reviewers telling you to use checkpatch.pl. thanks, greg k-h
Hi Kangzhen,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on westeri-thunderbolt/next]
[also build test WARNING on linus/master v6.3-rc1 next-20230309]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kangzhen-Lou/net-cdc_ncm-support-ACPI-MAC-address-pass-through-functionality/20230309-184736
base: https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git next
patch link: https://lore.kernel.org/r/20230309083436.6729-1-kangzhen.lou%40dell.com
patch subject: [PATCH] net: cdc_ncm: support ACPI MAC address pass through functionality
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230309/202303092024.QNv44vyX-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/97cd8ee9a774c36093af3d26255e415f6082b4a3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kangzhen-Lou/net-cdc_ncm-support-ACPI-MAC-address-pass-through-functionality/20230309-184736
git checkout 97cd8ee9a774c36093af3d26255e415f6082b4a3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/net/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303092024.QNv44vyX-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/net/usb/cdc_ncm.c:818:5: warning: no previous prototype for 'acpi_mac_passthru_invalid' [-Wmissing-prototypes]
818 | int acpi_mac_passthru_invalid(void)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/usb/cdc_ncm.c:851:5: warning: no previous prototype for 'get_acpi_mac_passthru' [-Wmissing-prototypes]
851 | int get_acpi_mac_passthru(char *MACAddress)
| ^~~~~~~~~~~~~~~~~~~~~
drivers/net/usb/cdc_ncm.c: In function 'cdc_ncm_get_ethernet_address':
>> drivers/net/usb/cdc_ncm.c:894:49: warning: passing argument 6 of 'usbnet_read_cmd' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
894 | iface_no, dev->net->dev_addr, ETH_ALEN);
| ~~~~~~~~^~~~~~~~~~
In file included from drivers/net/usb/cdc_ncm.c:53:
include/linux/usb/usbnet.h:181:49: note: expected 'void *' but argument is of type 'const unsigned char *'
181 | u16 value, u16 index, void *data, u16 size);
| ~~~~~~^~~~
In file included from include/linux/string.h:20,
from include/linux/bitmap.h:11,
from include/linux/cpumask.h:12,
from include/linux/mm_types_task.h:14,
from include/linux/mm_types.h:5,
from include/linux/buildid.h:5,
from include/linux/module.h:14,
from drivers/net/usb/cdc_ncm.c:41:
drivers/net/usb/cdc_ncm.c: In function 'cdc_ncm_determine_ethernet_addr':
>> drivers/net/usb/cdc_ncm.c:980:48: warning: passing argument 1 of '__builtin_memcpy' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
980 | memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
| ~~~~~~~~^~~~~~~~~~
arch/m68k/include/asm/string.h:52:42: note: in definition of macro 'memcpy'
52 | #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
| ^
drivers/net/usb/cdc_ncm.c:980:48: note: expected 'void *' but argument is of type 'const unsigned char *'
980 | memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
| ~~~~~~~~^~~~~~~~~~
arch/m68k/include/asm/string.h:52:42: note: in definition of macro 'memcpy'
52 | #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
| ^
vim +/acpi_mac_passthru_invalid +818 drivers/net/usb/cdc_ncm.c
817
> 818 int acpi_mac_passthru_invalid(void)
819 {
820 acpi_status status;
821 struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
822 union acpi_object *obj;
823 int ret = -EINVAL;
824
825 /* returns _AUXMAC_#AABBCCDDEEFF# */
826 status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);
827 obj = (union acpi_object *)buffer.pointer;
828
829 if (!ACPI_SUCCESS(status))
830 return -ENODEV;
831 if (obj->type != ACPI_TYPE_BUFFER || obj->string.length != 0x17) {
832 acpi_info("Invalid buffer for pass-thru MAC addr: (%d, %d)\n",
833 obj->type, obj->string.length);
834 goto amacout;
835 }
836 if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0 ||
837 strncmp(obj->string.pointer + 0x15, "#", 1) != 0) {
838 acpi_info("Invalid header when reading pass-thru MAC addr\n");
839 goto amacout;
840 }
841 /* return success, otherwise return non-zero if invalid buffer read or
842 * MAC pass through is disabled in BIOS
843 */
844 ret = 0;
845
846 amacout:
847 kfree(obj);
848 return ret;
849 }
850
> 851 int get_acpi_mac_passthru(char *MACAddress)
852 {
853 acpi_status status;
854 struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
855 union acpi_object *obj;
856 int ret = -EINVAL;
857 unsigned char buf[6];
858
859 /* returns _AUXMAC_#AABBCCDDEEFF# */
860 status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);
861 obj = (union acpi_object *)buffer.pointer;
862
863 if (!ACPI_SUCCESS(status))
864 return -ENODEV;
865
866 ret = hex2bin(buf, obj->string.pointer + 9, 6);
867 if (!(ret == 0 && is_valid_ether_addr(buf))) {
868 acpi_info("Invalid MAC for pass-thru MAC addr: %d, %pM\n",
869 ret, buf);
870 ret = -EINVAL;
871 goto amacout;
872 }
873 memcpy(MACAddress, buf, 6);
874 acpi_info("Pass-thru MAC addr %pM\n", MACAddress);
875
876 amacout:
877 kfree(obj);
878 return ret;
879 }
880
881 /* Provide method to get MAC address from the USB device's ethernet controller.
882 * If the device supports CDC_GET_ADDRESS, we should receive just six bytes.
883 * Otherwise, use the prior method by asking for the descriptor.
884 */
885 static int cdc_ncm_get_ethernet_address(struct usbnet *dev,
886 struct cdc_ncm_ctx *ctx)
887 {
888 int ret;
889 u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
890
891 ret = usbnet_read_cmd(dev, USB_CDC_GET_NET_ADDRESS,
892 USB_DIR_IN | USB_TYPE_CLASS
893 | USB_RECIP_INTERFACE, 0,
> 894 iface_no, dev->net->dev_addr, ETH_ALEN);
895
896 if (ret == ETH_ALEN) {
897 ret = 0; /* success */
898 } else {
899 ret = usbnet_get_ethernet_addr(dev,
900 ctx->ether_desc->iMACAddress);
901 }
902
903 return ret;
904 }
905
906 /* Provide method to push MAC address to the USB device's ethernet controller.
907 * If the device does not support CDC_SET_ADDRESS, there is no harm and we
908 * proceed as before.
909 */
910 static int cdc_ncm_set_ethernet_address(struct usbnet *dev,
911 struct sockaddr *addr)
912 {
913 int ret;
914 struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
915 u8 iface_no_data = ctx->data->cur_altsetting->desc.bInterfaceNumber;
916 u8 iface_no_control = ctx->control->cur_altsetting->desc.bInterfaceNumber;
917 int temp;
918
919 /* The host shall only send SET_NET_ADDRESS command while
920 * the NCM Data Interface is in alternate setting 0.
921 */
922 temp = usb_set_interface(dev->udev, iface_no_data, 0);
923 if (temp) {
924 dev_dbg(&dev->udev->dev, "set interface failed\n");
925 return -ENODEV;
926 }
927
928 ret = usbnet_write_cmd(dev, USB_CDC_SET_NET_ADDRESS,
929 USB_DIR_OUT | USB_TYPE_CLASS
930 | USB_RECIP_INTERFACE, 0,
931 iface_no_control, addr->sa_data, ETH_ALEN);
932
933 if (ret == ETH_ALEN)
934 ret = 0; // success
935 else if (ret < 0)
936 dev_dbg(&dev->udev->dev, "bad MAC address put, %d\n", ret);
937
938 /* restore alternate setting.
939 * The NCM data altsetting is fixed, so we hard-coded it.
940 */
941 temp = usb_set_interface(dev->udev, iface_no_data,
942 CDC_NCM_DATA_ALTSETTING_NCM);
943 if (temp) {
944 dev_dbg(&dev->udev->dev, "set interface failed\n");
945 return -ENODEV;
946 }
947
948 return ret;
949 }
950
951 static int cdc_ncm_determine_ethernet_addr(struct usb_interface *intf)
952 {
953 struct sockaddr sa;
954 struct usbnet *dev = usb_get_intfdata(intf);
955 struct cdc_ncm_ctx *ctx;
956 int ret = 0;
957
958 if (!dev)
959 return 0;
960
961 /* MAC pass through function only apply to Realtek RTL8153-DD chip */
962 if (!(dev->udev->descriptor.idVendor == 0x0bda
963 && dev->udev->descriptor.idProduct == 0x8153
964 && (dev->udev->descriptor.bcdDevice & 0xff00) == 0x3300))
965 return 0;
966
967 ctx = (struct cdc_ncm_ctx *)dev->data[0];
968 if (!ctx->ether_desc)
969 return 0;
970
971 ret = cdc_ncm_get_ethernet_address(dev, ctx);
972 if (ret) {
973 dev_dbg(&intf->dev, "failed to get mac address\n");
974 return ret;
975 }
976
977 if (!get_acpi_mac_passthru(sa.sa_data)) {
978 if (memcmp(dev->net->dev_addr, sa.sa_data, ETH_ALEN) != 0) {
979 if (!cdc_ncm_set_ethernet_address(dev, &sa))
> 980 memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
981 }
982 }
983
984 dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
985
986 return 0;
987 }
988
Hi Kangzhen,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on westeri-thunderbolt/next]
[also build test WARNING on linus/master v6.3-rc1 next-20230309]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kangzhen-Lou/net-cdc_ncm-support-ACPI-MAC-address-pass-through-functionality/20230309-184736
base: https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git next
patch link: https://lore.kernel.org/r/20230309083436.6729-1-kangzhen.lou%40dell.com
patch subject: [PATCH] net: cdc_ncm: support ACPI MAC address pass through functionality
config: ia64-allyesconfig (https://download.01.org/0day-ci/archive/20230309/202303092027.01TO9rlB-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/97cd8ee9a774c36093af3d26255e415f6082b4a3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kangzhen-Lou/net-cdc_ncm-support-ACPI-MAC-address-pass-through-functionality/20230309-184736
git checkout 97cd8ee9a774c36093af3d26255e415f6082b4a3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/net/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303092027.01TO9rlB-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/net/usb/cdc_ncm.c:818:5: warning: no previous prototype for 'acpi_mac_passthru_invalid' [-Wmissing-prototypes]
818 | int acpi_mac_passthru_invalid(void)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/usb/cdc_ncm.c:851:5: warning: no previous prototype for 'get_acpi_mac_passthru' [-Wmissing-prototypes]
851 | int get_acpi_mac_passthru(char *MACAddress)
| ^~~~~~~~~~~~~~~~~~~~~
drivers/net/usb/cdc_ncm.c: In function 'cdc_ncm_get_ethernet_address':
drivers/net/usb/cdc_ncm.c:894:49: warning: passing argument 6 of 'usbnet_read_cmd' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
894 | iface_no, dev->net->dev_addr, ETH_ALEN);
| ~~~~~~~~^~~~~~~~~~
In file included from drivers/net/usb/cdc_ncm.c:53:
include/linux/usb/usbnet.h:181:49: note: expected 'void *' but argument is of type 'const unsigned char *'
181 | u16 value, u16 index, void *data, u16 size);
| ~~~~~~^~~~
drivers/net/usb/cdc_ncm.c: In function 'cdc_ncm_determine_ethernet_addr':
>> drivers/net/usb/cdc_ncm.c:980:48: warning: passing argument 1 of 'memcpy' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
980 | memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
| ~~~~~~~~^~~~~~~~~~
In file included from include/linux/string.h:20,
from include/linux/bitmap.h:11,
from include/linux/cpumask.h:12,
from include/linux/mm_types_task.h:14,
from include/linux/mm_types.h:5,
from include/linux/buildid.h:5,
from include/linux/module.h:14,
from drivers/net/usb/cdc_ncm.c:41:
arch/ia64/include/asm/string.h:19:22: note: expected 'void *' but argument is of type 'const unsigned char *'
19 | extern void *memcpy (void *, const void *, __kernel_size_t);
| ^~~~~~
vim +980 drivers/net/usb/cdc_ncm.c
950
951 static int cdc_ncm_determine_ethernet_addr(struct usb_interface *intf)
952 {
953 struct sockaddr sa;
954 struct usbnet *dev = usb_get_intfdata(intf);
955 struct cdc_ncm_ctx *ctx;
956 int ret = 0;
957
958 if (!dev)
959 return 0;
960
961 /* MAC pass through function only apply to Realtek RTL8153-DD chip */
962 if (!(dev->udev->descriptor.idVendor == 0x0bda
963 && dev->udev->descriptor.idProduct == 0x8153
964 && (dev->udev->descriptor.bcdDevice & 0xff00) == 0x3300))
965 return 0;
966
967 ctx = (struct cdc_ncm_ctx *)dev->data[0];
968 if (!ctx->ether_desc)
969 return 0;
970
971 ret = cdc_ncm_get_ethernet_address(dev, ctx);
972 if (ret) {
973 dev_dbg(&intf->dev, "failed to get mac address\n");
974 return ret;
975 }
976
977 if (!get_acpi_mac_passthru(sa.sa_data)) {
978 if (memcmp(dev->net->dev_addr, sa.sa_data, ETH_ALEN) != 0) {
979 if (!cdc_ncm_set_ethernet_address(dev, &sa))
> 980 memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
981 }
982 }
983
984 dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
985
986 return 0;
987 }
988
Hi Kangzhen,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on westeri-thunderbolt/next]
[also build test ERROR on linus/master v6.3-rc1 next-20230310]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kangzhen-Lou/net-cdc_ncm-support-ACPI-MAC-address-pass-through-functionality/20230309-184736
base: https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git next
patch link: https://lore.kernel.org/r/20230309083436.6729-1-kangzhen.lou%40dell.com
patch subject: [PATCH] net: cdc_ncm: support ACPI MAC address pass through functionality
config: powerpc-g5_defconfig (https://download.01.org/0day-ci/archive/20230310/202303101331.rzCbgFQa-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
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
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/97cd8ee9a774c36093af3d26255e415f6082b4a3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kangzhen-Lou/net-cdc_ncm-support-ACPI-MAC-address-pass-through-functionality/20230309-184736
git checkout 97cd8ee9a774c36093af3d26255e415f6082b4a3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash arch/powerpc/kernel/ drivers/net/usb/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303101331.rzCbgFQa-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
>> drivers/net/usb/cdc_ncm.c:818:5: warning: no previous prototype for function 'acpi_mac_passthru_invalid' [-Wmissing-prototypes]
int acpi_mac_passthru_invalid(void)
^
drivers/net/usb/cdc_ncm.c:818:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int acpi_mac_passthru_invalid(void)
^
static
>> drivers/net/usb/cdc_ncm.c:851:5: warning: no previous prototype for function 'get_acpi_mac_passthru' [-Wmissing-prototypes]
int get_acpi_mac_passthru(char *MACAddress)
^
drivers/net/usb/cdc_ncm.c:851:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int get_acpi_mac_passthru(char *MACAddress)
^
static
>> drivers/net/usb/cdc_ncm.c:894:20: error: passing 'const unsigned char *' to parameter of type 'void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
iface_no, dev->net->dev_addr, ETH_ALEN);
^~~~~~~~~~~~~~~~~~
include/linux/usb/usbnet.h:181:35: note: passing argument to parameter 'data' here
u16 value, u16 index, void *data, u16 size);
^
drivers/net/usb/cdc_ncm.c:980:12: error: passing 'const unsigned char *' to parameter of type 'void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
^~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/string.h:27:28: note: passing argument to parameter here
extern void * memcpy(void *,const void *,__kernel_size_t);
^
2 warnings and 2 errors generated.
vim +894 drivers/net/usb/cdc_ncm.c
817
> 818 int acpi_mac_passthru_invalid(void)
819 {
820 acpi_status status;
821 struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
822 union acpi_object *obj;
823 int ret = -EINVAL;
824
825 /* returns _AUXMAC_#AABBCCDDEEFF# */
826 status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);
827 obj = (union acpi_object *)buffer.pointer;
828
829 if (!ACPI_SUCCESS(status))
830 return -ENODEV;
831 if (obj->type != ACPI_TYPE_BUFFER || obj->string.length != 0x17) {
832 acpi_info("Invalid buffer for pass-thru MAC addr: (%d, %d)\n",
833 obj->type, obj->string.length);
834 goto amacout;
835 }
836 if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0 ||
837 strncmp(obj->string.pointer + 0x15, "#", 1) != 0) {
838 acpi_info("Invalid header when reading pass-thru MAC addr\n");
839 goto amacout;
840 }
841 /* return success, otherwise return non-zero if invalid buffer read or
842 * MAC pass through is disabled in BIOS
843 */
844 ret = 0;
845
846 amacout:
847 kfree(obj);
848 return ret;
849 }
850
> 851 int get_acpi_mac_passthru(char *MACAddress)
852 {
853 acpi_status status;
854 struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
855 union acpi_object *obj;
856 int ret = -EINVAL;
857 unsigned char buf[6];
858
859 /* returns _AUXMAC_#AABBCCDDEEFF# */
860 status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);
861 obj = (union acpi_object *)buffer.pointer;
862
863 if (!ACPI_SUCCESS(status))
864 return -ENODEV;
865
866 ret = hex2bin(buf, obj->string.pointer + 9, 6);
867 if (!(ret == 0 && is_valid_ether_addr(buf))) {
868 acpi_info("Invalid MAC for pass-thru MAC addr: %d, %pM\n",
869 ret, buf);
870 ret = -EINVAL;
871 goto amacout;
872 }
873 memcpy(MACAddress, buf, 6);
874 acpi_info("Pass-thru MAC addr %pM\n", MACAddress);
875
876 amacout:
877 kfree(obj);
878 return ret;
879 }
880
881 /* Provide method to get MAC address from the USB device's ethernet controller.
882 * If the device supports CDC_GET_ADDRESS, we should receive just six bytes.
883 * Otherwise, use the prior method by asking for the descriptor.
884 */
885 static int cdc_ncm_get_ethernet_address(struct usbnet *dev,
886 struct cdc_ncm_ctx *ctx)
887 {
888 int ret;
889 u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
890
891 ret = usbnet_read_cmd(dev, USB_CDC_GET_NET_ADDRESS,
892 USB_DIR_IN | USB_TYPE_CLASS
893 | USB_RECIP_INTERFACE, 0,
> 894 iface_no, dev->net->dev_addr, ETH_ALEN);
895
896 if (ret == ETH_ALEN) {
897 ret = 0; /* success */
898 } else {
899 ret = usbnet_get_ethernet_addr(dev,
900 ctx->ether_desc->iMACAddress);
901 }
902
903 return ret;
904 }
905
> On Thu, Mar 09, 2023 at 04:34:36PM +0800, Kangzhen Lou wrote: > > Make cdc_ncm to support ACPI MAC address pass through functionality > > that also exists in the Realtek r8152 driver. > > > > RTL8153DD will load cdc_ncm driver by default with mainline 6.2 kernel. > > This is to support Realtek RTL8153DD Ethernet Interface Controller's > > MAC pass through function which used in dock, dongle or monitor. > > > > Although there is patch “ec51fbd1b8a2bca2948dede99c14ec63dc57ff6b” > > will make RTL8153DD load r8152-cfgselector instead cdc_ncm driver, > > would also submit this patch in case anyone need this feature in > > cdc_ncm in the future. > > Please reference commits in the correct way, as documented: > ec51fbd1b8a2 ("r8152: add USB device driver for config selection") > Thanks for reminder, my fault. > But because of that commit, why is your change needed at all? Because RTL8153DD supports both r8152 and cdc_ncm driver, and user can select to load one of them by udev rule, in case someone want to load cdc_ncm driver with MAC pass through function. > > > > > Signed-off-by: Kangzhen Lou <kangzhen.lou@dell.com> > > --- > > drivers/net/usb/cdc_ncm.c | 199 > > +++++++++++++++++++++++++++++++++++++- > > 1 file changed, 197 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c > > index 6ce8f4f0c70e..8179516b819c 100644 > > --- a/drivers/net/usb/cdc_ncm.c > > +++ b/drivers/net/usb/cdc_ncm.c > > @@ -53,6 +53,7 @@ > > #include <linux/usb/usbnet.h> > > #include <linux/usb/cdc.h> > > #include <linux/usb/cdc_ncm.h> > > +#include <linux/acpi.h> > > What about non-acpi systems? About non-acpi system, it should return at first place: if (!ACPI_SUCCESS(status)) return -ENODEV; If I don't answer your question, may I know your thought? > > > > > #if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM) > > static bool prefer_mbim = true; > > @@ -814,6 +815,177 @@ static const struct net_device_ops > cdc_ncm_netdev_ops = { > > .ndo_validate_addr = eth_validate_addr, > > }; > > > > +int acpi_mac_passthru_invalid(void) > > +{ > > + acpi_status status; > > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > > + union acpi_object *obj; > > + int ret = -EINVAL; > > + > > + /* returns _AUXMAC_#AABBCCDDEEFF# */ > > + status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer); > > + obj = (union acpi_object *)buffer.pointer; > > + > > + if (!ACPI_SUCCESS(status)) > > + return -ENODEV; > > + if (obj->type != ACPI_TYPE_BUFFER || obj->string.length != 0x17) { > > + acpi_info("Invalid buffer for pass-thru MAC addr: > (%d, %d)\n", > > + obj->type, obj->string.length); > > Why is that an "info" message? For the BIOS have different "\\_SB.AMAC" acpi object definition, may run into here. I'm ok change to acpi_warning. > > > + goto amacout; > > + } > > + if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0 || > > + strncmp(obj->string.pointer + 0x15, "#", 1) != 0) { > > + acpi_info("Invalid header when reading pass-thru MAC > addr\n"); > > Again, info? Shouldn't this be an error? Or just debug as userspace > can't do anything about it. If user disable MAC pass through in BIOS, it will also run into here. So it's not an error. Depends on user selection in BIOS. > > > + goto amacout; > > + } > > + /* return success, otherwise return non-zero if invalid buffer read or > > + * MAC pass through is disabled in BIOS > > + */ > > + ret = 0; > > + > > +amacout: > > + kfree(obj); > > + return ret; > > +} > > + > > +int get_acpi_mac_passthru(char *MACAddress) { > > + acpi_status status; > > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > > + union acpi_object *obj; > > + int ret = -EINVAL; > > + unsigned char buf[6]; > > + > > + /* returns _AUXMAC_#AABBCCDDEEFF# */ > > + status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer); > > + obj = (union acpi_object *)buffer.pointer; > > + > > + if (!ACPI_SUCCESS(status)) > > + return -ENODEV; > > + > > + ret = hex2bin(buf, obj->string.pointer + 9, 6); > > + if (!(ret == 0 && is_valid_ether_addr(buf))) { > > + acpi_info("Invalid MAC for pass-thru MAC addr: %d, %pM\n", > > + ret, buf); > > + ret = -EINVAL; > > + goto amacout; > > + } > > + memcpy(MACAddress, buf, 6); > > + acpi_info("Pass-thru MAC addr %pM\n", MACAddress); > > + > > +amacout: > > + kfree(obj); > > + return ret; > > +} > > + > > +/* Provide method to get MAC address from the USB device's ethernet > controller. > > + * If the device supports CDC_GET_ADDRESS, we should receive just six > bytes. > > + * Otherwise, use the prior method by asking for the descriptor. > > + */ > > +static int cdc_ncm_get_ethernet_address(struct usbnet *dev, > > + struct cdc_ncm_ctx *ctx) > > +{ > > + int ret; > > + u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; > > + > > + ret = usbnet_read_cmd(dev, USB_CDC_GET_NET_ADDRESS, > > + USB_DIR_IN | USB_TYPE_CLASS > > + | USB_RECIP_INTERFACE, 0, > > + iface_no, dev->net->dev_addr, ETH_ALEN); > > + > > + if (ret == ETH_ALEN) { > > + ret = 0; /* success */ > > + } else { > > + ret = usbnet_get_ethernet_addr(dev, > > + ctx->ether_desc->iMACAddress); > > + } > > + > > + return ret; > > +} > > + > > +/* Provide method to push MAC address to the USB device's ethernet > controller. > > + * If the device does not support CDC_SET_ADDRESS, there is no harm > > +and we > > + * proceed as before. > > + */ > > +static int cdc_ncm_set_ethernet_address(struct usbnet *dev, > > + struct sockaddr *addr) > > +{ > > + int ret; > > + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; > > + u8 iface_no_data = ctx->data->cur_altsetting- > >desc.bInterfaceNumber; > > + u8 iface_no_control = ctx->control->cur_altsetting- > >desc.bInterfaceNumber; > > + int temp; > > + > > + /* The host shall only send SET_NET_ADDRESS command while > > + * the NCM Data Interface is in alternate setting 0. > > + */ > > + temp = usb_set_interface(dev->udev, iface_no_data, 0); > > + if (temp) { > > + dev_dbg(&dev->udev->dev, "set interface failed\n"); > > + return -ENODEV; > > + } > > Always use checkpatch.pl before submitting patches so you don't get grumpy > reviewers telling you to use checkpatch.pl. I did run checkpatch.pl before submitting patches to make sure 0 error and 0 warning. Thanks for reminder again. > > thanks, > > greg k-h
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index 6ce8f4f0c70e..8179516b819c 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -53,6 +53,7 @@ #include <linux/usb/usbnet.h> #include <linux/usb/cdc.h> #include <linux/usb/cdc_ncm.h> +#include <linux/acpi.h> #if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM) static bool prefer_mbim = true; @@ -814,6 +815,177 @@ static const struct net_device_ops cdc_ncm_netdev_ops = { .ndo_validate_addr = eth_validate_addr, }; +int acpi_mac_passthru_invalid(void) +{ + acpi_status status; + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; + union acpi_object *obj; + int ret = -EINVAL; + + /* returns _AUXMAC_#AABBCCDDEEFF# */ + status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer); + obj = (union acpi_object *)buffer.pointer; + + if (!ACPI_SUCCESS(status)) + return -ENODEV; + if (obj->type != ACPI_TYPE_BUFFER || obj->string.length != 0x17) { + acpi_info("Invalid buffer for pass-thru MAC addr: (%d, %d)\n", + obj->type, obj->string.length); + goto amacout; + } + if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0 || + strncmp(obj->string.pointer + 0x15, "#", 1) != 0) { + acpi_info("Invalid header when reading pass-thru MAC addr\n"); + goto amacout; + } + /* return success, otherwise return non-zero if invalid buffer read or + * MAC pass through is disabled in BIOS + */ + ret = 0; + +amacout: + kfree(obj); + return ret; +} + +int get_acpi_mac_passthru(char *MACAddress) +{ + acpi_status status; + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; + union acpi_object *obj; + int ret = -EINVAL; + unsigned char buf[6]; + + /* returns _AUXMAC_#AABBCCDDEEFF# */ + status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer); + obj = (union acpi_object *)buffer.pointer; + + if (!ACPI_SUCCESS(status)) + return -ENODEV; + + ret = hex2bin(buf, obj->string.pointer + 9, 6); + if (!(ret == 0 && is_valid_ether_addr(buf))) { + acpi_info("Invalid MAC for pass-thru MAC addr: %d, %pM\n", + ret, buf); + ret = -EINVAL; + goto amacout; + } + memcpy(MACAddress, buf, 6); + acpi_info("Pass-thru MAC addr %pM\n", MACAddress); + +amacout: + kfree(obj); + return ret; +} + +/* Provide method to get MAC address from the USB device's ethernet controller. + * If the device supports CDC_GET_ADDRESS, we should receive just six bytes. + * Otherwise, use the prior method by asking for the descriptor. + */ +static int cdc_ncm_get_ethernet_address(struct usbnet *dev, + struct cdc_ncm_ctx *ctx) +{ + int ret; + u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; + + ret = usbnet_read_cmd(dev, USB_CDC_GET_NET_ADDRESS, + USB_DIR_IN | USB_TYPE_CLASS + | USB_RECIP_INTERFACE, 0, + iface_no, dev->net->dev_addr, ETH_ALEN); + + if (ret == ETH_ALEN) { + ret = 0; /* success */ + } else { + ret = usbnet_get_ethernet_addr(dev, + ctx->ether_desc->iMACAddress); + } + + return ret; +} + +/* Provide method to push MAC address to the USB device's ethernet controller. + * If the device does not support CDC_SET_ADDRESS, there is no harm and we + * proceed as before. + */ +static int cdc_ncm_set_ethernet_address(struct usbnet *dev, + struct sockaddr *addr) +{ + int ret; + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; + u8 iface_no_data = ctx->data->cur_altsetting->desc.bInterfaceNumber; + u8 iface_no_control = ctx->control->cur_altsetting->desc.bInterfaceNumber; + int temp; + + /* The host shall only send SET_NET_ADDRESS command while + * the NCM Data Interface is in alternate setting 0. + */ + temp = usb_set_interface(dev->udev, iface_no_data, 0); + if (temp) { + dev_dbg(&dev->udev->dev, "set interface failed\n"); + return -ENODEV; + } + + ret = usbnet_write_cmd(dev, USB_CDC_SET_NET_ADDRESS, + USB_DIR_OUT | USB_TYPE_CLASS + | USB_RECIP_INTERFACE, 0, + iface_no_control, addr->sa_data, ETH_ALEN); + + if (ret == ETH_ALEN) + ret = 0; // success + else if (ret < 0) + dev_dbg(&dev->udev->dev, "bad MAC address put, %d\n", ret); + + /* restore alternate setting. + * The NCM data altsetting is fixed, so we hard-coded it. + */ + temp = usb_set_interface(dev->udev, iface_no_data, + CDC_NCM_DATA_ALTSETTING_NCM); + if (temp) { + dev_dbg(&dev->udev->dev, "set interface failed\n"); + return -ENODEV; + } + + return ret; +} + +static int cdc_ncm_determine_ethernet_addr(struct usb_interface *intf) +{ + struct sockaddr sa; + struct usbnet *dev = usb_get_intfdata(intf); + struct cdc_ncm_ctx *ctx; + int ret = 0; + + if (!dev) + return 0; + + /* MAC pass through function only apply to Realtek RTL8153-DD chip */ + if (!(dev->udev->descriptor.idVendor == 0x0bda + && dev->udev->descriptor.idProduct == 0x8153 + && (dev->udev->descriptor.bcdDevice & 0xff00) == 0x3300)) + return 0; + + ctx = (struct cdc_ncm_ctx *)dev->data[0]; + if (!ctx->ether_desc) + return 0; + + ret = cdc_ncm_get_ethernet_address(dev, ctx); + if (ret) { + dev_dbg(&intf->dev, "failed to get mac address\n"); + return ret; + } + + if (!get_acpi_mac_passthru(sa.sa_data)) { + if (memcmp(dev->net->dev_addr, sa.sa_data, ETH_ALEN) != 0) { + if (!cdc_ncm_set_ethernet_address(dev, &sa)) + memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN); + } + } + + dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr); + + return 0; +} + int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting, int drvflags) { struct cdc_ncm_ctx *ctx; @@ -945,6 +1117,9 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_ } dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr); } + /* Execute MAC pass thru only if enabled in BIOS */ + if (acpi_mac_passthru_invalid() == 0) + cdc_ncm_determine_ethernet_addr(intf); /* finish setting up the device specific data */ cdc_ncm_setup(dev); @@ -1892,6 +2067,25 @@ static void cdc_ncm_status(struct usbnet *dev, struct urb *urb) } } +static int cdc_ncm_resume(struct usb_interface *intf) +{ + int ret; + + ret = usbnet_resume(intf); + if (ret == 0) + cdc_ncm_determine_ethernet_addr(intf); + + return ret; +} + +static int cdc_ncm_post_reset(struct usb_interface *intf) +{ + /* reset the MAC address in case of policy change */ + cdc_ncm_determine_ethernet_addr(intf); + + return 0; +} + static const struct driver_info cdc_ncm_info = { .description = "CDC NCM (NO ZLP)", .flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET @@ -2051,8 +2245,9 @@ static struct usb_driver cdc_ncm_driver = { .probe = usbnet_probe, .disconnect = usbnet_disconnect, .suspend = usbnet_suspend, - .resume = usbnet_resume, - .reset_resume = usbnet_resume, + .resume = cdc_ncm_resume, + .reset_resume = cdc_ncm_resume, + .post_reset = cdc_ncm_post_reset, .supports_autosuspend = 1, .disable_hub_initiated_lpm = 1, };
Make cdc_ncm to support ACPI MAC address pass through functionality that also exists in the Realtek r8152 driver. RTL8153DD will load cdc_ncm driver by default with mainline 6.2 kernel. This is to support Realtek RTL8153DD Ethernet Interface Controller's MAC pass through function which used in dock, dongle or monitor. Although there is patch “ec51fbd1b8a2bca2948dede99c14ec63dc57ff6b” will make RTL8153DD load r8152-cfgselector instead cdc_ncm driver, would also submit this patch in case anyone need this feature in cdc_ncm in the future. Signed-off-by: Kangzhen Lou <kangzhen.lou@dell.com> --- drivers/net/usb/cdc_ncm.c | 199 +++++++++++++++++++++++++++++++++++++- 1 file changed, 197 insertions(+), 2 deletions(-)