* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs
From: Thomas Petazzoni @ 2013-09-18 16:11 UTC (permalink / raw)
To: Grant Likely
Cc: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Lior Amsalem, Mark Rutland,
Sascha Hauer, Christian Gmeiner, Ezequiel Garcia, Gregory Clement,
Florian Fainelli,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20130918042923.5D845C42CF7-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
Dear Grant Likely,
On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote:
> I understand what you're trying to do here, but it causes a troublesome
> leakage of implementation detail into the binding, making the whole
> thing look very odd. This binding tries to make a fixed link look
> exactly like a real PHY even to the point of including a phandle to the
> phy. But having a phandle to a node which is *always* a direct child of
> the MAC node is redundant and a rather looney. Yes, doing it that way
> makes it easy for of_phy_find_device() to be transparent for fixed link,
> but that should *not* drive bindings, especially when that makes the
> binding really rather weird.
>
> Second, this new binding doesn't provide anything over and above the
> existing fixed-link binding. It may not be pretty, but it is
> estabilshed.
Have you followed the past discussions about this patch set? Basically
the *only* feedback I received on RFCv1 is that the fixed-link property
sucks, and everybody (including the known Device Tree binding
maintainer Mark Rutland) suggested to not use the fixed-link mechanism.
See http://article.gmane.org/gmane.linux.network/276932, where Mark
said:
""
I'm not sure grouping these values together is the best way of handling
this. It's rather opaque, and inflexible for future extension.
""
So, please DT maintainers, tell me what you want. I honestly don't care
whether fixed-link or a separate node is chosen. However, I do care
about being dragged around between two solutions just because the
former DT maintainer and the new DT maintainers do not agree.
Thanks,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] USBNET: fix handling padding packet
From: David Miller @ 2013-09-18 16:12 UTC (permalink / raw)
To: bjorn; +Cc: ming.lei, gregkh, oneukum, netdev, linux-usb, oliver
In-Reply-To: <87zjra5dpx.fsf@nemi.mork.no>
From: Bjørn Mork <bjorn@mork.no>
Date: Wed, 18 Sep 2013 17:52:42 +0200
> Ming Lei <ming.lei@canonical.com> writes:
>
>> There is no reason to forbid DMA SG for one driver which requires
>> padding, right?
>
> Yes there is: Added complexity for everybody, based on a combination of
> features which just does not make any sense.
>
> No modern device should need the padding. No old device will be able to
> use the SG feature as implemented. You only enable it on USB3, don't
> you? If this feature is restricted to USB3 capable devices, then it most
> certainly can be restricted to ZLP capable devices with absolutely no
> difference in the resulting set of supported devices.
I completely agree.
^ permalink raw reply
* Re: [PATCH -net] netpoll: fix NULL pointer dereference in netpoll_cleanup
From: David Miller @ 2013-09-18 16:15 UTC (permalink / raw)
To: nikolay; +Cc: netdev
In-Reply-To: <1379427155-8561-1-git-send-email-nikolay@redhat.com>
From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Tue, 17 Sep 2013 16:12:35 +0200
> @@ -1284,15 +1284,15 @@ EXPORT_SYMBOL_GPL(__netpoll_free_async);
>
> void netpoll_cleanup(struct netpoll *np)
> {
> - if (!np->dev)
> - return;
> -
> rtnl_lock();
> + if (!np->dev) {
> + rtnl_unlock();
> + return;
> + }
> __netpoll_cleanup(np);
> - rtnl_unlock();
> -
> dev_put(np->dev);
> np->dev = NULL;
> + rtnl_unlock();
> }
> EXPORT_SYMBOL(netpoll_cleanup);
I know it seems arbitrary, but please do this like:
lock();
if (condition) {
...
}
unlock();
rather than having multiple return/unlock code paths.
This style I am suggesting is easier to audit for locking problems.
Thanks.
^ permalink raw reply
* Re: [PATCH net] ip: generate unique IP identificator if local fragmentation is allowed
From: David Miller @ 2013-09-18 16:20 UTC (permalink / raw)
To: aatteka; +Cc: netdev
In-Reply-To: <1379456700-8033-1-git-send-email-aatteka@nicira.com>
From: Ansis Atteka <aatteka@nicira.com>
Date: Tue, 17 Sep 2013 15:25:00 -0700
> @@ -1317,6 +1317,7 @@ struct sk_buff *__ip_make_skb(struct sock *sk,
> ttl = ip_select_ttl(inet, &rt->dst);
>
> iph = (struct iphdr *)skb->data;
> +
> iph->version = 4;
> iph->ihl = 5;
> iph->tos = inet->tos;
> @@ -1324,7 +1325,7 @@ struct sk_buff *__ip_make_skb(struct sock *sk,
> iph->ttl = ttl;
> iph->protocol = sk->sk_protocol;
> ip_copy_addrs(iph, fl4);
> - ip_select_ident(iph, &rt->dst, sk);
> + ip_select_ident(skb, &rt->dst, sk);
>
> if (opt) {
> iph->ihl += opt->optlen>>2;
This transformation is not equivalent.
ip_select_ident() is going to use ip_hdr() which obtains the header
pointer via skb_network_header().
But here, the code is using skb->data for this, which is different.
^ permalink raw reply
* Re: [PATCH Resend 1/3] net: cdc-phonet: Staticize usbpn_probe
From: David Miller @ 2013-09-18 16:21 UTC (permalink / raw)
To: sachin.kamat; +Cc: netdev, remi
In-Reply-To: <1379475001-24505-1-git-send-email-sachin.kamat@linaro.org>
From: Sachin Kamat <sachin.kamat@linaro.org>
Date: Wed, 18 Sep 2013 08:59:59 +0530
> @@ -328,7 +328,8 @@ MODULE_DEVICE_TABLE(usb, usbpn_ids);
>
> static struct usb_driver usbpn_driver;
>
> -int usbpn_probe(struct usb_interface *intf, const struct usb_device_id *id)
> +static int
> +usbpn_probe(struct usb_interface *intf, const struct usb_device_id *id)
Please keep it to one line, it's 82 columns which is fine.
^ permalink raw reply
* Re: pull request net: batman-adv 2013-09-18
From: David Miller @ 2013-09-18 16:23 UTC (permalink / raw)
To: ordex; +Cc: netdev, b.a.t.m.a.n
In-Reply-To: <1379485658-2317-1-git-send-email-ordex@autistici.org>
From: Antonio Quartulli <ordex@autistici.org>
Date: Wed, 18 Sep 2013 08:27:37 +0200
> this is a very small (but important) fix intended for net/linux-3.{11,12} (since
> 3.12-rc1 is out I guess this patch needs to be enqueued for stable in order to
> reach 3.11?)
>
> This change fixes a regression introduced in 3.11 that prevents the Bridge Loop
> Avoidance component from correctly operate on VLANs.
Pulled and queued up for -stable.
^ permalink raw reply
* Re: [PATCH net 0/2] Fix cnic regressions
From: David Miller @ 2013-09-18 16:24 UTC (permalink / raw)
To: mchan; +Cc: netdev
In-Reply-To: <1379494239-5481-1-git-send-email-mchan@broadcom.com>
From: "Michael Chan" <mchan@broadcom.com>
Date: Wed, 18 Sep 2013 01:50:37 -0700
> Fix regressions caused by Doorbell change and macro change.
Both applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down
From: Ian Campbell @ 2013-09-18 16:25 UTC (permalink / raw)
To: Paul Durrant
Cc: Wei Liu, xen-devel@lists.xen.org, netdev@vger.kernel.org,
David Vrabel
In-Reply-To: <9AAE0902D5BC7E449B7C8E4E778ABCD010D0DB@AMSPEX01CL01.citrite.net>
On Wed, 2013-09-18 at 17:04 +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Sent: 18 September 2013 16:58
> > To: Wei Liu
> > Cc: Paul Durrant; xen-devel@lists.xen.org; netdev@vger.kernel.org; David
> > Vrabel
> > Subject: Re: [PATCH net-next v4] Don't destroy the netdev until the vif is
> > shut down
> >
> > On Wed, 2013-09-18 at 11:37 +0100, Wei Liu wrote:
> > > On Tue, Sep 17, 2013 at 05:46:08PM +0100, Paul Durrant wrote:
> > > > Without this patch, if a frontend cycles through states Closing
> > > > and Closed (which Windows frontends need to do) then the netdev
> > > > will be destroyed and requires re-invocation of hotplug scripts
> > > > to restore state before the frontend can move to Connected. Thus
> > > > when udev is not in use the backend gets stuck in InitWait.
> > > >
> > > > With this patch, the netdev is left alone whilst the backend is
> > > > still online and is only de-registered and freed just prior to
> > > > destroying the vif (which is also nicely symmetrical with the
> > > > netdev allocation and registration being done during probe) so
> > > > no re-invocation of hotplug scripts is required.
> > > >
> > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > > Cc: David Vrabel <david.vrabel@citrix.com>
> > > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > > Cc: Ian Campbell <ian.campbell@citrix.com>
> > >
> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> >
> > yeah, looks good, thanks.
> >
> > Paul, did you test this with non-Windows frontends too? and do things
> > like vif hot(un)plug still work?
> >
>
> Ian,
>
> I tested with a debian (wheezy) PV guest but I didn't test unplug. I
> cycled a windows frontend several times (which is how I spotted the
> tx_irq thing), and shutdown and brought up both the debian and windows
> guests several times. I can test unplug too if you'd like.
I don't think it needs to be a blocker for accepting this patch but it
would be good to try it, it's the sort of area which historically gets
broken by this sort of change.
Ian.
>
> Paul
^ permalink raw reply
* Re: [PATCH -net] netpoll: fix NULL pointer dereference in netpoll_cleanup
From: Joe Perches @ 2013-09-18 16:25 UTC (permalink / raw)
To: David Miller; +Cc: nikolay, netdev
In-Reply-To: <20130918.121539.1213515779326200577.davem@davemloft.net>
On Wed, 2013-09-18 at 12:15 -0400, David Miller wrote:
> From: Nikolay Aleksandrov <nikolay@redhat.com>
> Date: Tue, 17 Sep 2013 16:12:35 +0200
>
> > @@ -1284,15 +1284,15 @@ EXPORT_SYMBOL_GPL(__netpoll_free_async);
> >
> > void netpoll_cleanup(struct netpoll *np)
> > {
> > - if (!np->dev)
> > - return;
> > -
> > rtnl_lock();
> > + if (!np->dev) {
> > + rtnl_unlock();
> > + return;
> > + }
> > __netpoll_cleanup(np);
> > - rtnl_unlock();
> > -
> > dev_put(np->dev);
> > np->dev = NULL;
> > + rtnl_unlock();
> > }
> > EXPORT_SYMBOL(netpoll_cleanup);
>
> I know it seems arbitrary, but please do this like:
>
> lock();
> if (condition) {
> ...
> }
> unlock();
>
> rather than having multiple return/unlock code paths.
>
> This style I am suggesting is easier to audit for locking problems.
Another alternative is:
lock();
if (!condition)
goto out;
...
out:
unlock();
return ...;
}
^ permalink raw reply
* Re: [PATCH ] net:dccp: do not report ICMP redirects to user space
From: David Miller @ 2013-09-18 16:34 UTC (permalink / raw)
To: duanj.fnst; +Cc: netdev, hannes
In-Reply-To: <5239968F.1070402@cn.fujitsu.com>
From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
Date: Wed, 18 Sep 2013 20:03:27 +0800
> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>
> DCCP shouldn't be setting sk_err on redirects as it
> isn't an error condition. it should be doing exactly
> what tcp is doing and leaving the error handler without
> touching the socket.
>
> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
Applied.
^ permalink raw reply
* Re: mvneta: oops in __rcu_read_lock on mirabox
From: Thomas Petazzoni @ 2013-09-18 16:35 UTC (permalink / raw)
To: Ethan Tuttle
Cc: Willy Tarreau, Russell King - ARM Linux, Andrew Lunn,
Jason Cooper, netdev, Ezequiel Garcia, Gregory Clément,
linux-arm-kernel
In-Reply-To: <CACzLR4t2ABZdRk40VUhLtDKXX2PQiSVoQZQ4coQkNkvETnY6Rw@mail.gmail.com>
Dear Ethan Tuttle,
On Tue, 17 Sep 2013 23:30:56 -0700, Ethan Tuttle wrote:
> On Mon, Sep 16, 2013 at 11:01 PM, Willy Tarreau <w@1wt.eu> wrote:
> > Next step should be that you test both kernels to be sure.
>
> Thanks for the kernel images, Willy. I'm still experimenting but
> initial results are strange: I haven't seen a crash from the -ethan
> image you provided, nor by a kernel with that config that I built
> myself. The config is only different from my crashing config by a few
> options. So perhaps some combination of options prevents the crash.
> I'll see if I can narrow it down.
A toolchain generating some crappy code maybe? Ethan, Willy, comparing
your toolchain (compiler version, origin of the toolchain) could be
interesting.
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH ] ipv6: handle the update of the NDISC_REDIRECT error code in icmpv6_err_convert
From: David Miller @ 2013-09-18 16:36 UTC (permalink / raw)
To: duanj.fnst; +Cc: netdev, hannes
In-Reply-To: <523996E0.9080702@cn.fujitsu.com>
From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
Date: Wed, 18 Sep 2013 20:04:48 +0800
> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>
> when dealing with redirect message in udpv6_err() and
> rawv6_err() the err shoud be assigned to 0, not EPROTO.
>
> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
No, you should fix this the same way you handled DCCP, but skipping
the whole socket error setting path for NDISC_REDIRECT.
Clearing the socket error to zero is not correct, it means you will
lose any existing error setting.
^ permalink raw reply
* Re: [PATCH] net: tsi108: Prevent compiler warning
From: David Miller @ 2013-09-18 16:38 UTC (permalink / raw)
To: thierry.reding; +Cc: netdev, linux-kernel
In-Reply-To: <1379508595-30252-1-git-send-email-treding@nvidia.com>
From: Thierry Reding <thierry.reding@gmail.com>
Date: Wed, 18 Sep 2013 14:49:55 +0200
> The dump_eth_one() function is only used if DEBUG is enabled, so protect
> it by a corresponding #ifdef DEBUG block.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
I would prefer that this function and the one call site is simply
removed instead.
This kind of thing can be provided via ethtool register dumps
and therefore without all of the ugliness this ad-hoc stuff
presents.
^ permalink raw reply
* Re: [PATCH] powerpc/83xx: gianfar_ptp: select 1588 clock source through dts file
From: Aida Mynzhasova @ 2013-09-18 16:39 UTC (permalink / raw)
To: Claudiu Manoil; +Cc: richardcochran, netdev
In-Reply-To: <5239CA4F.9000600@freescale.com>
On 18.09.2013 19:44, Claudiu Manoil wrote:
>
> On 9/18/2013 4:21 PM, Aida Mynzhasova wrote:
>> IEEE 1588 timer reference clock source is determined through hard-coded
>> value in gianfar_ptp driver by default. This patch allows to select ptp
>> clock source by means of device tree file node.
>>
>> For instance:
>>
>> fsl,cksel = <0>;
>>
>
> Has this device tree binding been defined? (Where?)
> I don't see this property in the net-next.git tree at least.
>
> Claudiu
>
>
Hi Claudiu,
actually, I don't know where I should define this binding, my only idea
is to add "cksel" property description in
/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt. Am I right or I
need to do some additional changes?
Thanks.
^ permalink raw reply
* Re: [PATCH] powerpc/83xx: gianfar_ptp: select 1588 clock source through dts file
From: Aida Mynzhasova @ 2013-09-18 16:40 UTC (permalink / raw)
To: Richard Cochran; +Cc: netdev
In-Reply-To: <20130918160846.GA4397@netboy>
On 18.09.2013 20:08, Richard Cochran wrote:
> On Wed, Sep 18, 2013 at 05:21:04PM +0400, Aida Mynzhasova wrote:
>> IEEE 1588 timer reference clock source is determined through hard-coded
>> value in gianfar_ptp driver by default. This patch allows to select ptp
>> clock source by means of device tree file node.
>>
>> For instance:
>>
>> fsl,cksel = <0>;
>>
>> for using external (TSEC_TMR_CLK input) high precision timer
>> reference clock.
>>
>> Other acceptable values:
>>
>> <1> : eTSEC system clock
>> <2> : eTSEC1 transmit clock
>> <3> : RTC clock input
>
> Do the other clock sources even work at all?
>
> We were not able to get them working.
>
> Thanks,
> Richard
>
Hi Richard,
I've tried to use 2 clock sources: external from TSEC_TMR_CLK input (for
this I had to update multiplexer settings in uboot) and eTSEC system
clock - counter worked fine with both.
My attempts to use eTSEC1 transmit clock and RTC clock were unsuccessful
(the system hanged up) irrespective of using hard-coded value or entry
in dts file.
Thanks.
^ permalink raw reply
* Re: mvneta: oops in __rcu_read_lock on mirabox
From: Willy Tarreau @ 2013-09-18 16:49 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Ethan Tuttle, Russell King - ARM Linux, Andrew Lunn, Jason Cooper,
netdev, Ezequiel Garcia, Gregory Clément, linux-arm-kernel
In-Reply-To: <20130918183549.3e7b8f4c@skate>
On Wed, Sep 18, 2013 at 06:35:49PM +0200, Thomas Petazzoni wrote:
> Dear Ethan Tuttle,
>
> On Tue, 17 Sep 2013 23:30:56 -0700, Ethan Tuttle wrote:
> > On Mon, Sep 16, 2013 at 11:01 PM, Willy Tarreau <w@1wt.eu> wrote:
> > > Next step should be that you test both kernels to be sure.
> >
> > Thanks for the kernel images, Willy. I'm still experimenting but
> > initial results are strange: I haven't seen a crash from the -ethan
> > image you provided, nor by a kernel with that config that I built
> > myself. The config is only different from my crashing config by a few
> > options. So perhaps some combination of options prevents the crash.
> > I'll see if I can narrow it down.
>
> A toolchain generating some crappy code maybe? Ethan, Willy, comparing
> your toolchain (compiler version, origin of the toolchain) could be
> interesting.
I thought about this but it looks suspicious, I don't see why the toolchain
would produce random bitflips.
My toolchain is a linaro 4.7 gcc into which I have added support for a
"pj4b" CPU target which is essentially the same as cortex-a9 plus support
for the IDIV instruction in thumb mode.
But I can send it to Ethan if that helps.
Willy
^ permalink raw reply
* Re: [PATCH net] ip: generate unique IP identificator if local fragmentation is allowed
From: Ansis Atteka @ 2013-09-18 16:50 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20130918.122018.1088822749792394509.davem@davemloft.net>
On Wed, Sep 18, 2013 at 9:20 AM, David Miller <davem@davemloft.net> wrote:
> From: Ansis Atteka <aatteka@nicira.com>
> Date: Tue, 17 Sep 2013 15:25:00 -0700
>
>> @@ -1317,6 +1317,7 @@ struct sk_buff *__ip_make_skb(struct sock *sk,
>> ttl = ip_select_ttl(inet, &rt->dst);
>>
>> iph = (struct iphdr *)skb->data;
>> +
>> iph->version = 4;
>> iph->ihl = 5;
>> iph->tos = inet->tos;
>> @@ -1324,7 +1325,7 @@ struct sk_buff *__ip_make_skb(struct sock *sk,
>> iph->ttl = ttl;
>> iph->protocol = sk->sk_protocol;
>> ip_copy_addrs(iph, fl4);
>> - ip_select_ident(iph, &rt->dst, sk);
>> + ip_select_ident(skb, &rt->dst, sk);
>>
>> if (opt) {
>> iph->ihl += opt->optlen>>2;
>
> This transformation is not equivalent.
>
> ip_select_ident() is going to use ip_hdr() which obtains the header
> pointer via skb_network_header().
>
> But here, the code is using skb->data for this, which is different.
I dared to do that because three lines below ip_options_build() seems
to be already doing the same thing, if IP options are present:
struct sk_buff *__ip_make_skb() {
...
ip_select_ident(skb, &rt->dst, sk);
if (opt) {
iph->ihl += opt->optlen>>2;
ip_options_build(skb, opt, cork->addr, rt, 0);
}
and
void ip_options_build(struct sk_buff *skb, struct ip_options *opt,
__be32 daddr, struct rtable *rt, int is_frag) {
unsigned char *iph = skb_network_header(skb);
....
Another solution would be to pass local fragmentation flag as fourth
argument to ip_select_ident(). If you think that would be cleaner,
then I can reimplement it that way?
^ permalink raw reply
* Re: [PATCH net] ip: generate unique IP identificator if local fragmentation is allowed
From: David Miller @ 2013-09-18 16:55 UTC (permalink / raw)
To: aatteka; +Cc: netdev
In-Reply-To: <CAA=3Oq=Kxfe8KKnWV7QjLnT1Art3UEMJBTE=tXBLZPa6diug6g@mail.gmail.com>
From: Ansis Atteka <aatteka@nicira.com>
Date: Wed, 18 Sep 2013 09:50:31 -0700
> On Wed, Sep 18, 2013 at 9:20 AM, David Miller <davem@davemloft.net> wrote:
>> From: Ansis Atteka <aatteka@nicira.com>
>> Date: Tue, 17 Sep 2013 15:25:00 -0700
>>
>>> @@ -1317,6 +1317,7 @@ struct sk_buff *__ip_make_skb(struct sock *sk,
>>> ttl = ip_select_ttl(inet, &rt->dst);
>>>
>>> iph = (struct iphdr *)skb->data;
>>> +
>>> iph->version = 4;
>>> iph->ihl = 5;
>>> iph->tos = inet->tos;
>>> @@ -1324,7 +1325,7 @@ struct sk_buff *__ip_make_skb(struct sock *sk,
>>> iph->ttl = ttl;
>>> iph->protocol = sk->sk_protocol;
>>> ip_copy_addrs(iph, fl4);
>>> - ip_select_ident(iph, &rt->dst, sk);
>>> + ip_select_ident(skb, &rt->dst, sk);
>>>
>>> if (opt) {
>>> iph->ihl += opt->optlen>>2;
>>
>> This transformation is not equivalent.
>>
>> ip_select_ident() is going to use ip_hdr() which obtains the header
>> pointer via skb_network_header().
>>
>> But here, the code is using skb->data for this, which is different.
>
> I dared to do that because three lines below ip_options_build() seems
> to be already doing the same thing, if IP options are present:
>
> struct sk_buff *__ip_make_skb() {
> ...
> ip_select_ident(skb, &rt->dst, sk);
> if (opt) {
> iph->ihl += opt->optlen>>2;
> ip_options_build(skb, opt, cork->addr, rt, 0);
> }
>
> and
>
> void ip_options_build(struct sk_buff *skb, struct ip_options *opt,
> __be32 daddr, struct rtable *rt, int is_frag) {
> unsigned char *iph = skb_network_header(skb);
> ....
Then change the first hunk to use ip_hdr() if you are so confident.
Having inconsistent calculations of the ip header pointer is just
asking for trouble.
^ permalink raw reply
* Re: [PATCH] USBNET: fix handling padding packet
From: Ming Lei @ 2013-09-18 17:03 UTC (permalink / raw)
To: Bjørn Mork
Cc: David S. Miller, Greg Kroah-Hartman, Oliver Neukum,
Network Development, linux-usb, Oliver Neukum
In-Reply-To: <87zjra5dpx.fsf-lbf33ChDnrE/G1V5fR+Y7Q@public.gmane.org>
On Wed, Sep 18, 2013 at 11:52 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
> Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> writes:
>
>>> Are you really sure that the one driver/device using this really need
>>> the padding byte? If you could just make FLAG_SEND_ZLP part of the
>>> condition for enabling can_dma_sg, then all this extra complexity would
>>> be unnecessary. As the comment in front of the padding code says:
>>
>> At least for the effected driver of ax88179, the padding packet is needed
>> since the driver does set the padding flag in the header, see
>> ax88179_tx_fixup().
>
> Yes, I noticed that the driver doesn't set the flag. I just didn't put
> too much into that. I don't think that necessarily means that the
> padding is needed. It probably just means that the driver worked with
> the default padding enabled, so the author left it there.
>
> This flag should really have been inverted. ZLP should be the default
> for all new usbnet drivers.
In theory, it is, but who knows the reality.
>
> Why don't you test it on the device you tested the SG patch with? I am
> pretty sure it works just fine using proper ZLP transfer termination.
I should have planned to test it, but didn't know how to build TCP TSO
to make the whole frame length plus 8 dividable by 1024.
Could you or other guys share how to build such packet so that I can
do the test?
Once we can confirm the padding isn't needed, we can remove the
padding flag. But if the padding flag is still there, this patch should be
dropped.
>
>>> "strictly conforming cdc-ether devices should expect the ZLP here"
>>>
>>> There shouldn't be any problems requiring this conformance as a
>>> precondition for allowing SG. The requirements are already strict.
>>
>> There is no reason to forbid DMA SG for one driver which requires
>> padding, right?
>
> Yes there is: Added complexity for everybody, based on a combination of
> features which just does not make any sense.
>
> No modern device should need the padding. No old device will be able to
> use the SG feature as implemented. You only enable it on USB3, don't
> you? If this feature is restricted to USB3 capable devices, then it most
> certainly can be restricted to ZLP capable devices with absolutely no
> difference in the resulting set of supported devices.
See above, if we can prove that padding isn't needed, we can remove
the padding, then the patch can be dropped. If we can't, the patch is needed.
>
> Anyway, if you want to keep the padding for SG then maybe this will work
> and allow you to drop the extra struct usbnet field and allocation:
>
> if (skb_tailroom(skb) && !dev->can_dma_sg) {
> skb->data[skb->len] = 0;
> __skb_put(skb, 1);
> } else if (dev->can_dma_sg) {
> sg_set_buf(&urb->sg[urb->num_sgs++], skb->data, 1);
> }
>
> I.e. cheat and use the skb->data buffer twice, if that is allowed? The
> actual value of the padding byte should not matter, I believe?
If so, we can remove the assignment of zero to skb->data[skb->len],
but probably it might cause regression, that is why I wouldn't like to
do that. Also looks introducing one extra dev->padding_frame doesn't
cause much complexity.
Thanks
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] USBNET: fix handling padding packet
From: Oliver Neukum @ 2013-09-18 17:06 UTC (permalink / raw)
To: Bjørn Mork
Cc: Ming Lei, David S. Miller, Greg Kroah-Hartman,
Network Development, linux-usb
In-Reply-To: <87zjra5dpx.fsf@nemi.mork.no>
On Wed, 2013-09-18 at 17:52 +0200, Bjørn Mork wrote:
> No modern device should need the padding. No old device will be able to
> use the SG feature as implemented. You only enable it on USB3, don't
On XHCI.
> you? If this feature is restricted to USB3 capable devices, then it most
> certainly can be restricted to ZLP capable devices with absolutely no
> difference in the resulting set of supported devices.
No, USB 3.0 uses no companion controllers, so you can have devices
of any speed connected to it.
> Anyway, if you want to keep the padding for SG then maybe this will work
> and allow you to drop the extra struct usbnet field and allocation:
>
> if (skb_tailroom(skb) && !dev->can_dma_sg) {
> skb->data[skb->len] = 0;
> __skb_put(skb, 1);
> } else if (dev->can_dma_sg) {
> sg_set_buf(&urb->sg[urb->num_sgs++], skb->data, 1);
> }
>
> I.e. cheat and use the skb->data buffer twice, if that is allowed? The
> actual value of the padding byte should not matter, I believe?
That makes me immediately suspect a violation of the DMA rules.
Regards
Oliver
^ permalink raw reply
* [PATCH] mrp: add periodictimer to retry lost packets
From: Noel Burton-Krahn @ 2013-09-18 17:09 UTC (permalink / raw)
To: netdev
MRP doesn't implement the periodictimer in 802.1Q, so it never retries
if packets get lost. I ran into this problem when MRP sent a MVRP
JoinIn before the interface was fully up. The JoinIn was lost, MRP
didn't retry, and MVRP registration failed.
(Sorry for the repost, my earlier message wasn't formatted correctly)
Cheers,
--
Noel
diff --git a/include/net/mrp.h b/include/net/mrp.h
index 4fbf02a..0f7558b 100644
--- a/include/net/mrp.h
+++ b/include/net/mrp.h
@@ -112,6 +112,7 @@ struct mrp_applicant {
struct mrp_application *app;
struct net_device *dev;
struct timer_list join_timer;
+ struct timer_list periodic_timer;
spinlock_t lock;
struct sk_buff_head queue;
diff --git a/net/802/mrp.c b/net/802/mrp.c
index 1eb05d8..edcd458 100644
--- a/net/802/mrp.c
+++ b/net/802/mrp.c
@@ -24,6 +24,11 @@
static unsigned int mrp_join_time __read_mostly = 200;
module_param(mrp_join_time, uint, 0644);
MODULE_PARM_DESC(mrp_join_time, "Join time in ms (default 200ms)");
+
+static unsigned int mrp_periodic_time __read_mostly = 1000;
+module_param(mrp_periodic_time, uint, 0644);
+MODULE_PARM_DESC(mrp_periodic_time, "Periodic time in ms (default 1s)");
+
MODULE_LICENSE("GPL");
static const u8
@@ -595,6 +600,26 @@ static void mrp_join_timer(unsigned long data)
mrp_join_timer_arm(app);
}
+static void mrp_periodic_timer_arm(struct mrp_applicant *app)
+{
+ unsigned long delay;
+
+ delay = (u64)msecs_to_jiffies(mrp_periodic_time);
+ mod_timer(&app->periodic_timer, jiffies + delay);
+}
+
+static void mrp_periodic_timer(unsigned long data)
+{
+ struct mrp_applicant *app = (struct mrp_applicant *)data;
+
+ spin_lock(&app->lock);
+ mrp_mad_event(app, MRP_EVENT_PERIODIC);
+ mrp_pdu_queue(app);
+ spin_unlock(&app->lock);
+
+ mrp_periodic_timer_arm(app);
+}
+
static int mrp_pdu_parse_end_mark(struct sk_buff *skb, int *offset)
{
__be16 endmark;
@@ -845,6 +870,9 @@ int mrp_init_applicant(struct net_device *dev,
struct mrp_application *appl)
rcu_assign_pointer(dev->mrp_port->applicants[appl->type], app);
setup_timer(&app->join_timer, mrp_join_timer, (unsigned long)app);
mrp_join_timer_arm(app);
+ setup_timer(&app->periodic_timer, mrp_periodic_timer,
+ (unsigned long)app);
+ mrp_periodic_timer_arm(app);
return 0;
err3:
@@ -870,6 +898,7 @@ void mrp_uninit_applicant(struct net_device *dev,
struct mrp_application *appl)
* all pending messages before the applicant is gone.
*/
del_timer_sync(&app->join_timer);
+ del_timer_sync(&app->periodic_timer);
spin_lock_bh(&app->lock);
mrp_mad_event(app, MRP_EVENT_TX);
^ permalink raw reply related
* Re: [PATCH ] ip6tnl: do route updating for redirect in ip6_tnl_err()
From: Duan Jiong @ 2013-09-18 17:17 UTC (permalink / raw)
To: Duan Jiong, David Miller; +Cc: netdev, hannes
In-Reply-To: <5239970F.8000105@cn.fujitsu.com>
于 2013/9/18 20:05, Duan Jiong 写道:
> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>
> After the ip6_tnl_err() is called, the rel_msg is assigned
> to 0, and the ip4ip6_err()will return directly, the instruction
> below will not be executed.
>
> In rfc2473, we can know that the tunnel ICMP redirect
> message should not be reported to the source of the
> original packet, so we can handle it in ip4ip6_err().
>
> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> ---
> net/ipv6/ip6_tunnel.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 2d8f482..35c4b70 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -529,6 +529,9 @@ ip6_tnl_err(struct sk_buff *skb, __u8 ipproto, struct inet6_skb_parm *opt,
> rel_msg = 1;
> }
> break;
> + case NDISC_REDIRECT:
> + ip6_redirect(skb, net, 0, 0);
sorry, it should be
ip6_redirect(skb, dev_net(skb->dev), skb->dev->ifindex, 0);
> + break;
> }
>
> *type = rel_type;
> @@ -576,9 +579,6 @@ ip4ip6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
> rel_type = ICMP_DEST_UNREACH;
> rel_code = ICMP_FRAG_NEEDED;
> break;
> - case NDISC_REDIRECT:
> - rel_type = ICMP_REDIRECT;
> - rel_code = ICMP_REDIR_HOST;
> default:
> return 0;
> }
> @@ -637,8 +637,6 @@ ip4ip6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
>
> skb_dst(skb2)->ops->update_pmtu(skb_dst(skb2), NULL, skb2, rel_info);
> }
> - if (rel_type == ICMP_REDIRECT)
> - skb_dst(skb2)->ops->redirect(skb_dst(skb2), NULL, skb2);
>
> icmp_send(skb2, rel_type, rel_code, htonl(rel_info));
>
^ permalink raw reply
* Re: [PATCH] mrp: add periodictimer to retry lost packets
From: David Miller @ 2013-09-18 17:54 UTC (permalink / raw)
To: noel; +Cc: netdev
In-Reply-To: <CAB5uXS7b2CGjF=1OTzCmEeduGHSO_SOmMDv6=-bZU2VzaEQNfA@mail.gmail.com>
From: Noel Burton-Krahn <noel@burton-krahn.com>
Date: Wed, 18 Sep 2013 10:09:25 -0700
> MRP doesn't implement the periodictimer in 802.1Q, so it never retries
> if packets get lost. I ran into this problem when MRP sent a MVRP
> JoinIn before the interface was fully up. The JoinIn was lost, MRP
> didn't retry, and MVRP registration failed.
>
> (Sorry for the repost, my earlier message wasn't formatted correctly)
Still corrupted by your email client, it turned TAB characters
into spaces.
Still missing a signoff.
^ permalink raw reply
* Re: [PATCH ] ipv6: handle the update of the NDISC_REDIRECT error code in icmpv6_err_convert
From: Hannes Frederic Sowa @ 2013-09-18 18:00 UTC (permalink / raw)
To: David Miller; +Cc: duanj.fnst, netdev
In-Reply-To: <20130918.123613.61436246160249555.davem@davemloft.net>
On Wed, Sep 18, 2013 at 12:36:13PM -0400, David Miller wrote:
> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> Date: Wed, 18 Sep 2013 20:04:48 +0800
>
> > From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> >
> > when dealing with redirect message in udpv6_err() and
> > rawv6_err() the err shoud be assigned to 0, not EPROTO.
> >
> > Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>
> No, you should fix this the same way you handled DCCP, but skipping
> the whole socket error setting path for NDISC_REDIRECT.
>
> Clearing the socket error to zero is not correct, it means you will
> lose any existing error setting.
My reasoning when I proposed this change was that we should not stop
appending ipv6 redirects to the raw and udp sockets error queues because
userland could want to deal with them (and probably already does). Also,
we need to notify poll of the appended error, hence we still needed a
call to sk_err_report to fire up a POLL_ERR event. Because we don't know
if the router sending the redirect actually dropped the packet I thought
taht '0' would be the only sane choice. We currently report back EPROTO,
IPv4 does report (rightfully) EHOSTUNREACH.
Perhaps we should stay with EPROTO?
I agree, all other error functions should return early after updating
the routing table without reporting any error. Either they should not
call into this function or ignore err == 0 (I prefer the first).
I know, it is a bit hacky but this was the most sane idea I came up with.
(I still have not reviewed this patch, just wanted to provide a bit of
background that lead to this change.)
Greetings,
Hannes
^ permalink raw reply
* Re: [PATCH] mrp: add periodictimer to retry lost packets
From: Joe Perches @ 2013-09-18 18:20 UTC (permalink / raw)
To: Noel Burton-Krahn; +Cc: netdev
In-Reply-To: <CAB5uXS7b2CGjF=1OTzCmEeduGHSO_SOmMDv6=-bZU2VzaEQNfA@mail.gmail.com>
On Wed, 2013-09-18 at 10:09 -0700, Noel Burton-Krahn wrote:
> MRP doesn't implement the periodictimer in 802.1Q, so it never retries
> if packets get lost. I ran into this problem when MRP sent a MVRP
> JoinIn before the interface was fully up. The JoinIn was lost, MRP
> didn't retry, and MVRP registration failed.
Beyond the whitespace errors that make this not apply:
> diff --git a/net/802/mrp.c b/net/802/mrp.c
[]
> @@ -595,6 +600,26 @@ static void mrp_join_timer(unsigned long data)
> mrp_join_timer_arm(app);
> }
>
> +static void mrp_periodic_timer_arm(struct mrp_applicant *app)
> +{
> + unsigned long delay;
> +
> + delay = (u64)msecs_to_jiffies(mrp_periodic_time);
Useless cast to u64
> + mod_timer(&app->periodic_timer, jiffies + delay);
This might also be neater without the temporary
static void mrp_periodic_timer_arm(struct mrp_applicant *app)
{
mod_timer(&app->periodic_timer,
jiffies + msecs_to_jiffies(mrp_periodic_timer));
}
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox