Re: [PATCH] r8152: limit MAC pass-through to one device

[Oliver Neukum]

On Mi, 2018-10-10 at 17:18 +, mario.limoncie...@dell.com wrote:
> > 
> > MAC address having to be unique, a MAC coming from the host
> > must be used at most once at a time. Hence the users must
> > be recorded and additional users must fall back to conventional
> > methods.
> 
> I checked with the internal team and actually applies pass through MAC on both
> Windows Realtek driver and in UEFI network stack this applies to ALL supported
> Realtek devices (R8153-AD).

I may have formulated this badly. What happens if you attach two
devices of this type to the same host?

Regards
Oliver

[PATCH] r8152: limit MAC pass-through to one device

[Oliver Neukum]

MAC address having to be unique, a MAC coming from the host
must be used at most once at a time. Hence the users must
be recorded and additional users must fall back to conventional
methods.

Signed-off-by: Oliver Neukum 
Fixes: 34ee32c9a5696 ("r8152: Add support for setting pass through MAC address 
on RTL8153-AD")
---
 drivers/net/usb/r8152.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f1b5201cc320..7345a2258ee4 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -766,6 +766,9 @@ enum tx_csum_stat {
TX_CSUM_NONE
 };
 
+/* pass through MACs are per host, hence concurrent use is forbidden */
+static struct r8152 *pass_through_user = NULL;
+
 /* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
  * The RTL chips use a 64 element hash table based on the Ethernet CRC.
  */
@@ -1221,7 +1224,14 @@ static int set_ethernet_addr(struct r8152 *tp)
 * or system doesn't provide valid _SB.AMAC this will be
 * be expected to non-zero
 */
-   ret = vendor_mac_passthru_addr_read(tp, );
+   if (!pass_through_user) {
+   ret = vendor_mac_passthru_addr_read(tp, );
+   if (ret >= 0)
+   /* we must record the user against concurrent 
use */
+   pass_through_user = tp;
+   } else {
+   ret = -EBUSY;
+   }
if (ret < 0)
ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
}
@@ -5304,6 +5314,8 @@ static void rtl8152_disconnect(struct usb_interface *intf)
cancel_delayed_work_sync(>hw_phy_work);
tp->rtl_ops.unload(tp);
free_netdev(tp->netdev);
+   if (pass_through_user == tp)
+   pass_through_user = NULL;
}
 }
 
-- 
2.16.4

Re: Hangs in r8152 connected to power management in kernels at least up v4.17-rc4

[Oliver Neukum]

Am Mittwoch, den 16.05.2018, 10:00 + schrieb Hayes Wang:
> Oliver Neukum [mailto:oneu...@suse.com]
> > Sent: Wednesday, May 16, 2018 4:27 PM
> 
> [...]
> > > 
> > > Would usb_autopm_get_interface() take a long time?
> > > The driver would wake the device if it has suspended.
> > > I have no idea about how usb_autopm_get_interface() works, so I don't know
> > 
> > how to help.
> > 
> > Hi,
> > 
> > it basically calls r8152_resume() and makes a control request to the
> > hub. I think we are spinning in rtl8152_runtime_resume(), but where?
> > It has a lot of NAPI stuff. Any suggestions on how to instrument or
> > trace this?
> 
> Is rtl8152_runtime_resume() called? I don't see the name in the trace.

Good question. I see nothing else that could produce a live lock.
> 
> I guess the relative API in rtl8152_runtime_resume() are
>   ops->disable= rtl8153_disable;
>   ops->autosuspend_en = rtl8153_runtime_enable;
> 
> And I don't find any possible dead lock in rtl8152_runtime_resume().
> 
> Besides, I find a similar issue as following.
> https://www.spinics.net/lists/netdev/msg493512.html

Well, if we have an imbalance in NAPI it should strike whereever
it is used, not just in suspend(). Is there debugging for NAPI
we could activate?

Regards
Oliver

Re: Hangs in r8152 connected to power management in kernels at least up v4.17-rc4

[Oliver Neukum]

Am Mittwoch, den 16.05.2018, 03:37 + schrieb Hayes Wang:
> Oliver Neukum [mailto:oneu...@suse.com]
> > 
> > Hi,
> > 
> > I got reports about hangs with this trace:
> > 
> > May 13 01:36:55 neroon kernel: INFO: task kworker/0:0:4 blocked for more
> > than 60 seconds.
> > May 13 01:36:55 neroon kernel:   Tainted: G U
> > 4.17.0-rc4-1.g8257a00-vanilla #1
> > May 13 01:36:55 neroon kernel: "echo 0 >
> > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> > May 13 01:36:55 neroon kernel: kworker/0:0 D0 4  2
> > 0x8000
> > May 13 01:36:55 neroon kernel: Workqueue: events rtl_work_func_t [r8152]
> > May 13 01:36:55 neroon kernel: Call Trace:
> > May 13 01:36:55 neroon kernel:  ? __schedule+0x289/0x880
> > May 13 01:36:55 neroon kernel:  schedule+0x2f/0x90
> > May 13 01:36:55 neroon kernel:  rpm_resume+0xf9/0x7a0
> > May 13 01:36:55 neroon kernel:  ? wait_woken+0x80/0x80
> > May 13 01:36:55 neroon kernel:  rpm_resume+0x547/0x7a0
> > May 13 01:36:55 neroon kernel:  ? __switch_to_asm+0x40/0x70
> > May 13 01:36:55 neroon kernel:  ? __switch_to_asm+0x34/0x70
> > May 13 01:36:55 neroon kernel:  ? __switch_to_asm+0x40/0x70
> > May 13 01:36:55 neroon kernel:  ? __switch_to_asm+0x34/0x70
> > May 13 01:36:55 neroon kernel:  ? __switch_to_asm+0x40/0x70
> > May 13 01:36:55 neroon kernel:  __pm_runtime_resume+0x3a/0x50
> > May 13 01:36:55 neroon kernel:  usb_autopm_get_interface+0x1d/0x50 [usbcore]
> 
> Would usb_autopm_get_interface() take a long time?
> The driver would wake the device if it has suspended.
> I have no idea about how usb_autopm_get_interface() works, so I don't know 
> how to help.

Hi,

it basically calls r8152_resume() and makes a control request to the
hub. I think we are spinning in rtl8152_runtime_resume(), but where?
It has a lot of NAPI stuff. Any suggestions on how to instrument or
trace this?

Regards
Oliver

Hangs in r8152 connected to power management in kernels at least up v4.17-rc4

[Oliver Neukum]

Hi,

I got reports about hangs with this trace:

May 13 01:36:55 neroon kernel: INFO: task kworker/0:0:4 blocked for more than 
60 seconds.
May 13 01:36:55 neroon kernel:   Tainted: G U
4.17.0-rc4-1.g8257a00-vanilla #1
May 13 01:36:55 neroon kernel: "echo 0 > 
/proc/sys/kernel/hung_task_timeout_secs" disables this message.
May 13 01:36:55 neroon kernel: kworker/0:0 D0 4  2 0x8000
May 13 01:36:55 neroon kernel: Workqueue: events rtl_work_func_t [r8152]
May 13 01:36:55 neroon kernel: Call Trace:
May 13 01:36:55 neroon kernel:  ? __schedule+0x289/0x880
May 13 01:36:55 neroon kernel:  schedule+0x2f/0x90
May 13 01:36:55 neroon kernel:  rpm_resume+0xf9/0x7a0
May 13 01:36:55 neroon kernel:  ? wait_woken+0x80/0x80
May 13 01:36:55 neroon kernel:  rpm_resume+0x547/0x7a0
May 13 01:36:55 neroon kernel:  ? __switch_to_asm+0x40/0x70
May 13 01:36:55 neroon kernel:  ? __switch_to_asm+0x34/0x70
May 13 01:36:55 neroon kernel:  ? __switch_to_asm+0x40/0x70
May 13 01:36:55 neroon kernel:  ? __switch_to_asm+0x34/0x70
May 13 01:36:55 neroon kernel:  ? __switch_to_asm+0x40/0x70
May 13 01:36:55 neroon kernel:  __pm_runtime_resume+0x3a/0x50
May 13 01:36:55 neroon kernel:  usb_autopm_get_interface+0x1d/0x50 [usbcore]
May 13 01:36:55 neroon kernel:  rtl_work_func_t+0x3c/0x27c [r8152]
May 13 01:36:55 neroon kernel:  process_one_work+0x1d4/0x3f0
May 13 01:36:55 neroon kernel:  worker_thread+0x2b/0x3d0
May 13 01:36:55 neroon kernel:  ? process_one_work+0x3f0/0x3f0
May 13 01:36:55 neroon kernel:  kthread+0x113/0x130
May 13 01:36:55 neroon kernel:  ? kthread_create_worker_on_cpu+0x50/0x50
May 13 01:36:55 neroon kernel:  ret_from_fork+0x3a/0x50

Any idea?

Regards
Oliver

Re: [PATCH] cdc_ether: flag the Cinterion AHS8 modem by gemalto as WWAN

[Oliver Neukum]

Am Mittwoch, den 11.04.2018, 13:15 +0200 schrieb Bassem Boubaker:
>     The Cinterion AHS8 is a 3G device with one embedded WWAN interface
>     using cdc_ether as a driver.
> 
>     The modem is  controlled via AT commands through the exposed TTYs.
> 
>     AT+CGDCONT write command can be used to activate or deactivate a WWAN
>     connection for a PDP context defined with the same command. UE supports
>     one WWAN adapter.
> 
> Signed-off-by: Bassem Boubaker <bassem.bouba...@actia.fr>
Acked-by: Oliver Neukum <oneu...@suse.com>

Re: [2/2] net/usb/ax88179_178a: Delete three unnecessary variables in ax88179_chk_eee()

[Oliver Neukum]

Am Dienstag, den 13.03.2018, 08:24 +0100 schrieb SF Markus Elfring:
> > 
> > > 
> > > Use three values directly for a condition check without assigning them
> > > to intermediate variables.
> > 
> > Hi,
> > 
> > what is the benefit of this?
> 
> I proposed a small source code reduction.
> 
> Other software design directions might become more interesting for this use 
> case.

Yes and doing so you killed three meaningful names that tell
us what these checks actually test for. That is not an improvement.

Regards
Oliver

Re: [PATCH 2/2] net/usb/ax88179_178a: Delete three unnecessary variables in ax88179_chk_eee()

[Oliver Neukum]

Am Samstag, den 10.03.2018, 19:26 +0100 schrieb SF Markus Elfring:
> From: Markus Elfring 
> Date: Sat, 10 Mar 2018 18:53:28 +0100
> 
> Use three values directly for a condition check without assigning them
> to intermediate variables.

Hi,

what is the benefit of this? It looks like needless code churn to me.

Regards
Oliver

Re: [PATCH net-next 2/2] net: cdc_eem: clean up bind error path

[Oliver Neukum]

Am Mittwoch, den 07.03.2018, 10:46 +0100 schrieb Johan Hovold:
> Drop bogus call to usb_driver_release_interface() from an error path in
> the usbnet bind() callback, which is called during interface probe. At
> this point the interface is not bound and usb_driver_release_interface()
> returns early.
> 
> Also remove the bogus call to clear the interface data, which is owned
> by the usbnet driver and would not even have been set by the time bind()
> is called.
> 
> Signed-off-by: Johan Hovold <jo...@kernel.org>
Acked-by: Oliver Neukum <oneu...@suse.com>
> ---
>  drivers/net/usb/cdc_eem.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/net/usb/cdc_eem.c b/drivers/net/usb/cdc_eem.c
> index f7180f8db39e..61ea4eaace5d 100644
> --- a/drivers/net/usb/cdc_eem.c
> +++ b/drivers/net/usb/cdc_eem.c
> @@ -83,11 +83,8 @@ static int eem_bind(struct usbnet *dev, struct 
> usb_interface *intf)
>   int status = 0;
>  
>   status = usbnet_get_endpoints(dev, intf);
> - if (status < 0) {
> - usb_set_intfdata(intf, NULL);
> - usb_driver_release_interface(driver_of(intf), intf);
> + if (status < 0)
>   return status;
> - }
>  
>   /* no jumbogram (16K) support for now */
>  

Re: inconsistent lock state with usbnet/asix usb ethernet and xhci

[Oliver Neukum]

On Mon, 2018-03-05 at 08:45 +0100, Marek Szyprowski wrote:
> Hi Oliver,
> 
> On 2018-02-27 17:07, Oliver Neukum wrote:
> > Am Dienstag, den 27.02.2018, 07:13 -0800 schrieb Eric Dumazet:
> >> On Tue, 2018-02-27 at 07:09 -0800, Eric Dumazet wrote:
> >>>
> >>> Note that for this one, it seems we also could perform stats updates in
> >>> BH context, since skb is queued via defer_bh()
> >>>
> >>> But simplicity wins I guess.
> >> Thinking more about this, I am not sure we have any guarantee that TX
> >> and RX can not run on multiple cpus.
> >>
> >> Using an unique syncp is not going to be safe, even if we make lockdep
> >> happy enough with the local_irq save/restore.
> > Unfortunately you are right. It is not guaranteed for some hardware.
> 
> Does it mean that the fix proposed by Eric is not the proper solution?

For asix it should work, but asix is unlikely to be the only driver
with that issue. 32 bit recieves less testing nowadays.

Regards
Oliver

Re: inconsistent lock state with usbnet/asix usb ethernet and xhci

[Oliver Neukum]

Am Dienstag, den 27.02.2018, 07:13 -0800 schrieb Eric Dumazet:
> On Tue, 2018-02-27 at 07:09 -0800, Eric Dumazet wrote:
> > 
> > 
> > Note that for this one, it seems we also could perform stats updates in
> > BH context, since skb is queued via defer_bh()
> > 
> > But simplicity wins I guess.
> 
> Thinking more about this, I am not sure we have any guarantee that TX
> and RX can not run on multiple cpus.
> 
> Using an unique syncp is not going to be safe, even if we make lockdep
> happy enough with the local_irq save/restore.

Unfortunately you are right. It is not guaranteed for some hardware.

Regards
Oliver

Re: [PATCH] cdc_ether: flag the Cinterion PLS8 modem by gemalto as WWAN

[Oliver Neukum]

Am Dienstag, den 27.02.2018, 14:04 +0100 schrieb Bassem Boubaker:
>     The Cinterion PL8 is an LTE modem with 2 possible WWAN interfaces.
> 
>     The modem is  controlled via AT commands through the exposed TTYs.
> 
>     AT^SWWAN write command can be used to activate or deactivate a WWAN
>     connection for a PDP context defined with AT+CGDCONT. UE supports
>     two WWAN adapter. Both WWAN adapters can be activated a the same time
> 
> Signed-off-by: Bassem Boubaker <bassem.bouba...@actia.fr>
Acked-by: Oliver Neukum <oneu...@suse.com>

Re: [PATCH] cdc_ether: flag the Cinterion PLS8 modem by gemalto as WWAN

[Oliver Neukum]

Am Dienstag, den 27.02.2018, 13:29 +0100 schrieb Bjørn Mork :
> Bassem Boubaker  writes:
> 
> > 
> > +#define GEMALTO_VENDOR_ID  0x1e2d
> 
> This is defined as CINTERION_VENDOR_ID in drivers/usb/serial/option.c.
> 
> I have no idea which defintion is most correct, but I believe the macros
> should be kept identical whatever you decide. Anything else is just
> unnecessarily confusing.
> 
> IMHO the company name tracking macros have grown beyond useful a long
> time ago. They just make it harder to grep for the IDs without adding
> any useful information whatsoever. And because you have cases like this
> where the same number end up having different names, they sometimes hide
> information which a plain number would have revealed.

Hi,

