* Re: Synopsys Ethernet QoS Driver
From: Joao Pinto @ 2016-11-23 11:10 UTC (permalink / raw)
To: Giuseppe CAVALLARO, Joao Pinto, Lars Persson
Cc: Rayagond Kokatanur, Rabin Vincent, mued dib, David Miller,
Jeff Kirsher, jiri@mellanox.com, saeedm@mellanox.com,
idosch@mellanox.com, netdev, linux-kernel@vger.kernel.org,
CARLOS.PALMINHA@synopsys.com, Andreas Irestål,
alexandre.torgue@st.com, linux-arm-kernel@lists.infradead.org
In-Reply-To: <c37c47e1-8e21-1b11-ed15-6b899ed1dd03@st.com>
Hi Peppe and Lars,
On 23-11-2016 10:59, Giuseppe CAVALLARO wrote:
> Hello Joao, Lars.
>
> On 11/22/2016 3:16 PM, Joao Pinto wrote:
>>> Ok, it makes sense.
>>> > Just for curiosity the target setup is the following:
>>> > https://www.youtube.com/watch?v=8V-LB5y2Cos
>>> > but instead of using internal drivers, we desire to use mainline drivers only.
>>> >
>>> > Thanks!
>> Regarding this subject, I am thinking of making the following adaption:
>>
>> a) delete ethernet/synopsys
>> b) rename ethernet/stmicro/stmmac to ethernet/synopsys
>>
>> and send you a patch for you to evaluate. Both agree with the approach?
>> To have a new work base would be important, because I will add to the "new"
>> structure some missing QoS features like Multichannel support, CBS and later TSN.
>
> IMO, we have to agree on a common strategy making the change for
> net-next; I imaged the following steps:
Yes it makes totally sense.
>
> - to port missing feature or fixes from ethernet/synopsys
> inside the stmmac taking care about the documentation too.
@Lars: You are familiar with the synopsys qos driver. Could you please do this
porting. You can also make an analysis of what to port and I can do the porting
for you if you don't have the availability for it.
> - remove ethernet/synopsys
> - rename ethernet/stmicro/stmmac to ethernet/synopsys
I volunteer to do this task.
>
> These latest two have some relevant impacts.
>
> This change should be propagated to all the platforms that are using:
> CONFIG_SYNOPSYS_DWC_ETH_QOS and CONFIG_STMMAC_ETH
> plus device-tree compatibility.
I volunteer to do this task also.
>
> - enhance the stmmac with new features and new glue (part of these
> can be anticipated for sure).
I have to implement 3 new features for now, but I will take some time for it, so
I would suggest to make the previous task and incrementally add features.
>
> what do you think? does it make sense? If yes, we can also
> understand how/who starts.
>
> Regards,
> Peppe
Thanks and regards.
Joao
>
>> Thanks.
>
^ permalink raw reply
* Re: [PATCH] ipv6:ipv6_pinfo dereferenced after NULL check
From: Hannes Frederic Sowa @ 2016-11-23 11:32 UTC (permalink / raw)
To: r.thapliyal, Manjeet Pawar, davem@davemloft.net,
kuznet@ms2.inr.ac.ru, jmorris@namei.org, yoshfuji@linux-ipv6.org,
kaber@trash.net, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: PANKAJ MISHRA, Ajeet Kumar Yadav
In-Reply-To: <20161123044535epcms5p37a2a3f2c2c071fdac1ddb6b1a6c02cf6@epcms5p3>
On 23.11.2016 05:45, Rohit Thapliyal wrote:
> |>On 22.11.2016 07:27, Manjeet Pawar wrote:
> >> From: Rohit Thapliyal <r.thapliyal@samsung.com <mailto:r.thapliyal@samsung.com>>
> >>
> >> np checked for NULL and then dereferenced. It should be modified
> >> for NULL case.
> >>
> >> Signed-off-by: Rohit Thapliyal <r.thapliyal@samsung.com
> <mailto:r.thapliyal@samsung.com>>>
> >> Signed-off-by: Manjeet Pawar <manjeet.p@samsung.com
> <mailto:manjeet.p@samsung.com>>>
> >> ---
> >> net/ipv6/ip6_output.c | 9 +++++----
> >> 1 file changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> >> index 1dfc402..c2afa14 100644
> >> --- a/net/ipv6/ip6_output.c
> >> +++ b/net/ipv6/ip6_output.c
> >> @@ -205,14 +205,15 @@ int ip6_xmit(const struct sock *sk, struct sk_buff
> *skb, struct flowi6 *fl6,
> >> /*
> >> * Fill in the IPv6 header
> >> */
> >> - if (np)
> >> + if (np) {
> >> hlimit = np->hop_limit;
> >> + ip6_flow_hdr(
> >> + hdr, tclass, ip6_make_flowlabel(
> >> + net, skb, fl6->flowlabel,
> >> + np->autoflowlabel, fl6));
> >> + }
> >> if (hlimit < 0)
> >> hlimit = ip6_dst_hoplimit(dst);
> >>
> >> - ip6_flow_hdr(hdr, tclass, ip6_make_flowlabel(net, skb, fl6->flowlabel,
> >> - np->autoflowlabel, fl6));
> >> -
> >> hdr->payload_len = htons(seg_len);
> >> hdr->nexthdr = proto;
> >> hdr->hop_limit = hlimit;
> >>
> >
> >
> >We always should initialize hdr and not skip the ip6_flow_hdr call.|
>
> |if np becomes NULL, then anyways hdr won't be initialized due to NULL pointer
> dereference ip6_make_flowlabel.|
Which we would see as a crash. So far no crash has been reported in this
code. Doing a quick code review on the paths leading to ip6_xmit,
inet6_sk must always be set to actually reach up to this point.
Otherwise we would have crashes on all code paths much earler.
Anyway, I would be fine to keep the NULL check in this path, it looks
better because of the inet6_sk you pointed out above but I would
recommend to just use a "np ? np->autoflowlabel :
ip6_default_np_autolabel(net) in the ip6_flow_hdr function.
> |>Do you saw a bug or did you find this by code review? I wonder if np can
> >actually be NULL at this point. Maybe we can just eliminate the NULL check.|
>
> |
>
>
> I must admit that I found it just by code review, and so far didn't face any
> crash whatsoever.
> As we can see in inet6_sk, np could be NULL. Thus, the NULL check seems justified.
Thanks for looking at this!
Bye,
Hannes
^ permalink raw reply
* Re: Synopsys Ethernet QoS Driver
From: Lars Persson @ 2016-11-23 11:41 UTC (permalink / raw)
To: Joao Pinto
Cc: Giuseppe CAVALLARO, Rayagond Kokatanur, Rabin Vincent, mued dib,
David Miller, Jeff Kirsher, jiri@mellanox.com,
saeedm@mellanox.com, idosch@mellanox.com, netdev,
linux-kernel@vger.kernel.org, CARLOS.PALMINHA@synopsys.com,
Andreas Irestål, alexandre.torgue@st.com,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <7c259adb-5c73-f997-6b96-5be427157b08@synopsys.com>
> 23 nov. 2016 kl. 12:11 skrev Joao Pinto <Joao.Pinto@synopsys.com>:
>
> Hi Peppe and Lars,
>
>> On 23-11-2016 10:59, Giuseppe CAVALLARO wrote:
>> Hello Joao, Lars.
>>
>>> On 11/22/2016 3:16 PM, Joao Pinto wrote:
>>>>> Ok, it makes sense.
>>>>> Just for curiosity the target setup is the following:
>>>>> https://www.youtube.com/watch?v=8V-LB5y2Cos
>>>>> but instead of using internal drivers, we desire to use mainline drivers only.
>>>>>
>>>>> Thanks!
>>> Regarding this subject, I am thinking of making the following adaption:
>>>
>>> a) delete ethernet/synopsys
>>> b) rename ethernet/stmicro/stmmac to ethernet/synopsys
>>>
>>> and send you a patch for you to evaluate. Both agree with the approach?
>>> To have a new work base would be important, because I will add to the "new"
>>> structure some missing QoS features like Multichannel support, CBS and later TSN.
>>
>> IMO, we have to agree on a common strategy making the change for
>> net-next; I imaged the following steps:
>
> Yes it makes totally sense.
>
>>
>> - to port missing feature or fixes from ethernet/synopsys
>> inside the stmmac taking care about the documentation too.
>
> @Lars: You are familiar with the synopsys qos driver. Could you please do this
> porting. You can also make an analysis of what to port and I can do the porting
> for you if you don't have the availability for it.
As my main duty is changing diapers until March next year, please go ahead with this step if you can spend time on it before I am back in office.
Rabin Vincent can review and test that the port works properly on our Artpec-chips that use dwc_eth_qos.c today.
The main porting step is to implement the device tree binding in bindings/net/snps,dwc-qos-ethernet.txt. Also our chip has a strict requirement that the phy is enabled when the SWR reset bit is set (it needs a tx clock to complete the reset).
- Lars
>
>> - remove ethernet/synopsys
>> - rename ethernet/stmicro/stmmac to ethernet/synopsys
>
> I volunteer to do this task.
>
>>
>> These latest two have some relevant impacts.
>>
>> This change should be propagated to all the platforms that are using:
>> CONFIG_SYNOPSYS_DWC_ETH_QOS and CONFIG_STMMAC_ETH
>> plus device-tree compatibility.
>
> I volunteer to do this task also.
>
>>
>> - enhance the stmmac with new features and new glue (part of these
>> can be anticipated for sure).
>
> I have to implement 3 new features for now, but I will take some time for it, so
> I would suggest to make the previous task and incrementally add features.
>
>>
>> what do you think? does it make sense? If yes, we can also
>> understand how/who starts.
>>
>> Regards,
>> Peppe
>
> Thanks and regards.
>
> Joao
>
>>
>>> Thanks.
>>
>
^ permalink raw reply
* Re: Synopsys Ethernet QoS Driver
From: Joao Pinto @ 2016-11-23 11:43 UTC (permalink / raw)
To: Lars Persson, Joao Pinto
Cc: Giuseppe CAVALLARO, Rayagond Kokatanur, Rabin Vincent, mued dib,
David Miller, Jeff Kirsher, jiri@mellanox.com,
saeedm@mellanox.com, idosch@mellanox.com, netdev,
linux-kernel@vger.kernel.org, CARLOS.PALMINHA@synopsys.com,
Andreas Irestål, alexandre.torgue@st.com,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <899DC02E-84BB-489E-A1FE-5D8F3BB795B6@axis.com>
On 23-11-2016 11:41, Lars Persson wrote:
>
>> 23 nov. 2016 kl. 12:11 skrev Joao Pinto <Joao.Pinto@synopsys.com>:
>>
>> Hi Peppe and Lars,
>>
>>> On 23-11-2016 10:59, Giuseppe CAVALLARO wrote:
>>> Hello Joao, Lars.
>>>
>>>> On 11/22/2016 3:16 PM, Joao Pinto wrote:
>>>>>> Ok, it makes sense.
>>>>>> Just for curiosity the target setup is the following:
>>>>>> https://www.youtube.com/watch?v=8V-LB5y2Cos
>>>>>> but instead of using internal drivers, we desire to use mainline drivers only.
>>>>>>
>>>>>> Thanks!
>>>> Regarding this subject, I am thinking of making the following adaption:
>>>>
>>>> a) delete ethernet/synopsys
>>>> b) rename ethernet/stmicro/stmmac to ethernet/synopsys
>>>>
>>>> and send you a patch for you to evaluate. Both agree with the approach?
>>>> To have a new work base would be important, because I will add to the "new"
>>>> structure some missing QoS features like Multichannel support, CBS and later TSN.
>>>
>>> IMO, we have to agree on a common strategy making the change for
>>> net-next; I imaged the following steps:
>>
>> Yes it makes totally sense.
>>
>>>
>>> - to port missing feature or fixes from ethernet/synopsys
>>> inside the stmmac taking care about the documentation too.
>>
>> @Lars: You are familiar with the synopsys qos driver. Could you please do this
>> porting. You can also make an analysis of what to port and I can do the porting
>> for you if you don't have the availability for it.
>
> As my main duty is changing diapers until March next year, please go ahead with this step if you can spend time on it before I am back in office.
Congratulations :)!
>
> Rabin Vincent can review and test that the port works properly on our Artpec-chips that use dwc_eth_qos.c today.
>
> The main porting step is to implement the device tree binding in bindings/net/snps,dwc-qos-ethernet.txt. Also our chip has a strict requirement that the phy is enabled when the SWR reset bit is set (it needs a tx clock to complete the reset).
>
> - Lars
Ok, I will do the task.
@Peppe: Agree with the plan?
>
>>
>>> - remove ethernet/synopsys
>>> - rename ethernet/stmicro/stmmac to ethernet/synopsys
>>
>> I volunteer to do this task.
>>
>>>
>>> These latest two have some relevant impacts.
>>>
>>> This change should be propagated to all the platforms that are using:
>>> CONFIG_SYNOPSYS_DWC_ETH_QOS and CONFIG_STMMAC_ETH
>>> plus device-tree compatibility.
>>
>> I volunteer to do this task also.
>>
>>>
>>> - enhance the stmmac with new features and new glue (part of these
>>> can be anticipated for sure).
>>
>> I have to implement 3 new features for now, but I will take some time for it, so
>> I would suggest to make the previous task and incrementally add features.
>>
>>>
>>> what do you think? does it make sense? If yes, we can also
>>> understand how/who starts.
>>>
>>> Regards,
>>> Peppe
>>
>> Thanks and regards.
>>
>> Joao
>>
>>>
>>>> Thanks.
>>>
>>
^ permalink raw reply
* Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed
From: Allan W. Nielsen @ 2016-11-23 11:45 UTC (permalink / raw)
To: Florian Fainelli
Cc: Andrew Lunn, netdev, davem, bcm-kernel-feedback-list,
raju.lakkaraju, vivien.didelot
In-Reply-To: <1902f0f0-46e5-d3b3-90c1-10867f4fb826@gmail.com>
Hi,
On 22/11/16 12:07, Florian Fainelli wrote:
> On 11/22/2016 12:02 PM, Andrew Lunn wrote:
> >> +static int bcm7xxx_28nm_set_tunable(struct phy_device *phydev,
> >> + struct ethtool_tunable *tuna,
> >> + const void *data)
> >> +{
> >> + u8 count = *(u8 *)data;
> >> + int ret;
> >> +
> >> + switch (tuna->id) {
> >> + case ETHTOOL_PHY_DOWNSHIFT:
> >> + ret = bcm_phy_downshift_set(phydev, count);
> >> + break;
> >> + default:
> >> + return -EOPNOTSUPP;
> >> + }
> >> +
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* Disable EEE advertisment since this prevents the PHY
> >> + * from successfully linking up, trigger auto-negotiation restart
> >> + * to let the MAC decide what to do.
> >> + */
> >> + ret = bcm_phy_set_eee(phydev, count == DOWNSHIFT_DEV_DISABLE);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + return genphy_restart_aneg(phydev);
> >> +}
> >
> > Hi Florian
> >
> > Is the locking O.K. here? The core code does not take the phy lock.
> > But i think your shadow register accesses at least need to be
> > protected by the lock?
>
> There should be some kind of protection, but I was expecting it to be
> done at the caller level, so that when {get,set}_tunable run, they are
> serialized with respect to each other, clearly, by looking at the code,
> this is not the case.
>
> >
> > Maybe we should think about this locking a bit. It is normal for the
> > lock to be held when using ops in the phy driver structure. The
> > exception is suspend/resume. Maybe we should also take the lock before
> > calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()?
>
> Yes, that certainly seems like a good approach to me, let me cook a
> patch doing that.
Just for my understanding (such that I will not make the same mistake again)...
Why is it that phy functions such as get_wol needs to take the phy_lock and
others like get_tunable does not.
I do understand the arguments on why the lock should be held by the caller of
get_tunable, but I do not understand why the same argument does not apply for
get_wol.
/Allan
^ permalink raw reply
* Re: [PATCH] net: dsa: mv88e6xxx: egress all frames
From: Stefan Eichenberger @ 2016-11-23 12:00 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Stefan Eichenberger, vivien.didelot, f.fainelli, netdev
In-Reply-To: <20161122190206.GE14947@lunn.ch>
On Tue, Nov 22, 2016 at 08:02:06PM +0100, Andrew Lunn wrote:
> On Tue, Nov 22, 2016 at 07:37:33PM +0100, Stefan Eichenberger wrote:
> > Hi Andrew
> >
> > On Tue, Nov 22, 2016 at 04:03:30PM +0100, Andrew Lunn wrote:
> > > On Tue, Nov 22, 2016 at 11:39:44AM +0100, Stefan Eichenberger wrote:
> > > > Egress multicast and egress unicast is only enabled for CPU/DSA ports
> > > > but for switching operation it seems it should be enabled for all ports.
> > > > Do I miss something here?
> > > >
> > > > I did the following test:
> > > > brctl addbr br0
> > > > brctl addif br0 lan0
> > > > brctl addif br0 lan1
> > > >
> > > > In this scenario the unicast and multicast packets were not forwarded,
> > > > therefore ARP requests were not resolved, and no connection could be
> > > > established.
> > >
> > > Hi Stefan
> > >
> > > This is probably specific to the 6097 family. It works fine without
> > > this on other devices. Creating a bridge like above and pinging across
> > > it is one of my standard tests. But i only test modern devices like
> > > the 6165, 6352, 6351, 6390 families.
> >
> > Okay perfect, I wasn't 100% sure if I would have to configure something
> > additionally.
>
> No. The idea is you treat the interfaces as normal interfaces. You
> should not need to do anything additional to what you would do with a
> normal interface, when adding it to a bridge.
>
> > > In fact, you might need to review all the code and look where
> > > mv88e6xxx_6095_family(chip) is used and consider if you need to add
> > > mv88e6xxx_6097_family(chip). e.g.
> > >
> > > if (mv88e6xxx_6095_family(chip) || mv88e6xxx_6185_family(chip)) {
> > > /* Set the upstream port this port should use */
> > > reg |= dsa_upstream_port(ds);
> > > /* enable forwarding of unknown multicast addresses to
> > > * the upstream port
> > > */
> > > if (port == dsa_upstream_port(ds))
> > > reg |= PORT_CONTROL_2_FORWARD_UNKNOWN;
> > > }
> > >
> > > Maybe this is your problem?
> >
> > I think I still don't understand exactly how the driver works.
> >
> > My problem is that the multicast and broadcast frames are filtered and
> > the following counter is increasing in ethtool:
> > sw_in_filtered: 596
>
> This is not what is supposed to happen. Broadcast and multicast frames
> should go to all ports in the bridge. There are two different ways
> this can happen:
>
> 1) The mv88e6xxx driver started out with the host doing all bridge
> operations. The switch forwards all frames to the software bridge, and
> the software bridge then sends them out another port if needed.
>
> 2) We later added support for hardware bridging. That is, the switch
> itself bridges frames between ports. It will only pass frames to the
> software bridge if it does not know what to do with a frame itself.
Thanks for this explanation it helped a lot.
>
> Now, the different families are not 100% compatible with each
> other. We never had access to a 6097, so it has not been tested
> recently, and we have probably broken it... My guess would be,
> anywhere mv88e6xxx_6095_family(chip) is used, there also needs to be
> an mv88e6xxx_6097_family(chip). But i could be wrong.
I think I probably found the problem. For EDSA type switches the bit
PORT_CONTROL_FORWARD_UNKNOWN_MC is set on the cpu port but not for DSA
type switches. Broadcast addresses are threaded as multicast addresses,
so unknown frames will never leave the switch.
Do you know if there is a reason why this bit isn't set for DSA type
switches too? The patch would be extremely simple and it seems to work
perfectly with this bit set on the CPU port.
Thanks
Stefan
^ permalink raw reply
* Re: net/arp: ARP cache aging failed.
From: Eric Dumazet @ 2016-11-23 12:05 UTC (permalink / raw)
To: Julian Anastasov; +Cc: yuehaibing, davem, netdev
In-Reply-To: <alpine.LFD.2.11.1611230928140.1659@ja.home.ssi.bg>
On Wed, 2016-11-23 at 10:33 +0200, Julian Anastasov wrote:
> Hello,
>
> On Wed, 23 Nov 2016, yuehaibing wrote:
>
> > As to my topo,HOST1 and HOST3 share one route on HOST2, tcp connection between HOST2 and HOST3 may call tcp_ack to set dst->pending_confirm.
> >
> > So dst_neigh_output may wrongly freshed n->confirmed which stands for HOST1,however HOST1'MAC had been changed.
> >
> > The possibility of this occurred Significantly increases ,when ping and TCP transaction are set the same processor affinity on the HOST2.
> >
> > It seems that the issue is brought in commit 5110effee8fde2edfacac9cd12a9960ab2dc39ea ("net: Do delayed neigh confirmation.").
>
> Bad news. Problem is not in delayed confirmation but
> in the mechanism to use same dst for different neighbours on
> LAN. We don't have a dst->neighbour reference anymore.
>
> For IPv4 this is related to rt->rt_uses_gateway but
> also to DST_NOCACHE. In the other cases we can not call
> dst_confirm, may be we should lookup the neigh entry instead.
> But we need a way to reduce such lookups on every packet,
> for example, by remembering in struct sock and checking if
> some bits of jiffies (at least 4-5) are changed from
> previous lookup.
I thought bonding would keep the MAC address 'alive'.
If TCP packets are confirmed, this means the old MAC address is still
valid, what am I missing here ?
^ permalink raw reply
* Re: [PATCH net-next] net/sched: cls_flower: verify root pointer before dereferncing it
From: Daniel Borkmann @ 2016-11-23 11:29 UTC (permalink / raw)
To: Cong Wang, John Fastabend
Cc: Jiri Pirko, Roi Dayan, David S. Miller,
Linux Kernel Network Developers, Jiri Pirko, Or Gerlitz,
Cong Wang
In-Reply-To: <CAM_iQpWhGfrZgBD1Li+efZCv4Q6uGno1ttXcBL4C_F+4es4e_A@mail.gmail.com>
On 11/23/2016 06:24 AM, Cong Wang wrote:
> On Tue, Nov 22, 2016 at 3:36 PM, John Fastabend
> <john.fastabend@gmail.com> wrote:
>> On 16-11-22 12:41 PM, Daniel Borkmann wrote:
>>> On 11/22/2016 08:28 PM, Cong Wang wrote:
>>>> On Tue, Nov 22, 2016 at 8:11 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>> Tue, Nov 22, 2016 at 05:04:11PM CET, daniel@iogearbox.net wrote:
>>>>>> Hmm, I don't think we want to have such an additional test in fast
>>>>>> path for each and every classifier. Can we think of ways to avoid that?
>>>>>>
>>>>>> My question is, since we unlink individual instances from such
>>>>>> tp-internal
>>>>>> lists through RCU and release the instance through call_rcu() as
>>>>>> well as
>>>>>> the head (tp->root) via kfree_rcu() eventually, against what are we
>>>>>> protecting
>>>>>> setting RCU_INIT_POINTER(tp->root, NULL) in ->destroy() callback?
>>>>>> Something
>>>>>> not respecting grace period?
>>>>>
>>>>> If you call tp->ops->destroy in call_rcu, you don't have to set tp->root
>>>>> to null.
>>>
>>> But that's not really an answer to my question. ;)
>>>
>>>> We do need to respect the grace period if we touch the globally visible
>>>> data structure tp in tcf_destroy(). Therefore Roi's patch is not
>>>> fixing the
>>>> right place.
>>>
>>> I think there may be multiple issues actually.
>>>
>>> At the time we go into tc_classify(), from ingress as well as egress side,
>>> we're under RCU, but BH variant. In cls delete()/destroy() callbacks, we
>>> everywhere use call_rcu() and kfree_rcu(), same as for tcf_destroy() where
>>> we use kfree_rcu() on tp, although we iterate tps (and implicitly inner
>>> filters)
>>> via rcu_dereference_bh() from reader side. Is there a reason why we don't
>>> use call_rcu_bh() variant on destruction for all this instead?
>>
>> I can't think of any if its all under _bh we can convert the call_rcu to
>> call_rcu_bh it just needs an audit.
>>
>>> Just looking at cls_bpf and others, what protects
>>> RCU_INIT_POINTER(tp->root,
>>> NULL) against? The tp is unlinked in tc_ctl_tfilter() from the tp chain in
>>> tcf_destroy() cases. Still active readers under RCU BH can race against
>>> this
>>> (tp->root being NULL), as the commit identified. Only the get() callback
>>> checks
>>> for head against NULL, but both are serialized under rtnl, and the only
>>> place
>>> we call this is tc_ctl_tfilter(). Even if we create a new tp, head
>>> should not
>>> be NULL there, if it was assigned during the init() cb, but contains an
>>> empty
>>> list. (It's different for things like cls_cgroup, though.) So, I'm
>>> wondering
>>> if the RCU_INIT_POINTER(tp->root, NULL) can just be removed instead
>>> (unless I'm
>>> missing something obvious)?
>>
>> Just took a look at this I think there are a couple possible solutions.
>> The easiest is likely to fix all the call sites so that 'tp' is unlinked
>> before calling the destroy() handlers AND not doing the NULL set. I only
>> see one such call site where destroy is called before unlinking at the
>> moment. This should enforce that after a grace period there is no path
>> to reach the classifiers because 'tp' is unlinked. Calling destroy
>> before unlinking 'tp' however could cause a small race between grace
>> period of 'tp' and grace period of the filter.
>>
>> Another would be to only call the destroy path from the call_rcu path
>> of the 'tp' object so that destroy is only ever called after the object
>> is guaranteed to be unlinked from the tc_filter path.
>>
>> I think both solutions would be fine.
>>
>> Cong were you working on one of these? Or do you have another idea?
>
> Yeah, this is basic what I think as well, however, both are hard.
> On one hand, we can't detach the tp from the global singly-linked list
> before tcf_destroy() since we rely on its return value to make this decision.
> On the other hand, it is a singly-linked list, we have to pass in the address
> of its previous pointer to rcu callback to remove it, it seems racy as well
> since we modify a previous pointer which is still visible globally...
Can't we drop the 'force' parameter from tcf_destroy() and related cls
destroy() callbacks, and change the logic roughly like this:
[...]
case RTM_DELTFILTER:
err = tp->ops->delete(tp, fh, &drop_tp);
if (err == 0) {
struct tcf_proto *next = rtnl_dereference(tp->next);
tfilter_notify(net, skb, n, tp,
t->tcm_handle,
RTM_DELTFILTER, false);
if (drop_tp) {
RCU_INIT_POINTER(*back, next);
tcf_destroy(tp);
}
}
goto errout;
[...]
This one was the only tcf_destroy() instance with force=false. Why can't
the prior delete() callback make the decision whether the tp now has no
further internal filters and thus can be dropped. Afaik, delete() and
destroy() are protected by RTNL anyway. Thus, we could unlink the tp from
the list before tcf_destroy(), which should then work with grace period
as well. Given we remove the setting of tp->root to NULL, any outstanding
readers for that grace period should either still execute the 'scheduled
for removal' filter we just dropped, or find an empty list of filters.
> Hmm, perhaps we really have to switch to a doubly-linked list, that is
> list_head. I need to double check. And also the semantic of ->destroy()
> needs to revise too.
Can you elaborate why double-linked list? Isn't the tp list always protected
from modifications via RTNL in control path, and walked via rcu_dereference_bh()
in data path?
> So yeah, my commit should be blamed. :-/
^ permalink raw reply
* [PATCH 0/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code
From: Daniele Palmas @ 2016-11-23 12:36 UTC (permalink / raw)
To: Bjørn Mork, Oliver Neukum; +Cc: linux-usb, netdev, Daniele Palmas
Some latest QC based modems seem not to properly accept altsetting
toggling in cdc_ncm_bind_common, making them to fail. The workaround
was to introduce an empirically decided pause to avoid the failure.
This patch introduces a different approach: for MBIM devices, instead
of toggling interfaces, the MBIM class-specific request code
RESET_FUNCTION is used in order to reset the function to its initial
state, removing the need for the pause.
Patch has been tested with a few Telit QC and Intel based MBIM modems.
Patch has also been tested with an Intel NCM based device, for
regression checking.
Daniele Palmas (1):
NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying
ncm bind common code
drivers/net/usb/cdc_ncm.c | 39 +++++++++++++++++++++++++++------------
include/uapi/linux/usb/cdc.h | 1 +
2 files changed, 28 insertions(+), 12 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH 1/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code
From: Daniele Palmas @ 2016-11-23 12:36 UTC (permalink / raw)
To: Bjørn Mork, Oliver Neukum; +Cc: linux-usb, netdev, Daniele Palmas
In-Reply-To: <1479904606-30647-1-git-send-email-dnlplm@gmail.com>
Some latest QC based modems seem not to properly accept altsetting
toggling in cdc_ncm_bind_common, making them to fail. The workaround
was to introduce an empirically decided pause to avoid the failure.
This patch introduces a different approach: for MBIM devices, instead
of toggling interfaces, the MBIM class-specific request code
RESET_FUNCTION is used in order to reset the function to its initial
state, removing the need for the pause.
Signed-off-by: Daniele Palmas <dnlplm@gmail.com>
---
drivers/net/usb/cdc_ncm.c | 39 +++++++++++++++++++++++++++------------
include/uapi/linux/usb/cdc.h | 1 +
2 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 877c951..e8a7a76 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -498,6 +498,22 @@ static int cdc_ncm_init(struct usbnet *dev)
return err; /* GET_NTB_PARAMETERS is required */
}
+ /* Some modems (e.g. Telit LE922A6) need to reset the MBIM function
+ * or they will fail to work properly.
+ * For details on RESET_FUNCTION request see document
+ * "USB Communication Class Subclass Specification for MBIM"
+ * RESET_FUNCTION should be harmless for all the other MBIM modems
+ */
+ if (cdc_ncm_comm_intf_is_mbim(ctx->control->cur_altsetting)) {
+ err = usbnet_write_cmd(dev, USB_CDC_RESET_FUNCTION,
+ USB_TYPE_CLASS | USB_DIR_OUT
+ | USB_RECIP_INTERFACE,
+ 0, iface_no, NULL, 0);
+ if (err < 0) {
+ dev_err(&dev->intf->dev, "failed RESET_FUNCTION\n");
+ }
+ }
+
/* set CRC Mode */
if (cdc_ncm_flags(dev) & USB_CDC_NCM_NCAP_CRC_MODE) {
dev_dbg(&dev->intf->dev, "Setting CRC mode off\n");
@@ -842,25 +858,24 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
/* Reset data interface. Some devices will not reset properly
* unless they are configured first. Toggle the altsetting to
* force a reset
+ * This is applied only to ncm devices, since it has been verified
+ * to cause issues with some MBIM modems (e.g. Telit LE922A6).
+ * MBIM devices reset is achieved using MBIM request RESET_FUNCTION
+ * in cdc_ncm_init
*/
- usb_set_interface(dev->udev, iface_no, data_altsetting);
- temp = usb_set_interface(dev->udev, iface_no, 0);
- if (temp) {
- dev_dbg(&intf->dev, "set interface failed\n");
- goto error2;
+ if (!cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting)) {
+ usb_set_interface(dev->udev, iface_no, data_altsetting);
+ temp = usb_set_interface(dev->udev, iface_no, 0);
+ if (temp) {
+ dev_dbg(&intf->dev, "set interface failed\n");
+ goto error2;
+ }
}
/* initialize basic device settings */
if (cdc_ncm_init(dev))
goto error2;
- /* Some firmwares need a pause here or they will silently fail
- * to set up the interface properly. This value was decided
- * empirically on a Sierra Wireless MC7455 running 02.08.02.00
- * firmware.
- */
- usleep_range(10000, 20000);
-
/* configure data interface */
temp = usb_set_interface(dev->udev, iface_no, data_altsetting);
if (temp) {
diff --git a/include/uapi/linux/usb/cdc.h b/include/uapi/linux/usb/cdc.h
index e2bc417..30258fb 100644
--- a/include/uapi/linux/usb/cdc.h
+++ b/include/uapi/linux/usb/cdc.h
@@ -231,6 +231,7 @@ struct usb_cdc_mbim_extended_desc {
#define USB_CDC_SEND_ENCAPSULATED_COMMAND 0x00
#define USB_CDC_GET_ENCAPSULATED_RESPONSE 0x01
+#define USB_CDC_RESET_FUNCTION 0x05
#define USB_CDC_REQ_SET_LINE_CODING 0x20
#define USB_CDC_REQ_GET_LINE_CODING 0x21
#define USB_CDC_REQ_SET_CONTROL_LINE_STATE 0x22
--
2.7.4
^ permalink raw reply related
* [net-next PATCH v3] net: dummy: Introduce dummy virtual functions
From: Phil Sutter @ 2016-11-23 12:41 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Sabrina Dubroca
The idea for this was born when testing VF support in iproute2 which was
impeded by hardware requirements. In fact, not every VF-capable hardware
driver implements all netdev ops, so testing the interface is still hard
to do even with a well-sorted hardware shelf.
To overcome this and allow for testing the user-kernel interface, this
patch allows to turn dummy into a PF with a configurable amount of VFs.
Due to the assumption that all PFs are PCI devices, this implementation
is not completely straightforward: In order to allow for
rtnl_fill_ifinfo() to see the dummy VFs, a fake PCI parent device is
attached to the dummy netdev. This has to happen at the right spot so
register_netdevice() does not get confused. This patch abuses
ndo_fix_features callback for that. In ndo_uninit callback, the fake
parent is removed again for the same purpose.
Joint work with Sabrina Dubroca.
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v2:
- Fixed oops on reboot (need to initialize parent device mutex).
- Got rid of potential mem leak noticed by Eric Dumazet.
- Dropped stray newline insertion.
Changes since v1:
- Fixed issues reported by kbuild test robot:
- pci_dev->sriov is only present if CONFIG_PCI_ATS is active.
- pci_bus_type does not exist if CONFIG_PCI is not defined.
---
drivers/net/dummy.c | 205 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 203 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index 69fc8409a9733..829eb552c63c5 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -34,6 +34,8 @@
#include <linux/etherdevice.h>
#include <linux/init.h>
#include <linux/moduleparam.h>
+#include <linux/pci.h>
+#include "../pci/pci.h" /* for struct pci_sriov */
#include <linux/rtnetlink.h>
#include <net/rtnetlink.h>
#include <linux/u64_stats_sync.h>
@@ -42,6 +44,37 @@
#define DRV_VERSION "1.0"
static int numdummies = 1;
+static int num_vfs;
+
+static struct pci_sriov pdev_sriov;
+
+static struct pci_dev pci_pdev = {
+ .is_physfn = 0,
+#ifdef CONFIG_PCI_ATS
+ .sriov = &pdev_sriov,
+#endif
+#ifdef CONFIG_PCI
+ .dev.bus = &pci_bus_type,
+#endif
+};
+
+struct vf_data_storage {
+ unsigned char vf_mac[ETH_ALEN];
+ u16 pf_vlan; /* When set, guest VLAN config not allowed. */
+ u16 pf_qos;
+ __be16 vlan_proto;
+ u16 min_tx_rate;
+ u16 max_tx_rate;
+ u8 spoofchk_enabled;
+ bool rss_query_enabled;
+ u8 trusted;
+ int link_state;
+};
+
+struct dummy_priv {
+ int num_vfs;
+ struct vf_data_storage *vfinfo;
+};
/* fake multicast ability */
static void set_multicast_list(struct net_device *dev)
@@ -91,15 +124,31 @@ static netdev_tx_t dummy_xmit(struct sk_buff *skb, struct net_device *dev)
static int dummy_dev_init(struct net_device *dev)
{
+ struct dummy_priv *priv = netdev_priv(dev);
+
dev->dstats = netdev_alloc_pcpu_stats(struct pcpu_dstats);
if (!dev->dstats)
return -ENOMEM;
+ priv->num_vfs = num_vfs;
+ priv->vfinfo = NULL;
+
+ if (!num_vfs)
+ return 0;
+
+ priv->vfinfo = kcalloc(num_vfs, sizeof(struct vf_data_storage),
+ GFP_KERNEL);
+ if (!priv->vfinfo) {
+ free_percpu(dev->dstats);
+ return -ENOMEM;
+ }
+
return 0;
}
static void dummy_dev_uninit(struct net_device *dev)
{
+ dev->dev.parent = NULL;
free_percpu(dev->dstats);
}
@@ -112,6 +161,134 @@ static int dummy_change_carrier(struct net_device *dev, bool new_carrier)
return 0;
}
+/* fake, just to set fake PCI parent after netdev_register_kobject() */
+static netdev_features_t dummy_fix_features(struct net_device *dev,
+ netdev_features_t features)
+{
+ struct dummy_priv *priv = netdev_priv(dev);
+
+ if (priv->num_vfs) {
+ dev->dev.parent = &pci_pdev.dev;
+ if (!pci_pdev.is_physfn) {
+ mutex_init(&pci_pdev.dev.mutex);
+ pci_pdev.is_physfn = 1;
+ }
+ }
+
+ return features;
+}
+
+static int dummy_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
+{
+ struct dummy_priv *priv = netdev_priv(dev);
+
+ if (!is_valid_ether_addr(mac) || (vf >= priv->num_vfs))
+ return -EINVAL;
+
+ memcpy(priv->vfinfo[vf].vf_mac, mac, ETH_ALEN);
+
+ return 0;
+}
+
+static int dummy_set_vf_vlan(struct net_device *dev, int vf,
+ u16 vlan, u8 qos, __be16 vlan_proto)
+{
+ struct dummy_priv *priv = netdev_priv(dev);
+
+ if ((vf >= priv->num_vfs) || (vlan > 4095) || (qos > 7))
+ return -EINVAL;
+
+ priv->vfinfo[vf].pf_vlan = vlan;
+ priv->vfinfo[vf].pf_qos = qos;
+ priv->vfinfo[vf].vlan_proto = vlan_proto;
+
+ return 0;
+}
+
+static int dummy_set_vf_rate(struct net_device *dev, int vf, int min, int max)
+{
+ struct dummy_priv *priv = netdev_priv(dev);
+
+ if (vf >= priv->num_vfs)
+ return -EINVAL;
+
+ priv->vfinfo[vf].min_tx_rate = min;
+ priv->vfinfo[vf].max_tx_rate = max;
+
+ return 0;
+}
+
+static int dummy_set_vf_spoofchk(struct net_device *dev, int vf, bool val)
+{
+ struct dummy_priv *priv = netdev_priv(dev);
+
+ if (vf >= priv->num_vfs)
+ return -EINVAL;
+
+ priv->vfinfo[vf].spoofchk_enabled = val;
+
+ return 0;
+}
+
+static int dummy_set_vf_rss_query_en(struct net_device *dev, int vf, bool val)
+{
+ struct dummy_priv *priv = netdev_priv(dev);
+
+ if (vf >= priv->num_vfs)
+ return -EINVAL;
+
+ priv->vfinfo[vf].rss_query_enabled = val;
+
+ return 0;
+}
+
+static int dummy_set_vf_trust(struct net_device *dev, int vf, bool val)
+{
+ struct dummy_priv *priv = netdev_priv(dev);
+
+ if (vf >= priv->num_vfs)
+ return -EINVAL;
+
+ priv->vfinfo[vf].trusted = val;
+
+ return 0;
+}
+
+static int dummy_get_vf_config(struct net_device *dev,
+ int vf, struct ifla_vf_info *ivi)
+{
+ struct dummy_priv *priv = netdev_priv(dev);
+
+ if (vf >= priv->num_vfs)
+ return -EINVAL;
+
+ ivi->vf = vf;
+ memcpy(&ivi->mac, priv->vfinfo[vf].vf_mac, ETH_ALEN);
+ ivi->vlan = priv->vfinfo[vf].pf_vlan;
+ ivi->qos = priv->vfinfo[vf].pf_qos;
+ ivi->spoofchk = priv->vfinfo[vf].spoofchk_enabled;
+ ivi->linkstate = priv->vfinfo[vf].link_state;
+ ivi->min_tx_rate = priv->vfinfo[vf].min_tx_rate;
+ ivi->max_tx_rate = priv->vfinfo[vf].max_tx_rate;
+ ivi->rss_query_en = priv->vfinfo[vf].rss_query_enabled;
+ ivi->trusted = priv->vfinfo[vf].trusted;
+ ivi->vlan_proto = priv->vfinfo[vf].vlan_proto;
+
+ return 0;
+}
+
+static int dummy_set_vf_link_state(struct net_device *dev, int vf, int state)
+{
+ struct dummy_priv *priv = netdev_priv(dev);
+
+ if (vf >= priv->num_vfs)
+ return -EINVAL;
+
+ priv->vfinfo[vf].link_state = state;
+
+ return 0;
+}
+
static const struct net_device_ops dummy_netdev_ops = {
.ndo_init = dummy_dev_init,
.ndo_uninit = dummy_dev_uninit,
@@ -121,6 +298,15 @@ static const struct net_device_ops dummy_netdev_ops = {
.ndo_set_mac_address = eth_mac_addr,
.ndo_get_stats64 = dummy_get_stats64,
.ndo_change_carrier = dummy_change_carrier,
+ .ndo_fix_features = dummy_fix_features,
+ .ndo_set_vf_mac = dummy_set_vf_mac,
+ .ndo_set_vf_vlan = dummy_set_vf_vlan,
+ .ndo_set_vf_rate = dummy_set_vf_rate,
+ .ndo_set_vf_spoofchk = dummy_set_vf_spoofchk,
+ .ndo_set_vf_trust = dummy_set_vf_trust,
+ .ndo_get_vf_config = dummy_get_vf_config,
+ .ndo_set_vf_link_state = dummy_set_vf_link_state,
+ .ndo_set_vf_rss_query_en = dummy_set_vf_rss_query_en,
};
static void dummy_get_drvinfo(struct net_device *dev,
@@ -134,6 +320,14 @@ static const struct ethtool_ops dummy_ethtool_ops = {
.get_drvinfo = dummy_get_drvinfo,
};
+static void dummy_free_netdev(struct net_device *dev)
+{
+ struct dummy_priv *priv = netdev_priv(dev);
+
+ kfree(priv->vfinfo);
+ free_netdev(dev);
+}
+
static void dummy_setup(struct net_device *dev)
{
ether_setup(dev);
@@ -141,7 +335,7 @@ static void dummy_setup(struct net_device *dev)
/* Initialize the device structure. */
dev->netdev_ops = &dummy_netdev_ops;
dev->ethtool_ops = &dummy_ethtool_ops;
- dev->destructor = free_netdev;
+ dev->destructor = dummy_free_netdev;
/* Fill in device structure with ethernet-generic values. */
dev->flags |= IFF_NOARP;
@@ -169,6 +363,7 @@ static int dummy_validate(struct nlattr *tb[], struct nlattr *data[])
static struct rtnl_link_ops dummy_link_ops __read_mostly = {
.kind = DRV_NAME,
+ .priv_size = sizeof(struct dummy_priv),
.setup = dummy_setup,
.validate = dummy_validate,
};
@@ -177,12 +372,16 @@ static struct rtnl_link_ops dummy_link_ops __read_mostly = {
module_param(numdummies, int, 0);
MODULE_PARM_DESC(numdummies, "Number of dummy pseudo devices");
+module_param(num_vfs, int, 0);
+MODULE_PARM_DESC(num_vfs, "Number of dummy VFs per dummy device");
+
static int __init dummy_init_one(void)
{
struct net_device *dev_dummy;
int err;
- dev_dummy = alloc_netdev(0, "dummy%d", NET_NAME_UNKNOWN, dummy_setup);
+ dev_dummy = alloc_netdev(sizeof(struct dummy_priv),
+ "dummy%d", NET_NAME_UNKNOWN, dummy_setup);
if (!dev_dummy)
return -ENOMEM;
@@ -201,6 +400,8 @@ static int __init dummy_init_module(void)
{
int i, err = 0;
+ pdev_sriov.num_VFs = num_vfs;
+
rtnl_lock();
err = __rtnl_link_register(&dummy_link_ops);
if (err < 0)
--
2.10.0
^ permalink raw reply related
* Re: [PATCH] flowcache: Increase threshold for refusing new allocations
From: Steffen Klassert @ 2016-11-23 12:42 UTC (permalink / raw)
To: Miroslav Urbanek; +Cc: NetDev
In-Reply-To: <20161121144820.GA4048@miroslavurbanek.com>
On Mon, Nov 21, 2016 at 03:48:21PM +0100, Miroslav Urbanek wrote:
> The threshold for OOM protection is too small for systems with large
> number of CPUs. Applications report ENOBUFs on connect() every 10
> minutes.
>
> The problem is that the variable net->xfrm.flow_cache_gc_count is a
> global counter while the variable fc->high_watermark is a per-CPU
> constant. Take the number of CPUs into account as well.
>
> Fixes: 6ad3122a08e3 ("flowcache: Avoid OOM condition under preasure")
> Reported-by: Lukáš Koldrt <lk@excello.cz>
> Tested-by: Jan Hejl <jh@excello.cz>
> Signed-off-by: Miroslav Urbanek <mu@miroslavurbanek.com>
Applied to the ipsec tree, thanks!
^ permalink raw reply
* RE: [PATCH] net: fix bogus cast in skb_pagelen() and use unsigned variables
From: David Laight @ 2016-11-23 12:49 UTC (permalink / raw)
To: 'Alexey Dobriyan', davem@davemloft.net; +Cc: netdev@vger.kernel.org
In-Reply-To: <20161119010808.GF1200@avx2>
From: Alexey Dobriyan
> Sent: 19 November 2016 01:08
...
> - for (i = (int)skb_shinfo(skb)->nr_frags - 1; i >= 0; i--)
> + for (i = skb_shinfo(skb)->nr_frags - 1; (int)i >= 0; i--)
> len += skb_frag_size(&skb_shinfo(skb)->frags[i]);
Think I'd use:
for (i = skb_shinfo(skb)->nr_frags; i-- != 0; )
David
^ permalink raw reply
* Re: [PATCH net-next 1/4] net: mvneta: Convert to be 64 bits compatible
From: Gregory CLEMENT @ 2016-11-23 13:07 UTC (permalink / raw)
To: Jisheng Zhang, Arnd Bergmann
Cc: linux-arm-kernel, Thomas Petazzoni, Andrew Lunn, Jason Cooper,
netdev, linux-kernel, Marcin Wojtas, David S. Miller,
Sebastian Hesselbarth
In-Reply-To: <9432400.S1OrxC027t@wuerfel>
Hi Jisheng, Arnd,
Thanks for your feedback.
On mer., nov. 23 2016, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday, November 23, 2016 5:53:41 PM CET Jisheng Zhang wrote:
>> On Tue, 22 Nov 2016 22:04:12 +0100 Arnd Bergmann wrote:
>>
>> > On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:
>> > > +#ifdef CONFIG_64BIT
>> > > + void *data_tmp;
>> > > +
>> > > + /* In Neta HW only 32 bits data is supported, so in order to
>> > > + * obtain whole 64 bits address from RX descriptor, we store
>> > > + * the upper 32 bits when allocating buffer, and put it back
>> > > + * when using buffer cookie for accessing packet in memory.
>> > > + * Frags should be allocated from single 'memory' region,
>> > > + * hence common upper address half should be sufficient.
>> > > + */
>> > > + data_tmp = mvneta_frag_alloc(pp->frag_size);
>> > > + if (data_tmp) {
>> > > + pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
>> > > + mvneta_frag_free(pp->frag_size, data_tmp);
>> > > + }
>> > >
>> >
>> > How does this work when the region spans a n*4GB address boundary?
>>
>> indeed. We also make use of this driver on 64bit platforms. We use
>> different solution to make the driver 64bit safe.
>>
>> solA: make use of the reserved field in the mvneta_rx_desc, such
>> as reserved2 etc. Yes, the field is marked as "for future use, PnC", but
>> now it's not used at all. This is one possible solution however.
>
> Right, this sounds like the most straightforward choice.
The PnC (which stands for Parsing and Classification) is not used yet
indeed but this field will be needed when we will enable it. It is
something we want to do but it is not planned in a near future. However
from the datasheets I have it seems only present on the Armada XP. It is
not mentioned on datasheets for the Armada 38x or the Armada 3700.
That would mean it was safe to use on of this field in 64-bits mode on
the Armada 3700.
So I am going to take this approach.
Thanks,
Gregory
>
>> solB: allocate a shadow buf cookie during init, e.g
>>
>> rxq->descs_bufcookie = kmalloc(rxq->size * sizeof(void*), GFP_KERNEL);
>>
>> then modify mvneta_rx_desc_fill a bit to save the 64bit pointer in
>> the shadow buf cookie, e.g
>> static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc,
>> u32 phys_addr, u32 cookie,
>> struct mvneta_rx_queue *rxq)
>>
>> {
>> int i;
>>
>> rx_desc->buf_cookie = cookie;
>> rx_desc->buf_phys_addr = phys_addr;
>> i = rx_desc - rxq->descs;
>> rxq->descs_bufcookie[i] = cookie;
>> }
>>
>> then fetch the desc from the shadow buf cookie in all code path, such
>> as mvneta_rx() etc.
>>
>> Both solutions should not have the problems pointed out by Arnd.
>
> Wait, since you compute an index 'i' here, can't you just store 'i'
> directly in the descriptor instead of the pointer?
>
> Arnd
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* Re: [net-next PATCH v3] net: dummy: Introduce dummy virtual functions
From: Eric Dumazet @ 2016-11-23 13:26 UTC (permalink / raw)
To: Phil Sutter; +Cc: David Miller, netdev, Sabrina Dubroca
In-Reply-To: <20161123124125.3374-1-phil@nwl.cc>
On Wed, 2016-11-23 at 13:41 +0100, Phil Sutter wrote:
> +struct vf_data_storage {
> + unsigned char vf_mac[ETH_ALEN];
> + u16 pf_vlan; /* When set, guest VLAN config not allowed. */
> + u16 pf_qos;
> + __be16 vlan_proto;
> + u16 min_tx_rate;
> + u16 max_tx_rate;
> + u8 spoofchk_enabled;
> + bool rss_query_enabled;
> + u8 trusted;
> + int link_state;
> +};
Could you use proper indentation of field names ?
struct vf_data_storage {
u8 vf_mac[ETH_ALEN];
u16 pf_vlan; /* When set, guest VLAN config not allowed. */
u16 pf_qos;
__be16 vlan_proto;
u16 min_tx_rate;
u16 max_tx_rate;
u8 spoofchk_enabled;
bool rss_query_enabled;
u8 trusted;
int link_state;
};
struct dummy_priv {
int num_vfs;
struct vf_data_storage *vfinfo;
};
Thanks.
^ permalink raw reply
* [PATCH] can: bcm: fix support for CAN FD frames
From: Oliver Hartkopp @ 2016-11-23 13:33 UTC (permalink / raw)
To: linux-can, Andrey Konovalov, netdev; +Cc: Oliver Hartkopp
Since commit 6f3b911d5f29b98 ("can: bcm: add support for CAN FD frames") the
CAN broadcast manager supports CAN and CAN FD data frames.
As these data frames are embedded in struct can[fd]_frames which have a
different length the access to the provided array of CAN frames became
dependend of op->cfsiz. By using a struct canfd_frame pointer for the array of
CAN frames the new offset calculation based on op->cfsiz was accidently applied
to CAN FD frame element lengths.
This fix makes the pointer to the arrays of the different CAN frame types a
void pointer so that the offset calculation in bytes accesses the correct CAN
frame elements.
Reference: http://marc.info/?l=linux-netdev&m=147980658909653
Reported-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 8af9d25..436a753 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -77,7 +77,7 @@
(CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
(CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
-#define CAN_BCM_VERSION "20160617"
+#define CAN_BCM_VERSION "20161123"
MODULE_DESCRIPTION("PF_CAN broadcast manager protocol");
MODULE_LICENSE("Dual BSD/GPL");
@@ -109,8 +109,9 @@ struct bcm_op {
u32 count;
u32 nframes;
u32 currframe;
- struct canfd_frame *frames;
- struct canfd_frame *last_frames;
+ /* void pointers to arrays of struct can[fd]_frame */
+ void *frames;
+ void *last_frames;
struct canfd_frame sframe;
struct canfd_frame last_sframe;
struct sock *sk;
@@ -681,7 +682,7 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
if (op->flags & RX_FILTER_ID) {
/* the easiest case */
- bcm_rx_update_and_send(op, &op->last_frames[0], rxframe);
+ bcm_rx_update_and_send(op, op->last_frames, rxframe);
goto rx_starttimer;
}
@@ -1068,7 +1069,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
if (msg_head->nframes) {
/* update CAN frames content */
- err = memcpy_from_msg((u8 *)op->frames, msg,
+ err = memcpy_from_msg(op->frames, msg,
msg_head->nframes * op->cfsiz);
if (err < 0)
return err;
@@ -1118,7 +1119,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
}
if (msg_head->nframes) {
- err = memcpy_from_msg((u8 *)op->frames, msg,
+ err = memcpy_from_msg(op->frames, msg,
msg_head->nframes * op->cfsiz);
if (err < 0) {
if (op->frames != &op->sframe)
@@ -1163,6 +1164,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
/* check flags */
if (op->flags & RX_RTR_FRAME) {
+ struct canfd_frame *frame0 = op->frames;
/* no timers in RTR-mode */
hrtimer_cancel(&op->thrtimer);
@@ -1174,8 +1176,8 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
* prevent a full-load-loopback-test ... ;-]
*/
if ((op->flags & TX_CP_CAN_ID) ||
- (op->frames[0].can_id == op->can_id))
- op->frames[0].can_id = op->can_id & ~CAN_RTR_FLAG;
+ (frame0->can_id == op->can_id))
+ frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
} else {
if (op->flags & SETTIMER) {
--
2.10.2
^ permalink raw reply related
* Re: [RFC PATCH v2 1/2] macb: Add 1588 support in Cadence GEM.
From: Andrei Pistirica @ 2016-11-23 13:34 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, linux-kernel, linux-arm-kernel, davem, nicolas.ferre,
harinikatakamlinux, harini.katakam, punnaia, michals, anirudh,
boris.brezillon, alexandre.belloni, tbultel
In-Reply-To: <20161120191823.GA6950@localhost.localdomain>
On 20.11.2016 20:18, Richard Cochran wrote:
> On Fri, Nov 18, 2016 at 03:21:51PM +0100, Andrei Pistirica wrote:
>> - Frequency adjustment is not directly supported by this IP.
>
> This statement still makes no sense. Doesn't the following text...
This statement is inherited from original patch, and it refers to the
fact that it doesn't change directly the frequency, it changes the
increment value.
I'll remove it to avoid any confusion.
>
>> addend is the initial value ns increment and similarly addendesub.
>> The ppb (parts per billion) provided is used as
>> ns_incr = addend +/- (ppb/rate).
>> Similarly the remainder of the above is used to populate subns increment.
>
> describe how frequency adjustment is in fact supported?
Ack, I will clarify.
>
>> +config MACB_USE_HWSTAMP
>> + bool "Use IEEE 1588 hwstamp"
>> + depends on MACB
>> + default y
>> + select PTP_1588_CLOCK
>
> This "select" pattern is going to be replaced with "imply" soon.
>
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1269181.html
>
> You should use the new "imply" key word and reference that series in
> your change log.
Ack.
>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 3f385ab..2ee9af8 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -10,6 +10,10 @@
>> #ifndef _MACB_H
>> #define _MACB_H
>>
>> +#include <linux/net_tstamp.h>
>> +#include <linux/ptp_clock.h>
>> +#include <linux/ptp_clock_kernel.h>
>
> Don't need net_tstamp.h here. Move it into the .c file please.
Ack.
>
>> @@ -840,8 +902,26 @@ struct macb {
>>
>> unsigned int rx_frm_len_mask;
>> unsigned int jumbo_max_len;
>> +
>> +#ifdef CONFIG_MACB_USE_HWSTAMP
>> + unsigned int hwts_tx_en;
>> + unsigned int hwts_rx_en;
>
> These two can be bool'eans.
Ack.
>
>> + spinlock_t tsu_clk_lock;
>> + unsigned int tsu_rate;
>> +
>> + struct ptp_clock *ptp_clock;
>> + struct ptp_clock_info ptp_caps;
>> + unsigned int ns_incr;
>> + unsigned int subns_incr;
>
> These two are 32 bit register values, right? Then use the u32 type.
Yes. I will make the change.
>
>> +#endif
>> };
>
>> +static inline void macb_tsu_set_time(struct macb *bp,
>> + const struct timespec64 *ts)
>> +{
>> + u32 ns, sech, secl;
>> + s64 word_mask = 0xffffffff;
>> +
>> + sech = (u32)ts->tv_sec;
>> + secl = (u32)ts->tv_sec;
>> + ns = ts->tv_nsec;
>> + if (ts->tv_sec > word_mask)
>> + sech = (ts->tv_sec >> 32);
>> +
>> + spin_lock(&bp->tsu_clk_lock);
>> +
>> + /* TSH doesn't latch the time and no atomicity! */
>> + gem_writel(bp, TSH, sech);
>> + gem_writel(bp, TSL, secl);
>
> If TN overflows here then the clock will be off by one whole second!
> Why not clear TN first?
Ack.
>
>> + gem_writel(bp, TN, ns);
>> +
>> + spin_unlock(&bp->tsu_clk_lock);
>> +}
>> +
>> +static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
>> +{
>> + struct macb *bp = container_of(ptp, struct macb, ptp_caps);
>> + u32 addend, addend_frac, rem;
>> + u64 drift_tmr, diff, diff_frac = 0;
>> + int neg_adj = 0;
>> +
>> + if (ppb < 0) {
>> + neg_adj = 1;
>> + ppb = -ppb;
>> + }
>> +
>> + /* drift/period */
>> + drift_tmr = (bp->ns_incr * ppb) +
>> + ((bp->subns_incr * ppb) >> 16);
>
> What? Why the 16 bit shift? Last time your said it was 24 bits.
SAMA5D2/3/4 uses GEM-PTP version (16bit), while Harini wrote a driver
for GEM-GXL (24bit). Probably will be a patch on top of this one for
GXL. This confusion was because I tried to keep the original patch
unchanged.
>
>> + /* drift/cycle */
>> + diff = div_u64_rem(drift_tmr, 1000000000ULL, &rem);
>> +
>> + /* drift fraction/cycle, if necessary */
>> + if (rem) {
>> + u64 fraction = rem;
>> + fraction = fraction << 16;
>> +
>> + diff_frac = div_u64(fraction, 1000000000ULL);
>
> If you use a combined tuning word like I explained last time, then you
> will not need a second division.
From what I understand, your suggestion is:
(ns | frac) * ppb = (total_ns | total_frac)
(total_ns | total_frac) / 10^9 = (adj_ns | adj_frac)
This is correct iff total_ns/10^9 >= 1, but the problem is that there
are missed fractions due to the following approximation:
frac*ppb =~
(ns*ppb+frac*ppb*2^16)*2^16-10^9*2^16*flor(ns*ppb+frac*ppb*2^16, 10^9).
An example which uses values from a real test:
let ppb=4891, ns=12 and frac=3158
- using suggested algorithm, yields: adj_ns = 0 and adj_frac = 0
- using in-place algorithm, yields: adj_ns = 0, adj_frac = 4
You can check the calculus.
Therefore, I would like to keep the in-place algorithm.
>
> Also, please use the new adjfine() PHC method, as adjfreq() is now deprecated.
Yes, I will port the patches on net-next and use adjfine.
>
>> + }
>> +
>> + /* adjustmets */
>> + addend = neg_adj ? (bp->ns_incr - diff) : (bp->ns_incr + diff);
>> + addend_frac = neg_adj ? (bp->subns_incr - diff_frac) :
>> + (bp->subns_incr + diff_frac);
>> +
>> + spin_lock(&bp->tsu_clk_lock);
>> +
>> + gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, addend_frac));
>> + gem_writel(bp, TI, GEM_BF(NSINCR, addend));
>> +
>> + spin_unlock(&bp->tsu_clk_lock);
>> + return 0;
>> +}
>
>> +void macb_ptp_init(struct net_device *ndev)
>> +{
>> + struct macb *bp = netdev_priv(ndev);
>> + struct timespec64 now;
>> + u32 rem = 0;
>> +
>> + if (!(bp->caps | MACB_CAPS_GEM_HAS_PTP)){
>> + netdev_vdbg(bp->dev, "Platform does not support PTP!\n");
>> + return;
>> + }
>> +
>> + spin_lock_init(&bp->tsu_clk_lock);
>> +
>> + bp->ptp_caps = macb_ptp_caps;
>> + bp->tsu_rate = clk_get_rate(bp->pclk);
>> +
>> + getnstimeofday64(&now);
>> + macb_tsu_set_time(bp, (const struct timespec64 *)&now);
>> +
>> + bp->ns_incr = div_u64_rem(NSEC_PER_SEC, bp->tsu_rate, &rem);
>> + if (rem) {
>> + u64 adj = rem;
>> + /* Multiply by 2^16 as subns register is 16 bits */
>
> Last time you said:
>> + /* Multiple by 2^24 as subns field is 24 bits */
>
>> + adj <<= 16;
>> + bp->subns_incr = div_u64(adj, bp->tsu_rate);
>> + } else {
>> + bp->subns_incr = 0;
>> + }
>> +
>> + gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, bp->subns_incr));
>> + gem_writel(bp, TI, GEM_BF(NSINCR, bp->ns_incr));
>> + gem_writel(bp, TA, 0);
>> +
>> + bp->ptp_clock = ptp_clock_register(&bp->ptp_caps, NULL);
>
> You call regsiter, but you never call unregister!
Ack. I did a stupid mistake... sorry.
>
>> + if (IS_ERR(&bp->ptp_clock)) {
>> + bp->ptp_clock = NULL;
>> + pr_err("ptp clock register failed\n");
>> + return;
>> + }
>> +
>> + dev_info(&bp->pdev->dev, "%s ptp clock registered.\n", GMAC_TIMER_NAME);
>> +}
>> +
>> --
>> 1.9.1
>>
>
> Thanks,
> Richard
>
Regards,
Andrei
^ permalink raw reply
* Re: [RFC PATCH v2 1/2] macb: Add 1588 support in Cadence GEM.
From: Andrei Pistirica @ 2016-11-23 13:36 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, linux-kernel, linux-arm-kernel, davem, nicolas.ferre,
harinikatakamlinux, harini.katakam, punnaia, michals, anirudh,
boris.brezillon, alexandre.belloni, tbultel
In-Reply-To: <20161120193720.GA7874@localhost.localdomain>
On 20.11.2016 20:37, Richard Cochran wrote:
> On Fri, Nov 18, 2016 at 03:21:51PM +0100, Andrei Pistirica wrote:
>> +#ifdef CONFIG_MACB_USE_HWSTAMP
>> +void macb_ptp_init(struct net_device *ndev);
>> +#else
>> +void macb_ptp_init(struct net_device *ndev) { }
>
> static inline ^^^
I can do static inline only when PTP is not enabled (on else branch),
thus the empty function is defined in the header file (since the init
function is defined in macb_ptp and used in macb). To differentiate
between macb versions, I'll add a wrapper.
Would this be ok?
>
>> +#endif
>
>
>> +void macb_ptp_init(struct net_device *ndev)
>> +{
>> + struct macb *bp = netdev_priv(ndev);
>> + struct timespec64 now;
>> + u32 rem = 0;
>> +
>> + if (!(bp->caps | MACB_CAPS_GEM_HAS_PTP)){
>> + netdev_vdbg(bp->dev, "Platform does not support PTP!\n");
>> + return;
>> + }
>
> You would have needed '&' and not '|' here.
Yes. Another stupid mistake... sorry. I will be more careful next time.
>
> Also, using a flag limits the code to your platform. This works for
> you, but it is short sighted. The other MACB PTP blocks have
> different register layouts, and this patch does not lay the ground
> work for the others.
>
> The driver needs to be designed to support the other platforms.
It will support Xilinx.
>
> Thanks,
> Richard
>
Regards,
Andrei
^ permalink raw reply
* Re: [RFC PATCH v2 2/2] macb: Enable 1588 support in SAMA5D2 platform.
From: Andrei Pistirica @ 2016-11-23 13:35 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, linux-kernel, linux-arm-kernel, davem, nicolas.ferre,
harinikatakamlinux, harini.katakam, punnaia, michals, anirudh,
boris.brezillon, alexandre.belloni, tbultel
In-Reply-To: <20161120195413.GB7874@localhost.localdomain>
On 20.11.2016 20:54, Richard Cochran wrote:
> On Fri, Nov 18, 2016 at 03:21:52PM +0100, Andrei Pistirica wrote:
>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>> index d975882..eb66b76 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -697,6 +697,8 @@ static void macb_tx_interrupt(struct macb_queue *queue)
>>
>> /* First, update TX stats if needed */
>> if (skb) {
>> + macb_ptp_do_txstamp(bp, skb);
>
> This is in the hot path, and so you should have an inline wrapper that
> tests (bp->hwts_tx_en) and THEN calls into macb_ptp.c
Ack.
>
> In case PTP isn't configured, then the inline wrapper should be empty.
>
>> netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
>> macb_tx_ring_wrap(tail), skb->data);
>> bp->stats.tx_packets++;
>> @@ -853,6 +855,8 @@ static int gem_rx(struct macb *bp, int budget)
>> GEM_BFEXT(RX_CSUM, ctrl) & GEM_RX_CSUM_CHECKED_MASK)
>> skb->ip_summed = CHECKSUM_UNNECESSARY;
>>
>> + macb_ptp_do_rxstamp(bp, skb);
>
> Same here.
>
>> bp->stats.rx_packets++;
>> bp->stats.rx_bytes += skb->len;
>>
>> @@ -1946,6 +1950,8 @@ static int macb_open(struct net_device *dev)
>>
>> netif_tx_start_all_queues(dev);
>>
>> + macb_ptp_init(dev);
>
> This leaks PHC instances starting the second time that the interface goes up!
Yes, I will call unregister at interface down.
>
>> return 0;
>> }
>>
>> @@ -2204,7 +2210,7 @@ static const struct ethtool_ops gem_ethtool_ops = {
>> .get_regs_len = macb_get_regs_len,
>> .get_regs = macb_get_regs,
>> .get_link = ethtool_op_get_link,
>> - .get_ts_info = ethtool_op_get_ts_info,
>> + .get_ts_info = macb_get_ts_info,
>
> You enable the time stamping logic unconditionally here ...
I will add a wrapper to test if macb is gem and if it has PTP
capability, otherwise call ethtool_op_get_ts_info.
>
>> .get_ethtool_stats = gem_get_ethtool_stats,
>> .get_strings = gem_get_ethtool_strings,
>> .get_sset_count = gem_get_sset_count,
>> @@ -2221,7 +2227,14 @@ static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>> if (!phydev)
>> return -ENODEV;
>>
>> - return phy_mii_ioctl(phydev, rq, cmd);
>> + switch (cmd) {
>> + case SIOCSHWTSTAMP:
>> + return macb_hwtst_set(dev, rq, cmd);
>> + case SIOCGHWTSTAMP:
>> + return macb_hwtst_get(dev, rq);
>
> and here.
>
> Is that logic always available on every MACB device? If so, is the
> implementation also identical?
As before, I will add a wrapper and the related tests.
>
> Thanks,
> Richard
>
Regards,
Andrei
^ permalink raw reply
* Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable
From: Mark Lord @ 2016-11-23 13:41 UTC (permalink / raw)
To: Hayes Wang, netdev@vger.kernel.org
Cc: nic_swsd, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
In-Reply-To: <0835B3720019904CB8F7AA43166CEEB2010517CE@RTITMBSV03.realtek.com.tw>
What does this code do:
>static void r8153_set_rx_early_size(struct r8152 *tp)
>{
> u32 mtu = tp->netdev->mtu;
> u32 ocp_data = (agg_buf_sz - mtu - VLAN_ETH_HLEN - VLAN_HLEN) / 4;
>
> ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_SIZE, ocp_data);
>}
How is ocp_data used by the hardware?
Shouldn't the calculation also include sizeof(rx_desc) in there somewhere?
Thanks
--
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com
^ permalink raw reply
* [PATCH] can: rcar_canfd: Correct order of interrupt specifiers
From: Geert Uytterhoeven @ 2016-11-23 13:44 UTC (permalink / raw)
To: Wolfgang Grandegger, Marc Kleine-Budde, Ramesh Shanmugasundaram,
Chris Paterson
Cc: linux-can, netdev, devicetree, linux-renesas-soc,
Geert Uytterhoeven
According to both DTS (example and actual files), and Linux driver code,
the first interrupt specifier should be the Channel interrupt, while the
second interrupt specifier should be the Global interrupt.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Documentation/devicetree/bindings/net/can/rcar_canfd.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/net/can/rcar_canfd.txt b/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
index 22a6f10bab057f46..58c27cd839e3a2ac 100644
--- a/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
+++ b/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
@@ -11,7 +11,7 @@ Required properties:
family-specific and/or generic versions.
- reg: physical base address and size of the R-Car CAN FD register map.
-- interrupts: interrupt specifier for the Global & Channel interrupts
+- interrupts: interrupt specifiers for the Channel & Global interrupts
- clocks: phandles and clock specifiers for 3 clock inputs.
- clock-names: 3 clock input name strings: "fck", "canfd", "can_clk".
- pinctrl-0: pin control group to be used for this controller.
--
1.9.1
^ permalink raw reply related
* Re: [RFC net-next 0/3] net: bridge: Allow CPU port configuration
From: Ido Schimmel @ 2016-11-23 13:48 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, davem, bridge, stephen, vivien.didelot, andrew, jiri,
idosch
In-Reply-To: <e9bbd22e-215f-a1fd-62ba-c0d33b255154@gmail.com>
Hi Florian,
On Tue, Nov 22, 2016 at 09:56:30AM -0800, Florian Fainelli wrote:
> On 11/22/2016 09:41 AM, Ido Schimmel wrote:
> > Hi Florian,
> >
> > On Mon, Nov 21, 2016 at 11:09:22AM -0800, Florian Fainelli wrote:
> >> Hi all,
> >>
> >> This patch series allows using the bridge master interface to configure
> >> an Ethernet switch port's CPU/management port with different VLAN attributes than
> >> those of the bridge downstream ports/members.
> >>
> >> Jiri, Ido, Andrew, Vivien, please review the impact on mlxsw and mv88e6xxx, I
> >> tested this with b53 and a mockup DSA driver.
> >
> > We'll need to add a check in mlxsw and ignore any VLAN configuration for
> > the bridge device itself. Otherwise, any configuration done on br0 will
> > be propagated to all of its slaves, which is incorrect.
> >
> >>
> >> Open questions:
> >>
> >> - if we have more than one bridge on top of a physical switch, the driver
> >> should keep track of that and verify that we are not going to change
> >> the CPU port VLAN attributes in a way that results in incompatible settings
> >> to be applied
> >>
> >> - if the default behavior is to have all VLANs associated with the CPU port
> >> be ingressing/egressing tagged to the CPU, is this really useful?
> >
> > First of all, I want to be sure that when we say "CPU port", we're
> > talking about the same thing. In mlxsw, the CPU port is a pipe between
> > the device and the host, through which all packets trapped to the host
> > go through. So, when a packet is trapped, the driver reads its Rx
> > descriptor, checks through which port it ingressed, resolves its netdev,
> > sets skb->dev accordingly and injects it to the Rx path via
> > netif_receive_skb(). The CPU port itself isn't represented using a
> > netdev.
>
> In the case of DSA, the CPU port is a normal Ethernet MAC driver, but in
> premise, this driver plus the DSA tag protocol hook do exactly the same
> things as you just describe.
Thanks for the detailed explanation! I also took the time to read
dsa.txt, however I still don't quite understand the motivation for
VLAN filtering on the CPU port. In which cases would you like to prevent
packets from going to the host due to their VLAN header? This change
would make sense to me if you only had a limited number of VLANs you
could enable on the CPU port, but from your response I understand that
this isn't the case.
FWIW, I don't have a problem with patches if they are useful for you,
I'm just trying to understand the use case. We can easily patch mlxsw to
ignore calls with orig_dev=br0.
Thanks!
^ permalink raw reply
* RE: [PATCH] can: rcar_canfd: Correct order of interrupt specifiers
From: Ramesh Shanmugasundaram @ 2016-11-23 14:04 UTC (permalink / raw)
To: Geert Uytterhoeven, Wolfgang Grandegger, Marc Kleine-Budde,
Chris Paterson
Cc: linux-can@vger.kernel.org, netdev@vger.kernel.org,
devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org
In-Reply-To: <1479908686-14028-1-git-send-email-geert+renesas@glider.be>
Hi Geert,
> Subject: [PATCH] can: rcar_canfd: Correct order of interrupt specifiers
>
> According to both DTS (example and actual files), and Linux driver code,
> the first interrupt specifier should be the Channel interrupt, while the
> second interrupt specifier should be the Global interrupt.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Documentation/devicetree/bindings/net/can/rcar_canfd.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
> b/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
> index 22a6f10bab057f46..58c27cd839e3a2ac 100644
> --- a/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
> +++ b/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
> @@ -11,7 +11,7 @@ Required properties:
> family-specific and/or generic versions.
>
> - reg: physical base address and size of the R-Car CAN FD register map.
> -- interrupts: interrupt specifier for the Global & Channel interrupts
> +- interrupts: interrupt specifiers for the Channel & Global interrupts
Thanks for correcting it.
Acked-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
Thanks,
Ramesh
^ permalink raw reply
* Re: [PATCH] can: bcm: fix support for CAN FD frames
From: Andrey Konovalov @ 2016-11-23 14:15 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can, netdev
In-Reply-To: <20161123133325.1812-1-socketcan@hartkopp.net>
On Wed, Nov 23, 2016 at 2:33 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Since commit 6f3b911d5f29b98 ("can: bcm: add support for CAN FD frames") the
> CAN broadcast manager supports CAN and CAN FD data frames.
>
> As these data frames are embedded in struct can[fd]_frames which have a
> different length the access to the provided array of CAN frames became
> dependend of op->cfsiz. By using a struct canfd_frame pointer for the array of
> CAN frames the new offset calculation based on op->cfsiz was accidently applied
> to CAN FD frame element lengths.
>
> This fix makes the pointer to the arrays of the different CAN frame types a
> void pointer so that the offset calculation in bytes accesses the correct CAN
> frame elements.
>
> Reference: http://marc.info/?l=linux-netdev&m=147980658909653
>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> net/can/bcm.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 8af9d25..436a753 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -77,7 +77,7 @@
> (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
> (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
>
> -#define CAN_BCM_VERSION "20160617"
> +#define CAN_BCM_VERSION "20161123"
>
> MODULE_DESCRIPTION("PF_CAN broadcast manager protocol");
> MODULE_LICENSE("Dual BSD/GPL");
> @@ -109,8 +109,9 @@ struct bcm_op {
> u32 count;
> u32 nframes;
> u32 currframe;
> - struct canfd_frame *frames;
> - struct canfd_frame *last_frames;
> + /* void pointers to arrays of struct can[fd]_frame */
> + void *frames;
> + void *last_frames;
> struct canfd_frame sframe;
> struct canfd_frame last_sframe;
> struct sock *sk;
> @@ -681,7 +682,7 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>
> if (op->flags & RX_FILTER_ID) {
> /* the easiest case */
> - bcm_rx_update_and_send(op, &op->last_frames[0], rxframe);
> + bcm_rx_update_and_send(op, op->last_frames, rxframe);
> goto rx_starttimer;
> }
>
> @@ -1068,7 +1069,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>
> if (msg_head->nframes) {
> /* update CAN frames content */
> - err = memcpy_from_msg((u8 *)op->frames, msg,
> + err = memcpy_from_msg(op->frames, msg,
> msg_head->nframes * op->cfsiz);
> if (err < 0)
> return err;
> @@ -1118,7 +1119,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> }
>
> if (msg_head->nframes) {
> - err = memcpy_from_msg((u8 *)op->frames, msg,
> + err = memcpy_from_msg(op->frames, msg,
> msg_head->nframes * op->cfsiz);
> if (err < 0) {
> if (op->frames != &op->sframe)
> @@ -1163,6 +1164,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> /* check flags */
>
> if (op->flags & RX_RTR_FRAME) {
> + struct canfd_frame *frame0 = op->frames;
>
> /* no timers in RTR-mode */
> hrtimer_cancel(&op->thrtimer);
> @@ -1174,8 +1176,8 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> * prevent a full-load-loopback-test ... ;-]
> */
> if ((op->flags & TX_CP_CAN_ID) ||
> - (op->frames[0].can_id == op->can_id))
> - op->frames[0].can_id = op->can_id & ~CAN_RTR_FLAG;
> + (frame0->can_id == op->can_id))
> + frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
>
> } else {
> if (op->flags & SETTIMER) {
> --
> 2.10.2
>
Tested-by: Andrey Konovalov <andreyknvl@google.com>
^ permalink raw reply
* [PATCH 1/4] bindings: net: stmmac: correct note about TSO
From: Niklas Cassel @ 2016-11-23 14:24 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, David S. Miller, Giuseppe CAVALLARO,
Alexandre TORGUE, Phil Reid, Niklas Cassel, Eric Engestrom
Cc: Niklas Cassel, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: Niklas Cassel <niklas.cassel-VrBV9hrLPhE@public.gmane.org>
snps,tso was previously placed under AXI BUS Mode parameters,
suggesting that the property should be in the stmmac-axi-config node.
TSO (TCP Segmentation Offloading) has nothing to do with AXI BUS Mode
parameters, and the parser actually expects it to be in the root node,
not in the stmmac-axi-config.
Also added a note about snps,tso only being available on GMAC4 and newer.
Signed-off-by: Niklas Cassel <niklas.cassel-VrBV9hrLPhE@public.gmane.org>
---
Documentation/devicetree/bindings/net/stmmac.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
index 41b49e6075f5..b95ff998ba73 100644
--- a/Documentation/devicetree/bindings/net/stmmac.txt
+++ b/Documentation/devicetree/bindings/net/stmmac.txt
@@ -1,7 +1,7 @@
* STMicroelectronics 10/100/1000 Ethernet driver (GMAC)
Required properties:
-- compatible: Should be "snps,dwmac-<ip_version>" "snps,dwmac"
+- compatible: Should be "snps,dwmac-<ip_version>", "snps,dwmac"
For backwards compatibility: "st,spear600-gmac" is also supported.
- reg: Address and length of the register set for the device
- interrupt-parent: Should be the phandle for the interrupt controller
@@ -50,6 +50,8 @@ Optional properties:
- snps,ps-speed: port selection speed that can be passed to the core when
PCS is supported. For example, this is used in case of SGMII
and MAC2MAC connection.
+- snps,tso: this enables the TSO feature otherwise it will be managed by
+ MAC HW capability register. Only for GMAC4 and newer.
- AXI BUS Mode parameters: below the list of all the parameters to program the
AXI register inside the DMA module:
- snps,lpi_en: enable Low Power Interface
@@ -62,8 +64,6 @@ Optional properties:
- snps,fb: fixed-burst
- snps,mb: mixed-burst
- snps,rb: rebuild INCRx Burst
- - snps,tso: this enables the TSO feature otherwise it will be managed by
- MAC HW capability register.
- mdio: with compatible = "snps,dwmac-mdio", create and register mdio bus.
Examples:
--
2.1.4
--
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 related
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