* Re: PHYless ethernet switch MAC-MAC serdes connection
From: Vijay @ 2015-01-21 7:09 UTC (permalink / raw)
To: netdev
In-Reply-To: <54BE9B0B.7070507@gmail.com>
Florian Fainelli <f.fainelli <at> gmail.com> writes:
>
> Do lan1 and lan2 interfaces set the RUNNING flag, which would indicate
> that the switch driver has detected a carrier for these individual
ports?
>
> Is the DSA switch driver correctly polling for LAN ports link
statuses?
>
Yes RUNNING flag is set.
Whenever I unplug/plug Ethernet cable. I see notification from dsa about
link up/down correctly.
>
> DSA registers a slave MDIO bus interface which calls into the "real"
> MDIO bus hardware to talk to the switch. In your case, you have both
an
> external switch and an external PHY connected to the same MDIO bus,
> which is fine.
>
> Your DT looks sane, although I suspect that lan2's PHY address is 3,
and
> not 2, as address 0 is typically a special MDIO broadcast address, so
> Port 0's PHY can be read at PHY addr 1, Port 1's PHY can be read at
PHY
> addr 2 etc... If you remove "lan2", does it work slightly better?
>
No, lan 1 and lan2 addresses are 0 and 1 respectively. I have logged
these values and these are printed on console as:
DSA: dsa.c: Init slave bus
DSA: slave.c: dsa_slave_mii_bus_init sw addr =0x1f , slave_mii_bus_id =
0x0
DSA: dsa.c: register created slave_mii_bus
Debug: drivers/net/phy/phy_device.c: Creating PHY-> phy_id = 0x1410c89,
addr = 0x0
Creating PHY-> AUTONEG_ENABLE for phy_id = 0x1410c89, addr = 0x0
Debug: drivers/net/phy/phy_device.c: Creating PHY-> phy_id = 0x1410c89,
addr = 0x1
Creating PHY-> AUTONEG_ENABLE for phy_id = 0x1410c89, addr = 0x1
libphy: dsa slave smi: probed
net lan1: PHY already attached
DSA: slave.c: mii_bus-> name = dsa slave smi , id =dsa-0:1f
net lan2: PHY already attached
DSA: slave.c: mii_bus-> name = dsa slave smi , id =dsa-0:1f
My eth0 fixed-link address is also 0 and eth1 PHY address is 1.
Removing lan2 is not improving the situation.
Even if I do not set up eth1. Then same issue is happening between lan1
and lan2. Whichever is configured first ( using ifconfig) is able to
transmit packet (Tx increments). And rest all ports ( lan2) are just
receiving (their Tx counters remain 0). Even if I ping to lan2 ip,
response will be received only when ethernet cable is plugged to lan1 (
because it will only be able transmit the response), It does not matter
if cable is plugged to lan2 port.
^ permalink raw reply
* Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member
From: Jay Vosburgh @ 2015-01-21 7:14 UTC (permalink / raw)
To: Jonathan Toppins
Cc: netdev, Scott Feldman, Andy Gospodarek, Veaceslav Falico,
Nikolay Aleksandrov
In-Reply-To: <54BF3786.9050505@cumulusnetworks.com>
Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>On 1/19/15 4:16 PM, Jay Vosburgh wrote:
>> Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>>
>>> From: Scott Feldman <sfeldma@cumulusnetworks.com>
>>>
>>> Bonding driver parameter min_links is now used to signal upper-level
>>> protocols of bond status. The way it works is if the total number of
>>> active members in slaves drops below min_links, the bond link carrier
>>> will go down, signaling upper levels that bond is inactive. When active
>>> members returns to >= min_links, bond link carrier will go up (RUNNING),
>>> and protocols can resume. When bond is carrier down, member ports are
>>> in stp fwd state blocked (rather than normal disabled state), so
>>> low-level ctrl protocols (LACP) can still get in and be processed by
>>> bonding driver.
>>
>> Presuming that "stp" is Spanning Tree, is the last sentence
>> above actually describing the behavior of a bridge port when a bond is
>> the member of the bridge? I'm not sure I understand what "member ports"
>> refers to (bridge ports or bonding slaves).
>
>Ack, maybe replacing the last sentence with something like:
> When bond is carrier down, the slave ports are only forwarding
> low-level control protocols (e.g. LACP PDU) and discarding all other
> packets.
Ah, are you actually referring to the fact that slaves that are
up will still deliver packets to listeners that bind directly to the
slave or hook in through a rx_handler? This is, in part, the
"RX_HANDLER_EXACT" business in bond_handle_frame and
__netif_receive_skb_core.
The decision for that has nothing to do with the protocol; I
seem to recall that FCoE (or maybe it's iSCSI) does its regular traffic
reception this way (although via dev_add_pack, not an rx_handler) so it
can run traffic regardless of the bonding master's state.
>>> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>>> ret = 0;
>>> goto out;
>>> }
>>> +
>>> + bond_for_each_slave_rcu(bond, slave, iter)
>>> + if (SLAVE_AD_INFO(slave)->aggregator.is_active)
>>> + active_slaves++;
>>> +
>>> active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>>> - if (active) {
>>> + if (active && __agg_has_partner(active)) {
>>
>> Why "__agg_has_partner"? Since the "else" of this clause is:
>>
>> } else if (netif_carrier_ok(bond->dev)) {
>> netif_carrier_off(bond->dev);
>> }
>>
>> I'm wondering if this will do the right thing for the case that
>> there are no LACP partners at all (e.g., the switch ports do not have
>> LACP enabled), in which case the active aggregator should be a single
>> "individual" port as a fallback, but will not have a partner.
>>
>> -J
>>
>
>I see your point. The initial thinking was the logical bond carrier should
>not be brought up until the bond has a partner and is ready to pass
>traffic, otherwise we start blackholing frames. Looking over the code it
>seems the aggregator.is_individual flag is only set to true when a slave
>is in half-duplex, this seems odd?
The agg.is_individual flag and an "individual" aggregator are
subtly different things.
The is_individual flag is part of the implementation of the
standard requirement that half duplex ports are not allowed to enable
LACP (and thus cannot aggregate, and end up as "Individual" in
standard-ese). The standard capitalizes "Individual" when it describes
the cannot-aggregate property of a port (note that half duplex is only
one reason of many for a port being Individual).
An "individual" aggregator (my usage of 802.1AX 5.3.6 (b)) is an
aggregator containing exactly one port that is Individual. A port can
end up as Individual (for purposes of this discussion) either through
the is_individual business, or because the bonding port does run LACP,
but the link partner does not, and thus no LACPDUs are ever received.
For either of the above cases (is_individual or no-LACP-parter),
then the active aggregator will be an "individual" aggregator, but will
not have a parter (__agg_has_partner() will be false). The standard has
a bunch of verbiage about this in 802.1AX 5.3.5 - 5.3.9.
>My initial thinking to alleviate the concern is something like the
>following:
>
>if (active && !SLAVE_AD_INFO(slave)->aggregator.is_individual &&
> __agg_has_partner(active)) {
> /* set carrier based on min_links */
>} else if (active && SLAVE_AD_INFO(slave)->aggregator.is_individual) {
> /* set bond carrier state according to carrier state of slave */
>} else if (netif_carrier_ok(bond->dev)) {
> netif_carrier_off(bond->dev);
>}
I'm not sure you need to care about is_individual or
__agg_has_partner at all. If either of those conditions is true for the
active aggregator, it will contain exactly one port, and so if min_links
is 2, you'll have carrier off, and if min_links is 1 or less you'll have
carrier on.
If I'm reading the patch right, the real point (which isn't
really described very well in the change log) is that you're changing
the carrier decision to count only active ports in the active
aggregator, not the total number of ports as is currently done.
I'm not sure why this change is needed:
@@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
ret = 0;
goto out;
}
+
+ bond_for_each_slave_rcu(bond, slave, iter)
+ if (SLAVE_AD_INFO(slave)->aggregator.is_active)
+ active_slaves++;
+
active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
- if (active) {
+ if (active && __agg_has_partner(active)) {
/* are enough slaves available to consider link up? */
- if (active->num_of_ports < bond->params.min_links) {
+ if (active_slaves < bond->params.min_links) {
if (netif_carrier_ok(bond->dev)) {
netif_carrier_off(bond->dev);
goto out;
because a port (slave) that loses carrier or whose link partner
becomes unresponsive to LACPDUs will be removed from the aggregator. As
I recall, there are no "inactive" ports in an aggregator; all of them
have to match in terms of capabilities.
In other words, I'm unsure of when the count of is_active ports
will not match active->num_of_ports.
Also, the other parts of the patch add some extra updates to the
carrier state when a port is enabled or disabled, e.g.,
@@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
static inline void __disable_port(struct port *port)
{
bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER);
+ bond_3ad_set_carrier(port->slave->bond);
}
Again, I'm not sure why this is necessary, as the cases that
disable or enable a port will eventually call bond_3ad_set_carrier. For
example, ad_agg_selection_logic will, when changing active aggregator,
individually disable all ports of the old active and then may
individually enable ports of the new active if necessary, and then
finally call bond_3ad_set_carrier.
In what situations is the patch's behavior an improvement (i.e.,
is there a situation I'm missing that doesn't do it right)?
The last portion of the patch:
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond,
netdev_info(bond->dev, "Setting min links value to %llu\n",
newval->value);
bond->params.min_links = newval->value;
+ bond_set_carrier(bond);
return 0;
}
does seem to fix a legitimate bug, in that when min_links is
changed, it does not take effect in real time.
>Maybe I am missing something and there is a simpler option.
>
>Thinking about how to validate this, it seems having a bond with two
>slaves and both slaves in half-duplex will force an aggregator that is
>individual to be selected.
>
>Thoughts?
That's one way, yes. You'll also get an "individual" aggregator
if none of the link partners enable LACP.
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply
* Re
From: Mr Rainer Ritzdorf @ 2015-01-21 7:53 UTC (permalink / raw)
To: Recipients
Compliments, I have a safe, viable and verifiable business proposal for you. Please reply for a detailed information.
---
This email has been checked for viruses by Avast antivirus software.
http://www.avast.com
^ permalink raw reply
* Re: [PATCH 2/3] netlink: Mark dumps as inconsistent which have been interrupted by a resize
From: Ying Xue @ 2015-01-21 8:13 UTC (permalink / raw)
To: Thomas Graf, davem, kaber, herbert, paulmck; +Cc: netdev, netfilter-devel
In-Reply-To: <cce004895ab6bcef267e2133a1c7ecab0fac403b.1421759056.git.tgraf@suug.ch>
On 01/20/2015 09:20 PM, Thomas Graf wrote:
> A deferred resize of nl_table causes the offsets that Netlink diag keeps
> to become inaccurate. Mark the dump as inconsistent and have user space
> request a new dump.
>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
> net/netlink/af_netlink.c | 10 ++++++++++
> net/netlink/af_netlink.h | 1 +
> net/netlink/diag.c | 1 +
> 3 files changed, 12 insertions(+)
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 7a94185..e214557 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -91,6 +91,9 @@ static inline int netlink_is_kernel(struct sock *sk)
> struct netlink_table *nl_table;
> EXPORT_SYMBOL_GPL(nl_table);
>
> +atomic_t nl_table_seq;
It sounds like the atomic variable is not initialized.
Regards,
Ying
> +EXPORT_SYMBOL_GPL(nl_table_seq);
> +
> static DECLARE_WAIT_QUEUE_HEAD(nl_table_wait);
>
> static int netlink_dump(struct sock *sk);
> @@ -3071,6 +3074,12 @@ static const struct net_proto_family netlink_family_ops = {
> .owner = THIS_MODULE, /* for consistency 8) */
> };
>
> +static void nl_table_resize_notify(const struct rhashtable *ht,
> + enum rht_resize_op op)
> +{
> + atomic_inc(&nl_table_seq);
> +}
> +
> static int __net_init netlink_net_init(struct net *net)
> {
> #ifdef CONFIG_PROC_FS
> @@ -3124,6 +3133,7 @@ static int __init netlink_proto_init(void)
> .max_shift = 16, /* 64K */
> .grow_decision = rht_grow_above_75,
> .shrink_decision = rht_shrink_below_30,
> + .resize_notify = nl_table_resize_notify,
> };
>
> if (err != 0)
> diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
> index 7518375..51c23c0 100644
> --- a/net/netlink/af_netlink.h
> +++ b/net/netlink/af_netlink.h
> @@ -74,5 +74,6 @@ struct netlink_table {
>
> extern struct netlink_table *nl_table;
> extern rwlock_t nl_table_lock;
> +extern atomic_t nl_table_seq;
>
> #endif
> diff --git a/net/netlink/diag.c b/net/netlink/diag.c
> index 3ee63a3..50aa385 100644
> --- a/net/netlink/diag.c
> +++ b/net/netlink/diag.c
> @@ -112,6 +112,7 @@ static int __netlink_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
> int ret = 0, num = 0, i;
>
> req = nlmsg_data(cb->nlh);
> + cb->seq = atomic_read(&nl_table_seq);
>
> for (i = 0; i < htbl->size; i++) {
> struct rhash_head *pos;
>
^ permalink raw reply
* Re: [PATCH v2 0/4] net: stmmac: dwmac-rk: Fix phy regulator issues
From: Heiko Stübner @ 2015-01-21 8:32 UTC (permalink / raw)
To: Romain Perier, David Miller
Cc: peppe.cavallaro-qxv4g6HH51o, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
roger.chen-TNX95d0MmH7DzftRWevZcw
In-Reply-To: <1421737780-1533-1-git-send-email-romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Am Dienstag, 20. Januar 2015, 07:09:36 schrieb Romain Perier:
> This series fixes few issues in dwmac-rk:
>
> 1. Voltage settings was hardcoded into the driver for the phy regulator.
> The driver now uses the default voltage settings found in the devicetree,
> which are applied throught the regulator framework.
> 2. The regulator name used to power on or power off the phy was put in the
> devicetree variable "phy_regulator", which is not standard and added a lot
> of code for nothing. The driver now uses the devicetree property
> "phy-supply" and the corresponding functions to manipulate this regulator.
>
> The corresponding devicetree files are also updated. As this new binding for
> rk3288 has not been released with any official kernel yet (not until 3.20),
> I don't need to care about keeping compatibility with the old non standard
> property.
This series
Tested-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
Reviewed-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
@Dave: as said in the v1 mail, this would be nice to have for 3.20
--
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 net-next] rhashtable: rhashtable_remove() must unlink in both tbl and future_tbl
From: Ying Xue @ 2015-01-21 8:39 UTC (permalink / raw)
To: Thomas Graf, davem; +Cc: tipc-discussion, Netdev
In-Reply-To: <20150120165826.GK20315@casper.infradead.org>
On 01/21/2015 12:58 AM, Thomas Graf wrote:
> As removals can occur during resizes, entries may be referred to from
> both tbl and future_tbl when the removal is requested. Therefore
> rhashtable_remove() must unlink the entry in both tables if this is
> the case. The existing code did search both tables but stopped when it
> hit the first match.
>
> Failing to do so resulted in use after remove.
>
> Fixes: 97defe1 ("rhashtable: Per bucket locks & deferred expansion/shrinking")
> Reported-by: Ying Xue <ying.xue@windriver.com>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>
> Ying: This should fix the panic that was at the end of your TIPC
> related boot log. I'm still working on the use after free.
>
Your below changes are reasonable for me. But after I applied the patch,
my reported two issues still happened :( And failure logs are completely
same before.
Regards,
Ying
> lib/rhashtable.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/lib/rhashtable.c b/lib/rhashtable.c
> index a4449c4..b1aa10e 100644
> --- a/lib/rhashtable.c
> +++ b/lib/rhashtable.c
> @@ -592,6 +592,7 @@ bool rhashtable_remove(struct rhashtable *ht, struct rhash_head *obj)
> struct rhash_head *he;
> spinlock_t *lock;
> unsigned int hash;
> + bool ret = false;
>
> rcu_read_lock();
> tbl = rht_dereference_rcu(ht->tbl, ht);
> @@ -609,17 +610,16 @@ restart:
> }
>
> rcu_assign_pointer(*pprev, obj->next);
> - atomic_dec(&ht->nelems);
> -
> - spin_unlock_bh(lock);
> -
> - rhashtable_wakeup_worker(ht);
> -
> - rcu_read_unlock();
>
> - return true;
> + ret = true;
> + break;
> }
>
> + /* The entry may be linked in either 'tbl', 'future_tbl', or both.
> + * 'future_tbl' only exists for a short period of time during
> + * resizing. Thus traversing both is fine and the added cost is
> + * very rare.
> + */
> if (tbl != rht_dereference_rcu(ht->future_tbl, ht)) {
> spin_unlock_bh(lock);
>
> @@ -632,9 +632,15 @@ restart:
> }
>
> spin_unlock_bh(lock);
> +
> + if (ret) {
> + atomic_dec(&ht->nelems);
> + rhashtable_wakeup_worker(ht);
> + }
> +
> rcu_read_unlock();
>
> - return false;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(rhashtable_remove);
>
>
------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
^ permalink raw reply
* Re: [PATCH net-next] rhashtable: rhashtable_remove() must unlink in both tbl and future_tbl
From: Ying Xue @ 2015-01-21 8:51 UTC (permalink / raw)
To: Thomas Graf, richard.alpe@ericsson.com >> Richard Alpe
Cc: Netdev, tipc-discussion, davem
In-Reply-To: <20150120165826.GK20315@casper.infradead.org>
Hi Thomas,
I will be on vacation in next few days. So if you have an updated patch
resolving the issue, please let Richard(richard.alpe@ericsson.com) help
you to verify because he initially found the issue. After I identified
what it closely was related to rhashtable and it also happened in
netlink, I posted it on netdev mail list.
Regards,
Ying
On 01/21/2015 12:58 AM, Thomas Graf wrote:
> As removals can occur during resizes, entries may be referred to from
> both tbl and future_tbl when the removal is requested. Therefore
> rhashtable_remove() must unlink the entry in both tables if this is
> the case. The existing code did search both tables but stopped when it
> hit the first match.
>
> Failing to do so resulted in use after remove.
>
> Fixes: 97defe1 ("rhashtable: Per bucket locks & deferred expansion/shrinking")
> Reported-by: Ying Xue <ying.xue@windriver.com>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>
> Ying: This should fix the panic that was at the end of your TIPC
> related boot log. I'm still working on the use after free.
>
> lib/rhashtable.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/lib/rhashtable.c b/lib/rhashtable.c
> index a4449c4..b1aa10e 100644
> --- a/lib/rhashtable.c
> +++ b/lib/rhashtable.c
> @@ -592,6 +592,7 @@ bool rhashtable_remove(struct rhashtable *ht, struct rhash_head *obj)
> struct rhash_head *he;
> spinlock_t *lock;
> unsigned int hash;
> + bool ret = false;
>
> rcu_read_lock();
> tbl = rht_dereference_rcu(ht->tbl, ht);
> @@ -609,17 +610,16 @@ restart:
> }
>
> rcu_assign_pointer(*pprev, obj->next);
> - atomic_dec(&ht->nelems);
> -
> - spin_unlock_bh(lock);
> -
> - rhashtable_wakeup_worker(ht);
> -
> - rcu_read_unlock();
>
> - return true;
> + ret = true;
> + break;
> }
>
> + /* The entry may be linked in either 'tbl', 'future_tbl', or both.
> + * 'future_tbl' only exists for a short period of time during
> + * resizing. Thus traversing both is fine and the added cost is
> + * very rare.
> + */
> if (tbl != rht_dereference_rcu(ht->future_tbl, ht)) {
> spin_unlock_bh(lock);
>
> @@ -632,9 +632,15 @@ restart:
> }
>
> spin_unlock_bh(lock);
> +
> + if (ret) {
> + atomic_dec(&ht->nelems);
> + rhashtable_wakeup_worker(ht);
> + }
> +
> rcu_read_unlock();
>
> - return false;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(rhashtable_remove);
>
>
------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
^ permalink raw reply
* Re: [PATCH iproute2 3/3] ss: Unify tcp stats output
From: Hagen Paul Pfeifer @ 2015-01-21 8:54 UTC (permalink / raw)
To: Cong Wang; +Cc: Vadim Kochan, netdev, Stephen Hemminger, Eric Dumazet
In-Reply-To: <CAHA+R7PgJytCaeVz8aOyJ06JjKACMCrtp8oAnfuaSDSNJX8NvQ@mail.gmail.com>
20 January 2015 at 19:36, Cong Wang <cwang@twopensource.com> wrote:
> On the other hand, what blocks you from parsing the netlink message
> by yourself? Don't get me wrong, I am not a fan of netlink, but
> generally speaking, ss is not alone, too many scripts and applications
> nowadays parse iproute2 tools output.
Agree, thus the proposal:
1) *new* ss values are "prefixed" by an name to make it possible that
*humans* can understand the value ("make ss output human readable").
Concrete rule for scalar value: key:value. Concrete rule for list of
values: subsystem:(key1:val1,key1:val2). The actual ss output is
nearly formatted matching these rules. E.g. see socket memory for list
formated output. Sure, there are values like SACK that do not need any
prefix and can assumed to be known.
2) add JSON format for scripts and tools.
Hagen
^ permalink raw reply
* Re: [PATCH v2 0/7] Fix sti drivers whcih mix reg address spaces
From: Kishon Vijay Abraham I @ 2015-01-21 8:54 UTC (permalink / raw)
To: Peter Griffin, linux-arm-kernel, linux-kernel,
srinivas.kandagatla, maxime.coquelin, patrice.chotard,
peppe.cavallaro, arnd
Cc: lee.jones, devicetree, netdev
In-Reply-To: <1420643052-4506-1-git-send-email-peter.griffin@linaro.org>
Hi,
On Wednesday 07 January 2015 08:34 PM, Peter Griffin wrote:
> Hi folks,
>
> A V2 of this old series incorporating Arnd and Lees Feedback form v1.
>
> Following on from Arnds comments about the picophy driver here
> https://lkml.org/lkml/2014/11/13/161, this series fixes the
> remaining upstreamed drivers for STI, which are mixing address spaces
> in the reg property. We do this in a way similar to the keystone
> and bcm7445 platforms, by having sysconfig phandle/ offset pair
> (where only one register is required). Or phandle / integer array
> where multiple offsets in the same bank are needed).
>
> This series breaks DT compatability! But the platform support
> is WIP and only being used by the few developers who are upstreaming
> support for it. I've made each change to the driver / dt doc / dt
> file as a single atomic commit so the kernel will remain bisectable.
>
> This series then also enables the picophy driver, and adds back in
> the ehci/ohci dt nodes for stih410 which make use of the picophy.
>
> regards,
>
> Peter.
>
> Changes since v1:
> - Add missing space after */ (Lee)
> - Change comment to "indexed from" rather than "not indexed from" (Lee)
> - Change naming to phy1, phy2 rather than phy@1 phy@2 if there is no reg property (Arnd / Grant)
> - Rebased on v3.19-rc3 (me)
> - Checkpatch "no space before tabs" warning in stih41*.dtsi (me)
>
> Peter Griffin (7):
> phy: phy-stih407-usb: Pass sysconfig register offsets via syscfg
> property.
> phy: miphy365x: Pass sysconfig register offsets via syscfg dt
> property.
> ARM: STi: DT: STiH407: Add usb2 picophy dt nodes
> ARM: STi: DT: STiH410: Add usb2 picophy dt nodes
> ARM: STi: DT: STiH410: Add DT nodes for the ehci and ohci usb
> controllers.
> ARM: multi_v7_defconfig: Enable stih407 usb picophy
> stmmac: dwmac-sti: Pass sysconfig register offset via syscon dt
> property.
>
> .../devicetree/bindings/net/sti-dwmac.txt | 14 ++---
> .../devicetree/bindings/phy/phy-miphy365x.txt | 15 ++---
> .../devicetree/bindings/phy/phy-stih407-usb.txt | 10 +---
> arch/arm/boot/dts/stih407-family.dtsi | 9 +++
> arch/arm/boot/dts/stih410.dtsi | 70 ++++++++++++++++++++++
> arch/arm/boot/dts/stih415.dtsi | 12 ++--
> arch/arm/boot/dts/stih416.dtsi | 22 +++----
> arch/arm/configs/multi_v7_defconfig | 1 +
> drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c | 13 ++--
> drivers/phy/phy-miphy365x.c | 29 ++++-----
> drivers/phy/phy-stih407-usb.c | 25 ++++----
> 11 files changed, 143 insertions(+), 77 deletions(-)
>
Queued the first two patches of this series in linux-phy.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH net] ipv4: try to cache dst_entries which would cause a redirect
From: Julian Anastasov @ 2015-01-21 8:56 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: netdev, Marcelo Leitner, Florian Westphal
In-Reply-To: <16ab8e83e2c0719bd449f8e10f82637820624712.1421789827.git.hannes@stressinduktion.org>
Hello,
On Tue, 20 Jan 2015, Hannes Frederic Sowa wrote:
> Not caching dst_entries which cause redirects could be exploited by hosts
> on the same subnet, causing a severe DoS attack. This effect aggravated
> since commit f88649721268999 ("ipv4: fix dst race in sk_dst_get()").
>
> Lookups causing redirects will be allocated with DST_NOCACHE set which
> will force dst_release to free them via RCU. Unfortunately waiting for
> RCU grace period just takes too long, we can end up with >1M dst_entries
> waiting to be released and the system will run OOM. rcuos threads cannot
> catch up under high softirq load.
>
> Attaching the flag to emit a redirect later on to the specific skb allows
> us to cache those dst_entries thus reducing the pressure on allocation
> and deallocation.
>
> This issue was discovered by Marcelo Leitner.
Change looks good to me but additional place
should be changed too: inet_rtm_getroute() will call
ip_route_input() and later rt_fill_info() will put
rt_flags in rtm_flags. We have to set RTCF_DOREDIRECT
just in rtm_flags depending on IPSKB_DOREDIRECT becuase
iproute needs to print "redirect". You can test it with
ip route get ... iif INDEV
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH net-next 0/3] openvswitch: Add STT support.
From: Pravin Shelar @ 2015-01-21 9:08 UTC (permalink / raw)
To: Tom Herbert; +Cc: David Miller, Linux Netdev List
In-Reply-To: <CA+mtBx90TipY9uJvW2uQfrCYL9JNM9Rk75HzdJPhE0qsdy8dnw@mail.gmail.com>
On Tue, Jan 20, 2015 at 3:06 PM, Tom Herbert <therbert@google.com> wrote:
> On Tue, Jan 20, 2015 at 12:25 PM, Pravin B Shelar <pshelar@nicira.com> wrote:
>> Following patch series adds support for Stateless Transport
>> Tunneling protocol.
>> STT uses TCP segmentation offload available in most of NIC. On
>> packet xmit STT driver appends STT header along with TCP header
>> to the packet. For GSO packet GSO parameters are set according
>> to tunnel configuration and packet is handed over to networking
>> stack. This allows use of segmentation offload available in NICs
>>
>> Netperf unidirectional test gives ~9.4 Gbits/s performance on 10Gbit
>> NIC with 1500 byte MTU with two TCP streams.
>>
> Having packets marked TCP which really aren't TCP is a rather scary
> prospect to deploy in a real data center (TCP is kind of an important
> protocol ;-) ). Can you give some more motivation on this, more data
> that shows what the benefits are and how this compares to equivalent
> encapsulation protocols that implement GRO and GSO.
>
There are multi-year deployments of STT, So it is already in real data center.
Biggest advantage is STT does not need new NIC with tunnel offload.
Any NIC that supports TCP offload can be used to achieve better
performance.
Following are numbers you asked for.
Setup: net-next branch on server and client.
netperf: TCP unidirectional tests with 5 streams. Numbers are averaged
over 3 runs of 50 sec.
VXLAN:
CPU
Client: 1.6
Server: 14.2
Throughput: 5.6 Gbit/s
VXLAN with rcsum:
CPU
Client: 0.89
Server: 12.4
Throughput: 5.8 Gbit/s
STT:
CPU
Client: 1.28
Server: 4.0
Throughput: 9.5 Gbit/s
Thanks,
Pravin.
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Herbert Xu @ 2015-01-21 9:14 UTC (permalink / raw)
To: Thomas Graf
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121051555.GA23300@gondor.apana.org.au>
On Wed, Jan 21, 2015 at 04:15:55PM +1100, Herbert Xu wrote:
> On Wed, Jan 21, 2015 at 04:08:19PM +1100, Herbert Xu wrote:
> >
> > OK I think I have a solution for you guys. But first you'll need to
> > wait for me to undo the nulls stuff so I can steal that bit which
> > is central to my solution.
> >
> > Essentially I need a bit to indicate an entry in the bucket chain
> > should be skipped, either because it has just been removed or that
> > it is a walker entry (see xfrm_state_walk).
> >
> > The way it'll work then is exactly the same as xfrm_state_walk,
> > except that the linked list is broken up into individual buckets.
> >
> > Of course we'll still need to postpone resizes (and rehashes which
> > is what my work is about) during a walk but I think that's a fair
> > price to pay.
>
> I failed to address the security aspect of this approach. Obviously
> this can only work if the only entites doing the walk are trusted.
> Which means that the vast majority (if not all) hash table users
> would be excluded, in particular, netlink.
OK maybe we can get around this. So we will postpone the resize
or rehash when a walk is in place, however, when we hit a hard
limit, i.e., when insert would otherwise fail, then we do the
rehash regardless of any outstanding walks. The walks will just
behave erratically or we could force them to start from scratch
again.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [patch] bridge: simplify br_getlink() a bit
From: Dan Carpenter @ 2015-01-21 9:22 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, bridge, kernel-janitors, David S. Miller
Static checkers complain that we should maybe set "ret" before we do the
"goto out;". They interpret the NULL return from br_port_get_rtnl() as
a failure and forgetting to set the error code is a common bug in this
situation.
The code is confusing but it's actually correct. We are returning zero
deliberately. Let's re-write it a bit to be more clear.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 528cf27..3875ea51 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -311,17 +311,14 @@ errout:
int br_getlink(struct sk_buff *skb, u32 pid, u32 seq,
struct net_device *dev, u32 filter_mask)
{
- int err = 0;
struct net_bridge_port *port = br_port_get_rtnl(dev);
if (!port && !(filter_mask & RTEXT_FILTER_BRVLAN) &&
!(filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED))
- goto out;
+ return 0;
- err = br_fill_ifinfo(skb, port, pid, seq, RTM_NEWLINK, NLM_F_MULTI,
- filter_mask, dev);
-out:
- return err;
+ return br_fill_ifinfo(skb, port, pid, seq, RTM_NEWLINK, NLM_F_MULTI,
+ filter_mask, dev);
}
static int br_vlan_info(struct net_bridge *br, struct net_bridge_port *p,
^ permalink raw reply related
* Re: [PATCH 1/2] if_link: Add VF multicast promiscuous mode control
From: Bjørn Mork @ 2015-01-21 9:26 UTC (permalink / raw)
To: Hiroshi Shimamoto
Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
Choi, Sy Jong, linux-kernel@vger.kernel.org, Hayato Momma
In-Reply-To: <7F861DC0615E0C47A872E6F3C5FCDDBD05E07B7C@BPXM14GP.gisp.nec.co.jp>
Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> writes:
>> Why can't the ixgbevf driver just automatically signal the ixgbe driver
>> to enable multicast promiscuous mode whenever the list grows past the
>> limit?
>
> I had submitted a patch to change ixgbe and ixgbevf driver for this issue.
> https://lkml.org/lkml/2014/11/27/269
>
> The previous patch introduces API between ixgbe and ixgbevf driver to
> enable multicast promiscuous mode, and ixgbevf enables it automatically
> if the number of addresses is over than 30.
>
> I got some comment and I would like to clarify the point, but there was no
> answer.
> That's the reason I submitted this patch.
Thanks. Yes, now I understand why you want to have a policy knob.
I still think the policy could select between "automatic"/"disallowed"
instead of "enabled"/"disabled", but that's a minor detail. Likewise is
the actual implemention of "automatic". I think you could do that
within the current VF-PF protocol by overloading the MC address "count".
But a more generic question for netdev is: Does this VF policy API
really scale?
How many different VF policy tunables can you imaging add up over a few
years and drivers. Currently each policy flag require its own ndo hook.
I probably don't have much to say here, but IMHO this scheme had already
failed when .ndo_set_vf_spoofchk was added..
Bjørn
------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Thomas Graf @ 2015-01-21 9:37 UTC (permalink / raw)
To: Herbert Xu
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121050819.GA23062@gondor.apana.org.au>
On 01/21/15 at 04:08pm, Herbert Xu wrote:
> On Tue, Jan 20, 2015 at 03:35:56PM +0000, Thomas Graf wrote:
> > On 01/20/15 at 03:21pm, Patrick McHardy wrote:
> > > I think its preferrable to make the need to handle NETLINK_F_DUMP_INTR
> > > as noticable as possible and not hide it. Silent failure is the worst
> > > kind of failure.
> >
> > I agree to that. The point here is to avoid unnecessary use of
> > NETLINK_F_DUMP_INTR if all entries fit into a single message buffer.
>
> OK I think I have a solution for you guys. But first you'll need to
> wait for me to undo the nulls stuff so I can steal that bit which
> is central to my solution.
Without having seen your code, can we make it configurable on what
the bit is used for? Use of nulls marker is a strict requirement for
some targeted users of rhashtable.
> Essentially I need a bit to indicate an entry in the bucket chain
> should be skipped, either because it has just been removed or that
> it is a walker entry (see xfrm_state_walk).
>
> The way it'll work then is exactly the same as xfrm_state_walk,
> except that the linked list is broken up into individual buckets.
>
> Of course we'll still need to postpone resizes (and rehashes which
> is what my work is about) during a walk but I think that's a fair
> price to pay.
If I understand this correctly we also need to block out parallel
walkers and we need to start taking bucket locks while walking to
modify the walker mark bit in peace.
> This also means handling insertion failures but I think that
> should be acceptable if we make it based on a configurable maximum
> chain length along with forced resize/rehash where possible.
>
> Note that this can be made optional, i.e., if the user can afford
> memory to do their own walking (e.g., xfrm_state), then none of
> this needs to happen and it'll just work as it does now. IOW if
> you don't use this special rhashtable walk function then you're
> not affected.
That sounds like the best option to me.
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Herbert Xu @ 2015-01-21 9:38 UTC (permalink / raw)
To: Thomas Graf
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121093722.GM20315@casper.infradead.org>
On Wed, Jan 21, 2015 at 09:37:22AM +0000, Thomas Graf wrote:
>
> Without having seen your code, can we make it configurable on what
> the bit is used for? Use of nulls marker is a strict requirement for
> some targeted users of rhashtable.
What do they need this for?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Thomas Graf @ 2015-01-21 9:49 UTC (permalink / raw)
To: Herbert Xu
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121093836.GA25489@gondor.apana.org.au>
On 01/21/15 at 08:38pm, Herbert Xu wrote:
> On Wed, Jan 21, 2015 at 09:37:22AM +0000, Thomas Graf wrote:
> >
> > Without having seen your code, can we make it configurable on what
> > the bit is used for? Use of nulls marker is a strict requirement for
> > some targeted users of rhashtable.
>
> What do they need this for?
An entry can move between different tables and thus chains need to be
marked to identify what list a lookup ended up searching in. It's not
the nulls marker itself that is needed, it's the bits in the last next
pointer identifying the list that the nulls marker allows to be used
which are essential.
This is on my plate next. Most of the work in rhashtable was done in
preparation of translating the TCP established table over to
rhashtable.
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Thomas Graf @ 2015-01-21 9:56 UTC (permalink / raw)
To: Herbert Xu
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121091457.GA25289@gondor.apana.org.au>
On 01/21/15 at 08:14pm, Herbert Xu wrote:
> On Wed, Jan 21, 2015 at 04:15:55PM +1100, Herbert Xu wrote:
> > I failed to address the security aspect of this approach. Obviously
> > this can only work if the only entites doing the walk are trusted.
> > Which means that the vast majority (if not all) hash table users
> > would be excluded, in particular, netlink.
>
> OK maybe we can get around this. So we will postpone the resize
> or rehash when a walk is in place, however, when we hit a hard
> limit, i.e., when insert would otherwise fail, then we do the
> rehash regardless of any outstanding walks. The walks will just
> behave erratically or we could force them to start from scratch
> again.
Exactly. I think we also need a timer to abort walks because if
only a single walker is allowed, an attacker could start a walk and
not complete it to block out everybody else.
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Herbert Xu @ 2015-01-21 9:58 UTC (permalink / raw)
To: Thomas Graf
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121094928.GN20315@casper.infradead.org>
On Wed, Jan 21, 2015 at 09:49:28AM +0000, Thomas Graf wrote:
>
> An entry can move between different tables and thus chains need to be
> marked to identify what list a lookup ended up searching in. It's not
> the nulls marker itself that is needed, it's the bits in the last next
> pointer identifying the list that the nulls marker allows to be used
> which are essential.
Can you describe in more detail how it's going to be used? I don't
see how I could use the bit if you need it to indicate the end of
the list.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Herbert Xu @ 2015-01-21 9:59 UTC (permalink / raw)
To: Thomas Graf
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121095634.GO20315@casper.infradead.org>
On Wed, Jan 21, 2015 at 09:56:34AM +0000, Thomas Graf wrote:
>
> Exactly. I think we also need a timer to abort walks because if
> only a single walker is allowed, an attacker could start a walk and
> not complete it to block out everybody else.
My scheme should support an arbitrary number of walks. See how
xfrm_state_walk is implemented.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Patrick McHardy @ 2015-01-21 10:00 UTC (permalink / raw)
To: Thomas Graf; +Cc: Herbert Xu, davem, paulmck, ying.xue, netdev, netfilter-devel
In-Reply-To: <20150121095634.GO20315@casper.infradead.org>
On 21.01, Thomas Graf wrote:
> On 01/21/15 at 08:14pm, Herbert Xu wrote:
> > On Wed, Jan 21, 2015 at 04:15:55PM +1100, Herbert Xu wrote:
> > > I failed to address the security aspect of this approach. Obviously
> > > this can only work if the only entites doing the walk are trusted.
> > > Which means that the vast majority (if not all) hash table users
> > > would be excluded, in particular, netlink.
> >
> > OK maybe we can get around this. So we will postpone the resize
> > or rehash when a walk is in place, however, when we hit a hard
> > limit, i.e., when insert would otherwise fail, then we do the
> > rehash regardless of any outstanding walks. The walks will just
> > behave erratically or we could force them to start from scratch
> > again.
>
> Exactly. I think we also need a timer to abort walks because if
> only a single walker is allowed, an attacker could start a walk and
> not complete it to block out everybody else.
Restarting them automatically sounds reasonable, duplicate entries have
always been possible. That makes me think, we could have userspace
indicate support for NLM_F_DUMP_INTR and otherwise always restart.
That would solve the problem very easily.
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Thomas Graf @ 2015-01-21 10:23 UTC (permalink / raw)
To: Herbert Xu
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121095837.GA25750@gondor.apana.org.au>
On 01/21/15 at 08:58pm, Herbert Xu wrote:
> On Wed, Jan 21, 2015 at 09:49:28AM +0000, Thomas Graf wrote:
> >
> > An entry can move between different tables and thus chains need to be
> > marked to identify what list a lookup ended up searching in. It's not
> > the nulls marker itself that is needed, it's the bits in the last next
> > pointer identifying the list that the nulls marker allows to be used
> > which are essential.
>
> Can you describe in more detail how it's going to be used? I don't
> see how I could use the bit if you need it to indicate the end of
> the list.
The usage will be identical to how __inet_lookup_listener() uses it.
If at the end of the lookup, we ended up in a different table than
we started, the lookup is restarted as an entry has moved to another
table while we were moving over it.
^ permalink raw reply
* Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
From: Andri Yngvason @ 2015-01-21 10:33 UTC (permalink / raw)
To: Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger, Marc Kleine-Budde
Cc: Linux-CAN, netdev, LKML
In-Reply-To: <20150120214537.GB16828@linux>
Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>
> Replace most of the can interface's state and error counters
> handling with the new can-dev can_change_state() mechanism.
>
> Suggested-by: Andri Yngvason <andri.yngvason@marel.com>
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> ---
> drivers/net/can/usb/kvaser_usb.c | 114 +++++++++++++++++++--------------------
> 1 file changed, 55 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index 971c5f9..0386d3f 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -620,40 +620,43 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
> }
>
> static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
> - const struct kvaser_usb_error_summary *es)
> + const struct kvaser_usb_error_summary *es,
> + struct can_frame *cf)
> {
> struct net_device_stats *stats;
> - enum can_state new_state;
> -
> - stats = &priv->netdev->stats;
> - new_state = priv->can.state;
> + enum can_state cur_state, new_state, tx_state, rx_state;
>
> netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
>
> - if (es->status & M16C_STATE_BUS_OFF) {
> - priv->can.can_stats.bus_off++;
> + stats = &priv->netdev->stats;
> + new_state = cur_state = priv->can.state;
> +
> + if (es->status & M16C_STATE_BUS_OFF)
> new_state = CAN_STATE_BUS_OFF;
> - } else if (es->status & M16C_STATE_BUS_PASSIVE) {
> - if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
> - priv->can.can_stats.error_passive++;
> + else if (es->status & M16C_STATE_BUS_PASSIVE)
> new_state = CAN_STATE_ERROR_PASSIVE;
> - }
>
> if (es->status == M16C_STATE_BUS_ERROR) {
> - if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
> - ((es->txerr >= 96) || (es->rxerr >= 96))) {
> - priv->can.can_stats.error_warning++;
> + if ((cur_state < CAN_STATE_ERROR_WARNING) &&
> + ((es->txerr >= 96) || (es->rxerr >= 96)))
> new_state = CAN_STATE_ERROR_WARNING;
> - } else if (priv->can.state > CAN_STATE_ERROR_ACTIVE) {
> + else if (cur_state > CAN_STATE_ERROR_ACTIVE)
> new_state = CAN_STATE_ERROR_ACTIVE;
> - }
> }
>
> if (!es->status)
> new_state = CAN_STATE_ERROR_ACTIVE;
>
> + if (new_state != cur_state) {
> + tx_state = (es->txerr >= es->rxerr) ? new_state : 0;
> + rx_state = (es->txerr <= es->rxerr) ? new_state : 0;
> +
> + can_change_state(priv->netdev, cf, tx_state, rx_state);
> + new_state = priv->can.state;
> + }
> +
> if (priv->can.restart_ms &&
> - (priv->can.state >= CAN_STATE_BUS_OFF) &&
> + (cur_state >= CAN_STATE_BUS_OFF) &&
> (new_state < CAN_STATE_BUS_OFF)) {
> priv->can.can_stats.restarts++;
> }
> @@ -665,18 +668,17 @@ static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *pri
>
> priv->bec.txerr = es->txerr;
> priv->bec.rxerr = es->rxerr;
> - priv->can.state = new_state;
> }
>
> static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> const struct kvaser_msg *msg)
> {
> - struct can_frame *cf;
> + struct can_frame *cf, tmp_cf = { .can_id = CAN_ERR_FLAG, .can_dlc = CAN_ERR_DLC };
> struct sk_buff *skb;
> struct net_device_stats *stats;
> struct kvaser_usb_net_priv *priv;
> struct kvaser_usb_error_summary es = { };
> - enum can_state old_state;
> + enum can_state old_state, new_state;
>
> switch (msg->id) {
> case CMD_CAN_ERROR_EVENT:
> @@ -721,60 +723,54 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> }
>
> /* Update all of the can interface's state and error counters before
> - * trying any skb allocation that can actually fail with -ENOMEM.
> + * trying any memory allocation that can actually fail with -ENOMEM.
> + *
> + * We send a temporary stack-allocated error can frame to
> + * can_change_state() for the very same reason.
> + *
> + * TODO: Split can_change_state() responsibility between updating the
> + * can interface's state and counters, and the setting up of can error
> + * frame ID and data to userspace. Remove stack allocation afterwards.
> */
> old_state = priv->can.state;
> - kvaser_usb_rx_error_update_can_state(priv, &es);
> + kvaser_usb_rx_error_update_can_state(priv, &es, &tmp_cf);
> + new_state = priv->can.state;
>
> skb = alloc_can_err_skb(priv->netdev, &cf);
> if (!skb) {
> stats->rx_dropped++;
> return;
> }
> + memcpy(cf, &tmp_cf, sizeof(*cf));
>
> - if (es.status & M16C_STATE_BUS_OFF) {
> - cf->can_id |= CAN_ERR_BUSOFF;
> -
> - if (!priv->can.restart_ms)
> - kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
> - netif_carrier_off(priv->netdev);
> - } else if (es.status & M16C_STATE_BUS_PASSIVE) {
> - if (old_state != CAN_STATE_ERROR_PASSIVE) {
> - cf->can_id |= CAN_ERR_CRTL;
> -
> - if (es.txerr || es.rxerr)
> - cf->data[1] = (es.txerr > es.rxerr)
> - ? CAN_ERR_CRTL_TX_PASSIVE
> - : CAN_ERR_CRTL_RX_PASSIVE;
> - else
> - cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE |
> - CAN_ERR_CRTL_RX_PASSIVE;
> + if (new_state != old_state) {
> + if (es.status & M16C_STATE_BUS_OFF) {
> + if (!priv->can.restart_ms)
> + kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
> + netif_carrier_off(priv->netdev);
> + }
> +
> + if (es.status == M16C_STATE_BUS_ERROR) {
> + if ((old_state >= CAN_STATE_ERROR_WARNING) ||
> + (es.txerr < 96 && es.rxerr < 96)) {
> + if (old_state > CAN_STATE_ERROR_ACTIVE) {
> + cf->can_id |= CAN_ERR_PROT;
> + cf->data[2] = CAN_ERR_PROT_ACTIVE;
> + }
> + }
> }
> - }
>
> - if (es.status == M16C_STATE_BUS_ERROR) {
> - if ((old_state < CAN_STATE_ERROR_WARNING) &&
> - ((es.txerr >= 96) || (es.rxerr >= 96))) {
> - cf->can_id |= CAN_ERR_CRTL;
> - cf->data[1] = (es.txerr > es.rxerr)
> - ? CAN_ERR_CRTL_TX_WARNING
> - : CAN_ERR_CRTL_RX_WARNING;
> - } else if (old_state > CAN_STATE_ERROR_ACTIVE) {
> + if (!es.status) {
> cf->can_id |= CAN_ERR_PROT;
> cf->data[2] = CAN_ERR_PROT_ACTIVE;
> }
> - }
>
> - if (!es.status) {
> - cf->can_id |= CAN_ERR_PROT;
> - cf->data[2] = CAN_ERR_PROT_ACTIVE;
> - }
> -
> - if (priv->can.restart_ms &&
> - (old_state >= CAN_STATE_BUS_OFF) &&
> - (priv->can.state < CAN_STATE_BUS_OFF)) {
> - cf->can_id |= CAN_ERR_RESTARTED;
> - netif_carrier_on(priv->netdev);
> + if (priv->can.restart_ms &&
> + (old_state >= CAN_STATE_BUS_OFF) &&
> + (new_state < CAN_STATE_BUS_OFF)) {
> + cf->can_id |= CAN_ERR_RESTARTED;
> + netif_carrier_on(priv->netdev);
> + }
> }
>
> if (es.error_factor) {
> --
> 1.9.1
Looks good.
--
Andri
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Thomas Graf @ 2015-01-21 10:34 UTC (permalink / raw)
To: Herbert Xu
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121095837.GA25750@gondor.apana.org.au>
On 01/21/15 at 08:58pm, Herbert Xu wrote:
> On Wed, Jan 21, 2015 at 09:49:28AM +0000, Thomas Graf wrote:
> >
> > An entry can move between different tables and thus chains need to be
> > marked to identify what list a lookup ended up searching in. It's not
> > the nulls marker itself that is needed, it's the bits in the last next
> > pointer identifying the list that the nulls marker allows to be used
> > which are essential.
>
> Can you describe in more detail how it's going to be used? I don't
> see how I could use the bit if you need it to indicate the end of
> the list.
Another thought: Instead of storing that bit in the next pointer, we
could require the user to store this bit, i.e. two new function
pointers to rhashtable_params, set_walk_bit() and get_walk_bit(),
which take the hashed object as argument and if a rhashtable user
requires consistent dumps he can provide these functions to store
the flag.
^ permalink raw reply
* RE: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: David Laight @ 2015-01-21 10:36 UTC (permalink / raw)
To: 'Thomas Graf', Herbert Xu
Cc: Patrick McHardy, davem@davemloft.net, paulmck@linux.vnet.ibm.com,
ying.xue@windriver.com, netdev@vger.kernel.org,
netfilter-devel@vger.kernel.org
In-Reply-To: <20150121093722.GM20315@casper.infradead.org>
From: Thomas Graf
> On 01/21/15 at 04:08pm, Herbert Xu wrote:
...
> > Essentially I need a bit to indicate an entry in the bucket chain
> > should be skipped, either because it has just been removed or that
> > it is a walker entry (see xfrm_state_walk).
> >
> > The way it'll work then is exactly the same as xfrm_state_walk,
> > except that the linked list is broken up into individual buckets.
> >
> > Of course we'll still need to postpone resizes (and rehashes which
> > is what my work is about) during a walk but I think that's a fair
> > price to pay.
>
> If I understand this correctly we also need to block out parallel
> walkers and we need to start taking bucket locks while walking to
> modify the walker mark bit in peace.
Running 'walker | grep foo | less' really shouldn't stop any activity,
including further requests by the same person to dump the same table in a
different window.
I would also expect the above request to be able to continue correctly
when restarted hours later (unless something unusual - like a resize -
happens).
Suppressing 'table shrink' because you think a walker might be active
if probably a good idea, and just requires a timestamp of the last
walk action.
Thought...
It is possible to break walking only on hash chain boundaries?
At least in the case where the response buffer is large enough
for a full 'chain' of entries.
Also what happens if there is a page fault on any userpage?
You don't want to be holding a mutex.
David
^ 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