I concur. Could you redo the patch and just use a plain number?

Regards
Oliver

Re: inconsistent lock state with usbnet/asix usb ethernet and xhci

[Oliver Neukum]

Am Dienstag, den 27.02.2018, 08:26 +0100 schrieb Marek Szyprowski:

Hi,

> I've noticed that USBnet/ASIX AX88772B USB driver produces deplock kernel
> warning ("inconsistent lock state") on Chromebook2 Peach-PIT board. No

Is that 32 bit?

> special activity is needed to reproduce this issue, it happens almost
> on every boot. ASIX USB ethernet is connected to XHCI USB host controller
> on that board. Is it a known issue? Frankly I have no idea where to look

No, it is not known.

> to fix it. The same adapter connected to EHCI ports on other boards based
> on the same SoC works fine without any warnings.

Odd.

> And the log with mentioned warning:
> 
> [   17.768040] 
> [   17.772239] WARNING: inconsistent lock state
> [   17.776511] 4.16.0-rc3-next-20180227-7-g876c53a7493c #453 Not tainted
> [   17.783329] 
> [   17.787580] inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
> [   17.793607] swapper/0/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
> [   17.798751]  (>seq#5){?.-.}, at: [<9b22e5f0>] 
> asix_rx_fixup_internal+0x188/0x288

Looks like this triggers (in usbnet):

        u64_stats_update_begin(>syncp);
stats64->rx_packets++;
stats64->rx_bytes += skb->len;
u64_stats_update_end(>syncp);

That I considered to be called under lock.
Could you comment this out for testing?

Regards
Oliver

[PATCHv2] usbnet: silence an unnecessary warning

[Oliver Neukum]

That a kevent could not be scheduled is not an error.
Such handlers must be able to deal with multiple events anyway.
As the successful scheduling of a work is a debug event, make
the failure debug priority, too.

V2: coding style

Signed-off-by: Oliver Neukum <oneu...@suse.com>
Reported-by: Cristian Caravena <carav...@gmail.com>
---
 drivers/net/usb/usbnet.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index d56fe32bf48d..8a22ff67b026 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -457,12 +457,10 @@ static enum skb_state defer_bh(struct usbnet *dev, struct 
sk_buff *skb,
 void usbnet_defer_kevent (struct usbnet *dev, int work)
 {
set_bit (work, >flags);
-   if (!schedule_work (>kevent)) {
-   if (net_ratelimit())
-   netdev_err(dev->net, "kevent %d may have been 
dropped\n", work);
-   } else {
+   if (!schedule_work (>kevent))
+   netdev_dbg(dev->net, "kevent %d may have been dropped\n", work);
+   else
netdev_dbg(dev->net, "kevent %d scheduled\n", work);
-   }
 }
 EXPORT_SYMBOL_GPL(usbnet_defer_kevent);
 
-- 
2.13.6

Re: [PATCH] usbnet: silence an unnecessary warning

[Oliver Neukum]

Am Donnerstag, den 11.01.2018, 16:23 +0100 schrieb Bjørn Mork :
> Oliver Neukum <oneu...@suse.com> writes:
> 
> > 
> > That a kevent could not be scheduled is not an error.
> > Such handlers must be able to deal with multiple events anyway.
> > As the successful scheduling of a work is a debug event, make
> > the failure debug priority, too.
> > 
> > Signed-off-by: Oliver Neukum <oneu...@suse.com>
> > Reported-by: Cristian Caravena <carav...@gmail.com>
> > ---
> >  drivers/net/usb/usbnet.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> > index d56fe32bf48d..1e0bbe23f95c 100644
> > --- a/drivers/net/usb/usbnet.c
> > +++ b/drivers/net/usb/usbnet.c
> > @@ -458,8 +458,7 @@ void usbnet_defer_kevent (struct usbnet *dev, int work)
> >  {
> > set_bit (work, >flags);
> > if (!schedule_work (>kevent)) {
> > -   if (net_ratelimit())
> > -   netdev_err(dev->net, "kevent %d may have been 
> > dropped\n", work);
> > +   netdev_dbg(dev->net, "kevent %d may have been dropped\n", work);
> > } else {
> > netdev_dbg(dev->net, "kevent %d scheduled\n", work);
> > }
> 
> Great!  But why do you drop the ratelimit?  This can be very noisy when
> it hits.  I'd like to keep it ratelimited.

Because this is now a debug output and if you need to debug you may need
to verify whether your kevent will be handled. So not getting either of the
messages would indicate a bug. Thus limiting the rate would defeat the purpose.

Regards
Oliver

[PATCH] usbnet: silence an unnecessary warning

[Oliver Neukum]

That a kevent could not be scheduled is not an error.
Such handlers must be able to deal with multiple events anyway.
As the successful scheduling of a work is a debug event, make
the failure debug priority, too.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
Reported-by: Cristian Caravena <carav...@gmail.com>
---
 drivers/net/usb/usbnet.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index d56fe32bf48d..1e0bbe23f95c 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -458,8 +458,7 @@ void usbnet_defer_kevent (struct usbnet *dev, int work)
 {
set_bit (work, >flags);
if (!schedule_work (>kevent)) {
-   if (net_ratelimit())
-   netdev_err(dev->net, "kevent %d may have been 
dropped\n", work);
+   netdev_dbg(dev->net, "kevent %d may have been dropped\n", work);
} else {
netdev_dbg(dev->net, "kevent %d scheduled\n", work);
}
-- 
2.13.6

Re: [BUG] kaweth: a possible sleep-in-atomic bug in kaweth_start_xmit

[Oliver Neukum]

Am Mittwoch, den 13.12.2017, 16:57 +0800 schrieb Jia-Ju Bai:
> According to drivers/net/usb/kaweth.c, the driver may sleep under a 
> spinlock.
> The function call path is:
> kaweth_start_xmit (acquire the spinlock)
>kaweth_async_set_rx_mode
>  kaweth_control
>kaweth_internal_control_msg
>  usb_start_wait_urb
>wait_event_timeout --> may sleep
>usb_kill_urb --> may sleep
> 
> I do not find a good way to fix it, so I only report.
> This possible bug is found by my static analysis tool (DSAC) and checked 
> by my code review.
> 

Hi,

thanks for reporting. I need to get out my old test device.
It will take a few days. The obvious fix would be to set this
filter only on initialization. Unfortunately this needs to
be tested.

Regards
Oliver

Re: [PATCH] net: usb: asix: fill null-ptr-deref in asix_suspend

[Oliver Neukum]

Am Montag, den 06.11.2017, 17:05 +0100 schrieb Andrey Konovalov:
> On Mon, Nov 6, 2017 at 4:20 PM, Oliver Neukum <oneu...@suse.com> wrote:
> > 

> I do have a way to reproduce this.
> 
> As far as I understand, for this particular device ax88172_bind() is
> called, which doesn't assign anything to dev->driver_priv, so that's
> why it is NULL in suspend() and resume().

Thanks and ouch. That means it never worked for those devices.
That makes this a rather serious bug and your fix is right.

Regards
Oliver

Re: [PATCH] net: usb: asix: fill null-ptr-deref in asix_suspend

[Oliver Neukum]

Am Montag, den 06.11.2017, 13:30 +0100 schrieb Andrey Konovalov:
> On Mon, Nov 6, 2017 at 10:49 AM, Oliver Neukum <oneu...@suse.com> wrote:
> > 
> > 
> > 2. Will a device work after that? The appropriate fix may be to wait
> > until the device is properly initialized.
> 
> This shouldn't affect real devices as far as I understand. The crash
> can be caused by a crafted malicious device.

Hi!

Hm. That seems strange as driver_priv is kmalloced. Do you
still have a descriptor that causes this?
Shouldn't we rather reject such a broken device?

Regards
Oliver

Re: [PATCH net,stable] net: cdc_ether: fix divide by 0 on bad descriptors

[Oliver Neukum]

Am Montag, den 06.11.2017, 15:37 +0100 schrieb Bjørn Mork :
> Setting dev->hard_mtu to 0 will cause a divide error in
> usbnet_probe. Protect against devices with bogus CDC Ethernet
> functional descriptors by ignoring a zero wMaxSegmentSize.
> 
> Signed-off-by: Bjørn Mork <bj...@mork.no>
Acked-by: Oliver Neukum <oneu...@suse.com>

Re: [PATCH] net: usb: asix: fill null-ptr-deref in asix_suspend

[Oliver Neukum]

Am Donnerstag, den 02.11.2017, 21:26 +0100 schrieb Andrey Konovalov:
> When asix_suspend() is called dev->driver_priv might not have been
> assigned a value, so we need to check that it's not NULL.
> 
> Found by syzkaller.

Hi,

1. if that happens on suspend, it will also happen on resume
2. Will a device work after that? The appropriate fix may be to wait
until the device is properly initialized.

Regards
Oliver

Re: [PATCH] drivers/net/usb: add device id for TP-LINK UE300 USB 3.0 Ethernet

[Oliver Neukum]

Am Montag, den 23.10.2017, 18:10 +0800 schrieb Ran Wang:
> This product is named 'TP-LINK USB 3.0 Gigabit Ethernet Network
> Adapter (Model No.is UE300)'. It uses chip RTL8153 and works with
> driver drivers/net/usb/r8152.c
> 

Hi,

just for the record, have you confirm that it fails with cdc-ether?

Regards
Oliver

Re: [PATCH 07/58] net/usb/usbnet: Convert timers to use timer_setup()

[Oliver Neukum]

Am Montag, den 16.10.2017, 17:28 -0700 schrieb Kees Cook:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly. Since the callback is called from
> both a timer and a tasklet, adjust the tasklet to pass the timer address
> too. When tasklets have their .data field removed, this can be refactored
> to call a central function after resolving the correct container_of() for a
> separate callback function for timer and tasklet.
> 
> Cc: Oliver Neukum <oneu...@suse.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Kees Cook <keesc...@chromium.org>
Acked-by: Oliver Neukum <oneu...@suse.com>

Re: [PATCH] rndis_host: support Novatel Verizon USB730L

[Oliver Neukum]

Am Dienstag, den 03.10.2017, 16:01 +0200 schrieb Bjørn Mork:
> Adding anything else, e.g. based on the table at
> http://www.usb.org/developers/defined_class/#BaseClassEFh , is a bit
> more risky.  We don't know if a driver will work with *any* such device
> until we've actually seen one.
> 
> This is just my opinion, and probably full of bogus assumptions as
> usual.  I was sort of hoping that some expert would speak up so I didn't
> have to :-)

Outside the vendors, there is nobody.

Regards
Oliver

Re: [PATCH V2] r8152: add Linksys USB3GIGV1 id

[Oliver Neukum]

Am Dienstag, den 26.09.2017, 08:19 -0700 schrieb Doug Anderson:
> 
> I know that for at least some of the adapters in the CDC Ethernet
> blacklist it was claimed that the CDC Ethernet support in the adapter
> was kinda broken anyway so the blacklist made sense.  ...but for the
> Linksys Gigabit adapter the CDC Ethernet driver seems to work OK, it's
> just not quite as full featured / efficient as the R8152 driver.
> 
> Is that not a concern?  I guess you could tell people in this
> situation that they simply need to enable the R8152 driver to get
> continued support for their Ethernet adapter?

Hi,

yes, it is a valid concern. An #ifdef will be needed.

Regards
Oliver

Re: [PATCH] r8152: add Linksys USB3GIGV1 id

[Oliver Neukum]

Am Freitag, den 22.09.2017, 12:06 -0700 schrieb Grant Grundler:
> This Linksys dongle by default comes up in cdc_ether mode.
> This patch allows r8152 to claim the device:
>    Bus 002 Device 002: ID 13b1:0041 Linksys

Hi,

have you tested this in case cdc_ether is for some reason already
loaded? The patch seems to enable r8152 but does not disable
cdc_ether.

Regards
Oliver

Re: [RFC PATCH 2/3] usbnet: Avoid potential races in usbnet_deferred_kevent()

[Oliver Neukum]

Am Dienstag, den 19.09.2017, 13:53 -0700 schrieb Doug Anderson:
> Hi,
> 
> On Tue, Sep 19, 2017 at 1:37 PM, Oliver Neukum <oneu...@suse.com> wrote:
> > 
> > Am Dienstag, den 19.09.2017, 09:15 -0700 schrieb Douglas Anderson:
> > > 
> > > In general when you've got a flag communicating that "something needs
> > > to be done" you want to clear that flag _before_ doing the task.  If
> > > you clear the flag _after_ doing the task you end up with the risk
> > > that this will happen:
> > > 
> > > 1. Requester sets flag saying task A needs to be done.
> > > 2. Worker comes and stars doing task A.
> > > 3. Worker finishes task A but hasn't yet cleared the flag.
> > > 4. Requester wants to set flag saying task A needs to be done again.
> > > 5. Worker clears the flag without doing anything.
> > > 
> > > Let's make the usbnet codebase consistently clear the flag _before_ it
> > > does the requested work.  That way if there's another request to do
> > > the work while the work is already in progress it won't be lost.
> > > 
> > > NOTES:
> > > - No known bugs are fixed by this; it's just found by code inspection.
> > 
> > Hi,
> > 
> > unfortunately the patch is wrong. The flags must be cleared only
> > in case the handler is successful. That is not guaranteed.
> > 
> > Regards
> > Oliver
> > 
> > NACK
> 
> OK, thanks for reviewing!  I definitely wasn't super confident about
> the patch (hence the RFC).
> 
> Do you think that the races I identified are possible to hit?  In

As far as I can tell, we are safe, but you are right to say that the
driver is not quite clean at that point.

> other words: should I try to rework the patch somehow or just drop it?
>  Originally I had the patch setting the flags back to true in the
> failure cases, but then I convinced myself that wasn't needed.  I can
> certainly go back and try it that way...

Setting the flags again in the error case would certainly be an
improvement. I'd be happy with a patch doing that.

Regards
Oliver

Re: [RFC PATCH 2/3] usbnet: Avoid potential races in usbnet_deferred_kevent()

[Oliver Neukum]

Am Dienstag, den 19.09.2017, 13:51 -0700 schrieb Guenter Roeck:
> On Tue, Sep 19, 2017 at 1:37 PM, Oliver Neukum <oneu...@suse.com> wrote:
> > 
> > Am Dienstag, den 19.09.2017, 09:15 -0700 schrieb Douglas Anderson:
> > > 
[..]
> > > NOTES:
> > > - No known bugs are fixed by this; it's just found by code inspection.
> > 
> > Hi,
> > 
> > unfortunately the patch is wrong. The flags must be cleared only
> > in case the handler is successful. That is not guaranteed.
> > 
> 
> Just out of curiosity, what is the retry mechanism ? Whenever a new,
> possibly unrelated, event is scheduled ?

Hi,

that actually depends on the flag.
Look at the case of fail_lowmem. There we reschedule.

HTH
Oliver

Re: [RFC PATCH 2/3] usbnet: Avoid potential races in usbnet_deferred_kevent()

[Oliver Neukum]

Am Dienstag, den 19.09.2017, 09:15 -0700 schrieb Douglas Anderson:
> In general when you've got a flag communicating that "something needs
> to be done" you want to clear that flag _before_ doing the task.  If
> you clear the flag _after_ doing the task you end up with the risk
> that this will happen:
> 
> 1. Requester sets flag saying task A needs to be done.
> 2. Worker comes and stars doing task A.
> 3. Worker finishes task A but hasn't yet cleared the flag.
> 4. Requester wants to set flag saying task A needs to be done again.
> 5. Worker clears the flag without doing anything.
> 
> Let's make the usbnet codebase consistently clear the flag _before_ it
> does the requested work.  That way if there's another request to do
> the work while the work is already in progress it won't be lost.
> 
> NOTES:
> - No known bugs are fixed by this; it's just found by code inspection.

Hi,

unfortunately the patch is wrong. The flags must be cleared only
in case the handler is successful. That is not guaranteed.

Regards
Oliver

NACK

Re: [RFC PATCH 1/3] usbnet: Get rid of spammy usbnet "kevent X may have been dropped"

[Oliver Neukum]

Am Dienstag, den 19.09.2017, 09:15 -0700 schrieb Douglas Anderson:
> 
> ALSO NOTE: If somehow some of the types of work need to be repeated if
> usbnet_defer_kevent() is called multiple times then that should be
> quite easy to accomplish without dropping any work on the floor.  We
> can just keep an atomic count for that type of work and add a loop
> into usbnet_deferred_kevent().

Thanks for doing this, it is overdue.

Regards
Oliver

> Signed-off-by: Douglas Anderson <diand...@chromium.org>
Acked-by: Oliver Neukum <oneu...@suse.com>

Re: [PATCH V3] Set NTB format again after altsetting switch for Huawei devices

[Oliver Neukum]

Am Dienstag, den 11.07.2017, 17:21 +0200 schrieb Enrico Mioso:
> 
> Some firmwares in Huawei E3372H devices have been observed to switch back
> to NTB 32-bit format after altsetting switch.
> This patch implements a driver flag to check for the device settings and
> set NTB format to 16-bit again if needed.
> The flag has been activated for devices controlled by the huawei_cdc_ncm.c
> driver.

Hi,

does this cover reset_resume()?

Regards
Oliver

Re: [PATCH net-next 1/2] r8152: split rtl8152_resume function

[Oliver Neukum]

Am Montag, den 12.06.2017, 16:21 +0800 schrieb Hayes Wang:
> Split rtl8152_resume() into rtl8152_runtime_resume() and
> rtl8152_system_resume().
> 
> Signed-off-by: Hayes Wang 
> ---
>  drivers/net/usb/r8152.c | 99 
> ++---
>  1 file changed, 61 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 5a02053..3257955 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -3686,6 +3686,61 @@ static bool delay_autosuspend(struct r8152 *tp)
> return false;
>  }
>  
> +static int rtl8152_runtime_resume(struct r8152 *tp)
> +{
> +   struct net_device *netdev = tp->netdev;
> +
> +   if (netif_running(netdev) && netdev->flags & IFF_UP) {
> +   struct napi_struct *napi = >napi;
> +
> +   tp->rtl_ops.autosuspend_en(tp, false);
> +   napi_disable(napi);
> +   set_bit(WORK_ENABLE, >flags);
> +
> +   if (netif_carrier_ok(netdev)) {
> +   if (rtl8152_get_speed(tp) & LINK_STATUS) {
> +   rtl_start_rx(tp);
> +   } else {
> +   netif_carrier_off(netdev);
> +   tp->rtl_ops.disable(tp);
> +   netif_info(tp, link, netdev, "linking 
> down\n");
> +   }
> +   }
> +
> +   napi_enable(napi);
> +   clear_bit(SELECTIVE_SUSPEND, >flags);
> +   smp_mb__after_atomic();
> +
> +   if (!list_empty(>rx_done))
> +   napi_schedule(>napi);
> +
> +   usb_submit_urb(tp->intr_urb, GFP_KERNEL);

If you ever built a device with included storage, this can deadlock,
as you may want to wake up a device for memory that is needed to wake
up a device. Use GFP_NOIO in resume() and reset_resume(), always.

Regards
Oliver

[PATCH] r8152: give the device version

[Oliver Neukum]

Getting the device version out of the driver really aids debugging.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/r8152.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ddc62cb69be8..1a419a45e2a2 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -4368,6 +4368,8 @@ static u8 rtl_get_version(struct usb_interface *intf)
break;
}
 
+   dev_dbg(>dev, "Detected version 0x%04x\n", version);
+
return version;
 }
 
-- 
2.12.3

Re: [RFC V1 1/1] net: cdc_ncm: Reduce memory use when kernel memory low

[Oliver Neukum]

Am Montag, den 22.05.2017, 11:54 -0400 schrieb David Miller:
> 
> Unfortunately without a real notifier of some sort (there isn't one, and
> it isn't actually easy to come up with a clean way to do this which is
> probably why it doesn't exist yet in the first place) I really cannot
> recommend anything better.
> 
> That being said, probably for the time being we should just backoff each
> and every request, always trying initially to do the higher order thing.

We could use a counter. After the first failure, do it once, after the
second twice and so on. And reset the counter as a higher order
allocation works. (just bound it somewhere)

Regards
Oliver

Re: [RFC V1 1/1] net: cdc_ncm: Reduce memory use when kernel memory low

[Oliver Neukum]

Am Freitag, den 19.05.2017, 14:46 + schrieb David Laight:
> For XHCI it isn't too bad because it will do arbitrary scatter-gather
> (apart from the ring end).
> But I believe the earlier controllers only support fragments that
> end on usb packet boundaries.
> 
> I looked at the usbnet code a few years ago, I'm sure it ought to
> be possible to shortcut most of the code that uses URB and directly
> write to the xhci (in particular) ring descriptors.

Hi,

we cannot break the layering. URBs can support scatter/gather and
that infrastructure must be used. And usbnet will work with sg used
in skbs. What you should honor in general is not splitting packets.
So 512 byte chunks.

Regards
Oliver

[PATCH v2] cdc-ether: divorce initialisation with a filter reset and a generic method

[Oliver Neukum]

Some devices need their multicast filter reset but others are crashed by that.
So the methods need to be separated.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
Reported-by: "Ridgway, Keith" <kridg...@harris.com>
---
 drivers/net/usb/cdc_ether.c | 31 ---
 include/linux/usb/usbnet.h  |  1 +
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index f3ae88fdf332..8ab281b478f2 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -310,6 +310,26 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct 
usb_interface *intf)
return -ENODEV;
}
 
+   return 0;
+
+bad_desc:
+   dev_info(>udev->dev, "bad CDC descriptors\n");
+   return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(usbnet_generic_cdc_bind);
+
+
+/* like usbnet_generic_cdc_bind() but handles filter initialization
+ * correctly
+ */
+int usbnet_ether_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+   int rv;
+
+   rv = usbnet_generic_cdc_bind(dev, intf);
+   if (rv < 0)
+   goto bail_out;
+
/* Some devices don't initialise properly. In particular
 * the packet filter is not reset. There are devices that
 * don't do reset all the way. So the packet filter should
@@ -317,13 +337,10 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct 
usb_interface *intf)
 */
usbnet_cdc_update_filter(dev);
 
-   return 0;
-
-bad_desc:
-   dev_info(>udev->dev, "bad CDC descriptors\n");
-   return -ENODEV;
+bail_out:
+   return rv;
 }
-EXPORT_SYMBOL_GPL(usbnet_generic_cdc_bind);
+EXPORT_SYMBOL_GPL(usbnet_ether_cdc_bind);
 
 void usbnet_cdc_unbind(struct usbnet *dev, struct usb_interface *intf)
 {
@@ -417,7 +434,7 @@ int usbnet_cdc_bind(struct usbnet *dev, struct 
usb_interface *intf)
BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data)
< sizeof(struct cdc_state)));
 
-   status = usbnet_generic_cdc_bind(dev, intf);
+   status = usbnet_ether_cdc_bind(dev, intf);
if (status < 0)
return status;
 
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 7dffa5624ea6..97116379db5f 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -206,6 +206,7 @@ struct cdc_state {
 };
 
 extern int usbnet_generic_cdc_bind(struct usbnet *, struct usb_interface *);
+extern int usbnet_ether_cdc_bind(struct usbnet *dev, struct usb_interface 
*intf);
 extern int usbnet_cdc_bind(struct usbnet *, struct usb_interface *);
 extern void usbnet_cdc_unbind(struct usbnet *, struct usb_interface *);
 extern void usbnet_cdc_status(struct usbnet *, struct urb *);
-- 
2.12.0

Re: [PATCH 2/2] usbnet: Improve a size determination in usbnet_write_cmd_async()

[Oliver Neukum]

Am Montag, den 22.05.2017, 07:10 +0200 schrieb SF Markus Elfring:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Mon, 22 May 2017 06:42:33 +0200
> 
> Replace the specification of a data structure by a pointer dereference
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
Acked-by: Oliver Neukum <oneu...@suse.com>

Re: [PATCH 1/2] usbnet: Delete an error message for a failed memory allocation in usbnet_write_cmd_async()

[Oliver Neukum]

Am Montag, den 22.05.2017, 07:09 +0200 schrieb SF Markus Elfring:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Mon, 22 May 2017 06:33:48 +0200
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Link: 
> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
Acked-by: Oliver Neukum <oneu...@suse.com>

[RFC]RNDIS filter issue

[Oliver Neukum]

Hi,

now, as this patch is conceptually not my thing, here for review.

Regards
Oliver
From f8f775817b6f3bd636e3b45cb34db4af279492ff Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneu...@suse.com>
Date: Wed, 17 May 2017 11:40:12 +0200
Subject: [PATCH] cdc-ether: divorce initialisation with a filter reset and a
 generic method

Some devices need their multicast filter reset but others are crashed by that.
So the methods need to be separated.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
Reported-by: "Ridgway, Keith" <kridg...@harris.com>
---
 drivers/net/usb/cdc_ether.c | 32 
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index f3ae88fdf332..311f6df57316 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -310,6 +310,26 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 		return -ENODEV;
 	}
 
+	return 0;
+
+bad_desc:
+	dev_info(>udev->dev, "bad CDC descriptors\n");
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(usbnet_generic_cdc_bind);
+
+
+/* like usbnet_generic_cdc_bind() but handles filter initialization
+ * correctly
+ */
+int usbnet_ether_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	int rv;
+
+	rv = usbnet_generic_cdc_bind(dev, intf);
+	if (rv < 0)
+		goto bail_out;
+
 	/* Some devices don't initialise properly. In particular
 	 * the packet filter is not reset. There are devices that
 	 * don't do reset all the way. So the packet filter should
@@ -317,13 +337,10 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 	 */
 	usbnet_cdc_update_filter(dev);
 
-	return 0;
-
-bad_desc:
-	dev_info(>udev->dev, "bad CDC descriptors\n");
-	return -ENODEV;
+bail_out:
+	return rv;
 }
