Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[Ben Hutchings]

On Tue, 2016-03-08 at 21:18 +0100, Bjørn Mork wrote:
> Linus Torvalds  writes:
> 
> > 
> > So looking at this, I wonder...
> > 
> > Why is that FLAG_LINK_INTR thing not just always used?
> > 
> > The _only_ thing that FLAG_LINK_INTR does is to cause
> > 
> > usbnet_link_change(dev, 0, 0);
> > 
> > to be called after network device attach. That doesn't seem to be 
> > controversial.
> Not all usbnet drivers support carrier detection, which is required to
> ever bring the link up again.
> 
> > 
> > Looking at some examples, we have ax88179_178a.c that doesn't set the
> > flag, but instead does that usbnet_link_change() call at the end of
> > ax88179_bind().
> > 
> > There are a few drivers that seem to never call that
> > usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag.
> > Would they break?
> Yes.  Drivers without carrier detection will be "down" forever.
> 
> > 
> > Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be
> > anything "INTR" about it.
> Beats me.  I can only say that I always find naming difficult...
> We could ask Ben, who introduced it in:
[...]

It is supposed to imply that the device generates link-change
interrupts.  Of course it is also possible for a device driver to
satisfy the requirement by polling the link state.

Ben.

-- 
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[Oliver Neukum]

On Tue, 2016-03-08 at 21:18 +0100, Bjørn Mork wrote:
> > Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be
> > anything "INTR" about it.
> 
> Beats me.  I can only say that I always find naming difficult...
> We could ask Ben, who introduced it in:

It used to be done over USB interrupt endpoints.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[Bjørn Mork]

Linus Torvalds  writes:

> So looking at this, I wonder...
>
> Why is that FLAG_LINK_INTR thing not just always used?
>
> The _only_ thing that FLAG_LINK_INTR does is to cause
>
> usbnet_link_change(dev, 0, 0);
>
> to be called after network device attach. That doesn't seem to be 
> controversial.

Not all usbnet drivers support carrier detection, which is required to
ever bring the link up again.

> Looking at some examples, we have ax88179_178a.c that doesn't set the
> flag, but instead does that usbnet_link_change() call at the end of
> ax88179_bind().
>
> There are a few drivers that seem to never call that
> usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag.
> Would they break?

Yes.  Drivers without carrier detection will be "down" forever.

> Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be
> anything "INTR" about it.

Beats me.  I can only say that I always find naming difficult...
We could ask Ben, who introduced it in:


commit 37e8273cd30592d3a82bcb70cbb1bdc4eaeb6b71
Author: Ben Hutchings 
Date:   Wed Nov 4 15:29:52 2009 +

usbnet: Set link down initially for drivers that update link state

Some usbnet drivers update link state while others do not due to
hardware limitations.  Add a flag to distinguish those that do, and
set the link down initially for their devices.

This is intended to fix this bug: http://bugs.debian.org/444043

Signed-off-by: Ben Hutchings 
Signed-off-by: David S. Miller 



But I guess it doesn't really matter.



Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[Oliver Neukum]

On Tue, 2016-03-08 at 11:43 -0800, Linus Torvalds wrote:
> Why is that FLAG_LINK_INTR thing not just always used?
> 
> The _only_ thing that FLAG_LINK_INTR does is to cause
> 
> usbnet_link_change(dev, 0, 0);
> 
> to be called after network device attach. That doesn't seem to be
> controversial.

It depends on the devices' capabilities.
For a few old devices that would be deadly, because they cannot
indicate that a link goes up again.

> Looking at some examples, we have ax88179_178a.c that doesn't set the
> flag, but instead does that usbnet_link_change() call at the end of
> ax88179_bind().
> 
> There are a few drivers that seem to never call that
> usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag.
> Would they break?

Yes. If we did the call unconditionally. We could not do it,
then we'd see some spurious detection of interfaces being up,
but something breaks. Today I'd probably require a flag
for those cases that do not want the call to be made, but the
distinction as such is necessary.

Regards
Oliver



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[Linus Torvalds]

On Mon, Mar 7, 2016 at 12:15 PM, Bjørn Mork  wrote:
> usbnet_link_change will call schedule_work and should be
> avoided if bind is failing. Otherwise we will end up with
> scheduled work referring to a netdev which has gone away.
>
> Instead of making the call conditional, we can just defer
> it to usbnet_probe, using the driver_info flag made for
> this purpose.

So looking at this, I wonder...

Why is that FLAG_LINK_INTR thing not just always used?

The _only_ thing that FLAG_LINK_INTR does is to cause

usbnet_link_change(dev, 0, 0);

to be called after network device attach. That doesn't seem to be controversial.

Looking at some examples, we have ax88179_178a.c that doesn't set the
flag, but instead does that usbnet_link_change() call at the end of
ax88179_bind().

There are a few drivers that seem to never call that
usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag.
Would they break?

Stupid grep:

git grep -lw FLAG_ETHER |
xargs grep -L FLAG_LINK_INTR |
xargs grep -L usbnet_link_change |
sed 's:drivers/net/usb/::'

gives

cdc_eem.c
ch9200.c
cx82310_eth.c
int51x1.c
rndis_host.c

so maybe that FLAG_LINK_INTR si required.

Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be
anything "INTR" about it.

   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[David Miller]

From: Bjørn Mork 
Date: Mon, 7 Mar 2016 21:15:36 +0100

> usbnet_link_change will call schedule_work and should be
> avoided if bind is failing. Otherwise we will end up with
> scheduled work referring to a netdev which has gone away.
> 
> Instead of making the call conditional, we can just defer
> it to usbnet_probe, using the driver_info flag made for
> this purpose.
> 
> Fixes: 8a34b0ae8778 ("usbnet: cdc_ncm: apply usbnet_link_change")
> Reported-by: Andrey Konovalov 
> Suggested-by: Linus Torvalds 
> Signed-off-by: Bjørn Mork 
...
> Even with Oliver's generic fix we should still fix the inconsistency
> in cdc_ncm, as pointed out by Linus.
> 
> This is a slightly different approach than the patch proposed by Linus.
> When I started looking at this I couldn't figure out why we were doing
> this differently in this driver from all the other usbnet drivers
> disabling the link at probe time.  So let's make it consistent.  Then at
> least we get consistent bugs :)

Fair enough, applied and queued up for -stable.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html