linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
@ 2019-09-05 21:01 Charles.Hyde
  2019-09-06  2:13 ` Chunfeng Yun
  2019-09-06  7:59 ` Bjørn Mork
  0 siblings, 2 replies; 9+ messages in thread
From: Charles.Hyde @ 2019-09-05 21:01 UTC (permalink / raw)
  To: oliver, rjw, lenb
  Cc: Mario.Limonciello, chip.programmer, nic_swsd, linux-usb, linux-acpi

This change adds support to cdc_ncm for ACPI MAC address pass through
functionality that also exists in the Realtek r8152 driver.  This is in
support of Dell's Universal Dock D6000, to give it the same feature
capability as is currently available in Windows and advertized on Dell's
product web site.

Signed-off-by: Charles Hyde <charles.hyde@dellteam.com>
Cc: Mario Limonciello <mario.limonciello@dell.com>
Cc: chip.programmer@gmail.com
Cc: Oliver Neukum <oliver@neukum.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-usb@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
---
 drivers/net/usb/cdc_ncm.c | 74 +++++++++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 85093579612f..e0152d44f5af 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -52,6 +52,7 @@
 #include <linux/usb/usbnet.h>
 #include <linux/usb/cdc.h>
 #include <linux/usb/cdc_ncm.h>
+#include <acpi/acpi_mac_passthru.h>
 
 #if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM)
 static bool prefer_mbim = true;
@@ -833,6 +834,45 @@ static const struct net_device_ops cdc_ncm_netdev_ops = {
 	.ndo_validate_addr   = eth_validate_addr,
 };
 