-EXPORT_SYMBOL_GPL(usbnet_generic_cdc_bind);
+EXPORT_SYMBOL_GPL(usbnet_ether_cdc_bind);
 
 void usbnet_cdc_unbind(struct usbnet *dev, struct usb_interface *intf)
 {
@@ -417,7 +434,7 @@ int usbnet_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 	BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data)
 			< sizeof(struct cdc_state)));
 
-	status = usbnet_generic_cdc_bind(dev, intf);
+	status = usbnet_ether_cdc_bind(dev, intf);
 	if (status < 0)
 		return status;
 
@@ -472,7 +489,6 @@ static void usbnet_cdc_zte_status(struct usbnet *dev, struct urb *urb)
 
 	if (urb->actual_length < sizeof(*event))
 		return;
-
 	event = urb->transfer_buffer;
 
 	if (event->bNotificationType != USB_CDC_NOTIFY_NETWORK_CONNECTION) {
-- 
2.12.0

Re: [PATCH] cdc-ether: divorce initialisation with a filter reset and a generic method

[Oliver Neukum]

Am Donnerstag, den 18.05.2017, 12:09 +0200 schrieb Bjørn Mork:
> Oliver Neukum <oneu...@suse.com> writes:
> 
> > 
> > @@ -417,7 +434,7 @@ int usbnet_cdc_bind(struct usbnet *dev, struct 
> > usb_interface *intf)
> > BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data)
> > < sizeof(struct cdc_state)));
> >  
> > -   status = usbnet_generic_cdc_bind(dev, intf);
> > +   status = usbnet_ether_cdc_bind(dev, intf);
> > if (status < 0)
> > return status;
> >  
> > @@ -472,7 +489,6 @@ static void usbnet_cdc_zte_status(struct usbnet *dev, 
> > struct urb *urb)
> >  
> > if (urb->actual_length < sizeof(*event))
> > return;
> > -
> > event = urb->transfer_buffer;
> >  
> > if (event->bNotificationType != USB_CDC_NOTIFY_NETWORK_CONNECTION) {
> > @@ -493,7 +509,7 @@ static void usbnet_cdc_zte_status(struct usbnet *dev, 
> > struct urb *urb)
> >  static const struct driver_infocdc_info = {
> > .description =  "CDC Ethernet Device",
> > .flags =FLAG_ETHER | FLAG_POINTTOPOINT,
> > -   .bind = usbnet_cdc_bind,
> > +   .bind = usbnet_ether_cdc_bind,
> > .unbind =   usbnet_cdc_unbind,
> > .status =   usbnet_cdc_status,
> > .set_rx_mode =  usbnet_cdc_update_filter,
> 
> 
> I didn't quite get this.  You change the call in usbnet_cdc_bind() from
> usbnet_generic_cdc_bind() to usbnet_ether_cdc_bind(), which I believe is
> fine.  But then you update the .bind hook to point to usbnet_ether_cdc_bind.
> Why?  The only effect I can see is that usbnet_get_ethernet_addr() is
> skipped. That can't be correct?

You are right. I am an idiot. I wanted to target RNDIS and missed totally.
Thanks.

Dave please trash the patch.

Regards
Oliver

Re: [RFC V1 1/1] net: cdc_ncm: Reduce memory use when kernel memory low

[Oliver Neukum]

Am Mittwoch, den 17.05.2017, 14:18 -0400 schrieb David Miller:
> From: Bjørn Mork 
> Date: Tue, 16 May 2017 20:24:10 +0200
> 
> > Jim Baxter  writes:
> > 
> >> The CDC-NCM driver can require large amounts of memory to create
> >> skb's and this can be a problem when the memory becomes fragmented.
> >>
> >> This especially affects embedded systems that have constrained
> >> resources but wish to maximise the throughput of CDC-NCM with 16KiB
> >> NTB's.
> >>
> >> The issue is after running for a while the kernel memory can become
> >> fragmented and it needs compacting.
> >> If the NTB allocation is needed before the memory has been compacted
> >> the atomic allocation can fail which can cause increased latency,
> >> large re-transmissions or disconnections depending upon the data
> >> being transmitted at the time.
> >> This situation occurs for less than a second until the kernel has
> >> compacted the memory but the failed devices can take a lot longer to
> >> recover from the failed TX packets.
> >>
> >> To ease this temporary situation I modified the CDC-NCM TX path to
> >> temporarily switch into a reduced memory mode which allocates an NTB
> >> that will fit into a USB_CDC_NCM_NTB_MIN_OUT_SIZE (default 2048 Bytes)
> >> sized memory block and only transmit NTB's with a single network frame
> >> until the memory situation is resolved.
> >> Once the memory is compacted the CDC-NCM data can resume transmitting
> >> at the normal tx_max rate once again.
> > 
> > I must say that I don't like the additional complexity added here.  If
> > there are memory issues and you can reduce the buffer size to
> > USB_CDC_NCM_NTB_MIN_OUT_SIZE, then why don't you just set a lower tx_max
> > buffer size in the first place?
> > 
> >   echo 2048 > /sys/class/net/wwan0/cdc_ncm/tx_max
> 
> When there isn't memory pressure this will hurt performance of
> course.
> 
> It is a quite common paradigm to back down to 0 order memory requests
> when higher order ones fail, so this isn't such a bad change from the
> perspective.
> 
> However, one negative about it is that when the system is under memory
> stress it doesn't help at all to keep attemping high order allocations
> when the system hasn't recovered yet.  In fact, this can make it
> worse.

This makes me wonder why there is no notifier chain for this.
Or am I just too stupid to find it?

Regards
Oliver

[PATCH] cdc-ether: divorce initialisation with a filter reset and a generic method

[Oliver Neukum]

Some devices need their multicast filter reset but others are crashed by that.
So the methods need to be separated.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
Reported-by: "Ridgway, Keith" <kridg...@harris.com>
---
 drivers/net/usb/cdc_ether.c | 34 +-
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index f3ae88fdf332..70d823043803 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -310,6 +310,26 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct 
usb_interface *intf)
return -ENODEV;
}
 
