* Re: [net-next] dsa: slave: support phy devices on external MII bus
From: Martin Hundebøll @ 2017-10-18 17:30 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn; +Cc: netdev
In-Reply-To: <ad992ed8-37b8-9c52-848c-78fa0368de8f@gmail.com>
On 2017-10-18 18:51, Florian Fainelli wrote:
> On 10/18/2017 09:21 AM, Andrew Lunn wrote:
>> Hi Martin
>>
>> Sorry for starting a new thread. I deleted the patchset from my mailbox.
>>
>> Florian said:
>>
>>> The logic goes like this:
>>>
>>> - try to connect to the PHY via phy-handle
>>> - if we have a PHY we are connecting via phy-handle but we need to
>>> divert MDIO reads/writes connect using its address on the diverted
>>> bus
>>> - connect using a fixed PHY
>>> - finally try using the DSA slave MII bus which would connect to the switch internal PHYs
>>
>> This is not quite correct. Looking at the code:
>>
>> phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
>> ...
>>
>> if (phy_dn) {
>> int phy_id = of_mdio_parse_addr(&slave_dev->dev, phy_dn);
>>
>> /* If this PHY address is part of phys_mii_mask, which means
>> * that we need to divert reads and writes to/from it, then we
>> * want to bind this device using the slave MII bus created by
>> * DSA to make that happen.
>> */
>> if (!phy_is_fixed && phy_id >= 0 &&
>> (ds->phys_mii_mask & (1 << phy_id))) {
>> ret = dsa_slave_phy_connect(p, slave_dev, phy_id);
>> if (ret) {
>> netdev_err(slave_dev, "failed to connect to phy%d: %d\n", phy_id, ret);
>> of_node_put(phy_dn);
>> return ret;
>> }
>> } else {
>> p->phy = of_phy_connect(slave_dev, phy_dn,
>>
>> The first point really is:
>>
>> - try to connect to the PHY via phy-handle, if the phy_id is not
>> valid, or if the phy_id does not map to a phy that the switch says
>> does not exist.
>>
>> In your case, all these points are true, so it uses
>> dsa_slave_phy_connect(). But we actually want it to use
>> of_phy_connect(), which will use the correct bus.
>>
>> For some Marvell chips, you cannot actually go on ds->phys_mii_mask.
>> These devices can have in built PHYs and SERDES interfaces which can
>> be assigned to ports. These SERDES interfaces could have external PHYs
>> connected to them, and be on the external MDIO bus. So
>> ds->phys_mii_mask indicates there is a PHY, but the phy-handle points
>> to a different phy.
>>
>> So i think this code block needs to change. If we have a phy-handle,
>> use it. i.e. what Florian _thinks_ it should be doing. If not, then
>> use dsa_slave_phy_connect().
>
> I see what you mean now, the logic above gets defeated because it does
> not concern itself with the MDIO controller parent of the PHY node
> pointed to by phy-handle. So if like Martin you have two MDIO busses,
> but both happen to have MDIO addresses that are valid for both busses,
> the logic above gets defeated and we wrongly try to attach to the switch
> internal MDIO bus under ds->slave_mii_bus.
>
> The easiest fix would certainly to lookup the parent MDIO bus and do
> that only if ds->slave_mii_bus->of_node and the parent of the node
> pointed to 'phy-handle' match.
>
> Does that work for you?
>
As Andrew implies, I think we should rewrite the entire block to make it
more intuitive.
Are these the cases that should be handled?
0) Fixed link
Register using of_phy_register_fixed_link().
1) No phy-handle
Use dsa_slave_phy_connect() to connect on internal MDIO bus with phy
address from index/port-reg property.
2) Valid phy handle
Use of_phy_connect() to connect using parent MDIO bus handle.
I am most certainly missing some corner cases here, so please educate me!
// Martin
^ permalink raw reply
* Re: [PATCH 1/2] liquidio: remove unnecessary NULL check before kfree in delete_glists
From: Felix Manlunas @ 2017-10-18 17:47 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Derek Chickles, Satanand Burla, Felix Manlunas, Raghu Vatsavayi,
netdev, linux-kernel
In-Reply-To: <20171017185920.GA16976@embeddedor.com>
On Tue, Oct 17, 2017 at 01:59:20PM -0500, Gustavo A. R. Silva wrote:
> NULL check before freeing functions like kfree is not needed.
>
> This issue was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
> This code was tested by compilation only.
>
> drivers/net/ethernet/cavium/liquidio/lio_vf_main.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
> index 2e993ce..e4a112c 100644
> --- a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
> +++ b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
> @@ -435,8 +435,7 @@ static void delete_glists(struct lio *lio)
> do {
> g = (struct octnic_gather *)
> list_delete_head(&lio->glist[i]);
> - if (g)
> - kfree(g);
> + kfree(g);
> } while (g);
>
> if (lio->glists_virt_base && lio->glists_virt_base[i] &&
> --
> 2.7.4
>
Acked-by: Felix Manlunas <felix.manlunas@cavium.com>
^ permalink raw reply
* Re: [PATCH 2/2] liquidio: mark expected switch fall-through in octeon_destroy_resources
From: Felix Manlunas @ 2017-10-18 17:53 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Derek Chickles, Satanand Burla, Felix Manlunas, Raghu Vatsavayi,
netdev, linux-kernel
In-Reply-To: <20171017190145.GA17015@embeddedor.com>
On Tue, Oct 17, 2017 at 02:01:45PM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
> This code was tested by compilation only (GCC 7.2.0 was used).
> Please, verify if the actual intention of the code is to fall through.
>
> drivers/net/ethernet/cavium/liquidio/lio_vf_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
> index e4a112c..4c3b568 100644
> --- a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
> +++ b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
> @@ -747,7 +747,7 @@ static void octeon_destroy_resources(struct octeon_device *oct)
>
> if (lio_wait_for_oq_pkts(oct))
> dev_err(&oct->pci_dev->dev, "OQ had pending packets\n");
> -
> + /* fall through */
> case OCT_DEV_INTR_SET_DONE:
> /* Disable interrupts */
> oct->fn_list.disable_interrupt(oct, OCTEON_ALL_INTR);
> --
> 2.7.4
>
Acked-by: Felix Manlunas <felix.manlunas@cavium.com>
^ permalink raw reply
* RE: [PATCH v1 RFC 1/1] Add Microchip KSZ8795 DSA driver
From: Tristram.Ha @ 2017-10-18 17:55 UTC (permalink / raw)
To: pavel
Cc: andrew, f.fainelli, ruediger.schmitt, muvarov,
nathan.leigh.conrad, vivien.didelot, UNGLinuxDriver, netdev,
linux-kernel
In-Reply-To: <20171011205916.GC31288@amd>
> > +static void ksz8795_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> > + u64 *cnt)
> > +{
> > + u32 data;
> > + u16 ctrl_addr;
> > + u8 check;
> > + int loop;
> > +
> > + ctrl_addr = addr + SWITCH_COUNTER_NUM * port;
> > + ctrl_addr |= IND_ACC_TABLE(TABLE_MIB | TABLE_READ);
> > +
> > + mutex_lock(&dev->alu_mutex);
> > + ksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);
> > +
> > + /* It is almost guaranteed to always read the valid bit because of
> > + * slow SPI speed.
> > + */
> > + for (loop = 2; loop > 0; loop--) {
> > + ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> > +
> > + if (check & MIB_COUNTER_VALID) {
> > + ksz_read32(dev, REG_IND_DATA_LO, &data);
> > + if (check & MIB_COUNTER_OVERFLOW)
> > + *cnt += MIB_COUNTER_VALUE + 1;
> > + *cnt += data & MIB_COUNTER_VALUE;
> > + break;
> > + }
> > + }
>
> Hmm. Maybe, but should not this at least warn if if it can not get
> valid counter?
>
The checking of valid bit is implemented because of the chip datasheet.
But in my experience it never happens. A warning will be added, although I
do not see any benefit of it. It this warning ever comes up it just means
somehow the SPI access is completely broken down.
> > + /* It is almost guaranteed to always read the valid bit because of
> > + * slow SPI speed.
> > + */
> > + for (loop = 2; loop > 0; loop--) {
> > + ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> > +
> > + if (check & MIB_COUNTER_VALID) {
> > + ksz_read32(dev, REG_IND_DATA_LO, &data);
> > + if (addr < 2) {
> > + u64 total;
> > +
> > + total = check & MIB_TOTAL_BYTES_H;
> > + total <<= 32;
> > + *cnt += total;
> > + *cnt += data;
> > + if (check & MIB_COUNTER_OVERFLOW) {
> > + total = MIB_TOTAL_BYTES_H + 1;
> > + total <<= 32;
> > + *cnt += total;
> > + }
> > + } else {
> > + if (check & MIB_COUNTER_OVERFLOW)
> > + *cnt += MIB_PACKET_DROPPED + 1;
> > + *cnt += data & MIB_PACKET_DROPPED;
> > + }
> > + break;
> > + }
> > + }
>
> Same here. Plus, is overflow handling correct? There may be more than
> MIB_PACKET_DROPPED + 1 packets dropped between the checks.
>
MIB_PACKET_DROPPED is the maximum count like 0xffff. Plus 1 means 0x10000.
It is assumed the checking should be done fast enough to avoid overflow. This is
just assuming one overflow has been missed. To tell you the truth this code is
never checked for correctness.
> > +static void ksz8795_r_table(struct ksz_device *dev, int table, u16 addr,
> > + u64 *data)
> > +{
> > + u16 ctrl_addr;
> > +
> > + ctrl_addr = IND_ACC_TABLE(table | TABLE_READ) | addr;
> > +
> > + mutex_lock(&dev->alu_mutex);
> > + ksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);
> > + ksz_get(dev, REG_IND_DATA_HI, data, sizeof(u64));
> > + mutex_unlock(&dev->alu_mutex);
> > + *data = be64_to_cpu(*data);
> > +}
>
> It would be a tiny bit nicer to have be64 temporary variable and use
> it; having *data change endianness at runtime is "interesting".
> > + /* At least one valid entry in the table. */
> > + } else {
> > + u64 buf;
> > + int cnt;
> > +
> > + ksz_get(dev, REG_IND_DATA_HI, &buf, sizeof(buf));
> > + buf = be64_to_cpu(buf);
>
> Would it make sense to convert endianness inside ksz_get?
The ksz_get function utilizes the chip's capability to automatically
increase the register index so that their values can be retrieved in
one SPI transfer. It is mostly used for debugging for dumping chip
registers or programming MAC address (6 bytes). Here it is also used for
reading 8 bytes. If you do not want to see the conversion then another
access functions like ksz_read64 and ksz_write64 will need to be implemented.
^ permalink raw reply
* Re: [PATCH net-next 3/3] ipv6: obsolete cached dst when removing them from fib tree
From: Wei Wang @ 2017-10-18 17:56 UTC (permalink / raw)
To: Paolo Abeni
Cc: Eric Dumazet, Martin KaFai Lau, Xin Long,
Linux Kernel Network Developers, David S. Miller,
Hannes Frederic Sowa
In-Reply-To: <1508331782.2559.5.camel@redhat.com>
On Wed, Oct 18, 2017 at 6:03 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> On Tue, 2017-10-17 at 13:48 -0700, Wei Wang wrote:
>> On Tue, Oct 17, 2017 at 1:02 PM, Paolo Abeni <pabeni@redhat.com> wrote:
>> > Meanwhile others sockets may grab more references to (and use) the same
>> > aged-out dst.
>> >
>>
>> I don't think other sockets could grab more reference to this dst
>> because this dst should already be removed from the fib6 tree.
>
> With the current net-next code, the dst is not removed from the fib
> tree while someone else is holding it and dst_check() does not fail
> after that the cached dst is aged out. If a socket cache grab a
> reference to the CACHE dst, it will not release it untill the next
> sernum change, regardless of the dst aging.
>
>> > The commit 1e2ea8ad37be ("ipv6: set dst.obsolete when a cached route
>> > has expired") was the solution to the above issue prior to the recent
>> > refactor.
>> >
>>
>> I don't really understand how this commit is solving the above issue.
>> This commit still only ages out cached route if &rt->dst.__refcnt ==
>> 1. So if socket is holding refcnt to this dst and dst_check() is not
>> getting called, this cached route still won't get deleted.
>
> Setting obsolete to DST_OBSOLETE_KILL forced whoever was holding the
> dst reference to drop it on the next dst_check(), so that refcnt could
> go down.
>
Yes. Understood.
Martin and I had a discussion yesterday. We both think it is not a
good idea to set obolete to DST_OBSOLETE_KILL but not to remove it
from the fib6 tree.
It is because others who do a route lookup later might potentially
find this route and tries to use this route. However, dst_check() will
show this route is invalid. So the user will redo the route lookup.
But as this route is not yet deleted from the tree, it will find this
route again. This seems like a bad situation.
And again, setting obsolete to DST_OBSOLETE_KILL does not prevent some
idle socket holding on to this dst for a long time...
With the above said, I am now convinced what you have in your patch is
the correct thing to do. Just remove the cached route without checking
the refcnt when it is aged.
> Cheers,
>
> Paolo
^ permalink raw reply
* Re: [PATCH] lib/dynamic_queue_limits.c: relax BUG_ON to WARN_ON in dql_complete()
From: Ard Biesheuvel @ 2017-10-18 17:57 UTC (permalink / raw)
To: Eric Dumazet
Cc: linux-kernel@vger.kernel.org, <netdev@vger.kernel.org>,
David S. Miller
In-Reply-To: <1508344188.31614.124.camel@edumazet-glaptop3.roam.corp.google.com>
On 18 October 2017 at 17:29, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2017-10-18 at 16:45 +0100, Ard Biesheuvel wrote:
>> Even though calling dql_completed() with a count that exceeds the
>> queued count is a serious error, it still does not justify bringing
>> down the entire kernel with a BUG_ON(). So relax it to a WARN_ON()
>> instead.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> lib/dynamic_queue_limits.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c
>> index f346715e2255..24ce495d78f3 100644
>> --- a/lib/dynamic_queue_limits.c
>> +++ b/lib/dynamic_queue_limits.c
>> @@ -23,7 +23,7 @@ void dql_completed(struct dql *dql, unsigned int count)
>> num_queued = ACCESS_ONCE(dql->num_queued);
>>
>> /* Can't complete more than what's in queue */
>> - BUG_ON(count > num_queued - dql->num_completed);
>> + WARN_ON(count > num_queued - dql->num_completed);
>>
>> completed = dql->num_completed + count;
>> limit = dql->limit;
>
> So instead fixing the faulty driver, you'll have strange lockups, and
> force your users to reboot anyway, after annoying periods where
> "Internet does not work"
>
> These kinds of errors should be found when testing a new device driver
> or new kernel.
>
> Have you found the root cause ?
>
Not yet, and I don't intend to send out any patches for this
particular hardware until this is fixed.
But that still doesn't mean you should crash hard. As Linus puts it,
it is better to 'limp on' if you can (unless we're likely to corrupt
any non-volatile data, e.g., files on disk etc)
^ permalink raw reply
* [iproute PATCH] ss: Distinguish between IPv4 and IPv6 wildcard sockets
From: Phil Sutter @ 2017-10-18 17:58 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Humberto Alves, Eric Dumazet
In-Reply-To: <20171016194958.669625db@xeon-e3>
Commit aba9c23a6e1cb ("ss: enclose IPv6 address in brackets") unified
display of wildcard sockets in IPv4 and IPv6 to print the unspecified
address as '*'. Users then complained that they can't distinguish
between address families anymore, so change this again to what Stephen
Hemminger suggested:
| *:80 << both IPV6 and IPV4
| [::]:80 << IPV6_ONLY
| 0.0.0.0:80 << IPV4_ONLY
Note that on older kernels which don't support INET_DIAG_SKV6ONLY
attribute, pure IPv6 sockets will still show as '*'.
Cc: Humberto Alves <hjalves@live.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
misc/ss.c | 36 +++++++++++++++++-------------------
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/misc/ss.c b/misc/ss.c
index 09bff8a7e2d28..e37aba6022eb4 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -1041,7 +1041,8 @@ do_numeric:
return buf;
}
-static void inet_addr_print(const inet_prefix *a, int port, unsigned int ifindex)
+static void inet_addr_print(const inet_prefix *a, int port,
+ unsigned int ifindex, bool v6only)
{
char buf[1024];
const char *ap = buf;
@@ -1049,14 +1050,10 @@ static void inet_addr_print(const inet_prefix *a, int port, unsigned int ifindex
const char *ifname = NULL;
if (a->family == AF_INET) {
- if (a->data[0] == 0) {
- buf[0] = '*';
- buf[1] = 0;
- } else {
- ap = format_host(AF_INET, 4, a->data);
- }
+ ap = format_host(AF_INET, 4, a->data);
} else {
- if (!memcmp(a->data, &in6addr_any, sizeof(in6addr_any))) {
+ if (!v6only &&
+ !memcmp(a->data, &in6addr_any, sizeof(in6addr_any))) {
buf[0] = '*';
buf[1] = 0;
} else {
@@ -1728,12 +1725,12 @@ static void proc_ctx_print(struct sockstat *s)
}
}
-static void inet_stats_print(struct sockstat *s)
+static void inet_stats_print(struct sockstat *s, bool v6only)
{
sock_state_print(s);
- inet_addr_print(&s->local, s->lport, s->iface);
- inet_addr_print(&s->remote, s->rport, 0);
+ inet_addr_print(&s->local, s->lport, s->iface, v6only);
+ inet_addr_print(&s->remote, s->rport, 0, v6only);
proc_ctx_print(s);
}
@@ -2065,7 +2062,7 @@ static int tcp_show_line(char *line, const struct filter *f, int family)
s.rto = s.rto != 3 * hz ? s.rto / hz : 0;
s.ss.type = IPPROTO_TCP;
- inet_stats_print(&s.ss);
+ inet_stats_print(&s.ss, false);
if (show_options)
tcp_timer_print(&s);
@@ -2411,6 +2408,7 @@ static int inet_show_sock(struct nlmsghdr *nlh,
{
struct rtattr *tb[INET_DIAG_MAX+1];
struct inet_diag_msg *r = NLMSG_DATA(nlh);
+ unsigned char v6only = 0;
parse_rtattr(tb, INET_DIAG_MAX, (struct rtattr *)(r+1),
nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
@@ -2418,7 +2416,10 @@ static int inet_show_sock(struct nlmsghdr *nlh,
if (tb[INET_DIAG_PROTOCOL])
s->type = rta_getattr_u8(tb[INET_DIAG_PROTOCOL]);
- inet_stats_print(s);
+ if (s->local.family == AF_INET6 && tb[INET_DIAG_SKV6ONLY])
+ v6only = rta_getattr_u8(tb[INET_DIAG_SKV6ONLY]);
+
+ inet_stats_print(s, v6only);
if (show_options) {
struct tcpstat t = {};
@@ -2434,12 +2435,9 @@ static int inet_show_sock(struct nlmsghdr *nlh,
if (show_details) {
sock_details_print(s);
- if (s->local.family == AF_INET6 && tb[INET_DIAG_SKV6ONLY]) {
- unsigned char v6only;
-
- v6only = rta_getattr_u8(tb[INET_DIAG_SKV6ONLY]);
+ if (s->local.family == AF_INET6 && tb[INET_DIAG_SKV6ONLY])
printf(" v6only:%u", v6only);
- }
+
if (tb[INET_DIAG_SHUTDOWN]) {
unsigned char mask;
@@ -2910,7 +2908,7 @@ static int dgram_show_line(char *line, const struct filter *f, int family)
opt[0] = 0;
s.type = dg_proto == UDP_PROTO ? IPPROTO_UDP : 0;
- inet_stats_print(&s);
+ inet_stats_print(&s, false);
if (show_details && opt[0])
printf(" opt:\"%s\"", opt);
--
2.13.1
^ permalink raw reply related
* RE: [PATCH v1 RFC 7/7] Modify tag_ksz.c so that tail tag code can be used by other KSZ switch drivers
From: Tristram.Ha @ 2017-10-18 18:02 UTC (permalink / raw)
To: pavel
Cc: andrew, f.fainelli, ruediger.schmitt, muvarov,
nathan.leigh.conrad, vivien.didelot, UNGLinuxDriver, netdev,
linux-kernel
In-Reply-To: <20171011204528.GA31288@amd>
> > +#define KSZ9477_INGRESS_TAG_LEN 2
> > +#define KSZ9477_PTP_TAG_LEN 4
> > +#define KSZ9477_PTP_TAG_INDICATION 0x80
> > +
> > +#define KSZ9477_TAIL_TAG_OVERRIDE BIT(9)
> > +#define KSZ9477_TAIL_TAG_LOOKUP BIT(10)
> > +
> > +static int ksz9477_get_tag(u8 *tag, int *port)
> > +{
> > + int len = KSZ_EGRESS_TAG_LEN;
> > +
> > + /* Extra 4-bytes PTP timestamp */
> > + if (tag[0] & KSZ9477_PTP_TAG_INDICATION)
> > + len += KSZ9477_PTP_TAG_LEN;
> > + *port = tag[0] & 7;
> > + return len;
> > +}
> > +
> > +static void ksz9477_set_tag(void *ptr, u8 *addr, int p)
> > +{
> > + u16 *tag = (u16 *)ptr;
> > +
> > + *tag = 1 << p;
> > + if (!memcmp(addr, special_mult_addr, ETH_ALEN))
> > + *tag |= KSZ9477_TAIL_TAG_OVERRIDE;
> > + *tag = cpu_to_be16(*tag);
> > +}
>
> These are new features that were not there before, right?
Although it is the correct procedure, they will be removed until
this tail tag code has access to the main switch driver.
^ permalink raw reply
* [iproute PATCH] ss: Detect IPPROTO_ICMPV6 sockets
From: Phil Sutter @ 2017-10-18 18:08 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
Prefix IPPROTO_ICMPV6 sockets with 'icmp6' instead of '???'.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
misc/ss.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/misc/ss.c b/misc/ss.c
index e37aba6022eb4..b5c6bbc05766e 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -784,6 +784,8 @@ static const char *proto_name(int protocol)
return "sctp";
case IPPROTO_DCCP:
return "dccp";
+ case IPPROTO_ICMPV6:
+ return "icmp6";
}
return "???";
--
2.13.1
^ permalink raw reply related
* Re: [PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt routine
From: Troy Kisky @ 2017-10-18 18:10 UTC (permalink / raw)
To: Andy Duan, shawn.guo@linaro.org, netdev@vger.kernel.org,
davem@davemloft.net
Cc: Fabio Estevam, lznuaa@gmail.com, andrew@lunn.ch
In-Reply-To: <AM4PR0401MB226072C7D7775F16C533316FFF4D0@AM4PR0401MB2260.eurprd04.prod.outlook.com>
On 10/17/2017 7:30 PM, Andy Duan wrote:
> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Wednesday, October 18, 2017 5:34 AM
>>>> This is better for code locality and should slightly speed up normal
>> interrupts.
>>>>
>>>> This also allows PPS clock output to start working for i.mx7. This is
>>>> because
>>>> i.mx7 was already using the limit of 3 interrupts, and needed another.
>>>>
>>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>>>
>>>> ---
>>>>
>>>> v2: made this change independent of any devicetree change so that old
>>>> dtbs continue to work.
>>>>
>>>> Continue to register ptp clock if interrupt is not found.
>>>> ---
>>>> drivers/net/ethernet/freescale/fec.h | 3 +-
>>>> drivers/net/ethernet/freescale/fec_main.c | 25 ++++++----
>>>> drivers/net/ethernet/freescale/fec_ptp.c | 82
>>>> ++++++++++++++++++--------
>>>> -----
>>>> 3 files changed, 65 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/freescale/fec.h
>>>> b/drivers/net/ethernet/freescale/fec.h
>>>> index ede1876a9a19..be56ac1f1ac4 100644
>>>> --- a/drivers/net/ethernet/freescale/fec.h
>>>> +++ b/drivers/net/ethernet/freescale/fec.h
>>>> @@ -582,12 +582,11 @@ struct fec_enet_private {
>>>> u64 ethtool_stats[0];
>>>> };
>>>>
>>>> -void fec_ptp_init(struct platform_device *pdev);
>>>> +void fec_ptp_init(struct platform_device *pdev, int irq_index);
>>>> void fec_ptp_stop(struct platform_device *pdev); void
>>>> fec_ptp_start_cyclecounter(struct net_device *ndev); int
>>>> fec_ptp_set(struct net_device *ndev, struct ifreq *ifr); int
>>>> fec_ptp_get(struct net_device *ndev, struct ifreq *ifr); -uint
>>>> fec_ptp_check_pps_event(struct fec_enet_private *fep);
>>>>
>>>>
>>>>
>> /**********************************************************
>>>> ******************/
>>>> #endif /* FEC_H */
>>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>>> b/drivers/net/ethernet/freescale/fec_main.c
>>>> index 3dc2d771a222..21afabbc560f 100644
>>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>>> @@ -1602,10 +1602,6 @@ fec_enet_interrupt(int irq, void *dev_id)
>>>> ret = IRQ_HANDLED;
>>>> complete(&fep->mdio_done);
>>>> }
>>>> -
>>>> - if (fep->ptp_clock)
>>>> - if (fec_ptp_check_pps_event(fep))
>>>> - ret = IRQ_HANDLED;
>>>> return ret;
>>>> }
>>>>
>>>> @@ -3325,6 +3321,8 @@ fec_probe(struct platform_device *pdev)
>>>> struct device_node *np = pdev->dev.of_node, *phy_node;
>>>> int num_tx_qs;
>>>> int num_rx_qs;
>>>> + char irq_name[8];
>>>> + int irq_cnt;
>>>>
>>>> fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
>>>>
>>>> @@ -3465,18 +3463,27 @@ fec_probe(struct platform_device *pdev)
>>>> if (ret)
>>>> goto failed_reset;
>>>>
>>>> + irq_cnt = platform_irq_count(pdev);
>>>> + if (irq_cnt > FEC_IRQ_NUM)
>>>> + irq_cnt = FEC_IRQ_NUM; /* last for ptp */
>>>> + else if (irq_cnt == 2)
>>>> + irq_cnt = 1; /* last for ptp */
>>>> + else if (irq_cnt <= 0)
>>>> + irq_cnt = 1; /* Let the for loop fail */
>>>
>>> Don't do like this. Don't suppose pps interrupt is the last one.
>>
>>
>> I don't. If the pps interrupt is named, the named interrupt will be used. If it is
>> NOT named, the last interrupt is used, if 2 interrupts, or >3 interrupt are
>> provided.
>> Otherwise, no pps interrupt is assumed.
>> Fortunately this seems to be true currently.
>>
> If pps interrupt is not named, then it limit the last one is pps.
> We cannot get the pps interrupt based on current chip interrupt define, we never know the future chip how to define interrupt.
> Although your current implementation can work with current chips, but it is not really good solution.
>
>>
>>> And if irq_cnt is 1 like imx28/imx5x, the patch will break fec interrupt
>> function.
>>
>> How ? fec_ptp_init will not be called as bufdesc_ex is 0.
>>
> Imx28 also support enhanced buffer descriptor, if define the ptp clock in dts then bufdesc_ex also can be 1.
Only if FEC_QUIRK_HAS_BUFDESC_EX is set, which it is not. Here's the relevant code snippets
}, {
.name = "imx25-fec",
.driver_data = FEC_QUIRK_USE_GASKET | FEC_QUIRK_MIB_CLEAR,
}, {
.name = "imx27-fec",
.driver_data = FEC_QUIRK_MIB_CLEAR,
}, {
.name = "imx28-fec",
.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_SWAP_FRAME |
FEC_QUIRK_SINGLE_MDIO | FEC_QUIRK_HAS_RACC,
}, {
....
fep->bufdesc_ex = fep->quirks & FEC_QUIRK_HAS_BUFDESC_EX;
fep->clk_ptp = devm_clk_get(&pdev->dev, "ptp");
if (IS_ERR(fep->clk_ptp)) {
fep->clk_ptp = NULL;
fep->bufdesc_ex = false;
}
______________
You could make your way work though, if you remove the pps clock from imx50.dtsi, imx51.dtsi, and
imx53.dtsi.
Then you would not need the FEC_QUIRK_HAS_BUFDESC_EX quirk.
This is all a little off topic though.
>
> I still suggest to use v1 logic check pps interrupt that need to check irq name.
I could add that back. But it seems redundant when also looking up by name "int0", "int1", "int2".
I would think that we could recommend in the binding documentation that either all are
named, or none are. And I don't think we need to make work mistakes like
interrupt-names = "irq0", "ptp", "int1", "int2"
but
interrupt-names = "int0", "ptp", "int1", "int2"
should currently work.
Let me know if you want me to make work
interrupt-names = "", "ptp", "", ""
or if you just don't think this patch is necessary. I might have some more
reasons to try and persuade you.
>
>>
>> Also, if only 1 interrupt is provided, it is assumed there is no unnamed pps
>> interrupt.
>>
>>
>>>
>>> I suggest to use .platform_get_irq_byname() to get pps(ptp) interrupt like
>> your v1 logic check.
>>>
>>>> +
>>>> if (fep->bufdesc_ex)
>>>> - fec_ptp_init(pdev);
>>>> + fec_ptp_init(pdev, irq_cnt);
>>>>
>>>> ret = fec_enet_init(ndev);
>>>> if (ret)
>>>> goto failed_init;
>>>>
>>>> - for (i = 0; i < FEC_IRQ_NUM; i++) {
>>>> - irq = platform_get_irq(pdev, i);
>>>> + for (i = 0; i < irq_cnt; i++) {
>>>> + sprintf(irq_name, "int%d", i);
>>>> + irq = platform_get_irq_byname(pdev, irq_name);
>>>> + if (irq < 0)
>>>> + irq = platform_get_irq(pdev, i);
>>>> if (irq < 0) {
>>>> - if (i)
>>>> - break;
>>>> ret = irq;
>>>> goto failed_irq;
>>>> }
Thanks for the review
Troy
^ permalink raw reply
* [PATCH net-next] tcp: socket option to set TCP fast open key
From: Yuchung Cheng @ 2017-10-18 18:22 UTC (permalink / raw)
To: davem; +Cc: netdev, cpaasch, edumazet, ncardwell, maheshb, Yuchung Cheng
New socket option TCP_FASTOPEN_KEY to allow different keys per
listener. The listener by default uses the global key until the
socket option is set. The key is a 16 bytes long binary data. This
option has no effect on regular non-listener TCP sockets.
Signed-off-by: Yuchung Cheng <ycheng@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
include/net/request_sock.h | 2 ++
include/net/tcp.h | 5 +++--
include/uapi/linux/tcp.h | 1 +
net/ipv4/sysctl_net_ipv4.c | 3 ++-
net/ipv4/tcp.c | 33 +++++++++++++++++++++++++++
net/ipv4/tcp_fastopen.c | 56 +++++++++++++++++++++++++++++++++-------------
net/ipv4/tcp_ipv4.c | 1 +
7 files changed, 82 insertions(+), 19 deletions(-)
diff --git a/include/net/request_sock.h b/include/net/request_sock.h
index 23e22054aa60..347015515a7d 100644
--- a/include/net/request_sock.h
+++ b/include/net/request_sock.h
@@ -150,6 +150,8 @@ struct fastopen_queue {
spinlock_t lock;
int qlen; /* # of pending (TCP_SYN_RECV) reqs */
int max_qlen; /* != 0 iff TFO is currently enabled */
+
+ struct tcp_fastopen_context __rcu *ctx; /* cipher context for cookie */
};
/** struct request_sock_queue - queue of request_socks
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 3b3b9b968e2d..1efe8365cb28 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1555,9 +1555,10 @@ struct tcp_fastopen_request {
int copied; /* queued in tcp_connect() */
};
void tcp_free_fastopen_req(struct tcp_sock *tp);
-
+void tcp_fastopen_destroy_cipher(struct sock *sk);
void tcp_fastopen_ctx_destroy(struct net *net);
-int tcp_fastopen_reset_cipher(struct net *net, void *key, unsigned int len);
+int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
+ void *key, unsigned int len);
void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb);
struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
struct request_sock *req,
diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
index 15c25eccab2b..69c7493e42f8 100644
--- a/include/uapi/linux/tcp.h
+++ b/include/uapi/linux/tcp.h
@@ -119,6 +119,7 @@ enum {
#define TCP_FASTOPEN_CONNECT 30 /* Attempt FastOpen with connect */
#define TCP_ULP 31 /* Attach a ULP to a TCP connection */
#define TCP_MD5SIG_EXT 32 /* TCP MD5 Signature with extensions */
+#define TCP_FASTOPEN_KEY 33 /* Set the key for Fast Open (cookie) */
struct tcp_repair_opt {
__u32 opt_code;
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index cac8dd309f39..81d218346cf7 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -284,7 +284,8 @@ static int proc_tcp_fastopen_key(struct ctl_table *table, int write,
ret = -EINVAL;
goto bad_key;
}
- tcp_fastopen_reset_cipher(net, user_key, TCP_FASTOPEN_KEY_LENGTH);
+ tcp_fastopen_reset_cipher(net, NULL, user_key,
+ TCP_FASTOPEN_KEY_LENGTH);
}
bad_key:
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 3b34850d361f..8b1fa4dd4538 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2571,6 +2571,17 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
release_sock(sk);
return err;
}
+ case TCP_FASTOPEN_KEY: {
+ __u8 key[TCP_FASTOPEN_KEY_LENGTH];
+
+ if (optlen != sizeof(key))
+ return -EINVAL;
+
+ if (copy_from_user(key, optval, optlen))
+ return -EFAULT;
+
+ return tcp_fastopen_reset_cipher(net, sk, key, sizeof(key));
+ }
default:
/* fallthru */
break;
@@ -3157,6 +3168,28 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
return -EFAULT;
return 0;
+ case TCP_FASTOPEN_KEY: {
+ __u8 key[TCP_FASTOPEN_KEY_LENGTH];
+ struct tcp_fastopen_context *ctx;
+
+ if (get_user(len, optlen))
+ return -EFAULT;
+
+ rcu_read_lock();
+ ctx = rcu_dereference(icsk->icsk_accept_queue.fastopenq.ctx);
+ if (ctx)
+ memcpy(key, ctx->key, sizeof(key));
+ else
+ len = 0;
+ rcu_read_unlock();
+
+ len = min_t(unsigned int, len, sizeof(key));
+ if (put_user(len, optlen))
+ return -EFAULT;
+ if (copy_to_user(optval, key, len))
+ return -EFAULT;
+ return 0;
+ }
case TCP_THIN_LINEAR_TIMEOUTS:
val = tp->thin_lto;
break;
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index 7ee4aadcdd71..21075ce19cb6 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -29,7 +29,7 @@ void tcp_fastopen_init_key_once(struct net *net)
* for a valid cookie, so this is an acceptable risk.
*/
get_random_bytes(key, sizeof(key));
- tcp_fastopen_reset_cipher(net, key, sizeof(key));
+ tcp_fastopen_reset_cipher(net, NULL, key, sizeof(key));
}
static void tcp_fastopen_ctx_free(struct rcu_head *head)
@@ -40,6 +40,16 @@ static void tcp_fastopen_ctx_free(struct rcu_head *head)
kfree(ctx);
}
+void tcp_fastopen_destroy_cipher(struct sock *sk)
+{
+ struct tcp_fastopen_context *ctx;
+
+ ctx = rcu_dereference_protected(
+ inet_csk(sk)->icsk_accept_queue.fastopenq.ctx, 1);
+ if (ctx)
+ call_rcu(&ctx->rcu, tcp_fastopen_ctx_free);
+}
+
void tcp_fastopen_ctx_destroy(struct net *net)
{
struct tcp_fastopen_context *ctxt;
@@ -55,10 +65,12 @@ void tcp_fastopen_ctx_destroy(struct net *net)
call_rcu(&ctxt->rcu, tcp_fastopen_ctx_free);
}
-int tcp_fastopen_reset_cipher(struct net *net, void *key, unsigned int len)
+int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
+ void *key, unsigned int len)
{
- int err;
struct tcp_fastopen_context *ctx, *octx;
+ struct fastopen_queue *q;
+ int err;
ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -79,27 +91,39 @@ error: kfree(ctx);
}
memcpy(ctx->key, key, len);
- spin_lock(&net->ipv4.tcp_fastopen_ctx_lock);
- octx = rcu_dereference_protected(net->ipv4.tcp_fastopen_ctx,
- lockdep_is_held(&net->ipv4.tcp_fastopen_ctx_lock));
- rcu_assign_pointer(net->ipv4.tcp_fastopen_ctx, ctx);
- spin_unlock(&net->ipv4.tcp_fastopen_ctx_lock);
+ if (sk) {
+ q = &inet_csk(sk)->icsk_accept_queue.fastopenq;
+ spin_lock_bh(&q->lock);
+ octx = rcu_dereference_protected(q->ctx,
+ lockdep_is_held(&q->lock));
+ rcu_assign_pointer(q->ctx, ctx);
+ spin_unlock_bh(&q->lock);
+ } else {
+ spin_lock(&net->ipv4.tcp_fastopen_ctx_lock);
+ octx = rcu_dereference_protected(net->ipv4.tcp_fastopen_ctx,
+ lockdep_is_held(&net->ipv4.tcp_fastopen_ctx_lock));
+ rcu_assign_pointer(net->ipv4.tcp_fastopen_ctx, ctx);
+ spin_unlock(&net->ipv4.tcp_fastopen_ctx_lock);
+ }
if (octx)
call_rcu(&octx->rcu, tcp_fastopen_ctx_free);
return err;
}
-static bool __tcp_fastopen_cookie_gen(struct net *net,
- const void *path,
+static bool __tcp_fastopen_cookie_gen(struct sock *sk, const void *path,
struct tcp_fastopen_cookie *foc)
{
struct tcp_fastopen_context *ctx;
bool ok = false;
rcu_read_lock();
- ctx = rcu_dereference(net->ipv4.tcp_fastopen_ctx);
+
+ ctx = rcu_dereference(inet_csk(sk)->icsk_accept_queue.fastopenq.ctx);
+ if (!ctx)
+ ctx = rcu_dereference(sock_net(sk)->ipv4.tcp_fastopen_ctx);
+
if (ctx) {
crypto_cipher_encrypt_one(ctx->tfm, foc->val, path);
foc->len = TCP_FASTOPEN_COOKIE_SIZE;
@@ -115,7 +139,7 @@ static bool __tcp_fastopen_cookie_gen(struct net *net,
*
* XXX (TFO) - refactor when TCP_FASTOPEN_COOKIE_SIZE != AES_BLOCK_SIZE.
*/
-static bool tcp_fastopen_cookie_gen(struct net *net,
+static bool tcp_fastopen_cookie_gen(struct sock *sk,
struct request_sock *req,
struct sk_buff *syn,
struct tcp_fastopen_cookie *foc)
@@ -124,7 +148,7 @@ static bool tcp_fastopen_cookie_gen(struct net *net,
const struct iphdr *iph = ip_hdr(syn);
__be32 path[4] = { iph->saddr, iph->daddr, 0, 0 };
- return __tcp_fastopen_cookie_gen(net, path, foc);
+ return __tcp_fastopen_cookie_gen(sk, path, foc);
}
#if IS_ENABLED(CONFIG_IPV6)
@@ -132,13 +156,13 @@ static bool tcp_fastopen_cookie_gen(struct net *net,
const struct ipv6hdr *ip6h = ipv6_hdr(syn);
struct tcp_fastopen_cookie tmp;
- if (__tcp_fastopen_cookie_gen(net, &ip6h->saddr, &tmp)) {
+ if (__tcp_fastopen_cookie_gen(sk, &ip6h->saddr, &tmp)) {
struct in6_addr *buf = &tmp.addr;
int i;
for (i = 0; i < 4; i++)
buf->s6_addr32[i] ^= ip6h->daddr.s6_addr32[i];
- return __tcp_fastopen_cookie_gen(net, buf, foc);
+ return __tcp_fastopen_cookie_gen(sk, buf, foc);
}
}
#endif
@@ -313,7 +337,7 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
goto fastopen;
if (foc->len >= 0 && /* Client presents or requests a cookie */
- tcp_fastopen_cookie_gen(sock_net(sk), req, skb, &valid_foc) &&
+ tcp_fastopen_cookie_gen(sk, req, skb, &valid_foc) &&
foc->len == TCP_FASTOPEN_COOKIE_SIZE &&
foc->len == valid_foc.len &&
!memcmp(foc->val, valid_foc.val, foc->len)) {
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 5418ecf03b78..d80e1313200a 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1892,6 +1892,7 @@ void tcp_v4_destroy_sock(struct sock *sk)
/* If socket is aborted during connect operation */
tcp_free_fastopen_req(tp);
+ tcp_fastopen_destroy_cipher(sk);
tcp_saved_syn_free(tp);
sk_sockets_allocated_dec(sk);
--
2.15.0.rc1.287.g2b38de12cc-goog
^ permalink raw reply related
* Re: [PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt routine
From: Troy Kisky @ 2017-10-18 18:29 UTC (permalink / raw)
To: Andy Duan, shawn.guo@linaro.org, netdev@vger.kernel.org,
davem@davemloft.net
Cc: Fabio Estevam, lznuaa@gmail.com, andrew@lunn.ch
In-Reply-To: <677439a7-c225-c231-bbc8-986e2590ebed@boundarydevices.com>
On 10/18/2017 11:10 AM, Troy Kisky wrote:
> On 10/17/2017 7:30 PM, Andy Duan wrote:
>> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Wednesday, October 18, 2017 5:34 AM
>>>>> This is better for code locality and should slightly speed up normal
>>> interrupts.
>>>>>
>>>>> This also allows PPS clock output to start working for i.mx7. This is
>>>>> because
>>>>> i.mx7 was already using the limit of 3 interrupts, and needed another.
>>>>>
>>>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>>>>
>>>>> ---
>>>>>
>>>>> v2: made this change independent of any devicetree change so that old
>>>>> dtbs continue to work.
>>>>>
>>>>> Continue to register ptp clock if interrupt is not found.
>>>>> ---
>>>>> drivers/net/ethernet/freescale/fec.h | 3 +-
>>>>> drivers/net/ethernet/freescale/fec_main.c | 25 ++++++----
>>>>> drivers/net/ethernet/freescale/fec_ptp.c | 82
>>>>> ++++++++++++++++++--------
>>>>> -----
>>>>> 3 files changed, 65 insertions(+), 45 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/freescale/fec.h
>>>>> b/drivers/net/ethernet/freescale/fec.h
>>>>> index ede1876a9a19..be56ac1f1ac4 100644
>>>>> --- a/drivers/net/ethernet/freescale/fec.h
>>>>> +++ b/drivers/net/ethernet/freescale/fec.h
>>>>> @@ -582,12 +582,11 @@ struct fec_enet_private {
>>>>> u64 ethtool_stats[0];
>>>>> };
>>>>>
>>>>> -void fec_ptp_init(struct platform_device *pdev);
>>>>> +void fec_ptp_init(struct platform_device *pdev, int irq_index);
>>>>> void fec_ptp_stop(struct platform_device *pdev); void
>>>>> fec_ptp_start_cyclecounter(struct net_device *ndev); int
>>>>> fec_ptp_set(struct net_device *ndev, struct ifreq *ifr); int
>>>>> fec_ptp_get(struct net_device *ndev, struct ifreq *ifr); -uint
>>>>> fec_ptp_check_pps_event(struct fec_enet_private *fep);
>>>>>
>>>>>
>>>>>
>>> /**********************************************************
>>>>> ******************/
>>>>> #endif /* FEC_H */
>>>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>>>> b/drivers/net/ethernet/freescale/fec_main.c
>>>>> index 3dc2d771a222..21afabbc560f 100644
>>>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>>>> @@ -1602,10 +1602,6 @@ fec_enet_interrupt(int irq, void *dev_id)
>>>>> ret = IRQ_HANDLED;
>>>>> complete(&fep->mdio_done);
>>>>> }
>>>>> -
>>>>> - if (fep->ptp_clock)
>>>>> - if (fec_ptp_check_pps_event(fep))
>>>>> - ret = IRQ_HANDLED;
>>>>> return ret;
>>>>> }
>>>>>
>>>>> @@ -3325,6 +3321,8 @@ fec_probe(struct platform_device *pdev)
>>>>> struct device_node *np = pdev->dev.of_node, *phy_node;
>>>>> int num_tx_qs;
>>>>> int num_rx_qs;
>>>>> + char irq_name[8];
>>>>> + int irq_cnt;
>>>>>
>>>>> fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
>>>>>
>>>>> @@ -3465,18 +3463,27 @@ fec_probe(struct platform_device *pdev)
>>>>> if (ret)
>>>>> goto failed_reset;
>>>>>
>>>>> + irq_cnt = platform_irq_count(pdev);
>>>>> + if (irq_cnt > FEC_IRQ_NUM)
>>>>> + irq_cnt = FEC_IRQ_NUM; /* last for ptp */
>>>>> + else if (irq_cnt == 2)
>>>>> + irq_cnt = 1; /* last for ptp */
>>>>> + else if (irq_cnt <= 0)
>>>>> + irq_cnt = 1; /* Let the for loop fail */
>>>>
>>>> Don't do like this. Don't suppose pps interrupt is the last one.
>>>
>>>
>>> I don't. If the pps interrupt is named, the named interrupt will be used. If it is
>>> NOT named, the last interrupt is used, if 2 interrupts, or >3 interrupt are
>>> provided.
>>> Otherwise, no pps interrupt is assumed.
>>> Fortunately this seems to be true currently.
>>>
>> If pps interrupt is not named, then it limit the last one is pps.
>> We cannot get the pps interrupt based on current chip interrupt define, we never know the future chip how to define interrupt.
>> Although your current implementation can work with current chips, but it is not really good solution.
>>
>>>
>>>> And if irq_cnt is 1 like imx28/imx5x, the patch will break fec interrupt
>>> function.
>>>
>>> How ? fec_ptp_init will not be called as bufdesc_ex is 0.
>>>
>> Imx28 also support enhanced buffer descriptor, if define the ptp clock in dts then bufdesc_ex also can be 1.
>
>
> Only if FEC_QUIRK_HAS_BUFDESC_EX is set, which it is not. Here's the relevant code snippets
>
> }, {
> .name = "imx25-fec",
> .driver_data = FEC_QUIRK_USE_GASKET | FEC_QUIRK_MIB_CLEAR,
> }, {
> .name = "imx27-fec",
> .driver_data = FEC_QUIRK_MIB_CLEAR,
> }, {
> .name = "imx28-fec",
> .driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_SWAP_FRAME |
> FEC_QUIRK_SINGLE_MDIO | FEC_QUIRK_HAS_RACC,
> }, {
>
> ....
> fep->bufdesc_ex = fep->quirks & FEC_QUIRK_HAS_BUFDESC_EX;
> fep->clk_ptp = devm_clk_get(&pdev->dev, "ptp");
> if (IS_ERR(fep->clk_ptp)) {
> fep->clk_ptp = NULL;
> fep->bufdesc_ex = false;
> }
> ______________
>
> You could make your way work though, if you remove the pps clock from imx50.dtsi, imx51.dtsi, and
> imx53.dtsi.
Whoops I meant "ptp" clock.
That brings up a question though.
interrupt-names = "int0", "int1", "int2", "pps"
may be more accurate. Is there any desire for me to use "pps" instead ?
>
> Then you would not need the FEC_QUIRK_HAS_BUFDESC_EX quirk.
> This is all a little off topic though.
>
>
>>
>> I still suggest to use v1 logic check pps interrupt that need to check irq name.
>
>
> I could add that back. But it seems redundant when also looking up by name "int0", "int1", "int2".
>
> I would think that we could recommend in the binding documentation that either all are
> named, or none are. And I don't think we need to make work mistakes like
>
> interrupt-names = "irq0", "ptp", "int1", "int2"
>
> but
> interrupt-names = "int0", "ptp", "int1", "int2"
> should currently work.
>
>
> Let me know if you want me to make work
> interrupt-names = "", "ptp", "", ""
>
> or if you just don't think this patch is necessary. I might have some more
> reasons to try and persuade you.
>
>
>
>>
>>>
>>> Also, if only 1 interrupt is provided, it is assumed there is no unnamed pps
>>> interrupt.
>>>
>>>
>>>>
>>>> I suggest to use .platform_get_irq_byname() to get pps(ptp) interrupt like
>>> your v1 logic check.
>>>>
>>>>> +
>>>>> if (fep->bufdesc_ex)
>>>>> - fec_ptp_init(pdev);
>>>>> + fec_ptp_init(pdev, irq_cnt);
>>>>>
>>>>> ret = fec_enet_init(ndev);
>>>>> if (ret)
>>>>> goto failed_init;
>>>>>
>>>>> - for (i = 0; i < FEC_IRQ_NUM; i++) {
>>>>> - irq = platform_get_irq(pdev, i);
>>>>> + for (i = 0; i < irq_cnt; i++) {
>>>>> + sprintf(irq_name, "int%d", i);
>>>>> + irq = platform_get_irq_byname(pdev, irq_name);
>>>>> + if (irq < 0)
>>>>> + irq = platform_get_irq(pdev, i);
>>>>> if (irq < 0) {
>>>>> - if (i)
>>>>> - break;
>>>>> ret = irq;
>>>>> goto failed_irq;
>>>>> }
>
>
> Thanks for the review
>
> Troy
>
^ permalink raw reply
* [RFC] ip: introduce IFA_F_DHCP flag
From: Girish Moodalbail @ 2017-10-18 18:16 UTC (permalink / raw)
To: netdev, davem
This flag identifies that the address was obtained through DHCP.
Today there is no easy way to find out whether an address on an
interface is DHCP controlled or is static. Either you will need to
grep for 'dhclient' process (or something else in case one is using a
different DHCP client) or if you are using NetworkManager (or some
such), then you will need to query through their interface to find out
if an address is DHCP or not.
This flag will be set by the DHCP clients in the userspace when it
brings up the DHCP address on an interface. For example: ISC DHCP
client (aka dhclient) today brings up the address on an interface by
running ip-address(8) command (in dhclient-script). This command can
be extended to include 'dhcp' keyword in its 'add' or 'replace'
subcommand. Once this flag is set, the show subcommand can display the
keyword 'dhcp' against the address to indicate that the address was
obtained through DHCP.
This flag can also be set and obtained programmatically using
AF_NETLINK. Besides providing observability, this flag will be useful
for applications that need to prevent/allow certain settings on
addresses based on whether they are DHCP or not.
Signed-off-by: Girish Moodalbail <girish.moodalbail@oracle.com>
---
include/uapi/linux/if_addr.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
index 4318ab1..61aa8f8 100644
--- a/include/uapi/linux/if_addr.h
+++ b/include/uapi/linux/if_addr.h
@@ -52,6 +52,7 @@ enum {
#define IFA_F_NOPREFIXROUTE 0x200
#define IFA_F_MCAUTOJOIN 0x400
#define IFA_F_STABLE_PRIVACY 0x800
+#define IFA_F_DHCP 0x1000
struct ifa_cacheinfo {
__u32 ifa_prefered;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next] net: ipv4: Change fib notifiers to take a fib_alias
From: David Ahern @ 2017-10-18 18:39 UTC (permalink / raw)
To: netdev; +Cc: jiri, idosch, David Ahern
All of the notifier data (fib_info, tos, type and table id) are
contained in the fib_alias. Pass it to the notifier instead of
each data separately shortening the argument list by 3.
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/ipv4/fib_trie.c | 39 +++++++++++++++------------------------
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index c636650a6a70..aaa1ba09afaa 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -87,32 +87,30 @@
static int call_fib_entry_notifier(struct notifier_block *nb, struct net *net,
enum fib_event_type event_type, u32 dst,
- int dst_len, struct fib_info *fi,
- u8 tos, u8 type, u32 tb_id)
+ int dst_len, struct fib_alias *fa)
{
struct fib_entry_notifier_info info = {
.dst = dst,
.dst_len = dst_len,
- .fi = fi,
- .tos = tos,
- .type = type,
- .tb_id = tb_id,
+ .fi = fa->fa_info,
+ .tos = fa->fa_tos,
+ .type = fa->fa_type,
+ .tb_id = fa->tb_id,
};
return call_fib4_notifier(nb, net, event_type, &info.info);
}
static int call_fib_entry_notifiers(struct net *net,
enum fib_event_type event_type, u32 dst,
- int dst_len, struct fib_info *fi,
- u8 tos, u8 type, u32 tb_id)
+ int dst_len, struct fib_alias *fa)
{
struct fib_entry_notifier_info info = {
.dst = dst,
.dst_len = dst_len,
- .fi = fi,
- .tos = tos,
- .type = type,
- .tb_id = tb_id,
+ .fi = fa->fa_info,
+ .tos = fa->fa_tos,
+ .type = fa->fa_type,
+ .tb_id = fa->tb_id,
};
return call_fib4_notifiers(net, event_type, &info.info);
}
@@ -1216,9 +1214,7 @@ int fib_table_insert(struct net *net, struct fib_table *tb,
new_fa->fa_default = -1;
call_fib_entry_notifiers(net, FIB_EVENT_ENTRY_REPLACE,
- key, plen, fi,
- new_fa->fa_tos, cfg->fc_type,
- tb->tb_id);
+ key, plen, new_fa);
rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen,
tb->tb_id, &cfg->fc_nlinfo, nlflags);
@@ -1273,8 +1269,7 @@ int fib_table_insert(struct net *net, struct fib_table *tb,
tb->tb_num_default++;
rt_cache_flush(cfg->fc_nlinfo.nl_net);
- call_fib_entry_notifiers(net, event, key, plen, fi, tos, cfg->fc_type,
- tb->tb_id);
+ call_fib_entry_notifiers(net, event, key, plen, new_fa);
rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen, new_fa->tb_id,
&cfg->fc_nlinfo, nlflags);
succeeded:
@@ -1574,8 +1569,7 @@ int fib_table_delete(struct net *net, struct fib_table *tb,
return -ESRCH;
call_fib_entry_notifiers(net, FIB_EVENT_ENTRY_DEL, key, plen,
- fa_to_delete->fa_info, tos,
- fa_to_delete->fa_type, tb->tb_id);
+ fa_to_delete);
rtmsg_fib(RTM_DELROUTE, htonl(key), fa_to_delete, plen, tb->tb_id,
&cfg->fc_nlinfo, 0);
@@ -1892,9 +1886,7 @@ int fib_table_flush(struct net *net, struct fib_table *tb)
call_fib_entry_notifiers(net, FIB_EVENT_ENTRY_DEL,
n->key,
- KEYLENGTH - fa->fa_slen,
- fi, fa->fa_tos, fa->fa_type,
- tb->tb_id);
+ KEYLENGTH - fa->fa_slen, fa);
hlist_del_rcu(&fa->fa_list);
fib_release_info(fa->fa_info);
alias_free_mem_rcu(fa);
@@ -1932,8 +1924,7 @@ static void fib_leaf_notify(struct net *net, struct key_vector *l,
continue;
call_fib_entry_notifier(nb, net, FIB_EVENT_ENTRY_ADD, l->key,
- KEYLENGTH - fa->fa_slen, fi, fa->fa_tos,
- fa->fa_type, fa->tb_id);
+ KEYLENGTH - fa->fa_slen, fa);
}
}
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] lib/dynamic_queue_limits.c: relax BUG_ON to WARN_ON in dql_complete()
From: Eric Dumazet @ 2017-10-18 18:45 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-kernel@vger.kernel.org, <netdev@vger.kernel.org>,
David S. Miller
In-Reply-To: <CAKv+Gu8hpcTBC-TxF898whrX0rYgiqf_ZiqUw2bzo6N_jOyLKw@mail.gmail.com>
On Wed, 2017-10-18 at 18:57 +0100, Ard Biesheuvel wrote:
> On 18 October 2017 at 17:29, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Wed, 2017-10-18 at 16:45 +0100, Ard Biesheuvel wrote:
> >> Even though calling dql_completed() with a count that exceeds the
> >> queued count is a serious error, it still does not justify bringing
> >> down the entire kernel with a BUG_ON(). So relax it to a WARN_ON()
> >> instead.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >> lib/dynamic_queue_limits.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c
> >> index f346715e2255..24ce495d78f3 100644
> >> --- a/lib/dynamic_queue_limits.c
> >> +++ b/lib/dynamic_queue_limits.c
> >> @@ -23,7 +23,7 @@ void dql_completed(struct dql *dql, unsigned int count)
> >> num_queued = ACCESS_ONCE(dql->num_queued);
> >>
> >> /* Can't complete more than what's in queue */
> >> - BUG_ON(count > num_queued - dql->num_completed);
> >> + WARN_ON(count > num_queued - dql->num_completed);
> >>
> >> completed = dql->num_completed + count;
> >> limit = dql->limit;
> >
> > So instead fixing the faulty driver, you'll have strange lockups, and
> > force your users to reboot anyway, after annoying periods where
> > "Internet does not work"
> >
> > These kinds of errors should be found when testing a new device driver
> > or new kernel.
> >
> > Have you found the root cause ?
> >
>
> Not yet, and I don't intend to send out any patches for this
> particular hardware until this is fixed.
>
> But that still doesn't mean you should crash hard. As Linus puts it,
> it is better to 'limp on' if you can (unless we're likely to corrupt
> any non-volatile data, e.g., files on disk etc)
How many BUG() do you plan to change to WARN() exactly ?
If you want to comply to Linus wish, just compile your kernel
with appropriate option.
CONFIG_BUG=n
^ permalink raw reply
* Re: [net-next v2 3/4] openvswitch: Add meter infrastructure
From: Pravin Shelar @ 2017-10-18 18:47 UTC (permalink / raw)
To: Andy Zhou; +Cc: Linux Kernel Network Developers, Joe Stringer, Greg Rose
In-Reply-To: <1508225805-11613-4-git-send-email-azhou@ovn.org>
On Tue, Oct 17, 2017 at 12:36 AM, Andy Zhou <azhou@ovn.org> wrote:
> OVS kernel datapath so far does not support Openflow meter action.
> This is the first stab at adding kernel datapath meter support.
> This implementation supports only drop band type.
>
> Signed-off-by: Andy Zhou <azhou@ovn.org>
> ---
> net/openvswitch/Makefile | 1 +
> net/openvswitch/datapath.c | 14 +-
> net/openvswitch/datapath.h | 3 +
> net/openvswitch/meter.c | 604 +++++++++++++++++++++++++++++++++++++++++++++
> net/openvswitch/meter.h | 54 ++++
> 5 files changed, 674 insertions(+), 2 deletions(-)
> create mode 100644 net/openvswitch/meter.c
> create mode 100644 net/openvswitch/meter.h
>
This patch mostly looks good. I have one comment below.
> +static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct nlattr **a = info->attrs;
> + struct dp_meter *meter, *old_meter;
> + struct sk_buff *reply;
> + struct ovs_header *ovs_reply_header;
> + struct ovs_header *ovs_header = info->userhdr;
> + struct datapath *dp;
> + int err;
> + u32 meter_id;
> + bool failed;
> +
> + meter = dp_meter_create(a);
> + if (IS_ERR_OR_NULL(meter))
> + return PTR_ERR(meter);
> +
> + reply = ovs_meter_cmd_reply_start(info, OVS_METER_CMD_SET,
> + &ovs_reply_header);
> + if (IS_ERR(reply)) {
> + err = PTR_ERR(reply);
> + goto exit_free_meter;
> + }
> +
> + ovs_lock();
> + dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
> + if (!dp) {
> + err = -ENODEV;
> + goto exit_unlock;
> + }
> +
> + if (!a[OVS_METER_ATTR_ID]) {
> + err = -ENODEV;
> + goto exit_unlock;
> + }
> +
> + meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);
> +
> + /* Cannot fail after this. */
> + old_meter = lookup_meter(dp, meter_id);
I do not see RCU read lock taken here. This is not correctness issue
but it could cause RCU checker to spit out warning message. You could
do same trick that is done in get_dp() to avoid this issue.
Can you also test the code with rcu sparse check config option enabled?
Thanks.
^ permalink raw reply
* Re: [PATCH v7 02/10] arm: dts: sunxi: Restore EMAC changes
From: Corentin Labbe @ 2017-10-18 18:50 UTC (permalink / raw)
To: Andrew Lunn
Cc: robh+dt, mark.rutland, maxime.ripard, wens, linux,
catalin.marinas, will.deacon, peppe.cavallaro, alexandre.torgue,
f.fainelli, frowand.list, netdev, devicetree, linux-arm-kernel,
linux-kernel, linux-sunxi
In-Reply-To: <20171018164450.GE15371@lunn.ch>
On Wed, Oct 18, 2017 at 06:44:50PM +0200, Andrew Lunn wrote:
> On Wed, Oct 18, 2017 at 01:44:50PM +0200, Corentin Labbe wrote:
> > The original dwmac-sun8i DT bindings have some issue on how to handle
> > integrated PHY and was reverted in last RC of 4.13.
> > But now we have a solution so we need to get back that was reverted.
> >
> > This patch restore arm DT about dwmac-sun8i
> > This reverts commit fe45174b72ae ("arm: dts: sunxi: Revert EMAC changes")
> >
> > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> > ---
> > arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts | 9 ++++++++
> > arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts | 19 +++++++++++++++++
> > arch/arm/boot/dts/sun8i-h3-nanopi-m1-plus.dts | 19 +++++++++++++++++
> > arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts | 7 ++++++
> > arch/arm/boot/dts/sun8i-h3-orangepi-2.dts | 8 +++++++
> > arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 8 +++++++
> > arch/arm/boot/dts/sun8i-h3-orangepi-pc-plus.dts | 5 +++++
> > arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts | 8 +++++++
> > arch/arm/boot/dts/sun8i-h3-orangepi-plus.dts | 22 +++++++++++++++++++
> > arch/arm/boot/dts/sun8i-h3-orangepi-plus2e.dts | 16 ++++++++++++++
> > arch/arm/boot/dts/sunxi-h3-h5.dtsi | 26 +++++++++++++++++++++++
> > 11 files changed, 147 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > index b1502df7b509..6713d0f2b3f4 100644
> > --- a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > +++ b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > @@ -56,6 +56,8 @@
> >
> > aliases {
> > serial0 = &uart0;
> > + /* ethernet0 is the H3 emac, defined in sun8i-h3.dtsi */
> > + ethernet0 = &emac;
> > ethernet1 = &xr819;
> > };
> >
> > @@ -102,6 +104,13 @@
> > status = "okay";
> > };
> >
> > +&emac {
> > + phy-handle = <&int_mii_phy>;
> > + phy-mode = "mii";
> > + allwinner,leds-active-low;
> > + status = "okay";
> > +};
> > +
> > &mmc0 {
> > pinctrl-names = "default";
> > pinctrl-0 = <&mmc0_pins_a>;
> > diff --git a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> > index e1dba9ffa94b..f2292deaa590 100644
> > --- a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> > +++ b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> > @@ -52,6 +52,7 @@
> > compatible = "sinovoip,bpi-m2-plus", "allwinner,sun8i-h3";
> >
> > aliases {
> > + ethernet0 = &emac;
> > serial0 = &uart0;
> > serial1 = &uart1;
> > };
> > @@ -111,6 +112,24 @@
> > status = "okay";
> > };
> >
> > +&emac {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&emac_rgmii_pins>;
> > + phy-supply = <®_gmac_3v3>;
> > + phy-handle = <&ext_rgmii_phy>;
> > + phy-mode = "rgmii";
> > +
> > + allwinner,leds-active-low;
> > + status = "okay";
> > +};
> > +
>
>
> > +&external_mdio {
> > + ext_rgmii_phy: ethernet-phy@1 {
> > + compatible = "ethernet-phy-ieee802.3-c22";
> > + reg = <0>;
> > + };
> > +};
> > +
>
> Hi Corentin
>
> I'm wondering about the order of the patches. Does the external_mdio
> node actually exist at this point? Or only later when other patches
> are applied?
>
You are right order of patch are wrong, I need to cut this one in two.
"Revert²" sunxi-h3-h5.dtsi
apply mdiomux
"Revert²" all board nodes
Regards
Corentin Labbe
^ permalink raw reply
* Re: [net-next] dsa: slave: support phy devices on external MII bus
From: Florian Fainelli @ 2017-10-18 18:54 UTC (permalink / raw)
To: Martin Hundebøll, Andrew Lunn; +Cc: netdev
In-Reply-To: <8994adaa-6223-20f1-9221-49e9b644d03f@prevas.dk>
On 10/18/2017 10:30 AM, Martin Hundebøll wrote:
>
>
> On 2017-10-18 18:51, Florian Fainelli wrote:
>> On 10/18/2017 09:21 AM, Andrew Lunn wrote:
>>> Hi Martin
>>>
>>> Sorry for starting a new thread. I deleted the patchset from my mailbox.
>>>
>>> Florian said:
>>>
>>>> The logic goes like this:
>>>>
>>>> - try to connect to the PHY via phy-handle
>>>> - if we have a PHY we are connecting via phy-handle but we need to
>>>> divert MDIO reads/writes connect using its address on the diverted
>>>> bus
>>>> - connect using a fixed PHY
>>>> - finally try using the DSA slave MII bus which would connect to the
>>>> switch internal PHYs
>>>
>>> This is not quite correct. Looking at the code:
>>>
>>> phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
>>> ...
>>>
>>> if (phy_dn) {
>>> int phy_id = of_mdio_parse_addr(&slave_dev->dev,
>>> phy_dn);
>>>
>>> /* If this PHY address is part of phys_mii_mask,
>>> which means
>>> * that we need to divert reads and writes to/from
>>> it, then we
>>> * want to bind this device using the slave MII bus
>>> created by
>>> * DSA to make that happen.
>>> */
>>> if (!phy_is_fixed && phy_id >= 0 &&
>>> (ds->phys_mii_mask & (1 << phy_id))) {
>>> ret = dsa_slave_phy_connect(p, slave_dev,
>>> phy_id);
>>> if (ret) {
>>> netdev_err(slave_dev, "failed to
>>> connect to phy%d: %d\n", phy_id, ret);
>>> of_node_put(phy_dn);
>>> return ret;
>>> }
>>> } else {
>>> p->phy = of_phy_connect(slave_dev, phy_dn,
>>>
>>> The first point really is:
>>>
>>> - try to connect to the PHY via phy-handle, if the phy_id is not
>>> valid, or if the phy_id does not map to a phy that the switch says
>>> does not exist.
>>>
>>> In your case, all these points are true, so it uses
>>> dsa_slave_phy_connect(). But we actually want it to use
>>> of_phy_connect(), which will use the correct bus.
>>>
>>> For some Marvell chips, you cannot actually go on ds->phys_mii_mask.
>>> These devices can have in built PHYs and SERDES interfaces which can
>>> be assigned to ports. These SERDES interfaces could have external PHYs
>>> connected to them, and be on the external MDIO bus. So
>>> ds->phys_mii_mask indicates there is a PHY, but the phy-handle points
>>> to a different phy.
>>>
>>> So i think this code block needs to change. If we have a phy-handle,
>>> use it. i.e. what Florian _thinks_ it should be doing. If not, then
>>> use dsa_slave_phy_connect().
>>
>> I see what you mean now, the logic above gets defeated because it does
>> not concern itself with the MDIO controller parent of the PHY node
>> pointed to by phy-handle. So if like Martin you have two MDIO busses,
>> but both happen to have MDIO addresses that are valid for both busses,
>> the logic above gets defeated and we wrongly try to attach to the switch
>> internal MDIO bus under ds->slave_mii_bus.
>>
>> The easiest fix would certainly to lookup the parent MDIO bus and do
>> that only if ds->slave_mii_bus->of_node and the parent of the node
>> pointed to 'phy-handle' match.
>>
>> Does that work for you?
>>
>
> As Andrew implies, I think we should rewrite the entire block to make it
> more intuitive.
I don't particularly care what the fate of that code is, as long as it
does not break on my systems, you break it, you fix it.
>
> Are these the cases that should be handled?
>
> 0) Fixed link
> Register using of_phy_register_fixed_link().
Yes.
>
> 1) No phy-handle
> Use dsa_slave_phy_connect() to connect on internal MDIO bus with phy
> address from index/port-reg property.
Yes.
>
> 2) Valid phy handle
> Use of_phy_connect() to connect using parent MDIO bus handle.
Yes, but with the caveat already covered today: there is a possible
problem with having to divert MDIO accesses of a PHY pointed by
phy-handle towards the internal switch bus because of specific problems
such as those explained in drivers/net/bcm_sf2.c, I don't mind trying to
do things smarter or in a different way, but that needs to be something
possible.
--
Florian
^ permalink raw reply
* Re: [PATCH V2 net-next] liquidio: pass date and time info to NIC firmware
From: Andrew Lunn @ 2017-10-18 19:00 UTC (permalink / raw)
To: Felix Manlunas
Cc: davem, netdev, raghu.vatsavayi, derek.chickles, satananda.burla,
veerasenareddy.burru
In-Reply-To: <20171018172445.GA14012@felix-thinkpad.cavium.com>
Hi Felix
> +static void lio_sync_octeon_time(struct work_struct *work)
> +{
> + struct cavium_wk *wk = (struct cavium_wk *)work;
> + struct lio *lio = (struct lio *)wk->ctxptr;
> + struct octeon_device *oct = lio->oct_dev;
> + struct octeon_soft_command *sc;
> + struct timespec64 ts;
> + struct lio_time *lt;
> + int ret;
> +
> + sc = octeon_alloc_soft_command(oct, sizeof(struct lio_time), 0, 0);
> + if (!sc) {
> + dev_err(&oct->pci_dev->dev,
> + "Failed to sync time to octeon: soft command allocation failed\n");
> + return;
> + }
> +
> + lt = (struct lio_time *)sc->virtdptr;
> +
> + /* Get time of the day */
> + getnstimeofday64(&ts);
> @@ -890,11 +894,29 @@ int octeon_download_firmware(struct octeon_device *oct, const u8 *data,
> load_addr += size;
> }
> }
> +
> + /* Get time of the day */
> + do_gettimeofday(&time);
> + time_to_tm(time.tv_sec, (-sys_tz.tz_minuteswest) * 60, &tm_val);
So here, you correct for the timezone the machine is in. However,
lio_sync_octeon_time() uses getnstimeofday64(), which as far as i
know, does not adjust for the time zone.
Maybe put your machine into Pacific/Kiritimati and see what happens?
Andrew
^ permalink raw reply
* Re: [PATCH net-next 3/3] ipv6: obsolete cached dst when removing them from fib tree
From: Martin KaFai Lau @ 2017-10-18 19:05 UTC (permalink / raw)
To: Wei Wang, Paolo Abeni
Cc: Eric Dumazet, Xin Long, Linux Kernel Network Developers,
David S. Miller, Hannes Frederic Sowa
In-Reply-To: <CAEA6p_DC7gZnaiKdQv7q3P6DcH31c1ioCzVk5HrapAjQG7hRKg@mail.gmail.com>
On Wed, Oct 18, 2017 at 05:56:39PM +0000, Wei Wang wrote:
> On Wed, Oct 18, 2017 at 6:03 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> > On Tue, 2017-10-17 at 13:48 -0700, Wei Wang wrote:
> >> On Tue, Oct 17, 2017 at 1:02 PM, Paolo Abeni <pabeni@redhat.com> wrote:
> >> > Meanwhile others sockets may grab more references to (and use) the same
> >> > aged-out dst.
> >> >
> >>
> >> I don't think other sockets could grab more reference to this dst
> >> because this dst should already be removed from the fib6 tree.
> >
> > With the current net-next code, the dst is not removed from the fib
> > tree while someone else is holding it and dst_check() does not fail
> > after that the cached dst is aged out. If a socket cache grab a
> > reference to the CACHE dst, it will not release it untill the next
> > sernum change, regardless of the dst aging.
> >
> >> > The commit 1e2ea8ad37be ("ipv6: set dst.obsolete when a cached route
> >> > has expired") was the solution to the above issue prior to the recent
> >> > refactor.
> >> >
> >>
> >> I don't really understand how this commit is solving the above issue.
> >> This commit still only ages out cached route if &rt->dst.__refcnt ==
> >> 1. So if socket is holding refcnt to this dst and dst_check() is not
> >> getting called, this cached route still won't get deleted.
> >
> > Setting obsolete to DST_OBSOLETE_KILL forced whoever was holding the
> > dst reference to drop it on the next dst_check(), so that refcnt could
> > go down.
> >
>
> Yes. Understood.
> Martin and I had a discussion yesterday. We both think it is not a
> good idea to set obolete to DST_OBSOLETE_KILL but not to remove it
> from the fib6 tree.
> It is because others who do a route lookup later might potentially
> find this route and tries to use this route. However, dst_check() will
> show this route is invalid. So the user will redo the route lookup.
> But as this route is not yet deleted from the tree, it will find this
> route again. This seems like a bad situation.
> And again, setting obsolete to DST_OBSOLETE_KILL does not prevent some
> idle socket holding on to this dst for a long time...
>
> With the above said, I am now convinced what you have in your patch is
> the correct thing to do. Just remove the cached route without checking
> the refcnt when it is aged.
Another thing (not limited to this case),
Considering we have a limited size in the exception table now and
the oldest one will get removed when the table is full,
do we still need to purge this periodically in gc?
I would like to see the IPv6's gc eventually goes away.
If we need to expire the pmtu value of a RTF_CACHE rt,
can dst.expires be checked during the lookup (like what
ipv4 is doing) and then remove it from the exception table?
^ permalink raw reply
* Re: [net-next] dsa: slave: support phy devices on external MII bus
From: Andrew Lunn @ 2017-10-18 19:09 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Martin Hundebøll, netdev
In-Reply-To: <3b972ab8-3d3e-7666-5c26-f111f5cdd61a@gmail.com>
> Yes, but with the caveat already covered today: there is a possible
> problem with having to divert MDIO accesses of a PHY pointed by
> phy-handle towards the internal switch bus because of specific problems
> such as those explained in drivers/net/bcm_sf2.c, I don't mind trying to
> do things smarter or in a different way, but that needs to be something
> possible.
Hi Florian
Are you referring to:
/* Include the pseudo-PHY address to divert reads towards our
* workaround. This is only required for 7445D0, since 7445E0
* disconnects the internal switch pseudo-PHY such that we can use the
* regular SWITCH_MDIO master controller instead.
*
* Here we flag the pseudo PHY as needing special treatment and would
* otherwise make all other PHY read/writes go to the master MDIO bus
* controller that comes with this switch backed by the "mdio-unimac"
* driver.
*/
?
Do you have an example device tree fragment, just to make it clearer?
Thanks
Andrew
^ permalink raw reply
* [PATCH net-next] net-tun: fix panics at dismantle time
From: Eric Dumazet @ 2017-10-18 19:12 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Petar Penkov, Mahesh Bandewar
From: Eric Dumazet <edumazet@google.com>
syzkaller got crashes at dismantle time [1]
It is not correct to test (tun->flags & IFF_NAPI) in tun_napi_disable()
and tun_napi_del() : Each tun_file can have different mode, depending
on how they were created.
Similarly I have changed tun_get_user() and tun_poll_controller()
to use the new tfile->napi_enabled boolean.
[ 154.331360] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 154.339220] IP: [<ffffffff9634cad6>] hrtimer_active+0x26/0x60
[ 154.344983] PGD 0
[ 154.347009] Oops: 0000 [#1] SMP
[ 154.350680] gsmi: Log Shutdown Reason 0x03
[ 154.379572] task: ffff994719150dc0 ti: ffff99475c0ae000 task.ti: ffff99475c0ae000
[ 154.387043] RIP: 0010:[<ffffffff9634cad6>] [<ffffffff9634cad6>] hrtimer_active+0x26/0x60
[ 154.395232] RSP: 0018:ffff99475c0afce8 EFLAGS: 00010246
[ 154.400542] RAX: ffff994754850ac0 RBX: ffff994753e65408 RCX: ffff994753e65388
[ 154.407666] RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff994753e65408
[ 154.414790] RBP: ffff99475c0afce8 R08: 0000000000000000 R09: 0000000000000000
[ 154.421921] R10: ffff99475f6f5910 R11: 0000000000000001 R12: 0000000000000000
[ 154.429044] R13: ffff99417deab668 R14: ffff99417deaa780 R15: ffff99475f45dde0
[ 154.436174] FS: 0000000000000000(0000) GS:ffff994767a00000(0000) knlGS:0000000000000000
[ 154.444249] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 154.449986] CR2: 0000000000000000 CR3: 00000005a8a0e000 CR4: 0000000000022670
[ 154.457110] Stack:
[ 154.459120] ffff99475c0afd28 ffffffff9634d614 1000000000000000 0000000000000000
[ 154.466598] ffffe54240000000 ffff994753e65408 ffff994753e653a8 ffff99417deab668
[ 154.474067] ffff99475c0afd48 ffffffff9634d6fd ffff99474c2be678 ffff994753e65398
[ 154.481537] Call Trace:
[ 154.483985] [<ffffffff9634d614>] hrtimer_try_to_cancel+0x24/0xf0
[ 154.490074] [<ffffffff9634d6fd>] hrtimer_cancel+0x1d/0x30
[ 154.495563] [<ffffffff96860b3c>] napi_disable+0x3c/0x70
[ 154.500875] [<ffffffff9678ae62>] __tun_detach+0xd2/0x360
[ 154.506272] [<ffffffff9678b117>] tun_chr_close+0x27/0x40
[ 154.511669] [<ffffffff9646ebe6>] __fput+0xd6/0x1e0
[ 154.516548] [<ffffffff9646ed3e>] ____fput+0xe/0x10
[ 154.521429] [<ffffffff963035a2>] task_work_run+0x72/0x90
[ 154.526827] [<ffffffff962e9407>] do_exit+0x317/0xb60
[ 154.531879] [<ffffffff962e9c8f>] do_group_exit+0x3f/0xa0
[ 154.537275] [<ffffffff962e9d07>] SyS_exit_group+0x17/0x20
[ 154.542769] [<ffffffff969784be>] entry_SYSCALL_64_fastpath+0x12/0x17
Fixes: 943170998b20 ("net-tun: enable NAPI for TUN/TAP driver")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
drivers/net/tun.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 57e4c31fa84adc4d9af6ab69a87feac23a8b034e..aef6c7f2f429559ba060b30beb9f729bc71a9c5a 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -175,6 +175,7 @@ struct tun_file {
unsigned int ifindex;
};
struct napi_struct napi;
+ bool napi_enabled;
struct mutex napi_mutex; /* Protects access to the above napi */
struct list_head next;
struct tun_struct *detached;
@@ -276,6 +277,7 @@ static int tun_napi_poll(struct napi_struct *napi, int budget)
static void tun_napi_init(struct tun_struct *tun, struct tun_file *tfile,
bool napi_en)
{
+ tfile->napi_enabled = napi_en;
if (napi_en) {
netif_napi_add(tun->dev, &tfile->napi, tun_napi_poll,
NAPI_POLL_WEIGHT);
@@ -286,13 +288,13 @@ static void tun_napi_init(struct tun_struct *tun, struct tun_file *tfile,
static void tun_napi_disable(struct tun_struct *tun, struct tun_file *tfile)
{
- if (tun->flags & IFF_NAPI)
+ if (tfile->napi_enabled)
napi_disable(&tfile->napi);
}
static void tun_napi_del(struct tun_struct *tun, struct tun_file *tfile)
{
- if (tun->flags & IFF_NAPI)
+ if (tfile->napi_enabled)
netif_napi_del(&tfile->napi);
}
@@ -1055,7 +1057,8 @@ static void tun_poll_controller(struct net_device *dev)
rcu_read_lock();
for (i = 0; i < tun->numqueues; i++) {
tfile = rcu_dereference(tun->tfiles[i]);
- napi_schedule(&tfile->napi);
+ if (tfile->napi_enabled)
+ napi_schedule(&tfile->napi);
}
rcu_read_unlock();
}
@@ -1749,7 +1752,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
napi_gro_frags(&tfile->napi);
local_bh_enable();
mutex_unlock(&tfile->napi_mutex);
- } else if (tun->flags & IFF_NAPI) {
+ } else if (tfile->napi_enabled) {
struct sk_buff_head *queue = &tfile->sk.sk_write_queue;
int queue_len;
^ permalink raw reply related
* Re: [net-next] dsa: slave: support phy devices on external MII bus
From: Florian Fainelli @ 2017-10-18 19:18 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Martin Hundebøll, netdev
In-Reply-To: <20171018190905.GH15371@lunn.ch>
On 10/18/2017 12:09 PM, Andrew Lunn wrote:
>> Yes, but with the caveat already covered today: there is a possible
>> problem with having to divert MDIO accesses of a PHY pointed by
>> phy-handle towards the internal switch bus because of specific problems
>> such as those explained in drivers/net/bcm_sf2.c, I don't mind trying to
>> do things smarter or in a different way, but that needs to be something
>> possible.
>
> Hi Florian
>
> Are you referring to:
>
> /* Include the pseudo-PHY address to divert reads towards our
> * workaround. This is only required for 7445D0, since 7445E0
> * disconnects the internal switch pseudo-PHY such that we can use the
> * regular SWITCH_MDIO master controller instead.
> *
> * Here we flag the pseudo PHY as needing special treatment and would
> * otherwise make all other PHY read/writes go to the master MDIO bus
> * controller that comes with this switch backed by the "mdio-unimac"
> * driver.
> */
> ?
Yes.
>
> Do you have an example device tree fragment, just to make it clearer?
Sure:
switch_top@f0b00000 {
compatible = "simple-bus";
#size-cells = <1>;
#address-cells = <1>;
ranges = <0 0xf0b00000 0x40804>;
ethernet_switch@0 {
compatible = "brcm,bcm7445-switch-v4.0";
#size-cells = <0>;
#address-cells = <1>;
reg = <0x0 0x40000
0x40000 0x110
0x40340 0x30
0x40380 0x30
0x40400 0x34
0x40600 0x208>;
reg-names = "core", "reg", intrl2_0", "intrl2_1",
"fcb, "acb";
interrupts = <0 0x18 0
0 0x19 0>;
brcm,num-gphy = <1>;
brcm,num-rgmii-ports = <2>;
brcm,fcb-pause-override;
brcm,acb-packets-inflight;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
label = "gphy";
reg = <0>;
phy-handle = <&phy5>;
};
port@1 {
label = "rgmii_1";
reg = <1>;
/* connects to the external BCM53125 switch below */
fixed-link {
speed = <1000>;
full-duplex;
}
};
};
};
mdio@403c0 {
reg = <0x403c0 0x8 0x40300 0x18>;
#address-cells = <0x1>;
#size-cells = <0x0>;
compatible = "brcm,bcm7445-mdio-v4.0", "brcm,unimac-mdio";
reg-names = "mdio", "mdio_indir_rw";
phy5: ethernet-phy@5 {
reg = <0x5>;
compatible = "brcm,28nm-gphy", "ethernet-phy-ieee802.3-c22";
};
/* External BCM53125 switch that requires the workaround above */
ethernet-switch@0 {
reg = <0>;
compatible = "brcm,bcm53125";
ports {
...
};
};
};
In fact, this now makes me think that I had to put this workaround in
place solely because at some point I modeled the external BCM53125
switch as a PHY device, and so I had to have phy_connect() work correctly!
Now that we have a proper MDIO device class, this may actually no longer
be necessary at all within net/dsa/slave.c::dsa_slave_phy_setup() since
the probe paths would work completely differently and the offending port
(port 1) would have a fixed-link property. I still need my local
workaround in bcm_sf2.c of course.
I guess, thanks for making me post this example and realize this is
probably no longer necessary :)
--
Florian
^ permalink raw reply
* Re: [PATCH net-next] net: ipv4: Change fib notifiers to take a fib_alias
From: Ido Schimmel @ 2017-10-18 19:26 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, jiri
In-Reply-To: <1508351953-26442-1-git-send-email-dsahern@gmail.com>
On Wed, Oct 18, 2017 at 11:39:13AM -0700, David Ahern wrote:
> All of the notifier data (fib_info, tos, type and table id) are
> contained in the fib_alias. Pass it to the notifier instead of
> each data separately shortening the argument list by 3.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
LGTM.
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Thanks!
^ permalink raw reply
* Re: [net-next] dsa: slave: support phy devices on external MII bus
From: Andrew Lunn @ 2017-10-18 19:31 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Martin Hundebøll, netdev
In-Reply-To: <2e120f66-eb0b-7bfe-450c-db061defca09@gmail.com>
> I guess, thanks for making me post this example and realize this is
> probably no longer necessary :)
Hi Florian
You are welcome. Please test and report back. This code is complex, so
removing part of it would be great.
Andrew
^ 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