+static int get_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;
+
+	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;
+	}
+
+	/* Check for a Dell Universal Dock D6000 before checking if ACPI
+	 * supports MAC address pass through.
+	 */
+	if (strstr(dev->udev->product, "D6000")) {
+		sa.sa_family = dev->net->type;
+		if (get_acpi_mac_passthru(sa.sa_data)) {
+			if (!memcmp(dev->net->dev_addr, sa.sa_data,
+				    ETH_ALEN)) {
+				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;
@@ -983,14 +1023,8 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
 	usb_set_intfdata(ctx->data, dev);
 	usb_set_intfdata(ctx->control, dev);
 
-	if (ctx->ether_desc) {
-		temp = usbnet_get_ethernet_addr(dev, ctx->ether_desc->iMACAddress);
-		if (temp) {
-			dev_dbg(&intf->dev, "failed to get mac address\n");
-			goto error2;
-		}
-		dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
-	}
+	if (get_ethernet_addr(intf))
+		goto error2;
 
 	/* finish setting up the device specific data */
 	cdc_ncm_setup(dev);
@@ -1716,6 +1750,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)
+		get_ethernet_addr(intf);
+
+	return ret;
+}
+
+static int cdc_ncm_post_reset(struct usb_interface *intf)
+{
+	/* reset the MAC address in case of policy change */
+	get_ethernet_addr(intf);
+
+	return 0;
+}
+
 static const struct driver_info cdc_ncm_info = {
 	.description = "CDC NCM",
 	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
@@ -1848,8 +1901,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,
 };
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
  2019-09-05 21:01 [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality Charles.Hyde
@ 2019-09-06  2:13 ` Chunfeng Yun
  2019-09-06 13:25   ` chip.programmer
  2019-09-06  7:59 ` Bjørn Mork
  1 sibling, 1 reply; 9+ messages in thread
From: Chunfeng Yun @ 2019-09-06  2:13 UTC (permalink / raw)
  To: Charles.Hyde
  Cc: oliver, rjw, lenb, Mario.Limonciello, chip.programmer, nic_swsd,
	linux-usb, linux-acpi

On Thu, 2019-09-05 at 21:01 +0000, Charles.Hyde@dellteam.com wrote:
> This change adds support to cdc_ncm for ACPI MAC address pass through
> functionality that also exists in the Realtek r8152 driver.  This is in
> support of Dell's Universal Dock D6000, to give it the same feature
> capability as is currently available in Windows and advertized on Dell's
> product web site.
> 
> Signed-off-by: Charles Hyde <charles.hyde@dellteam.com>
> Cc: Mario Limonciello <mario.limonciello@dell.com>
> Cc: chip.programmer@gmail.com
> Cc: Oliver Neukum <oliver@neukum.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-acpi@vger.kernel.org
> ---
>  drivers/net/usb/cdc_ncm.c | 74 +++++++++++++++++++++++++++++++++------
>  1 file changed, 64 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 85093579612f..e0152d44f5af 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -52,6 +52,7 @@
>  #include <linux/usb/usbnet.h>
>  #include <linux/usb/cdc.h>
>  #include <linux/usb/cdc_ncm.h>
> +#include <acpi/acpi_mac_passthru.h>
>  
>  #if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM)
>  static bool prefer_mbim = true;
> @@ -833,6 +834,45 @@ static const struct net_device_ops cdc_ncm_netdev_ops = {
>  	.ndo_validate_addr   = eth_validate_addr,
>  };
>  
> +static int get_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;
> +
> +	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;
> +	}
> +
> +	/* Check for a Dell Universal Dock D6000 before checking if ACPI
> +	 * supports MAC address pass through.
> +	 */
> +	if (strstr(dev->udev->product, "D6000")) {
> +		sa.sa_family = dev->net->type;
> +		if (get_acpi_mac_passthru(sa.sa_data)) {
> +			if (!memcmp(dev->net->dev_addr, sa.sa_data,
> +				    ETH_ALEN)) {
> +				if (!cdc_ncm_set_ethernet_address(dev, &sa))
How about use one if-statement instead of these three if-statement?

> +					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;
> @@ -983,14 +1023,8 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
>  	usb_set_intfdata(ctx->data, dev);
>  	usb_set_intfdata(ctx->control, dev);
>  
> -	if (ctx->ether_desc) {
> -		temp = usbnet_get_ethernet_addr(dev, ctx->ether_desc->iMACAddress);
> -		if (temp) {
> -			dev_dbg(&intf->dev, "failed to get mac address\n");
> -			goto error2;
> -		}
> -		dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
> -	}
> +	if (get_ethernet_addr(intf))
> +		goto error2;
>  
>  	/* finish setting up the device specific data */
>  	cdc_ncm_setup(dev);
> @@ -1716,6 +1750,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)
> +		get_ethernet_addr(intf);
> +
> +	return ret;
> +}
> +
> +static int cdc_ncm_post_reset(struct usb_interface *intf)
> +{
> +	/* reset the MAC address in case of policy change */
> +	get_ethernet_addr(intf);
> +
> +	return 0;
> +}
> +
>  static const struct driver_info cdc_ncm_info = {
>  	.description = "CDC NCM",
>  	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
> @@ -1848,8 +1901,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,
>  };



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
  2019-09-05 21:01 [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality Charles.Hyde
  2019-09-06  2:13 ` Chunfeng Yun
@ 2019-09-06  7:59 ` Bjørn Mork
  2019-09-06 15:23   ` Dan Williams
  1 sibling, 1 reply; 9+ messages in thread
From: Bjørn Mork @ 2019-09-06  7:59 UTC (permalink / raw)
  To: Charles.Hyde
  Cc: oliver, rjw, lenb, Mario.Limonciello, chip.programmer, nic_swsd,
	linux-usb, linux-acpi

<Charles.Hyde@dellteam.com> writes:

> +	if (strstr(dev->udev->product, "D6000")) {

Huh? Can you please test that on all USB devices ever made?


Bjørn

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
  2019-09-06  2:13 ` Chunfeng Yun
@ 2019-09-06 13:25   ` chip.programmer
  0 siblings, 0 replies; 9+ messages in thread
From: chip.programmer @ 2019-09-06 13:25 UTC (permalink / raw)
  To: Chunfeng Yun, Charles.Hyde
  Cc: oliver, rjw, lenb, Mario.Limonciello, nic_swsd, linux-usb, linux-acpi

>> + if (strstr(dev->udev->product, "D6000")) {
>> + sa.sa_family = dev->net->type;
>> + if (get_acpi_mac_passthru(sa.sa_data)) {
>> + if (!memcmp(dev->net->dev_addr, sa.sa_data,
>> +     ETH_ALEN)) {
>> + if (!cdc_ncm_set_ethernet_address(dev, &sa))

> How about use one if-statement instead of these three if-statement?

While using a single compound if statement is possible, my experience is 
that using multiple if statements will make the code easier to debug and 
maintain by others who come after me.  What is possible to do with code is 
not necessarily what should be done.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
  2019-09-06  7:59 ` Bjørn Mork
@ 2019-09-06 15:23   ` Dan Williams
  0 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2019-09-06 15:23 UTC (permalink / raw)
  To: Bjørn Mork, Charles.Hyde
  Cc: oliver, rjw, lenb, Mario.Limonciello, chip.programmer, nic_swsd,
	linux-usb, linux-acpi

On Fri, 2019-09-06 at 09:59 +0200, Bjørn Mork wrote:
> <Charles.Hyde@dellteam.com> writes:
> 
> > +	if (strstr(dev->udev->product, "D6000")) {
> 
> Huh? Can you please test that on all USB devices ever made?

Yeah. Can't VID/PID be used as the filter here instead?

Dan


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
  2019-09-03 17:50   ` Charles.Hyde
@ 2019-09-03 17:57     ` Mario.Limonciello
  0 siblings, 0 replies; 9+ messages in thread
From: Mario.Limonciello @ 2019-09-03 17:57 UTC (permalink / raw)
  To: Charles.Hyde, oneukum, lenb, rjw; +Cc: nic_swsd, linux-acpi, linux-usb

> -----Original Message-----
> From: Hyde, Charles - Dell Team
> Sent: Tuesday, September 3, 2019 12:50 PM
> To: Oliver Neukum; lenb@kernel.org; rjw@rjwysocki.net
> Cc: Limonciello, Mario; nic_swsd@realtek.com; linux-acpi@vger.kernel.org; linux-
> usb@vger.kernel.org
> Subject: RE: [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through
> functionality
> 
> > > This change adds support to cdc_ncm for ACPI MAC address pass through
> > > functionality that also exists in the Realtek r8152 driver.  This is
> > > in support of Dell's Universal Dock D6000, to give it the same feature
> > > capability as is currently available in Windows and advertized on
> > > Dell's product web site.
> > >
> > > Signed-off-by: Charles Hyde <charles.hyde@dellteam.com>
> > > Cc: Mario Limonciello <mario.limonciello@dell.com>
> > > Cc: Oliver Neukum <oliver@neukum.org>
> > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > > Cc: Len Brown <lenb@kernel.org>
> > > Cc: linux-usb@vger.kernel.org
> > > Cc: linux-acpi@vger.kernel.org
> > > ---
> > >  drivers/net/usb/cdc_ncm.c | 67
> > > +++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 64 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> > > index 85093579612f..11a04dc2298d 100644
> > > --- a/drivers/net/usb/cdc_ncm.c
> > > +++ b/drivers/net/usb/cdc_ncm.c
> > > @@ -52,6 +52,7 @@
> > >  #include <linux/usb/usbnet.h>
> > >  #include <linux/usb/cdc.h>
> > >  #include <linux/usb/cdc_ncm.h>
> > > +#include <acpi/acpi_mac_passthru.h>
> > >
> > >  #if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM)
> > >  static bool prefer_mbim = true;
> > > @@ -984,11 +985,30 @@ int cdc_ncm_bind_common(struct usbnet *dev,
> > struct usb_interface *intf, u8 data_
> > >  	usb_set_intfdata(ctx->control, dev);
> > >
> > >  	if (ctx->ether_desc) {
> > > -		temp = usbnet_get_ethernet_addr(dev, ctx->ether_desc-
> > >iMACAddress);
> > > +		struct sockaddr sa;
> > > +
> > > +		temp = cdc_ncm_get_ethernet_address(dev, ctx);
> > >  		if (temp) {
> > >  			dev_dbg(&intf->dev, "failed to get mac address\n");
> > >  			goto error2;
> > >  		}
> > > +
> > > +		/* Check for a Dell Universal Dock D6000 before checking if
> > > +		 * ACPI supports MAC address pass through.
> > > +		 */
> > > +		if (!strstr(dev->udev->product, "D6000"))
> > > +			goto skip_acpi_mapt_in_bind;
> > > +
> > > +		if (get_acpi_mac_passthru(sa.sa_data) != 0)
> > > +			goto skip_acpi_mapt_in_bind;
> > > +
> > > +		if (memcmp(dev->net->dev_addr, sa.sa_data, ETH_ALEN) == 0)
> > > +			goto skip_acpi_mapt_in_bind;
> > > +
> > > +		if (cdc_ncm_set_ethernet_address(dev, &sa) == 0)
> > > +			memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
> > > +
> > > +skip_acpi_mapt_in_bind:
> > >  		dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net-
> > >dev_addr);
> > >  	}
> > >
> > > @@ -1716,6 +1736,47 @@ static void cdc_ncm_status(struct usbnet *dev,
> > struct urb *urb)
> > >  	}
> > >  }
> > >
> > > +static int cdc_ncm_resume(struct usb_interface *intf) {
> > > +	struct usbnet *dev = usb_get_intfdata(intf);
> > > +	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
> > > +	int ret;
> > > +
> > > +	ret = usbnet_resume(intf);
> > > +	if (ret != 0)
> > > +		goto error2;
> > > +
> > > +	if (ctx->ether_desc) {
> > > +		struct sockaddr sa;
> > > +
> > > +		if (cdc_ncm_get_ethernet_address(dev, ctx)) {
> > > +			dev_dbg(&intf->dev, "failed to get mac address\n");
> > > +			goto error2;
> > > +		}
> > > +
> > > +		/* Check for a Dell Universal Dock D6000 before checking if
> > > +		 * ACPI supports MAC address pass through.
> > > +		 */
> > > +		if (!strstr(dev->udev->product, "D6000"))
> > > +			goto skip_acpi_mapt_in_resume;
> >
> > This is too ugly. Use a flag for the need to restore the MAC.
> 
> It is ugly to me, also, because of restrictions that Linux source cannot be longer
> than 80 columns.  I much prefer readability and maintainability over 80 column
> line length limits.   Besides, I have not used an 80 column keypunch machine in
> more than 35 years.

This is kernel style though.
https://www.kernel.org/doc/html/v5.2/process/coding-style.html

> 
> > And have you consider the case somebody triggers a reset through usbfs? It
> > looks to me like you should encapsulate the restoration of the MAC and also
> > use it in post_reset()
> >
> > 	Regards
> > 		Oliver
> 
> Will there need to be a pre_reset() function to go with the post_reset() you are
> suggesting?

It should match implementing what 25766271e42f6b15b72ba156cb42a3fea91b5b21
did for r8152.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
  2019-09-02  9:49 ` Oliver Neukum
@ 2019-09-03 17:50   ` Charles.Hyde
  2019-09-03 17:57     ` Mario.Limonciello
  0 siblings, 1 reply; 9+ messages in thread
From: Charles.Hyde @ 2019-09-03 17:50 UTC (permalink / raw)
  To: oneukum, lenb, rjw; +Cc: Mario.Limonciello, nic_swsd, linux-acpi, linux-usb

> > This change adds support to cdc_ncm for ACPI MAC address pass through
> > functionality that also exists in the Realtek r8152 driver.  This is
> > in support of Dell's Universal Dock D6000, to give it the same feature
> > capability as is currently available in Windows and advertized on
> > Dell's product web site.
> >
> > Signed-off-by: Charles Hyde <charles.hyde@dellteam.com>
> > Cc: Mario Limonciello <mario.limonciello@dell.com>
> > Cc: Oliver Neukum <oliver@neukum.org>
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: linux-usb@vger.kernel.org
> > Cc: linux-acpi@vger.kernel.org
> > ---
> >  drivers/net/usb/cdc_ncm.c | 67
> > +++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 64 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> > index 85093579612f..11a04dc2298d 100644
> > --- a/drivers/net/usb/cdc_ncm.c
> > +++ b/drivers/net/usb/cdc_ncm.c
> > @@ -52,6 +52,7 @@
> >  #include <linux/usb/usbnet.h>
> >  #include <linux/usb/cdc.h>
> >  #include <linux/usb/cdc_ncm.h>
> > +#include <acpi/acpi_mac_passthru.h>
> >
> >  #if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM)
> >  static bool prefer_mbim = true;
> > @@ -984,11 +985,30 @@ int cdc_ncm_bind_common(struct usbnet *dev,
> struct usb_interface *intf, u8 data_
> >  	usb_set_intfdata(ctx->control, dev);
> >
> >  	if (ctx->ether_desc) {
> > -		temp = usbnet_get_ethernet_addr(dev, ctx->ether_desc-
> >iMACAddress);
> > +		struct sockaddr sa;
> > +
> > +		temp = cdc_ncm_get_ethernet_address(dev, ctx);
> >  		if (temp) {
> >  			dev_dbg(&intf->dev, "failed to get mac address\n");
> >  			goto error2;
> >  		}
> > +
> > +		/* Check for a Dell Universal Dock D6000 before checking if
> > +		 * ACPI supports MAC address pass through.
> > +		 */
> > +		if (!strstr(dev->udev->product, "D6000"))
> > +			goto skip_acpi_mapt_in_bind;
> > +
> > +		if (get_acpi_mac_passthru(sa.sa_data) != 0)
> > +			goto skip_acpi_mapt_in_bind;
> > +
> > +		if (memcmp(dev->net->dev_addr, sa.sa_data, ETH_ALEN) == 0)
> > +			goto skip_acpi_mapt_in_bind;
> > +
> > +		if (cdc_ncm_set_ethernet_address(dev, &sa) == 0)
> > +			memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
> > +
> > +skip_acpi_mapt_in_bind:
> >  		dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net-
> >dev_addr);
> >  	}
> >
> > @@ -1716,6 +1736,47 @@ static void cdc_ncm_status(struct usbnet *dev,
> struct urb *urb)
> >  	}
> >  }
> >
> > +static int cdc_ncm_resume(struct usb_interface *intf) {
> > +	struct usbnet *dev = usb_get_intfdata(intf);
> > +	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
> > +	int ret;
> > +
> > +	ret = usbnet_resume(intf);
> > +	if (ret != 0)
> > +		goto error2;
> > +
> > +	if (ctx->ether_desc) {
> > +		struct sockaddr sa;
> > +
> > +		if (cdc_ncm_get_ethernet_address(dev, ctx)) {
> > +			dev_dbg(&intf->dev, "failed to get mac address\n");
> > +			goto error2;
> > +		}
> > +
> > +		/* Check for a Dell Universal Dock D6000 before checking if
> > +		 * ACPI supports MAC address pass through.
> > +		 */
> > +		if (!strstr(dev->udev->product, "D6000"))
> > +			goto skip_acpi_mapt_in_resume;
> 
> This is too ugly. Use a flag for the need to restore the MAC.

It is ugly to me, also, because of restrictions that Linux source cannot be longer than 80 columns.  I much prefer readability and maintainability over 80 column line length limits.   Besides, I have not used an 80 column keypunch machine in more than 35 years.

> And have you consider the case somebody triggers a reset through usbfs? It
> looks to me like you should encapsulate the restoration of the MAC and also
> use it in post_reset()
> 
> 	Regards
> 		Oliver

Will there need to be a pre_reset() function to go with the post_reset() you are suggesting?

Charles

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
  2019-08-30 19:38 Charles.Hyde
@ 2019-09-02  9:49 ` Oliver Neukum
  2019-09-03 17:50   ` Charles.Hyde
  0 siblings, 1 reply; 9+ messages in thread
From: Oliver Neukum @ 2019-09-02  9:49 UTC (permalink / raw)
  To: Charles.Hyde, lenb, rjw
  Cc: Mario.Limonciello, nic_swsd, linux-acpi, linux-usb

Am Freitag, den 30.08.2019, 19:38 +0000 schrieb
Charles.Hyde@dellteam.com:
> This change adds support to cdc_ncm for ACPI MAC address pass through
> functionality that also exists in the Realtek r8152 driver.  This is in
> support of Dell's Universal Dock D6000, to give it the same feature
> capability as is currently available in Windows and advertized on Dell's
> product web site.
> 
> Signed-off-by: Charles Hyde <charles.hyde@dellteam.com>
> Cc: Mario Limonciello <mario.limonciello@dell.com>
> Cc: Oliver Neukum <oliver@neukum.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-acpi@vger.kernel.org
> ---
>  drivers/net/usb/cdc_ncm.c | 67 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 85093579612f..11a04dc2298d 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -52,6 +52,7 @@
>  #include <linux/usb/usbnet.h>
>  #include <linux/usb/cdc.h>
>  #include <linux/usb/cdc_ncm.h>
> +#include <acpi/acpi_mac_passthru.h>
>  
>  #if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM)
>  static bool prefer_mbim = true;
> @@ -984,11 +985,30 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
>  	usb_set_intfdata(ctx->control, dev);
>  
>  	if (ctx->ether_desc) {
> -		temp = usbnet_get_ethernet_addr(dev, ctx->ether_desc->iMACAddress);
> +		struct sockaddr sa;
> +
> +		temp = cdc_ncm_get_ethernet_address(dev, ctx);
>  		if (temp) {
>  			dev_dbg(&intf->dev, "failed to get mac address\n");
>  			goto error2;
>  		}
> +
> +		/* Check for a Dell Universal Dock D6000 before checking if
> +		 * ACPI supports MAC address pass through.
> +		 */
> +		if (!strstr(dev->udev->product, "D6000"))
> +			goto skip_acpi_mapt_in_bind;
> +
> +		if (get_acpi_mac_passthru(sa.sa_data) != 0)
> +			goto skip_acpi_mapt_in_bind;
> +
> +		if (memcmp(dev->net->dev_addr, sa.sa_data, ETH_ALEN) == 0)
> +			goto skip_acpi_mapt_in_bind;
> +
> +		if (cdc_ncm_set_ethernet_address(dev, &sa) == 0)
> +			memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
> +
> +skip_acpi_mapt_in_bind:
>  		dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
>  	}
>  
> @@ -1716,6 +1736,47 @@ static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
>  	}
>  }
>  
> +static int cdc_ncm_resume(struct usb_interface *intf)
> +{
> +	struct usbnet *dev = usb_get_intfdata(intf);
> +	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
> +	int ret;
> +
> +	ret = usbnet_resume(intf);
> +	if (ret != 0)
> +		goto error2;
> +
> +	if (ctx->ether_desc) {
> +		struct sockaddr sa;
> +
> +		if (cdc_ncm_get_ethernet_address(dev, ctx)) {
> +			dev_dbg(&intf->dev, "failed to get mac address\n");
> +			goto error2;
> +		}
> +
> +		/* Check for a Dell Universal Dock D6000 before checking if
> +		 * ACPI supports MAC address pass through.
> +		 */
> +		if (!strstr(dev->udev->product, "D6000"))
> +			goto skip_acpi_mapt_in_resume;

This is too ugly. Use a flag for the need to restore the MAC.
And have you consider the case somebody triggers a reset through
usbfs? It looks to me like you should encapsulate the restoration
of the MAC and also use it in post_reset()

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
@ 2019-08-30 19:38 Charles.Hyde
  2019-09-02  9:49 ` Oliver Neukum
  0 siblings, 1 reply; 9+ messages in thread
From: Charles.Hyde @ 2019-08-30 19:38 UTC (permalink / raw)
  To: oliver, rjw, lenb; +Cc: linux-usb, linux-acpi, nic_swsd, Mario.Limonciello

This change adds support to cdc_ncm for ACPI MAC address pass through
functionality that also exists in the Realtek r8152 driver.  This is in
support of Dell's Universal Dock D6000, to give it the same feature
capability as is currently available in Windows and advertized on Dell's
product web site.

Signed-off-by: Charles Hyde <charles.hyde@dellteam.com>
Cc: Mario Limonciello <mario.limonciello@dell.com>
Cc: Oliver Neukum <oliver@neukum.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-usb@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
---
 drivers/net/usb/cdc_ncm.c | 67 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 85093579612f..11a04dc2298d 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -52,6 +52,7 @@
 #include <linux/usb/usbnet.h>
 #include <linux/usb/cdc.h>
 #include <linux/usb/cdc_ncm.h>
+#include <acpi/acpi_mac_passthru.h>
 
 #if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM)
 static bool prefer_mbim = true;
@@ -984,11 +985,30 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
 	usb_set_intfdata(ctx->control, dev);
 
 	if (ctx->ether_desc) {
-		temp = usbnet_get_ethernet_addr(dev, ctx->ether_desc->iMACAddress);
+		struct sockaddr sa;
+
+		temp = cdc_ncm_get_ethernet_address(dev, ctx);
 		if (temp) {
 			dev_dbg(&intf->dev, "failed to get mac address\n");
 			goto error2;
 		}
+
+		/* Check for a Dell Universal Dock D6000 before checking if
+		 * ACPI supports MAC address pass through.
+		 */
+		if (!strstr(dev->udev->product, "D6000"))
+			goto skip_acpi_mapt_in_bind;
+
+		if (get_acpi_mac_passthru(sa.sa_data) != 0)
+			goto skip_acpi_mapt_in_bind;
+
+		if (memcmp(dev->net->dev_addr, sa.sa_data, ETH_ALEN) == 0)
+			goto skip_acpi_mapt_in_bind;
+
+		if (cdc_ncm_set_ethernet_address(dev, &sa) == 0)
+			memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
+
+skip_acpi_mapt_in_bind:
 		dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
 	}
 
@@ -1716,6 +1736,47 @@ static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
 	}
 }
 
+static int cdc_ncm_resume(struct usb_interface *intf)
+{
+	struct usbnet *dev = usb_get_intfdata(intf);
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	int ret;
+
+	ret = usbnet_resume(intf);
+	if (ret != 0)
+		goto error2;
+
+	if (ctx->ether_desc) {
+		struct sockaddr sa;
+
+		if (cdc_ncm_get_ethernet_address(dev, ctx)) {
+			dev_dbg(&intf->dev, "failed to get mac address\n");
+			goto error2;
+		}
+
+		/* Check for a Dell Universal Dock D6000 before checking if
+		 * ACPI supports MAC address pass through.
+		 */
+		if (!strstr(dev->udev->product, "D6000"))
+			goto skip_acpi_mapt_in_resume;
+
+		if (get_acpi_mac_passthru(sa.sa_data) != 0)
+			goto skip_acpi_mapt_in_resume;
+
+		if (memcmp(dev->net->dev_addr, sa.sa_data, ETH_ALEN) == 0)
+			goto skip_acpi_mapt_in_resume;
+
+		if (cdc_ncm_set_ethernet_address(dev, &sa) == 0)
+			memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
+
+skip_acpi_mapt_in_resume:
+		dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
+	}
+
+error2:
+	return ret;
+}
+
 static const struct driver_info cdc_ncm_info = {
 	.description = "CDC NCM",
 	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
@@ -1848,8 +1909,8 @@ 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,
 	.supports_autosuspend = 1,
 	.disable_hub_initiated_lpm = 1,
 };
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-09-06 15:23 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-05 21:01 [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality Charles.Hyde
2019-09-06  2:13 ` Chunfeng Yun
2019-09-06 13:25   ` chip.programmer
2019-09-06  7:59 ` Bjørn Mork
2019-09-06 15:23   ` Dan Williams
  -- strict thread matches above, loose matches on Subject: below --
2019-08-30 19:38 Charles.Hyde
2019-09-02  9:49 ` Oliver Neukum
2019-09-03 17:50   ` Charles.Hyde
2019-09-03 17:57     ` Mario.Limonciello

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).