+   return 0;
+
+bad_desc:
+   dev_info(>udev->dev, "bad CDC descriptors\n");
+   return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(usbnet_generic_cdc_bind);
+
+
+/* like usbnet_generic_cdc_bind() but handles filter initialization
+ * correctly
+ */
+int usbnet_ether_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+   int rv;
+
+   rv = usbnet_generic_cdc_bind(dev, intf);
+   if (rv < 0)
+   goto bail_out;
+
/* Some devices don't initialise properly. In particular
 * the packet filter is not reset. There are devices that
 * don't do reset all the way. So the packet filter should
@@ -317,13 +337,10 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct 
usb_interface *intf)
 */
usbnet_cdc_update_filter(dev);
 
-   return 0;
-
-bad_desc:
-   dev_info(>udev->dev, "bad CDC descriptors\n");
-   return -ENODEV;
+bail_out:
+   return rv;
 }
-EXPORT_SYMBOL_GPL(usbnet_generic_cdc_bind);
+EXPORT_SYMBOL_GPL(usbnet_ether_cdc_bind);
 
 void usbnet_cdc_unbind(struct usbnet *dev, struct usb_interface *intf)
 {
@@ -417,7 +434,7 @@ int usbnet_cdc_bind(struct usbnet *dev, struct 
usb_interface *intf)
BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data)
< sizeof(struct cdc_state)));
 
-   status = usbnet_generic_cdc_bind(dev, intf);
+   status = usbnet_ether_cdc_bind(dev, intf);
if (status < 0)
return status;
 
@@ -472,7 +489,6 @@ static void usbnet_cdc_zte_status(struct usbnet *dev, 
struct urb *urb)
 
if (urb->actual_length < sizeof(*event))
return;
-
event = urb->transfer_buffer;
 
if (event->bNotificationType != USB_CDC_NOTIFY_NETWORK_CONNECTION) {
@@ -493,7 +509,7 @@ static void usbnet_cdc_zte_status(struct usbnet *dev, 
struct urb *urb)
 static const struct driver_infocdc_info = {
.description =  "CDC Ethernet Device",
.flags =FLAG_ETHER | FLAG_POINTTOPOINT,
-   .bind = usbnet_cdc_bind,
+   .bind = usbnet_ether_cdc_bind,
.unbind =   usbnet_cdc_unbind,
.status =   usbnet_cdc_status,
.set_rx_mode =  usbnet_cdc_update_filter,
-- 
2.12.0

Re: [RFC V1 1/1] net: cdc_ncm: Reduce memory use when kernel memory low

[Oliver Neukum]

Am Dienstag, den 16.05.2017, 20:24 +0200 schrieb Bjørn Mork:
> 
> I must say that I don't like the additional complexity added here.  If
> there are memory issues and you can reduce the buffer size to
> USB_CDC_NCM_NTB_MIN_OUT_SIZE, then why don't you just set a lower tx_max
> buffer size in the first place?
> 
>   echo 2048 > /sys/class/net/wwan0/cdc_ncm/tx_max
> 

Hi,

that would hurt performance across the board though.
Can we trigger memory compactation earlier?

Regards
Oliver

Re: [PATCH] usbnet: no address filtering on RNDIS

[Oliver Neukum]

Am Montag, den 15.05.2017, 10:16 -0400 schrieb David Miller:
> From: Bjørn Mork <bj...@mork.no>
> Date: Mon, 15 May 2017 14:20:20 +0200
> 
> > Oliver Neukum <oneu...@suse.com> writes:
> > 
> >> RNDIS does not do multicast filtering and the commands crash a few devices.
> >> Make it conditional.
> > 
> > 
> > Strange.  I thought we already discussed this when the filter reset
> > request was initially added 3 years ago.  Ref
> > https://patchwork.ozlabs.org/patch/374137/
> 
> I think the suggestion at the end of that thread is a better way to
> handle this.

Indeed. I will redo the patch.

Regards
Oliver

[PATCH] usbnet: no address filtering on RNDIS

[Oliver Neukum]

RNDIS does not do multicast filtering and the commands crash a few devices.
Make it conditional.

Fixes: b527caee1b91946db844b1dc63d4f726958891c8
Signed-off-by: Oliver Neukum <oneu...@suse.com>
Reported-by: "Ridgway, Keith" <kridg...@harris.com>
---
 drivers/net/usb/cdc_ether.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index f3ae88fdf332..64b1a6bdef98 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -313,9 +313,11 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct 
usb_interface *intf)
/* Some devices don't initialise properly. In particular
 * the packet filter is not reset. There are devices that
 * don't do reset all the way. So the packet filter should
-* be set to a sane initial value.
+* be set to a sane initial value, if filtering is supported.
+* RNDIS does not support it.
 */
-   usbnet_cdc_update_filter(dev);
+   if (!rndis)
+   usbnet_cdc_update_filter(dev);
 
return 0;
 
-- 
2.12.0

question on devices that have a UUID but no MAC

[Oliver Neukum]

Hi,

is there a canonical way to use a device's UUID to generate a MAC
inside the kernel? If I add it, where does it belong into?

Regards
Oliver

Re: [PATCH net-next 13/14] usbnet: kaweth: Use net_device_stats from struct net_device

[Oliver Neukum]

Am Freitag, den 07.04.2017, 10:17 +0200 schrieb Tobias Klauser:
> Instead of using a private copy of struct net_device_stats in struct
> kaweth_device, use stats from struct net_device. Also remove the now
> unnecessary .ndo_get_stats function.
> 
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
Acked-by: Oliver Neukum <oneu...@suse.com>

Re: [PATCH] usbnet: make sure no NULL pointer is passed through

[Oliver Neukum]

Am Donnerstag, den 06.04.2017, 13:19 -0700 schrieb David Miller:
> From: Oliver Neukum <oneu...@suse.com>
> Date: Wed,  5 Apr 2017 14:14:39 +0200
> 
> > Coverity reports:
>  ...
> > It is valid to offer commands without a buffer, but then you need a size
> > of zero. This should actually be checked.
> > 
> > Signed-off-by: Oliver Neukum <oneu...@suse.com>
> 
> Applied, thanks Oliver.
> 
> I had to apply this by hand via my inbox rather than using patchwork
> because those coverity report delimiters cause patchwork to chop off
> most of your commit message.
> 

Hi,

thanks. That is not good. It seems to me that a Coverity report
should be in the change log. Do you have suggestions how to do
this in a friendly manner?

Regards
Oliver

[PATCH] usbnet: make sure no NULL pointer is passed through

[Oliver Neukum]

Coverity reports:

** CID 751368:  Null pointer dereferences  (FORWARD_NULL)
/drivers/net/usb/usbnet.c: 1925 in __usbnet_read_cmd()


*** CID 751368:  Null pointer dereferences  (FORWARD_NULL)
/drivers/net/usb/usbnet.c: 1925 in __usbnet_read_cmd()
1919 EXPORT_SYMBOL(usbnet_link_change);
1920
1921 
/*-*/
1922 static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
1923 u16 value, u16 index, void *data, u16 size)
1924 {
>>> CID 751368:  Null pointer dereferences  (FORWARD_NULL)
>>> Assigning: "buf" = "NULL".
1925void *buf = NULL;
1926int err = -ENOMEM;
1927
1928netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
1929   " value=0x%04x index=0x%04x size=%d\n",
1930   cmd, reqtype, value, index, size);

** CID 751370:  Null pointer dereferences  (FORWARD_NULL)
/drivers/net/usb/usbnet.c: 1952 in __usbnet_write_cmd()


*** CID 751370:  Null pointer dereferences  (FORWARD_NULL)
/drivers/net/usb/usbnet.c: 1952 in __usbnet_write_cmd()
1946 }
1947
1948 static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
1949  u16 value, u16 index, const void *data,
1950  u16 size)
1951 {
>>> CID 751370:  Null pointer dereferences  (FORWARD_NULL)
>>> Assigning: "buf" = "NULL".
1952void *buf = NULL;
1953int err = -ENOMEM;
1954
1955netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
1956   " value=0x%04x index=0x%04x size=%d\n",
1957   cmd, reqtype, value, index, size);

** CID 1325026:  Null pointer dereferences  (FORWARD_NULL)
/drivers/net/usb/ch9200.c: 143 in control_write()

It is valid to offer commands without a buffer, but then you need a size
of zero. This should actually be checked.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/usbnet.c | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3de65ea..4532448 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1929,7 +1929,7 @@ static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, 
u8 reqtype,
   " value=0x%04x index=0x%04x size=%d\n",
   cmd, reqtype, value, index, size);
 
-   if (data) {
+   if (size) {
buf = kmalloc(size, GFP_KERNEL);
if (!buf)
goto out;
@@ -1938,8 +1938,13 @@ static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, 
u8 reqtype,
err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
  cmd, reqtype, value, index, buf, size,
  USB_CTRL_GET_TIMEOUT);
-   if (err > 0 && err <= size)
-   memcpy(data, buf, err);
+   if (err > 0 && err <= size) {
+if (data)
+memcpy(data, buf, err);
+else
+netdev_dbg(dev->net,
+"Huh? Data requested but thrown away.\n");
+}
kfree(buf);
 out:
return err;
@@ -1960,7 +1965,13 @@ static int __usbnet_write_cmd(struct usbnet *dev, u8 
cmd, u8 reqtype,
buf = kmemdup(data, size, GFP_KERNEL);
if (!buf)
goto out;
-   }
+   } else {
+if (size) {
+WARN_ON_ONCE(1);
+err = -EINVAL;
+goto out;
+}
+}
 
err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
  cmd, reqtype, value, index, buf, size,
-- 
2.10.2

Re: [PATCHv3] net: usbnet: support 64bit stats in qmi_wwan driver

[Oliver Neukum]

Am Freitag, den 31.03.2017, 10:48 +0200 schrieb Bjørn Mork:
> You get *all* the "0" line drivers for free, not only "qmi_wwan".  No
> code changes needed, except for adding the single .ndo line to drivers
> overriding the usbnet default net_device_ops. And even that only applies
> to a few of them.  Most will be OK if you just change the usbnet
> default.
> 
> I don't think the size of a complete series will be terrifying to
> anyone.

It would really be nice to do that.
However, if you really don't want to do it, well you wrote
a patch. But I am afraid dropping the error count is not acceptable.

Regards
Oliver

Re: [PATCHv2] net: usbnet: support 64bit stats in qmi_wwan driver

[Oliver Neukum]

Am Freitag, den 24.03.2017, 07:17 -0700 schrieb Eric Dumazet:
> On Fri, 2017-03-24 at 15:42 +1000, Greg Ungerer wrote:
> 
> > 
> > The usbnet core is used by a number of drivers. This patch only
> > updates the qmi-wwan driver to use stats64. If you remove the
> > dev->stats.rx_* updates all those other driver users will have
> > no counts.
> 
> I see. Then I guess the u64 stuff could be done only if running 32bit
> kernels. (The existing stats are using 'unsigned long' so are already
> 64bit wide on 64bit kernels)

We want every driver to use u64 if possible. If something
is exported we want a defined size.

Regards
Oliver

Re: [PATCH] net: usbnet: support 64bit stats in qmi_wwan driver

[Oliver Neukum]

Am Donnerstag, den 23.03.2017, 11:25 +1000 schrieb Greg Ungerer:
> Add support for the net stats64 counters to the usbnet core and then to
> the qmi_wwan driver.
> 
> This is a strait forward addition of 64bit counters for RX and TX packets
> and byte counts. It is done in the same style as for the other net drivers
> that support stats64.
> 
> The bulk of the change is to the usbnet core. Then it is trivial to use
> that in the qmi_wwan.c driver. It would be very simple to extend this
> support to other usbnet based drivers.
> 
> The motivation to add this is that it is not particularly difficult to
> get the RX and TX byte counts to wrap on 32bit platforms.

Hi,

you need to export the symbol usbnet_get_stats64
Other than that it looks good.

Regards
Oliver

Re: [PATCH 01/11] net: usb: usbnet: add new api ethtool_{get|set}_link_ksettings

[Oliver Neukum]

Am Donnerstag, den 16.03.2017, 23:18 +0100 schrieb Philippe Reynes:
> The ethtool api {get|set}_settings is deprecated.
> We add the new api {get|set}_link_ksettings to this driver.
> 
> As I don't have the hardware, I'd be very pleased if
> someone may test this patch.
> 

Unfortunately I lack hardware to test.
Phillipe and I had a patch collision. David, please take his
patch for the next merge window. It looks good and is comprehensive and
nobody has reported issues. We will never find testers for all those
drivers on this list.

Regards
Oliver

Re: [PATCH 11/11] net: usb: usb: remove old api ethtool_{get|set}_settings

[Oliver Neukum]

Am Donnerstag, den 16.03.2017, 23:18 +0100 schrieb Philippe Reynes:
> The function usbnet_{get|set}_settings is no longer used,
> so we remove it.
> 
> Signed-off-by: Philippe Reynes <trem...@gmail.com>
Acked-by: Oliver Neukum <oneu...@suse.com>

Re: [PATCH 01/11] net: usb: usbnet: add new api ethtool_{get|set}_link_ksettings

[Oliver Neukum]

Am Donnerstag, den 16.03.2017, 23:18 +0100 schrieb Philippe Reynes:
> The ethtool api {get|set}_settings is deprecated.
> We add the new api {get|set}_link_ksettings to this driver.
> 
> As I don't have the hardware, I'd be very pleased if
> someone may test this patch.
> 
> Signed-off-by: Philippe Reynes <trem...@gmail.com>
Acked-by: Oliver Neukum <oneu...@suse.com>

[RFC/RFT PATCH 1/9] net: usb: usbnet: add ksettings method

[Oliver Neukum]

This patch adds support for the new ksettings API of the ethtool
methods to usbnet.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/usbnet.c   | 35 +++
 include/linux/usb/usbnet.h |  4 
 2 files changed, 39 insertions(+)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3de65ea..3d7103a 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -947,6 +947,41 @@ EXPORT_SYMBOL_GPL(usbnet_open);
  * they'll probably want to use this base set.
  */
 
