> 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@xxxxxxxx>
> > ---
> > 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