+int usbnet_get_ksettings(struct net_device *net,
+   struct ethtool_link_ksettings *cmd)
+{
+   struct usbnet *dev = netdev_priv(net);
+
+   if (!dev->mii.mdio_read)
+   return -EOPNOTSUPP;
+
+   return mii_ethtool_get_link_ksettings(>mii, cmd);
+}
+EXPORT_SYMBOL_GPL(usbnet_get_ksettings);
+
+int usbnet_set_ksettings(struct net_device *net,
+   const struct ethtool_link_ksettings *cmd)
+{
+   struct usbnet *dev = netdev_priv(net);
+   int retval;
+
+   if (!dev->mii.mdio_write)
+   return -EOPNOTSUPP;
+
+   retval = mii_ethtool_set_link_ksettings(>mii, cmd);
+
+   /* link speed/duplex might have changed */
+   if (dev->driver_info->link_reset)
+   dev->driver_info->link_reset(dev);
+
+   /* hard_mtu or rx_urb_size may change in link_reset() */
+   usbnet_update_max_qlen(dev);
+
+   return retval;
+
+}
+EXPORT_SYMBOL_GPL(usbnet_set_ksettings);
+
 int usbnet_get_settings (struct net_device *net, struct ethtool_cmd *cmd)
 {
struct usbnet *dev = netdev_priv(net);
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 6e0ce8c..c32ee9a 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -261,6 +261,10 @@ extern void usbnet_pause_rx(struct usbnet *);
 extern void usbnet_resume_rx(struct usbnet *);
 extern void usbnet_purge_paused_rxq(struct usbnet *);
 
+extern int usbnet_get_ksettings(struct net_device *net,
+   struct ethtool_link_ksettings *cmd);
+extern int usbnet_set_ksettings(struct net_device *net,
+   const struct ethtool_link_ksettings *cmd);
 extern int usbnet_get_settings(struct net_device *net,
   struct ethtool_cmd *cmd);
 extern int usbnet_set_settings(struct net_device *net,
-- 
2.10.2

[RFC/RFT PATCH 4/9] net: usb: dm9601: use ksettings API

[Oliver Neukum]

This changes dm9601 to use the new ksettings API of usbnet
and export the new methods to the network layer.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/dm9601.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
index 0b4bdd3..b604cd4 100644
--- a/drivers/net/usb/dm9601.c
+++ b/drivers/net/usb/dm9601.c
@@ -281,9 +281,9 @@ static const struct ethtool_ops dm9601_ethtool_ops = {
.set_msglevel   = usbnet_set_msglevel,
.get_eeprom_len = dm9601_get_eeprom_len,
.get_eeprom = dm9601_get_eeprom,
-   .get_settings   = usbnet_get_settings,
-   .set_settings   = usbnet_set_settings,
.nway_reset = usbnet_nway_reset,
+   .set_link_ksettings = usbnet_set_ksettings,
+   .get_link_ksettings = usbnet_get_ksettings
 };
 
 static void dm9601_set_multicast(struct net_device *net)
-- 
2.10.2

[RFC/RFT PATCH 6/9] net: usb: smsc75xx: use ksettings API

[Oliver Neukum]

This changes smsc75xx to use the new ksettings API of usbnet
and export the new methods to the network layer.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/smsc75xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 0b17b40..9d9a07b 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -743,13 +743,13 @@ static const struct ethtool_ops smsc75xx_ethtool_ops = {
.get_drvinfo= usbnet_get_drvinfo,
.get_msglevel   = usbnet_get_msglevel,
.set_msglevel   = usbnet_set_msglevel,
-   .get_settings   = usbnet_get_settings,
-   .set_settings   = usbnet_set_settings,
.get_eeprom_len = smsc75xx_ethtool_get_eeprom_len,
.get_eeprom = smsc75xx_ethtool_get_eeprom,
.set_eeprom = smsc75xx_ethtool_set_eeprom,
.get_wol= smsc75xx_ethtool_get_wol,
.set_wol= smsc75xx_ethtool_set_wol,
+   .get_link_ksettings = usbnet_get_ksettings,
+   .set_link_ksettings = usbnet_set_ksettings
 };
 
 static int smsc75xx_ioctl(struct net_device *netdev, struct ifreq *rq, int cmd)
-- 
2.10.2

[RFC/RFT PATCH 7/9] net: usb: smsc95xx: use ksettings API

[Oliver Neukum]

This provides an implementation of the new ksettings API.
The methods provided by usbnet are extended.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/smsc95xx.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 831aa33..ea0d681 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -853,32 +853,32 @@ static void set_mdix_status(struct net_device *net, __u8 
mdix_ctrl)
pdata->mdix_ctrl = mdix_ctrl;
 }
 
-static int smsc95xx_get_settings(struct net_device *net,
-struct ethtool_cmd *cmd)
+static int smsc95xx_get_ksettings(struct net_device *net,
+struct ethtool_link_ksettings *cmd)
 {
struct usbnet *dev = netdev_priv(net);
struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
int retval;
 
-   retval = usbnet_get_settings(net, cmd);
+   retval = usbnet_get_ksettings(net, cmd);
 
-   cmd->eth_tp_mdix = pdata->mdix_ctrl;
-   cmd->eth_tp_mdix_ctrl = pdata->mdix_ctrl;
+   cmd->base.eth_tp_mdix = pdata->mdix_ctrl;
+   cmd->base.eth_tp_mdix_ctrl = pdata->mdix_ctrl;
 
return retval;
 }
 
-static int smsc95xx_set_settings(struct net_device *net,
-struct ethtool_cmd *cmd)
+static int smsc95xx_set_ksettings(struct net_device *net,
+const struct ethtool_link_ksettings *cmd)
 {
struct usbnet *dev = netdev_priv(net);
struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
int retval;
 
-   if (pdata->mdix_ctrl != cmd->eth_tp_mdix_ctrl)
-   set_mdix_status(net, cmd->eth_tp_mdix_ctrl);
+   if (pdata->mdix_ctrl != cmd->base.eth_tp_mdix_ctrl)
+   set_mdix_status(net, cmd->base.eth_tp_mdix_ctrl);
 
-   retval = usbnet_set_settings(net, cmd);
+   retval = usbnet_set_ksettings(net, cmd);
 
return retval;
 }
@@ -889,8 +889,6 @@ static const struct ethtool_ops smsc95xx_ethtool_ops = {
.get_drvinfo= usbnet_get_drvinfo,
.get_msglevel   = usbnet_get_msglevel,
.set_msglevel   = usbnet_set_msglevel,
-   .get_settings   = smsc95xx_get_settings,
-   .set_settings   = smsc95xx_set_settings,
.get_eeprom_len = smsc95xx_ethtool_get_eeprom_len,
.get_eeprom = smsc95xx_ethtool_get_eeprom,
.set_eeprom = smsc95xx_ethtool_set_eeprom,
@@ -898,6 +896,8 @@ static const struct ethtool_ops smsc95xx_ethtool_ops = {
.get_regs   = smsc95xx_ethtool_getregs,
.get_wol= smsc95xx_ethtool_get_wol,
.set_wol= smsc95xx_ethtool_set_wol,
+   .get_link_ksettings =   smsc95xx_get_ksettings,
+   .set_link_ksettings =   smsc95xx_set_ksettings
 };
 
 static int smsc95xx_ioctl(struct net_device *netdev, struct ifreq *rq, int cmd)
-- 
2.10.2

[RFC/RFT PATCH 8/9] net: usb: asix: use ksettings API

[Oliver Neukum]

This changes asix to use the new ksettings API of usbnet
and export the new methods to the network layer.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/asix_devices.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 0dd5106..5997b2b 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -20,6 +20,7 @@
  */
 
 #include "asix.h"
+#include "linux/usb/usbnet.h"
 
 #define PHY_MODE_MARVELL   0x
 #define MII_MARVELL_LED_CTRL   0x0018
@@ -136,9 +137,9 @@ static const struct ethtool_ops ax88172_ethtool_ops = {
.get_eeprom_len = asix_get_eeprom_len,
.get_eeprom = asix_get_eeprom,
.set_eeprom = asix_set_eeprom,
-   .get_settings   = usbnet_get_settings,
-   .set_settings   = usbnet_set_settings,
.nway_reset = usbnet_nway_reset,
+   .get_link_ksettings = usbnet_get_ksettings,
+   .set_link_ksettings = usbnet_set_ksettings
 };
 
 static void ax88172_set_multicast(struct net_device *net)
@@ -301,9 +302,9 @@ static const struct ethtool_ops ax88772_ethtool_ops = {
.get_eeprom_len = asix_get_eeprom_len,
.get_eeprom = asix_get_eeprom,
.set_eeprom = asix_set_eeprom,
-   .get_settings   = usbnet_get_settings,
-   .set_settings   = usbnet_set_settings,
.nway_reset = usbnet_nway_reset,
+   .get_link_ksettings = usbnet_get_ksettings,
+   .set_link_ksettings = usbnet_set_ksettings
 };
 
 static int ax88772_link_reset(struct usbnet *dev)
@@ -775,9 +776,9 @@ static const struct ethtool_ops ax88178_ethtool_ops = {
.get_eeprom_len = asix_get_eeprom_len,
.get_eeprom = asix_get_eeprom,
.set_eeprom = asix_set_eeprom,
-   .get_settings   = usbnet_get_settings,
-   .set_settings   = usbnet_set_settings,
.nway_reset = usbnet_nway_reset,
+   .get_link_ksettings = usbnet_get_ksettings,
+   .set_link_ksettings = usbnet_set_ksettings
 };
 
 static int marvell_phy_init(struct usbnet *dev)
-- 
2.10.2

[RFC/RFT PATCH 5/9] net: usb: mcs7830: use ksettings API

[Oliver Neukum]

This changes mcs7830 to use the new ksettings API of usbnet
and export the new methods to the network layer.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/mcs7830.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/mcs7830.c b/drivers/net/usb/mcs7830.c
index 4f345bd..9e5f75c 100644
--- a/drivers/net/usb/mcs7830.c
+++ b/drivers/net/usb/mcs7830.c
@@ -464,9 +464,9 @@ static const struct ethtool_ops mcs7830_ethtool_ops = {
.get_link   = usbnet_get_link,
.get_msglevel   = usbnet_get_msglevel,
.set_msglevel   = usbnet_set_msglevel,
-   .get_settings   = usbnet_get_settings,
-   .set_settings   = usbnet_set_settings,
.nway_reset = usbnet_nway_reset,
+   .get_link_ksettings = usbnet_get_ksettings,
+   .set_link_ksettings = usbnet_set_ksettings
 };
 
 static const struct net_device_ops mcs7830_netdev_ops = {
-- 
2.10.2

[RFC/RFT PATCH 2/9] net: usb: cdc-ncm: use ksettings API

[Oliver Neukum]

This changes CDC-NCM to use the new ksettings API of usbnet
and export the new methods to the network layer.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/cdc_ncm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index f317984..311c7b6 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -131,8 +131,6 @@ static void cdc_ncm_get_strings(struct net_device 
__always_unused *netdev, u32 s
 static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 
new_tx);
 
 static const struct ethtool_ops cdc_ncm_ethtool_ops = {
-   .get_settings  = usbnet_get_settings,
-   .set_settings  = usbnet_set_settings,
.get_link  = usbnet_get_link,
.nway_reset= usbnet_nway_reset,
.get_drvinfo   = usbnet_get_drvinfo,
@@ -142,6 +140,8 @@ static const struct ethtool_ops cdc_ncm_ethtool_ops = {
.get_sset_count= cdc_ncm_get_sset_count,
.get_strings   = cdc_ncm_get_strings,
.get_ethtool_stats = cdc_ncm_get_ethtool_stats,
+   .get_link_ksettings = usbnet_get_ksettings,
+   .set_link_ksettings = usbnet_set_ksettings
 };
 
 static u32 cdc_ncm_check_rx_max(struct usbnet *dev, u32 new_rx)
-- 
2.10.2

[RFC/RFT PATCH 3/9] net: usb: sierra-net: use ksettings API

[Oliver Neukum]

This changes sierra-net to use the new ksettings API of usbnet
and export the new methods to the network layer.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/sierra_net.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index ac69f28..d9bda48 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -648,9 +648,9 @@ static const struct ethtool_ops sierra_net_ethtool_ops = {
.get_link = sierra_net_get_link,
.get_msglevel = usbnet_get_msglevel,
.set_msglevel = usbnet_set_msglevel,
-   .get_settings = usbnet_get_settings,
-   .set_settings = usbnet_set_settings,
.nway_reset = usbnet_nway_reset,
+   .get_link_ksettings = usbnet_get_ksettings,
+   .set_link_ksettings = usbnet_set_ksettings
 };
 
 static int sierra_net_get_fw_attr(struct usbnet *dev, u16 *datap)
-- 
2.10.2

[RFC/RFT] the new ksetting methods in subnet's subdrivers

[Oliver Neukum]

The API for ethtool has been changed. Rather than implement the change
sparately in each subdriver, usbnet should be changed and the subdrivers
should expand upon the generic method usbnet provides.
This set implements the new methods, introduces them into the subdrivers
and rips out the old stuff, lest it be used in new drivers.

NAPI on USB network drivers

[Oliver Neukum]

Hi,

looking at r8152 I noticed that it uses NAPI. I never considered
this for the generic USB networking code as you cannot disable
interrupts for USB. Is it still worth it? What are the benefits?

Regards
Oliver

Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Oliver Neukum]

On Thu, 2016-11-10 at 12:09 +0100, Bjørn Mork wrote:
> Kai-Heng Feng <kai.heng.f...@canonical.com> writes:
> > On Wed, Nov 9, 2016 at 8:32 PM, Bjørn Mork <bj...@mork.no> wrote:
> >> Oliver Neukum <oneu...@suse.com> writes:
> >>
> >>> On Tue, 2016-11-08 at 13:44 -0500, Alan Stern wrote:
> >>>
> >>>> These problems could very well be caused by running at SuperSpeed
> >>>> (USB-3) instead of high speed (USB-2).
> >
> > Yes, it's running at SuperSpeed, on a Kabylake laptop.
> >
> > It does not have this issue on a Broadwell laptop, also running at 
> > SuperSpeed.
> 
> Then I must join Oliver, being very surprised by where in the stack you
> attempt to fix the issue.  What you write above indicates a problem in
> pci bridge or usb host controller, doesn't it?

Indeed. And this means we need an XHCI specialist.
Mathias, we have a failure specific to one implementation of XHCI.

Regards
Oliver

Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Oliver Neukum]

On Tue, 2016-11-08 at 13:44 -0500, Alan Stern wrote:

> These problems could very well be caused by running at SuperSpeed
> (USB-3) instead of high speed (USB-2).
> 
> Is there any way to test what happens when the device is attached to 
> the computer by a USB-2 cable?  That would prevent it from operating at 
> SuperSpeed.
> 
> The main point, however, is that the proposed patch doesn't seem to
> address the true problem, which is that the device gets suspended
> between probes.  The patch only tries to prevent it from being
> suspended during a probe -- which is already prevented by the USB core.

But why doesn't it fail during normal operation?

I suspect that its firmware requires the altsetting

/* should we change control altsetting on a NCM/MBIM function? */
if (cdc_ncm_select_altsetting(intf) == CDC_NCM_COMM_ALTSETTING_MBIM) {
data_altsetting = CDC_NCM_DATA_ALTSETTING_MBIM;
ret = cdc_mbim_set_ctrlalt(dev, intf, 
CDC_NCM_COMM_ALTSETTING_MBIM);

to be set before it accepts a suspension.

Regards
Oliver

Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Oliver Neukum]

On Fri, 2016-11-04 at 17:57 +0800, Kai-Heng Feng wrote:
> Sometimes cdc_mbim failed to probe if runtime pm is enabled:
> [9.305626] cdc_mbim: probe of 2-2:1.12 failed with error -22
> 
> This can be solved by increase its pm usage counter.
> 
> Signed-off-by: Kai-Heng Feng 

For the record:

NAK. This fixes a symptom. If this patch helps something is broken in
device core. We need to find that.

Regards
Oliver

Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Oliver Neukum]

On Fri, 2016-11-04 at 09:26 -0400, Alan Stern wrote:
> On Fri, 4 Nov 2016, Kai-Heng Feng wrote:
> 
> > Sometimes cdc_mbim failed to probe if runtime pm is enabled:
> > [9.305626] cdc_mbim: probe of 2-2:1.12 failed with error -22
> > 
> > This can be solved by increase its pm usage counter.
> 
> This should not be needed.  The USB core increments the PM usage 
> counter of a device before probing its interfaces.

Indeed. Yet we have experimental evidence.

Kai-Heng Feng, could you please enable dynamic debugging
for 
drivers/usb/core/driver.c

so that we can see what is going on with the usage counters?

Regards
Oliver

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

[Oliver Neukum]

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

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

Regards
Oliver

[PATCH] kaweth: remove obsolete debugging statements

[Oliver Neukum]

SOme statements in the driver only served to inform
which functions were entered. Ftrace can do that just as good without
needing memory. Remove the statements.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/kaweth.c | 15 ---
 1 file changed, 15 deletions(-)

diff --git a/drivers/net/usb/kaweth.c b/drivers/net/usb/kaweth.c
index 528b9c9..66b34dd 100644
--- a/drivers/net/usb/kaweth.c
+++ b/drivers/net/usb/kaweth.c
@@ -265,8 +265,6 @@ static int kaweth_control(struct kaweth_device *kaweth,
struct usb_ctrlrequest *dr;
int retval;
 
-   netdev_dbg(kaweth->net, "kaweth_control()\n");
-
if(in_interrupt()) {
netdev_dbg(kaweth->net, "in_interrupt()\n");
return -EBUSY;
@@ -300,8 +298,6 @@ static int kaweth_read_configuration(struct kaweth_device 
*kaweth)
 {
int retval;
 
-   netdev_dbg(kaweth->net, "Reading kaweth configuration\n");
-
retval = kaweth_control(kaweth,
usb_rcvctrlpipe(kaweth->dev, 0),
KAWETH_COMMAND_GET_ETHERNET_DESC,
@@ -451,8 +447,6 @@ static int kaweth_trigger_firmware(struct kaweth_device 
*kaweth,
kaweth->firmware_buf[6] = 0x00;
kaweth->firmware_buf[7] = 0x00;
 
-   netdev_dbg(kaweth->net, "Triggering firmware\n");
-
return kaweth_control(kaweth,
  usb_sndctrlpipe(kaweth->dev, 0),
  KAWETH_COMMAND_SCAN,
@@ -471,7 +465,6 @@ static int kaweth_reset(struct kaweth_device *kaweth)
 {
int result;
 
-   netdev_dbg(kaweth->net, "kaweth_reset(%p)\n", kaweth);
result = usb_reset_configuration(kaweth->dev);
mdelay(10);
 
@@ -685,8 +678,6 @@ static int kaweth_open(struct net_device *net)
struct kaweth_device *kaweth = netdev_priv(net);
int res;
 
-   netdev_dbg(kaweth->net, "Opening network device.\n");
-
res = usb_autopm_get_interface(kaweth->intf);
if (res) {
dev_err(>intf->dev, "Interface cannot be resumed.\n");
@@ -951,7 +942,6 @@ static int kaweth_suspend(struct usb_interface *intf, 
pm_message_t message)
struct kaweth_device *kaweth = usb_get_intfdata(intf);
unsigned long flags;
 
-   dev_dbg(>dev, "Suspending device\n");
spin_lock_irqsave(>device_lock, flags);
kaweth->status |= KAWETH_STATUS_SUSPENDING;
spin_unlock_irqrestore(>device_lock, flags);
@@ -968,7 +958,6 @@ static int kaweth_resume(struct usb_interface *intf)
struct kaweth_device *kaweth = usb_get_intfdata(intf);
unsigned long flags;
 
-   dev_dbg(>dev, "Resuming device\n");
spin_lock_irqsave(>device_lock, flags);
kaweth->status &= ~KAWETH_STATUS_SUSPENDING;
spin_unlock_irqrestore(>device_lock, flags);
@@ -1190,8 +1179,6 @@ err_fw:
dev_info(dev, "kaweth interface created at %s\n",
 kaweth->net->name);
 
-   dev_dbg(dev, "Kaweth probe returning.\n");
-
return 0;
 
 err_intfdata:
@@ -1219,8 +1206,6 @@ static void kaweth_disconnect(struct usb_interface *intf)
struct kaweth_device *kaweth = usb_get_intfdata(intf);
struct net_device *netdev;
 
-   dev_info(>dev, "Unregistering\n");
-
usb_set_intfdata(intf, NULL);
if (!kaweth) {
dev_warn(>dev, "unregistering non-existent device\n");
-- 
2.6.2

Re: [PATCH] usbnet: ax88179_178a: Add support for writing the EEPROM

[Oliver Neukum]

On Thu, 2016-08-25 at 12:07 +0200, Alban Bedel wrote:
> On Thu, 25 Aug 2016 11:16:36 +0200
> Oliver Neukum <oneu...@suse.com> wrote:
> 
> > On Wed, 2016-08-24 at 16:40 +0200, Alban Bedel wrote:
> > > On Wed, 24 Aug 2016 16:30:39 +0200
> > > Oliver Neukum <oneu...@suse.com> wrote:
> > >   
> > > > On Wed, 2016-08-24 at 15:52 +0200, Alban Bedel wrote:  
> > 
> > > > > +   if (block != data)
> > > > > +   kfree(block);
> > > > 
> > > > And if block == dta, what frees the memory?  
> > > 
> > > In this case this function didn't allocate any memory, so there is
> > > nothing to free.  
> > 
> > Hi,
> > 
> > I see. kfree() has a check for NULL, so you could drop the
> > test, but it doesn't matter much either way.
> 
> I think you misunderstand something here. data is the buffer passed
> by the caller and block is a local variable. There is two cases:
> 
> 1) The data to write is block aligned, then we use the caller buffer
>as is and set block = data.
> 2) The requested data is not block aligned, then we kalloc block.
> 
> In both case the writing loop then use the block pointer. Afterwards
> we only need to kfree block in case 2, that is when block != data.

Thanks for the clarification. Maybe worth a comment in the code?

Regards
Oliver

Re: [PATCH] usbnet: ax88179_178a: Add support for writing the EEPROM

[Oliver Neukum]

On Wed, 2016-08-24 at 16:40 +0200, Alban Bedel wrote:
> On Wed, 24 Aug 2016 16:30:39 +0200
> Oliver Neukum <oneu...@suse.com> wrote:
> 
> > On Wed, 2016-08-24 at 15:52 +0200, Alban Bedel wrote:

> > > +   if (block != data)
> > > +   kfree(block);  
> > 
> > And if block == dta, what frees the memory?
> 
> In this case this function didn't allocate any memory, so there is
> nothing to free.

Hi,

I see. kfree() has a check for NULL, so you could drop the
test, but it doesn't matter much either way.

Regards
Oliver

Re: [PATCH] usbnet: ax88179_178a: Add support for writing the EEPROM

[Oliver Neukum]

On Wed, 2016-08-24 at 15:52 +0200, Alban Bedel wrote:
> Implement the .set_eeprom callback to allow setting the MAC address
> as well as a few other parameters. Note that the EEPROM must have a
> correct PID/VID checksum set otherwise the SROM is used and reads
> return the SROM content.
> 
> Signed-off-by: Alban Bedel 
> ---
>  drivers/net/usb/ax88179_178a.c | 57
> ++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/net/usb/ax88179_178a.c
> b/drivers/net/usb/ax88179_178a.c
> index e6338c16081a..e6a986303dad 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -28,6 +28,7 @@
>  
>  #define AX88179_PHY_ID 0x03
>  #define AX_EEPROM_LEN  0x100
> +#define AX_EEPROM_BLOCK0x40u
>  #define AX88179_EEPROM_MAGIC   0x17900b95
>  #define AX_MCAST_FLTSIZE   8
>  #define AX_MAX_MCAST   64
> @@ -43,6 +44,7 @@
>  #define AX_ACCESS_PHY  0x02
>  #define AX_ACCESS_EEPROM   0x04
>  #define AX_ACCESS_EFUS 0x05
> +#define AX_RELOAD_EEPROM   0x06
>  #define AX_PAUSE_WATERLVL_HIGH 0x54
>  #define AX_PAUSE_WATERLVL_LOW  0x55
>  
> @@ -620,6 +622,60 @@ ax88179_get_eeprom(struct net_device *net, struct
> ethtool_eeprom *eeprom,
> return 0;
>  }
>  
> +static int
> +ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom
> *eeprom,
> +  u8 *data)
> +{
> +   struct usbnet *dev = netdev_priv(net);
> +   unsigned int offset = eeprom->offset;
> +   unsigned int len = eeprom->len;
> +   int i, err = 0;
> +   u8 *block;
> +
> +   /* The EEPROM data must be aligned on blocks of 64 bytes */
> +   if ((offset % AX_EEPROM_BLOCK) || (len % AX_EEPROM_BLOCK)) {
> +   offset = eeprom->offset / AX_EEPROM_BLOCK *
> AX_EEPROM_BLOCK;
> +   len = eeprom->len + eeprom->offset - offset;
> +   len = DIV_ROUND_UP(len, AX_EEPROM_BLOCK) *
> AX_EEPROM_BLOCK;
> +
> +   block = kmalloc(len, GFP_KERNEL);
> +   if (!block)
> +   return -ENOMEM;
> +
> +   /* Copy the current data, we could skip some but KISS
> */
> +   for (i = 0; i < len; i += AX_EEPROM_BLOCK) {
> +   err = __ax88179_read_cmd(dev,
> AX_ACCESS_EEPROM,
> +(offset + i) >> 1,
> +AX_EEPROM_BLOCK >> 1,
> +AX_EEPROM_BLOCK,
> +[i], 0);
> +   if (err < 0) {
> +   kfree(block);
> +   return err;
> +   }
> +   }
> +   memcpy(block + eeprom->offset - offset, data,
> eeprom->len);
> +   } else {
> +   block = data;
> +   }
> +
> +   for (i = 0; err >= 0 && i < len; i += AX_EEPROM_BLOCK) {
> +   err = ax88179_write_cmd(dev, AX_ACCESS_EEPROM,
> +   (offset + i) >> 1,
> +   AX_EEPROM_BLOCK >> 1,
> +   AX_EEPROM_BLOCK, [i]);
> +   }
> +
> +   if (block != data)
> +   kfree(block);

And if block == dta, what frees the memory?

Regards
Oliver

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-08-08 at 20:30 +0200, Bjørn Mork wrote:
> Note that this doesn't include *every* ethernet driver, although there
> certainly are some examples.  There are also a number of serious
> vendors, providing vendor supported drivers for cards with no known
> issues of this kind.
> 
> Where exactly would you like to see this implemented if it isn't going
> into those specific usbnet drivers?

Higher up. I'd prefer a flag to tell the network core that a driver
is unsure about the MAC and the core should deal with it.

Regards
Oliver

[PATCH v2 0/2]fixes to kaweth in response to Umap2 testing

[Oliver Neukum]

These patches fix an oops in firmware downloading and an oops due
to a memory allocation failure

[PATCH v2 2/2] kaweth: fix oops upon failed memory allocation

[Oliver Neukum]

Just return an error upon failure.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/kaweth.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/kaweth.c b/drivers/net/usb/kaweth.c
index 37bf715..528b9c9 100644
--- a/drivers/net/usb/kaweth.c
+++ b/drivers/net/usb/kaweth.c
@@ -1009,6 +1009,7 @@ static int kaweth_probe(
struct net_device *netdev;
const eth_addr_t bcast_addr = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
int result = 0;
+   int rv = -EIO;
 
dev_dbg(dev,
"Kawasaki Device Probe (Device number:%d): 
0x%4.4x:0x%4.4x:0x%4.4x\n",
@@ -1049,6 +1050,10 @@ static int kaweth_probe(
/* Download the firmware */
dev_info(dev, "Downloading firmware...\n");
kaweth->firmware_buf = (__u8 *)__get_free_page(GFP_KERNEL);
+   if (!kaweth->firmware_buf) {
+   rv = -ENOMEM;
+   goto err_free_netdev;
+   }
if ((result = kaweth_download_firmware(kaweth,
  "kaweth/new_code.bin",
  100,
@@ -1203,7 +1208,7 @@ err_only_tx:
 err_free_netdev:
free_netdev(netdev);
 
-   return -EIO;
+   return rv;
 }
 
 /
-- 
2.1.4

[PATCH v2 1/2] kaweth: fix firmware download

[Oliver Neukum]

This fixes the oops discovered by the Umap2 project and Alan Stern.
The intf member needs to be set before the firmware is downloaded.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/kaweth.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/usb/kaweth.c b/drivers/net/usb/kaweth.c
index 770212b..37bf715 100644
--- a/drivers/net/usb/kaweth.c
+++ b/drivers/net/usb/kaweth.c
@@ -1029,6 +1029,7 @@ static int kaweth_probe(
kaweth = netdev_priv(netdev);
kaweth->dev = udev;
kaweth->net = netdev;
+   kaweth->intf = intf;
 
spin_lock_init(>device_lock);
init_waitqueue_head(>term_wait);
@@ -1139,8 +1140,6 @@ err_fw:
 
dev_dbg(dev, "Initializing net device.\n");
 
-   kaweth->intf = intf;
-
kaweth->tx_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!kaweth->tx_urb)
goto err_free_netdev;
-- 
2.1.4

Re: [PATCH 2/2] kaweth: fix oops upon failed memory allocation

[Oliver Neukum]

On Wed, 2016-08-17 at 15:15 +0200, Bjørn Mork wrote:
> Oliver Neukum <oneu...@suse.com> writes:

> Eh, that should be "goto err_free_netdev;", shouldn't it?

Right, thanks. Fixed version coming up.

Regards
Oliver

[PATCH 2/2] kaweth: fix oops upon failed memory allocation

[Oliver Neukum]

Just return an error upon failure.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/kaweth.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/usb/kaweth.c b/drivers/net/usb/kaweth.c
index 37bf715..3cd6906 100644
--- a/drivers/net/usb/kaweth.c
+++ b/drivers/net/usb/kaweth.c
@@ -1049,6 +1049,8 @@ static int kaweth_probe(
/* Download the firmware */
dev_info(dev, "Downloading firmware...\n");
kaweth->firmware_buf = (__u8 *)__get_free_page(GFP_KERNEL);
+   if (!kaweth->firmware_buf)
+   return -ENOMEM;
if ((result = kaweth_download_firmware(kaweth,
  "kaweth/new_code.bin",
  100,
-- 
2.1.4

[PATCH 1/2] kaweth: fix firmware download

[Oliver Neukum]

This fixes the oops discovered by the Umap2 project and Alan Stern.
The intf member needs to be set before the firmware is downloaded.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/kaweth.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/usb/kaweth.c b/drivers/net/usb/kaweth.c
index 770212b..37bf715 100644
--- a/drivers/net/usb/kaweth.c
+++ b/drivers/net/usb/kaweth.c
@@ -1029,6 +1029,7 @@ static int kaweth_probe(
kaweth = netdev_priv(netdev);
kaweth->dev = udev;
kaweth->net = netdev;
+   kaweth->intf = intf;
 
spin_lock_init(>device_lock);
init_waitqueue_head(>term_wait);
@@ -1139,8 +1140,6 @@ err_fw:
 
dev_dbg(dev, "Initializing net device.\n");
 
-   kaweth->intf = intf;
-
kaweth->tx_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!kaweth->tx_urb)
goto err_free_netdev;
-- 
2.1.4

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-08-08 at 14:44 +0200, Bjørn Mork wrote:
> Oliver Neukum <oneu...@suse.com> writes:

> > I don't see how it would be specific for a subsystem. If the patch
> > is correct, it belongs into the networking core.
> 
> The bug is in the firmware implementation of the "read unique vendor
> assigned mac address" functions, and should therefore be fixed there.

Well, if you define the semantics of the operation in that manner, it
certainly does. That however is not given. You could also define it
as returning the MAC the hardware listens to. The difference was just
unclear when the API was defined.

> It cannot be put into the networking core because "read unique vendor
> assigned mac address" is a hardware dependent function.  It's
> implemented in each ethernet driver based of whatever interface the
> firmware provides to that register.

Again, that depends on that particular semantics.

> IMHO, usbnet_get_ethernet_addr() would be the most appropriate place for
> cdc_ether and other CDC drivers. And generic_rndis_bind() is the correct
> place for rndis_host.
> 
> Putting this in usbnet_get_ethernet_addr() will also fix the XMM7160
> based devices having an FF:FF:FF:FF:FF:FF mac address (sic).  I'm pretty
> sure there are other examples too.  There is no end to the creative
> crazyness of firmware engineers...

It is clear that that would work. No question about that.

But why fix similar issues at two different places? And what about
PCI or other cards that show the same problem?

Regards
Oliver

Re: [PATCH net-next v3] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Tue, 2016-07-19 at 16:54 +0200, Kristian Evensen wrote:
> The firmware in several ZTE devices (at least the MF823/831/910
> modems/mifis) use OS fingerprinting to determine which type of device
> to
> export. In addition, these devices export a REST API which can be used
> to
> control the type of device. So far, on Linux, the devices have been
> seen as
> RNDIS or CDC Ether.
> 
> When CDC Ether is used, devices of the same type are, as with RNDIS,
> exported with the same, bogus random MAC address. In addition, the
> devices
> (at least on all firmware revisions I have found) use the bogus MAC
> when
> sending traffic routed from external networks. And as a final feature,
> the
> devices sometimes export the link state incorrectly. There are also
> references online to several other ZTE devices displaying this
> behavior,
> with several different PIDs and MAC addresses.
> 
> This patch tries to improve the handling of ZTE devices by doing the
> following:
> 
> * Create a new driver_info-struct that is used by ZTE devices that do
> not
> have an explicit entry in the product table. This struct is the same
> as the
> default cdc_ether driver info, but a new bind- and an
> rx_fixup-function
> have been added.
> 
> * In the new bind function, we check if we have read a random MAC from
> the
> device. If we have, then we generate a new random MAC address. This
> will
> ensure that all devices get a unique MAC.
> 
> * The rx_fixup-function replaces the destination MAC address in the
> skb
> with that of the device. I have not seen a revision of these devices
> that
> behaves correctly (i.e., sets the right destination MAC), so I chose
> not to
> do any comparison with for example the known, bogus addresses.
> 
> * The MF823/MF832/MF910 sometimes export cdc carrier on twice on
> connect
> (the correct behavior is off then on). Work around this by manually
> setting
> carrier to off if an on-notification is received and the NOCARRIER-bit
> is
> not set.
> 
> This change will affect all devices, but it should take care of
> similar
> mistakes made by other manufacturers. I tried to think of/look/test
> for
> problems/regressions that could be introduced by this behavior, but
> could
> not find any. However, my familiarity with this code path is not that
> great, so there could be something I have overlooked.
> 
> I have tested this patch with multiple revisions of all three devices,
> and
> they behave as expected. In other words, they all got a valid, random
> MAC,
> the correct operational state and I can receive/sent traffic without
> problems. I also tested with some other cdc_ether devices I have and
> did
> not find any problems/regressions caused by the two general changes.
> 
> v2->v3:
> * I had forgot to remove the random MAC generation from
> usbnet_cdc_bind()
> (thanks Olive).
> * Rework logic in the ZTE bind-function a bit.
> 
> v1->v2:
> * Only generate random MAC for ZTE devices (thanks Oliver Neukum).
> * Set random MAC and do RX fixup for all ZTE devices that do not have
> a
> product-entry, as the bogus MAC have been seen on devices with several
> different PIDs/MAC addresses. In other words, it seems to be the
> default
> behavior of ZTE CDC Ether devices (thanks Lars Melin).
> 
> Signed-off-by: Kristian Evensen <kristian.even...@gmail.com>
Acked-by: Oliver Neukum <oneu...@suse.com>

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Tue, 2016-07-19 at 08:40 +0200, Kristian Evensen wrote:
> On Tue, Jul 19, 2016 at 8:20 AM, Oliver Neukum <oneu...@suse.com> wrote:
> >> I had a look at some other drivers, and I think we need to be very
> >> careful about making setting a random MAC too generic. For example, we
> >> might be unlucky and break the possibly_iphdr()-code/assumption in
> >> qmi_wwan.c. And there is probably more code/assumptions like that in
> >> the network stack.
> >
> > In this case please use special cases for usbnet, too.
> > We need a quirk.
> 
> I guess I can match on the VID/PID in usbnet, but won't it be cleaner
> to add a new bind() function (in cdc_ether) which matches the two PIDs
> and leave usbnet as is? Or am I misunderstanding how to add this
> functionality to usbnet?

It would be cleaner, but it seems to me that multiple quirky devices
driven by diverse drivers use those bogus MACs. If you want to catch
them at a central place, it has to be usbnet. It is a matter of taste.
I am fine with either solution.

Regards
Oliver

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-07-18 at 17:04 +0200, Kristian Evensen wrote:
> On Mon, Jul 18, 2016 at 4:14 PM, Oliver Neukum <oneu...@suse.com> wrote:
> >> Ok, sounds good. So far, I have only seen the random MAC issue with
> >> the three previously mentioned devices, but who knows how many else is
> >> out there with the same error ... I don't think it should be in the
> >> core ethernet code, at least not yet, but I agree it would make sense
> >> to move it to for example usbnet_core(). If you agree, I can prepare a
> >> patch for it.
> >
> > I don't see how it would be specific for a subsystem. If the patch
> > is correct, it belongs into the networking core.
> 
> I had a look at some other drivers, and I think we need to be very
> careful about making setting a random MAC too generic. For example, we
> might be unlucky and break the possibly_iphdr()-code/assumption in
> qmi_wwan.c. And there is probably more code/assumptions like that in
> the network stack.

In this case please use special cases for usbnet, too.
We need a quirk.

Regards
Oliver

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-07-18 at 16:10 +0200, Kristian Evensen wrote:
> On Mon, Jul 18, 2016 at 3:50 PM, Oliver Neukum <oneu...@suse.com> wrote:
> > No, you misunderstand me. I don't want quirks if we can avoid it.
> > But if you need to do this for rndis_host and cdc_ether and maybe other
> > drivers you should not be touching drivers. This belongs into the core
> > ethernet code. Your code is good, but you are putting it in the wrong
> > places.
> 
> Ok, sounds good. So far, I have only seen the random MAC issue with
> the three previously mentioned devices, but who knows how many else is
> out there with the same error ... I don't think it should be in the
> core ethernet code, at least not yet, but I agree it would make sense
> to move it to for example usbnet_core(). If you agree, I can prepare a
> patch for it.

I don't see how it would be specific for a subsystem. If the patch
is correct, it belongs into the networking core.

Regards
Oliver

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-07-18 at 15:23 +0200, Kristian Evensen wrote:
> Hi,
> 
> On Mon, Jul 18, 2016 at 3:01 PM, Oliver Neukum <oneu...@suse.com> wrote:
> > On Mon, 2016-07-18 at 14:24 +0200, Kristian Evensen wrote:
> >> The firmware in the ZTE MF823/831/910 modems/mifis use OS fingerprinting to
> >> determine which type of device to export. In addition, these devices export
> >> a REST API which can also be used to control the type of device. So far, on
> >> Linux, the devices have been seen as RNDIS or CDC Ether.
> >>
> >> When CDC Ether is used, devices of the same type are, as with RNDIS,
> >> exported with the same, bogus random MAC address. In addition, the devices
> >
> > Please explain. If the MAC is random, I fail to see why the host would
> > be any better at making up a MAC.
> 
> Sorry for not explaining this properly. The problem is that all
> devices of the same type store the same value in iMACAddress. On all
> MF831/MF910s (that I have seen) 36:4b:50:b7:ef:da is stored in this
> value, while 36:4b:50:b7:ef:38 is stored on MF823. In order to ensure
> that all devices on a host has a unique MAC-address, I think it is
> better to generate a new random MAC on the host than to trust the
> value returned from the device.

OK, thanks for explaining.

> >> * If a random MAC is read from device, then generate a new random MAC
> >> address. This fix will apply to all devices using cdc_ether, but that
> >> should be ok as we will also fix similar mistakes from other
> >> manufacturers.
> >
> > I am not really happy with this.
> >
> > If this is specific to the device a quirk should be used. If it is a
> > truly generic misdesign, it does not belong into cdc-ether. It should go
> > into the generic layer.
> 
> We saw the same behaviour when these devices are exposed as RNDIS and
> decided to apply a generic fix instead of limiting ourself to for
> example the two, known bogus MAC address. See commit
> a5a18bdf7453d505783e40e47ebb84bfdd35f93b and the thread "[PATCH]
> rndis_host: Set random MAC for ZTE MF910"
> (http://www.spinics.net/lists/linux-usb/msg143727.html) for the
> discussion.
> 
> However, I have no strong objections towards for example checking
> against the two bad MAC addresses before generating a random MAC.

No, you misunderstand me. I don't want quirks if we can avoid it.
But if you need to do this for rndis_host and cdc_ether and maybe other
drivers you should not be touching drivers. This belongs into the core
ethernet code. Your code is good, but you are putting it in the wrong
places.

Regards
Oliver

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-07-18 at 14:24 +0200, Kristian Evensen wrote:
> The firmware in the ZTE MF823/831/910 modems/mifis use OS fingerprinting to
> determine which type of device to export. In addition, these devices export
> a REST API which can also be used to control the type of device. So far, on
> Linux, the devices have been seen as RNDIS or CDC Ether.
> 
> When CDC Ether is used, devices of the same type are, as with RNDIS,
> exported with the same, bogus random MAC address. In addition, the devices

Please explain. If the MAC is random, I fail to see why the host would
be any better at making up a MAC.

> (at least on all firmware revisions I have found) use this MAC when sending
> traffic routed from external networks. And as a final feature, the devices
> sometimes export the link state incorrectly.
> 
> This patch tries to improve the handling of these devices by doing the
> following:
> 
> * If a random MAC is read from device, then generate a new random MAC
> address. This fix will apply to all devices using cdc_ether, but that
> should be ok as we will also fix similar mistakes from other
> manufacturers.

I am not really happy with this.

If this is specific to the device a quirk should be used. If it is a
truly generic misdesign, it does not belong into cdc-ether. It should go
into the generic layer.

> * The MF823/MF832/MF910 sometimes export cdc carrier on twice on connect
> (the correct behavior is off then on). Work around this by manually setting
> carrier to off if an on-notification is received and the NOCARRIER-bit is
> not set.
> 
> This change will also affect all devices, but as with the MAC-fix it should
> take care of similar mistakes. I tried to think of/look/test for
> problems/regressions that could be introduced by this behavior, but could
> not find any. However, my familiarity with this code path is not that
> great, so there could be something I have overlooked.

Looks OK

> * Add an rx_fixup-function (and a new driver info-struct) which is used by
> these three devices (identified either as PID 0x1405 or 0x1408). The
> rx_fixup-function replaces the destination MAC address in the skb with that
> of the device. I have not seen a revision of these three device that
> behaves correctly (i.e., sets the right destination MAC), so I chose not to
> do any comparison with for example the known, bogus addresses.

Looks OK

Regards
Oliver

[PATCH 2/5] cdc-acm: use the common parser

[Oliver Neukum]

This introduces the common parser for extra CDC headers now that it no longer
depends on usbnet.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/usb/class/cdc-acm.c | 69 +++--
 1 file changed, 10 insertions(+), 59 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 94a14f5..70bd642 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1147,6 +1147,7 @@ static int acm_probe(struct usb_interface *intf,
 {
struct usb_cdc_union_desc *union_header = NULL;
struct usb_cdc_country_functional_desc *cfd = NULL;
+   struct usb_cdc_call_mgmt_descriptor *cmd = NULL;
unsigned char *buffer = intf->altsetting->extra;
int buflen = intf->altsetting->extralen;
struct usb_interface *control_interface;
@@ -1155,18 +1156,16 @@ static int acm_probe(struct usb_interface *intf,
struct usb_endpoint_descriptor *epread = NULL;
struct usb_endpoint_descriptor *epwrite = NULL;
struct usb_device *usb_dev = interface_to_usbdev(intf);
+   struct usb_cdc_parsed_header hdr;
struct acm *acm;
int minor;
int ctrlsize, readsize;
u8 *buf;
-   u8 ac_management_function = 0;
-   u8 call_management_function = 0;
int call_interface_num = -1;
int data_interface_num = -1;
unsigned long quirks;
int num_rx_buf;
int i;
-   unsigned int elength = 0;
int combined_interfaces = 0;
struct device *tty_dev;
int rv = -ENOMEM;
@@ -1210,61 +1209,11 @@ static int acm_probe(struct usb_interface *intf,
}
}
 
-   while (buflen > 0) {
-   elength = buffer[0];
-   if (!elength) {
-   dev_err(>dev, "skipping garbage byte\n");
-   elength = 1;
-   goto next_desc;
-   }
-   if (buffer[1] != USB_DT_CS_INTERFACE) {
-   dev_err(>dev, "skipping garbage\n");
-   goto next_desc;
-   }
-
-   switch (buffer[2]) {
-   case USB_CDC_UNION_TYPE: /* we've found it */
-   if (elength < sizeof(struct usb_cdc_union_desc))
-   goto next_desc;
-   if (union_header) {
-   dev_err(>dev, "More than one "
-   "union descriptor, skipping ...\n");
-   goto next_desc;
-   }
-   union_header = (struct usb_cdc_union_desc *)buffer;
-   break;
-   case USB_CDC_COUNTRY_TYPE: /* export through sysfs*/
-   if (elength < sizeof(struct 
usb_cdc_country_functional_desc))
-   goto next_desc;
-   cfd = (struct usb_cdc_country_functional_desc *)buffer;
-   break;
-   case USB_CDC_HEADER_TYPE: /* maybe check version */
-   break; /* for now we ignore it */
-   case USB_CDC_ACM_TYPE:
-   if (elength < 4)
-   goto next_desc;
-   ac_management_function = buffer[3];
-   break;
-   case USB_CDC_CALL_MANAGEMENT_TYPE:
-   if (elength < 5)
-   goto next_desc;
-   call_management_function = buffer[3];
-   call_interface_num = buffer[4];
-   break;
-   default:
-   /*
-* there are LOTS more CDC descriptors that
-* could legitimately be found here.
-*/
-   dev_dbg(>dev, "Ignoring descriptor: "
-   "type %02x, length %ud\n",
-   buffer[2], elength);
-   break;
-   }
-next_desc:
-   buflen -= elength;
-   buffer += elength;
-   }
+   cdc_parse_cdc_header(, intf, buffer, buflen);
+   union_header = hdr.usb_cdc_union_desc;
+   cmd = hdr.usb_cdc_call_mgmt_descriptor;
+   if (cmd)
+   call_interface_num = cmd->bDataInterface;
 
if (!union_header) {
if (call_interface_num > 0) {
@@ -1394,7 +1343,8 @@ made_compressed_probe:
acm->data = data_interface;
acm->minor = minor;
acm->dev = usb_dev;
-   acm->ctrl_caps = ac_management_function;
+   if (hdr.usb_cdc_acm_descriptor)
+   acm->ctrl_caps = hdr.usb_cdc_acm_descriptor->bmCapabilities;
if (quirks & NO_CAP_LINE)

[PATCH 0 / 5] move the common CDC parser

[Oliver Neukum]

Experience has shown that making all CDC drivers depend on usbnet
is not practical, because some of them are not network drivers.
So this patch moves the common parser from usbnet into the messages
helpers of usbcore.
The rest of the series applies it to the non-network CDC drivers.

I hope it can go through Greg's tree although it touches usbnet.

[PATCH 1/5] usbnet: move the CDC parser into USB core

[Oliver Neukum]

The dependencies were impossible to handle preventing
drivers for CDC devices not which are not network drivers
from using the common parser.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/usbnet.c   | 138 
 drivers/usb/core/message.c | 153 +
 2 files changed, 153 insertions(+), 138 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 61ba464..b56d78a 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -42,7 +42,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -1968,143 +1967,6 @@ out:
return err;
 }
 
-int cdc_parse_cdc_header(struct usb_cdc_parsed_header *hdr,
-   struct usb_interface *intf,
-   u8 *buffer,
-   int buflen)
-{
-   /* duplicates are ignored */
-   struct usb_cdc_union_desc *union_header = NULL;
-
-   /* duplicates are not tolerated */
-   struct usb_cdc_header_desc *header = NULL;
-   struct usb_cdc_ether_desc *ether = NULL;
-   struct usb_cdc_mdlm_detail_desc *detail = NULL;
-   struct usb_cdc_mdlm_desc *desc = NULL;
-
-   unsigned int elength;
-   int cnt = 0;
-
-   memset(hdr, 0x00, sizeof(struct usb_cdc_parsed_header));
-   hdr->phonet_magic_present = false;
-   while (buflen > 0) {
-   elength = buffer[0];
-   if (!elength) {
-   dev_err(>dev, "skipping garbage byte\n");
-   elength = 1;
-   goto next_desc;
-   }
-   if (buffer[1] != USB_DT_CS_INTERFACE) {
-   dev_err(>dev, "skipping garbage\n");
-   goto next_desc;
-   }
-
-   switch (buffer[2]) {
-   case USB_CDC_UNION_TYPE: /* we've found it */
-   if (elength < sizeof(struct usb_cdc_union_desc))
-   goto next_desc;
-   if (union_header) {
-   dev_err(>dev, "More than one union 
descriptor, skipping ...\n");
-   goto next_desc;
-   }
-   union_header = (struct usb_cdc_union_desc *)buffer;
-   break;
-   case USB_CDC_COUNTRY_TYPE:
-   if (elength < sizeof(struct 
usb_cdc_country_functional_desc))
-   goto next_desc;
-   hdr->usb_cdc_country_functional_desc =
-   (struct usb_cdc_country_functional_desc 
*)buffer;
-   break;
-   case USB_CDC_HEADER_TYPE:
-   if (elength != sizeof(struct usb_cdc_header_desc))
-   goto next_desc;
-   if (header)
-   return -EINVAL;
-   header = (struct usb_cdc_header_desc *)buffer;
-   break;
-   case USB_CDC_ACM_TYPE:
-   if (elength < sizeof(struct usb_cdc_acm_descriptor))
-   goto next_desc;
-   hdr->usb_cdc_acm_descriptor =
-   (struct usb_cdc_acm_descriptor *)buffer;
-   break;
-   case USB_CDC_ETHERNET_TYPE:
-   if (elength != sizeof(struct usb_cdc_ether_desc))
-   goto next_desc;
-   if (ether)
-   return -EINVAL;
-   ether = (struct usb_cdc_ether_desc *)buffer;
-   break;
-   case USB_CDC_CALL_MANAGEMENT_TYPE:
-   if (elength < sizeof(struct 
usb_cdc_call_mgmt_descriptor))
-   goto next_desc;
-   hdr->usb_cdc_call_mgmt_descriptor =
-   (struct usb_cdc_call_mgmt_descriptor *)buffer;
-   break;
-   case USB_CDC_DMM_TYPE:
-   if (elength < sizeof(struct usb_cdc_dmm_desc))
-   goto next_desc;
-   hdr->usb_cdc_dmm_desc =
-   (struct usb_cdc_dmm_desc *)buffer;
-   break;
-   case USB_CDC_MDLM_TYPE:
-   if (elength < sizeof(struct usb_cdc_mdlm_desc *))
-   goto next_desc;
-   if (desc)
-   return -EINVAL;
-   desc = (struct usb_cdc_mdlm_desc *)buffer;
-   break;
-   case USB_CDC_MDLM_DETAIL_TYPE:
-   if (elength < sizeof(struct usb_cdc_mdlm_detail_desc *)

[PATCH 3/5] cdc-acm: cleanup error handling

[Oliver Neukum]

A small update to unify error handling during probe().

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/usb/class/cdc-acm.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 70bd642..1857fad 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1328,11 +1328,8 @@ made_compressed_probe:
goto alloc_fail;
 
minor = acm_alloc_minor(acm);
-   if (minor < 0) {
-   dev_err(>dev, "no more free acm devices\n");
-   kfree(acm);
-   return -ENODEV;
-   }
+   if (minor < 0)
+   goto alloc_fail1;
 
ctrlsize = usb_endpoint_maxp(epctrl);
readsize = usb_endpoint_maxp(epread) *
@@ -1523,6 +1520,7 @@ alloc_fail4:
usb_free_coherent(usb_dev, ctrlsize, acm->ctrl_buffer, acm->ctrl_dma);
 alloc_fail2:
acm_release_minor(acm);
+alloc_fail1:
kfree(acm);
 alloc_fail:
return rv;
-- 
2.1.4

[PATCH 5/5] cdc-acm: beautify probe()

[Oliver Neukum]

This removes some overly long lines by renaming variables and giving
them local scope.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/usb/class/cdc-acm.c | 44 
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 1857fad..369bde0 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1146,8 +1146,7 @@ static int acm_probe(struct usb_interface *intf,
 const struct usb_device_id *id)
 {
struct usb_cdc_union_desc *union_header = NULL;
-   struct usb_cdc_country_functional_desc *cfd = NULL;
-   struct usb_cdc_call_mgmt_descriptor *cmd = NULL;
+   struct usb_cdc_call_mgmt_descriptor *cmgmd = NULL;
unsigned char *buffer = intf->altsetting->extra;
int buflen = intf->altsetting->extralen;
struct usb_interface *control_interface;
@@ -1156,13 +1155,13 @@ static int acm_probe(struct usb_interface *intf,
struct usb_endpoint_descriptor *epread = NULL;
struct usb_endpoint_descriptor *epwrite = NULL;
struct usb_device *usb_dev = interface_to_usbdev(intf);
-   struct usb_cdc_parsed_header hdr;
+   struct usb_cdc_parsed_header h;
struct acm *acm;
int minor;
int ctrlsize, readsize;
u8 *buf;
-   int call_interface_num = -1;
-   int data_interface_num = -1;
+   int call_intf_num = -1;
+   int data_intf_num = -1;
unsigned long quirks;
int num_rx_buf;
int i;
@@ -1209,20 +1208,22 @@ static int acm_probe(struct usb_interface *intf,
}
}
 
-   cdc_parse_cdc_header(, intf, buffer, buflen);
-   union_header = hdr.usb_cdc_union_desc;
-   cmd = hdr.usb_cdc_call_mgmt_descriptor;
-   if (cmd)
-   call_interface_num = cmd->bDataInterface;
+   cdc_parse_cdc_header(, intf, buffer, buflen);
+   union_header = h.usb_cdc_union_desc;
+   cmgmd = h.usb_cdc_call_mgmt_descriptor;
+   if (cmgmd)
+   call_intf_num = cmgmd->bDataInterface;
 
if (!union_header) {
-   if (call_interface_num > 0) {
+   if (call_intf_num > 0) {
dev_dbg(>dev, "No union descriptor, using call 
management descriptor\n");
/* quirks for Droids MuIn LCD */
-   if (quirks & NO_DATA_INTERFACE)
+   if (quirks & NO_DATA_INTERFACE) {
data_interface = usb_ifnum_to_if(usb_dev, 0);
-   else
-   data_interface = usb_ifnum_to_if(usb_dev, 
(data_interface_num = call_interface_num));
+   } else {
+   data_intf_num = call_intf_num;
+   data_interface = usb_ifnum_to_if(usb_dev, 
data_intf_num);
+   }
control_interface = intf;
} else {
if (intf->cur_altsetting->desc.bNumEndpoints != 3) {
@@ -1236,8 +1237,9 @@ static int acm_probe(struct usb_interface *intf,
}
}
} else {
+   data_intf_num = union_header->bSlaveInterface0;
control_interface = usb_ifnum_to_if(usb_dev, 
union_header->bMasterInterface0);
-   data_interface = usb_ifnum_to_if(usb_dev, (data_interface_num = 
union_header->bSlaveInterface0));
+   data_interface = usb_ifnum_to_if(usb_dev, data_intf_num);
}
 
if (!control_interface || !data_interface) {
@@ -1245,7 +1247,7 @@ static int acm_probe(struct usb_interface *intf,
return -ENODEV;
}
 
-   if (data_interface_num != call_interface_num)
+   if (data_intf_num != call_intf_num)
dev_dbg(>dev, "Separate call control interface. That is 
not fully supported.\n");
 
if (control_interface == data_interface) {
@@ -1340,8 +1342,8 @@ made_compressed_probe:
acm->data = data_interface;
acm->minor = minor;
acm->dev = usb_dev;
-   if (hdr.usb_cdc_acm_descriptor)
-   acm->ctrl_caps = hdr.usb_cdc_acm_descriptor->bmCapabilities;
+   if (h.usb_cdc_acm_descriptor)
+   acm->ctrl_caps = h.usb_cdc_acm_descriptor->bmCapabilities;
if (quirks & NO_CAP_LINE)
acm->ctrl_caps &= ~USB_CDC_CAP_LINE;
acm->ctrlsize = ctrlsize;
@@ -1435,8 +1437,10 @@ made_compressed_probe:
if (i < 0)
goto alloc_fail7;
 
-   cfd = hdr.usb_cdc_country_functional_desc;
-   if (cfd) { /* export the country data */
+   if (h.usb_cdc_country_functional_desc) { /* export the country data */
+   struct usb_cdc_country_functional_desc * cfd =
+

[PATCH 4/5] cdc-wdm: use the common CDC parser

[Oliver Neukum]

Now that the common parser resides in USB core, it can
be used for CDC-WDM.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/usb/class/cdc-wdm.c | 30 +-
 1 file changed, 5 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 61ea879..337948c 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -875,38 +875,18 @@ static int wdm_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
int rv = -EINVAL;
struct usb_host_interface *iface;
struct usb_endpoint_descriptor *ep;
-   struct usb_cdc_dmm_desc *dmhd;
+   struct usb_cdc_parsed_header hdr;
u8 *buffer = intf->altsetting->extra;
int buflen = intf->altsetting->extralen;
u16 maxcom = WDM_DEFAULT_BUFSIZE;
 
if (!buffer)
goto err;
-   while (buflen > 2) {
-   if (buffer[1] != USB_DT_CS_INTERFACE) {
-   dev_err(>dev, "skipping garbage\n");
-   goto next_desc;
-   }
 
-   switch (buffer[2]) {
-   case USB_CDC_HEADER_TYPE:
-   break;
-   case USB_CDC_DMM_TYPE:
-   dmhd = (struct usb_cdc_dmm_desc *)buffer;
-   maxcom = le16_to_cpu(dmhd->wMaxCommand);
-   dev_dbg(>dev,
-   "Finding maximum buffer length: %d", maxcom);
-   break;
-   default:
-   dev_err(>dev,
-   "Ignoring extra header, type %d, length %d\n",
-   buffer[2], buffer[0]);
-   break;
-   }
-next_desc:
-   buflen -= buffer[0];
-   buffer += buffer[0];
-   }
+   cdc_parse_cdc_header(, intf, buffer, buflen);
+
+   if (hdr.usb_cdc_dmm_desc)
+   maxcom = le16_to_cpu(hdr.usb_cdc_dmm_desc->wMaxCommand);
 
iface = intf->cur_altsetting;
if (iface->desc.bNumEndpoints != 1)
-- 
2.1.4

Re: [PATCH net,stable] cdc_ncm: workaround for EM7455 "silent" data interface

[Oliver Neukum]

On Mon, 2016-07-04 at 13:40 +0200, Bjørn Mork wrote:
> Oliver Neukum <oneu...@suse.com> writes:
> > On Sun, 2016-07-03 at 22:24 +0200, Bjørn Mork wrote:
> >> Several Lenovo users have reported problems with their Sierra
> >> Wireless EM7455 modem. The driver has loaded successfully and
> >> the MBIM management channel has appeared to work, including
> >> establishing a connection to the mobile network. But no frames
> >> have been received over the data interface.
> >
> > If this is needed in open() it must also be needed in reset_resume()
> 
> reset_resume needs fixing in general. This is completely unrelated to
> this bug. In fact, as we don't do any NCM control messages there in the

I see. In general this would be a problem.

> current resume, I'm not sure this firmware bug is triggered by it.  But
> it's untestable, because:

OK, something needs to be done, but this patch is not the place to do
it.

Regards
Oliver

Re: [PATCH net,stable] cdc_ncm: workaround for EM7455 "silent" data interface

[Oliver Neukum]

On Sun, 2016-07-03 at 22:24 +0200, Bjørn Mork wrote:
> Several Lenovo users have reported problems with their Sierra
> Wireless EM7455 modem. The driver has loaded successfully and
> the MBIM management channel has appeared to work, including
> establishing a connection to the mobile network. But no frames
> have been received over the data interface.

If this is needed in open() it must also be needed in reset_resume()

Regards
Oliver

Re: [1/1] net: pegasus: remove dead coding

[Oliver Neukum]

On Tue, 2016-05-17 at 23:30 -0700, Guenter Roeck wrote:
> On Wed, May 18, 2016 at 02:13:30AM +0200, Heinrich Schuchardt wrote:
> > (!count || count < 4) is always true.
> 
> Even if count >= 4 ?

The check for !count is redundant, though. Gcc, however,
will surely simplify the expression.

Regards
Oliver

[PATCH 1/3] brcm80211: correct speed testing

[Oliver Neukum]

Allow for SS+ USB

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 869eb82..056bda3 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1368,7 +1368,9 @@ brcmf_usb_probe(struct usb_interface *intf, const struct 
usb_device_id *id)
 
devinfo->ifnum = desc->bInterfaceNumber;
 
-   if (usb->speed == USB_SPEED_SUPER)
+   if (usb->speed == USB_SPEED_SUPER_PLUS)
+   brcmf_dbg(USB, "Broadcom super speed plus USB WLAN interface 
detected\n");
+   else if (usb->speed == USB_SPEED_SUPER)
brcmf_dbg(USB, "Broadcom super speed USB WLAN interface 
detected\n");
else if (usb->speed == USB_SPEED_HIGH)
brcmf_dbg(USB, "Broadcom high speed USB WLAN interface 
detected\n");
-- 
2.1.4

[PATCH 3/3] rtl8152: correct speed testing

[Oliver Neukum]

Allow for SS+ USB

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/net/usb/r8152.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index d1f78c2..3f9f6ed 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3366,7 +3366,7 @@ static void r8153_init(struct r8152 *tp)
ocp_write_word(tp, MCU_TYPE_PLA, PLA_LED_FEATURE, ocp_data);
 
ocp_data = FIFO_EMPTY_1FB | ROK_EXIT_LPM;
-   if (tp->version == RTL_VER_04 && tp->udev->speed != USB_SPEED_SUPER)
+   if (tp->version == RTL_VER_04 && tp->udev->speed < USB_SPEED_SUPER)
ocp_data |= LPM_TIMER_500MS;
else
ocp_data |= LPM_TIMER_500US;
@@ -4211,6 +4211,7 @@ static int rtl8152_probe(struct usb_interface *intf,
 
switch (udev->speed) {
case USB_SPEED_SUPER:
+   case USB_SPEED_SUPER_PLUS:
tp->coalesce = COALESCE_SUPER;
break;
case USB_SPEED_HIGH:
-- 
2.1.4