* Re: Linux 4.12+ memory leak on router with i40e NICs
From: Alexander Duyck @ 2017-10-19 15:40 UTC (permalink / raw)
To: Anders K. Pedersen | Cohaesio
Cc: pstaszewski@itcare.pl, netdev@vger.kernel.org,
pavlos.parissis@gmail.com, intel-wired-lan@lists.osuosl.org,
alexander.h.duyck@intel.com
In-Reply-To: <1508415582.1186.49.camel@cohaesio.com>
On Thu, Oct 19, 2017 at 5:19 AM, Anders K. Pedersen | Cohaesio
<akp@cohaesio.com> wrote:
> Hi Alex,
>
> On ons, 2017-10-18 at 16:37 -0700, Alexander Duyck wrote:
>> When we last talked I had asked if you could do a git bisect to find
>> the memory leak and you said you would look into it. The most useful
>> way to solve this would be to do a git bisect between your current
>> kernel and the 4.11 kernel to find the point at which this started.
>> If
>> we can do that then fixing this becomes much simpler as we just have
>> to fix the patch that introduced the issue.
>
> We're also seeing a smaller memory leak (about 1 GB per day) than the
> original one even with the "Fix memory leak related filter programming
> status" fix applied. So far I've determined that the leak is present on
> 4.13.7 and was introduced between 4.11 and 4.12, so I'll do another
> round of bisection to identify the patch that introduced this.
>
> Since the router must run for a couple of hours before I can be sure
> whether a kernel is good or bad, and I can't reboot it during working
> hours, it'll probably be about a week before I have a result.
>
> --
> Venlig hilsen / Best Regards
>
> Anders K. Pedersen
> Senior Technical Manager
Anders,
I'll do some digging on my side to see if I can find any other memory
leaks that might be floating around in the driver that could have been
introduced during that time-frame.
One thing you might try that would help with your testing would be to
just disable the ATR functionality in i40e. You can do that with the
ethtool command "ethtool --set-priv-flags <iface> flow-director-atr
off". That should allow you to bisect this without needing to deal
with the "programming status" patches since you won't be programming
ATR filters which is what caused that leak.
Thanks for looking into this.
- Alex
^ permalink raw reply
* Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
From: Franklin S Cooper Jr @ 2017-10-19 15:40 UTC (permalink / raw)
To: Marc Kleine-Budde, Sekhar Nori, Mario Hüttel, Yang, Wenyou,
wg, socketcan, quentin.schulz, edumazet, linux-can, netdev,
linux-kernel
Cc: Wenyou Yang, Dong Aisheng, Quadros, Roger
In-Reply-To: <48317843-b488-8ccb-c6c5-e8cbb90ff5e3@pengutronix.de>
On 10/19/2017 09:55 AM, Marc Kleine-Budde wrote:
> On 10/19/2017 03:54 PM, Franklin S Cooper Jr wrote:
>> On 10/19/2017 06:32 AM, Marc Kleine-Budde wrote:
>>> On 10/19/2017 01:09 PM, Sekhar Nori wrote:
>>>> On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote:
>>>>> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>>>>>>> Sounds reasonable. What's the status of this series?
>>>>>>>>
>>>>>>>> I have had some offline discussions with Franklin on this, and I am not
>>>>>>>> fully convinced that DT is the way to go here (although I don't have the
>>>>>>>> agreement with Franklin there).
>>>>>>>
>>>>>>> Probably the fundamental area where we disagree is what "default" SSP
>>>>>>> value should be used. Based on a short (< 1 ft) point to point test
>>>>>>> using a SSP of 50% worked fine. However, I'm not convinced that this
>>>>>>> default value of 50% will work in a more "traditional" CAN bus at higher
>>>>>>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>>>>>>> board in even the simplest test cases.
>>>>>>>
>>>>>>> I believe that this default SSP should be a DT property that allows any
>>>>>>> board to determine what default value works best in general.
>>>>>>
>>>>>> With that, I think, we are taking DT from describing board/hardware
>>>>>> characteristics to providing default values that software should use.
>>>>>
>>>>> If the default value is board specific and cannot be calculated in
>>>>> general or from other values present in the DT, then it's from my point
>>>>> of view describing the hardware.
>>>>>
>>>>>> In any case, if Marc and/or Wolfgang are okay with it, binding
>>>>>> documentation for such a property should be sent to DT maintainers for
>>>>>> review.
>>>>>>
>>>>>>>>
>>>>>>>> There are two components in configuring the secondary sample point. It
>>>>>>>> is the transceiver loopback delay and an offset (example half of the bit
>>>>>>>> time in data phase).
>>>>>>>>
>>>>>>>> While the transceiver loopback delay is pretty board dependent (and thus
>>>>>>>> amenable to DT encoding), I am not quite sure the offset can be
>>>>>>>> configured in DT because its not really board dependent.
>>>>>>>>
>>>>>>>> Unfortunately, offset calculation does not seem to be an exact science.
>>>>>>>> There are recommendations ranging from using 50% of bit time to making
>>>>>>>> it same as the sample point configured. This means users who need to
>>>>>>>> change the SSP due to offset variations need to change their DT even
>>>>>>>> without anything changing on their board.
>>>>>>>>
>>>>>>>> Since we have a netlink socket interface to configure sample point, I
>>>>>>>> wonder if that should be extended to configure SSP too (or at least the
>>>>>>>> offset part of SSP)?
>>>>>>>
>>>>>>> Sekhar is right that ideally the user should be able to set the SSP at
>>>>>>> runtime. However, my issue is that based on my experience CAN users
>>>>>>> expect the driver to just work the majority of times. For unique use
>>>>>>> cases where the driver calculated values don't work then the user should
>>>>>>> be able to override it. This should only be done for a very small
>>>>>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>>>>>> many users of MCAN may find that the default SSP doesn't work and must
>>>>>>> always use runtime overrides to get anything to work. I don't think that
>>>>>>> is a good user experience which is why I don't like the idea.
>>>>>>
>>>>>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>>>>>> work" without doing any bittiming related setup.
>>>>>
>>>>> From my point of view I'd rather buy a board from a HW vendor where
>>>>> CAN-FD works, rather than a board where I have to tweak the bit-timing
>>>>> for a simple CAN-FD test setup.
>>>>>
>>>>> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
>>>>> MBit/s -> 50%". Do we need an array of tuples in general?
>>
>> Internally what I proposed was a binding that allowed you to pass in an
>> array of a range of baud rates and then a SSP for that baud rate range.
>> Therefore, if the baud rate being used impacted what SSP worked then it
>> allows someone to provide a range of defaults. Of course a person also
>> has the ability to use a single large range thus implementing a single
>> default SSP value.
>
> A single tuple is just a special case of a list of tuples :)
>
> I was thinking of something like this:
>
> First we need a struct defining the bitrate spp relationship.
>
> The driver provides default values by a sorted array of these structs
> together with array length.
>
> During device registration we assign these default values to the actual
> driver instance.
>
> The netlink code can read and overwrite the current values. Maybe it can
> read the default values.
>
> The bitrate calculation code calculates the to be used spp value.
>
> The driver sets the value during the open callback.
>
>>> Do we need more than one tuple here?
>>>
>>>>> If good default values are transceiver and board specific, they can go
>>>>> into the DT. We need a generic (this means driver agnostic) binding for
>>>>> this. If this table needs to be tweaked for special purpose, then we can
>>>>> add a netlink interface for this as well.
>>>>>
>>>>> Comments?
>>>>
>>>> I dont know how a good default (other than 50% as the starting point)
>>>> can be arrived at without doing any actual measurements on the actual
>>>> network. Since we do know that the value has to be tweaked, agree that
>>>> netlink interface has to be provided.
>>
>> Now I have seen in non public documentations that setting SP to SSP also
>> works.
>
> This can already by done now, without the need for new interfaces.
>
>> This makes a bit more sense to me and I'm alot more comfortable going
>> with this. However, since its based on non public information I can't
>> justify it beyond that "it works for me". But I'm alot more
>> comfortable going with then saying "hey this default value works for
>> TI's dra76 evm. Therefore, every MCAN board will be stuck by default
>> for a value that works for us". So if there is no push back with
>> going with SSP = db SP with no documentation to back up why that is
>> being used then I will try that out and send patches.
>
> This means we postpone the whole add-new-interface-dance until the
> SPP=SP approach doesn't work for some usecase?
My justification behind using SSP = SP will be "it works during my
test". If that is fine then I am ok with postponing any new bindings.
>
> Marc
>
^ permalink raw reply
* Re: [PATCH net-next] ipv6: let trace_fib6_table_lookup() dereference the fib table
From: David Ahern @ 2017-10-19 15:38 UTC (permalink / raw)
To: Paolo Abeni, netdev; +Cc: David S. Miller, Steven Rostedt, Ingo Molnar
In-Reply-To: <48235df8fdf787356070d8baa1ac9816ef5736d4.1508398120.git.pabeni@redhat.com>
On 10/19/17 1:31 AM, Paolo Abeni wrote:
> The perf traces for ipv6 routing code show a relevant cost around
> trace_fib6_table_lookup(), even if no trace is enabled. This is
> due to the fib6_table de-referencing currently performed by the
> caller.
>
> Let's the tracing code pay this overhead, passing to the trace
> helper the table pointer. This gives small but measurable
> performance improvement under UDP flood.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> include/trace/events/fib6.h | 6 +++---
> net/ipv6/route.c | 12 ++++++------
> 2 files changed, 9 insertions(+), 9 deletions(-)
Makes sense.
Acked-by: David Ahern <dsa@cumulusnetworks.com>
^ permalink raw reply
* Re: RFC(v2): Audit Kernel Container IDs
From: Paul Moore @ 2017-10-19 15:36 UTC (permalink / raw)
To: Aleksa Sarai, Eric W. Biederman
Cc: James Bottomley, cgroups-u79uwXL29TY76Z2rM5mHXA,
mszeredi-H+wXaHxf7aLQT0dZR+AlfA, Andy Lutomirski,
jlayton-H+wXaHxf7aLQT0dZR+AlfA, Carlos O'Donell, API,
Linux Containers, Linux Kernel, Viro, David Howells,
Linux FS Devel, linux-audit-H+wXaHxf7aLQT0dZR+AlfA, Simo Sorce,
Development, Casey Schaufler, Eric Paris, Steve Grubb,
trondmy-7I+n7zu2hftEKMMhf/gKZA
In-Reply-To: <871sm0j7bm.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
On Wed, Oct 18, 2017 at 8:43 PM, Eric W. Biederman
<ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
> Aleksa Sarai <asarai-l3A5Bk7waGM@public.gmane.org> writes:
>>>> The security implications are that anything that can change the label
>>>> could also hide itself and its doings from the audit system and thus
>>>> would be used as a means to evade detection. I actually think this
>>>> means the label should be write once (once you've set it, you can't
>>>> change it) ...
>>>
>>> Richard and I have talked about a write once approach, but the
>>> thinking was that you may want to allow a nested container
>>> orchestrator (Why? I don't know, but people always want to do the
>>> craziest things.) and a write-once policy makes that impossible. If
>>> we punt on the nested orchestrator, I believe we can seriously think
>>> about a write-once policy to simplify things.
>>
>> Nested containers are a very widely used use-case (see LXC system containers,
>> inside of which people run other container runtimes). So I would definitely
>> consider it something that "needs to be supported in some way". While the LXC
>> guys might be a *tad* crazy, the use-case isn't. :P
No worries, we're all a little crazy in our own special ways ;)
Kidding aside, thanks for explaining the use case.
> Of course some of that gets to running auditd inside a container which
> we don't have yet either.
>
> So I think to start it is perfectly fine to figure out the non-nested
> case first and what makes sense there. Then to sort out the nested
> container case.
>
> The solution might be that a process gets at most one id per ``audit
> namespace''.
In an attempt to stay on-topic, let's try to stick with "audit
container ID" or "container ID" if you must. I really want to avoid
the term "audit namespace" simply because the term "namespace" implies
some things which we aren't planning on doing.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: Get rid of RCU callbacks in TC filters?
From: John Fastabend @ 2017-10-19 15:34 UTC (permalink / raw)
To: paulmck, Cong Wang
Cc: Jamal Hadi Salim, Chris Mi, Linux Kernel Network Developers,
Daniel Borkmann, Eric Dumazet, David Miller, Jiri Pirko
In-Reply-To: <20171018193548.GM3521@linux.vnet.ibm.com>
On 10/18/2017 12:35 PM, Paul E. McKenney wrote:
> On Wed, Oct 18, 2017 at 10:36:28AM -0700, Cong Wang wrote:
>> Hi, all
>>
>> Recently, the RCU callbacks used in TC filters and TC actions keep
>> drawing my attention, they introduce at least 4 race condition bugs:
>>
>> 1. A simple one fixed by Daniel:
>>
>> commit c78e1746d3ad7d548bdf3fe491898cc453911a49
>> Author: Daniel Borkmann <daniel@iogearbox.net>
>> Date: Wed May 20 17:13:33 2015 +0200
>>
>> net: sched: fix call_rcu() race on classifier module unloads
>>
>> 2. A very nasty one fixed by me:
>>
>> commit 1697c4bb5245649a23f06a144cc38c06715e1b65
>> Author: Cong Wang <xiyou.wangcong@gmail.com>
>> Date: Mon Sep 11 16:33:32 2017 -0700
>>
>> net_sched: carefully handle tcf_block_put()
>>
>> 3. Two more bugs found by Chris:
>> https://patchwork.ozlabs.org/patch/826696/
>> https://patchwork.ozlabs.org/patch/826695/
>>
>>
>> Usually RCU callbacks are simple, however for TC filters and actions,
>> they are complex because at least TC actions could be destroyed
>> together with the TC filter in one callback. And RCU callbacks are
>> invoked in BH context, without locking they are parallel too. All of
>> these contribute to the cause of these nasty bugs. It looks like they
>> bring us more problems than benefits.
>>
>> Therefore, I have been thinking about getting rid of these callbacks,
>> because they are not strictly necessary, callers of these call_rcu()
>> are all on slow path and have RTNL lock, so blocking is permitted in
>> their contexts, and _I think_ it does not harm to use
>> synchronize_rcu() on slow paths, at least I can argue RTNL lock is
>> already there and is a bottleneck if we really care. :)
>>
>> There are 3 solutions here:
>>
>> 1) Get rid of these RCU callbacks and use synchronize_rcu(). The
>> downside is this could hurt the performance of deleting TC filters,
>> but again it is slow path comparing to skb classification path. Note,
>> it is _not_ merely replacing call_rcu() with synchronize_rcu(),
>> because many call_rcu()'s are actually in list iterations, we have to
>> use a local list and call list_del_rcu()+list_add() before
>> synchronize_rcu() (Or is there any other API I am not aware of?). If
>> people really hate synchronize_rcu() because of performance, we could
>> also defer the work to a workqueue and callers could keep their
>> performance as they are.
>>
>> 2) Introduce a spinlock to serialize these RCU callbacks. But as I
>> said in commit 1697c4bb5245 ("net_sched: carefully handle
>> tcf_block_put()"), it is very hard to do because of tcf_chain_dump().
>> Potentially we need to do a lot of work to make it possible, if not
>> impossible.
>>
>> 3) Keep these RCU callbacks and fix all race conditions. Like what
>> Chris tries to do in his patchset, but my argument is that we can not
>> prove we are really race-free even with Chris' patches and his patches
>> are already large enough.
>>
My take on this would be to stay with the current RCU callbacks and try
to simplify the implementation. Falling back to sync operations seems
like a step backwards to me. I know update/delete of filters is currently
a pain point for some use cases so getting the RTNL out of the way may
become a requirement to support those (alternatively maybe batching is
good enough).
I guess at a high level with Cris' patches actions are now doing reference
counting correctly. If shared filters also do reference counting similarly
we should be OK right? (yes I know simplifying maybe too much to be
meaningful)
Are we aware of any outstanding problem areas?
>>
>> What do you think? Any other ideas?
>
> 4) Move from call_rcu() to synchronize_rcu(), but if feasible use one
> synchronize_rcu() for multiple deletions/iterations.
>
> 5) Keep call_rcu(), but have the RCU callback schedule a workqueue.
> The workqueue could then use blocking primitives, for example, acquiring
> RTNL.
>
> 6) As with #5, have the RCU callback schedule a workqueue, but aggregate
> workqueue scheduling using a timer. This would reduce the number of
> RTNL acquisitions.
>
> 7) As with #5, have the RCU callback schedule a workqueue, but have each
> iterator accumulate a list of things removed and do call_rcu() on the
> list. This is an alternative way of aggregating to reduce the number
> of RTNL acquisitions.
>
> There are many other ways to skin this cat.
>
> Thanx, Paul
>
Batching will probably be needed soon regardless for the hardware offload
folks.
Thanks,
John
^ permalink raw reply
* Re: [PATCH net-next] geneve: Get rid of is_all_zero(), streamline is_tnl_info_zero()
From: Stefano Brivio @ 2017-10-19 15:31 UTC (permalink / raw)
To: David S . Miller, netdev; +Cc: pravin shelar
In-Reply-To: <94e6eb12a3260e8f569dd072686067ff52c89199.1508411792.git.sbrivio@redhat.com>
On Thu, 19 Oct 2017 13:31:39 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:
> No need to re-invent memchr_inv() with !is_all_zero(). While at
> it, replace conditional and return clauses with a single return
> clause in is_tnl_info_zero().
>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> drivers/net/geneve.c | 19 +++----------------
> 1 file changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index f6404074b7b0..869726660b83 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -1140,24 +1140,11 @@ static struct geneve_dev *geneve_find_dev(struct geneve_net *gn,
> return t;
> }
>
> -static bool is_all_zero(const u8 *fp, size_t size)
> -{
> - int i;
> -
> - for (i = 0; i < size; i++)
> - if (fp[i])
> - return false;
> - return true;
> -}
> -
> static bool is_tnl_info_zero(const struct ip_tunnel_info *info)
> {
> - if (info->key.tun_id || info->key.tun_flags || info->key.tos ||
> - info->key.ttl || info->key.label || info->key.tp_src ||
> - !is_all_zero((const u8 *)&info->key.u, sizeof(info->key.u)))
> - return false;
> - else
> - return true;
> + return !(info->key.tun_id || !info->key.tun_flags || !info->key.tos ||
> + info->key.ttl || !info->key.label || !info->key.tp_src ||
> + memchr_inv(&info->key.u, 0, sizeof(info->key.u)));
NAK, I messed up the check entirely, I'll send a v2, sorry for the
noise.
--
Stefano
^ permalink raw reply
* [PATCH net 7/7] net: aquantia: Bad udp rate on default interrupt coalescing
From: Igor Russkikh @ 2017-10-19 15:23 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, David Arcari, Pavel Belous, Nadezhda Krupnina,
Simon Edelhaus, Igor Russkikh
In-Reply-To: <cover.1508159232.git.igor.russkikh@aquantia.com>
Default Tx rates cause very long ISR delays on Tx.
0xff is 510us delay, giving only ~ 2000 interrupts per seconds for
Tx rings cleanup. With these settings udp tx rate was never higher than
~800Mbps on a single stream. Changing min delay to 0xF makes it
way better with ~6Gbps
TCP stream performance is almost unaffected by this change, since LSO
optimizations play important role.
CPU load is affected insignificantly by this change.
Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index 11f7e71..ec68c20 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -821,12 +821,12 @@ static int hw_atl_b0_hw_interrupt_moderation_set(struct aq_hw_s *self)
itr_rx |= rx_max_timer << 0x10U;
} else {
static unsigned int hw_atl_b0_timers_table_tx_[][2] = {
- {0xffU, 0xffU}, /* 10Gbit */
- {0xffU, 0x1ffU}, /* 5Gbit */
- {0xffU, 0x1ffU}, /* 5Gbit 5GS */
- {0xffU, 0x1ffU}, /* 2.5Gbit */
- {0xffU, 0x1ffU}, /* 1Gbit */
- {0xffU, 0x1ffU}, /* 100Mbit */
+ {0xfU, 0xffU}, /* 10Gbit */
+ {0xfU, 0x1ffU}, /* 5Gbit */
+ {0xfU, 0x1ffU}, /* 5Gbit 5GS */
+ {0xfU, 0x1ffU}, /* 2.5Gbit */
+ {0xfU, 0x1ffU}, /* 1Gbit */
+ {0xfU, 0x1ffU}, /* 100Mbit */
};
static unsigned int hw_atl_b0_timers_table_rx_[][2] = {
--
2.7.4
^ permalink raw reply related
* [PATCH net 6/7] net: aquantia: Enable coalescing management via ethtool interface
From: Igor Russkikh @ 2017-10-19 15:23 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, David Arcari, Pavel Belous, Nadezhda Krupnina,
Simon Edelhaus, Igor Russkikh
In-Reply-To: <cover.1508159232.git.igor.russkikh@aquantia.com>
Aquantia NIC allows both TX and RX interrupt throttle rate (ITR)
management, but this was used in a very limited way via predefined
values. This patch allows to setup ITR default values via module
command line arguments and via standard ethtool coalescing settings.
Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
drivers/net/ethernet/aquantia/atlantic/aq_cfg.h | 8 ++-
.../net/ethernet/aquantia/atlantic/aq_ethtool.c | 65 ++++++++++++++++++
drivers/net/ethernet/aquantia/atlantic/aq_hw.h | 3 +-
drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 36 +++++++---
drivers/net/ethernet/aquantia/atlantic/aq_nic.h | 4 +-
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c | 20 +++---
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 76 ++++++++++++----------
.../aquantia/atlantic/hw_atl/hw_atl_b0_internal.h | 3 +
.../aquantia/atlantic/hw_atl/hw_atl_utils.h | 2 -
9 files changed, 155 insertions(+), 62 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_cfg.h b/drivers/net/ethernet/aquantia/atlantic/aq_cfg.h
index 0fdaaa6..57e7968 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_cfg.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_cfg.h
@@ -22,8 +22,12 @@
#define AQ_CFG_FORCE_LEGACY_INT 0U
-#define AQ_CFG_IS_INTERRUPT_MODERATION_DEF 1U
-#define AQ_CFG_INTERRUPT_MODERATION_RATE_DEF 0xFFFFU
+#define AQ_CFG_INTERRUPT_MODERATION_OFF 0
+#define AQ_CFG_INTERRUPT_MODERATION_ON 1
+#define AQ_CFG_INTERRUPT_MODERATION_AUTO 0xFFFFU
+
+#define AQ_CFG_INTERRUPT_MODERATION_USEC_MAX (0x1FF * 2)
+
#define AQ_CFG_IRQ_MASK 0x1FFU
#define AQ_CFG_VECS_MAX 8U
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
index 3eab408..d5e99b4 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
@@ -221,6 +221,69 @@ static int aq_ethtool_get_rxnfc(struct net_device *ndev,
return err;
}
+int aq_ethtool_get_coalesce(struct net_device *ndev,
+ struct ethtool_coalesce *coal)
+{
+ struct aq_nic_s *aq_nic = netdev_priv(ndev);
+ struct aq_nic_cfg_s *cfg = aq_nic_get_cfg(aq_nic);
+
+ if (cfg->itr == AQ_CFG_INTERRUPT_MODERATION_ON ||
+ cfg->itr == AQ_CFG_INTERRUPT_MODERATION_AUTO) {
+ coal->rx_coalesce_usecs = cfg->rx_itr;
+ coal->tx_coalesce_usecs = cfg->tx_itr;
+ coal->rx_max_coalesced_frames = 0;
+ coal->tx_max_coalesced_frames = 0;
+ } else {
+ coal->rx_coalesce_usecs = 0;
+ coal->tx_coalesce_usecs = 0;
+ coal->rx_max_coalesced_frames = 1;
+ coal->tx_max_coalesced_frames = 1;
+ }
+ return 0;
+}
+
+int aq_ethtool_set_coalesce(struct net_device *ndev,
+ struct ethtool_coalesce *coal)
+{
+ struct aq_nic_s *aq_nic = netdev_priv(ndev);
+ struct aq_nic_cfg_s *cfg = aq_nic_get_cfg(aq_nic);
+
+ /* This is not yet supported
+ */
+ if (coal->use_adaptive_rx_coalesce || coal->use_adaptive_tx_coalesce)
+ return -EOPNOTSUPP;
+
+ /* Atlantic only supports timing based coalescing
+ */
+ if (coal->rx_max_coalesced_frames > 1 ||
+ coal->rx_coalesce_usecs_irq ||
+ coal->rx_max_coalesced_frames_irq)
+ return -EOPNOTSUPP;
+
+ if (coal->tx_max_coalesced_frames > 1 ||
+ coal->tx_coalesce_usecs_irq ||
+ coal->tx_max_coalesced_frames_irq)
+ return -EOPNOTSUPP;
+
+ /* We do not support frame counting. Check this
+ */
+ if (!(coal->rx_max_coalesced_frames == !coal->rx_coalesce_usecs))
+ return -EOPNOTSUPP;
+ if (!(coal->tx_max_coalesced_frames == !coal->tx_coalesce_usecs))
+ return -EOPNOTSUPP;
+
+ if (coal->rx_coalesce_usecs > AQ_CFG_INTERRUPT_MODERATION_USEC_MAX ||
+ coal->tx_coalesce_usecs > AQ_CFG_INTERRUPT_MODERATION_USEC_MAX)
+ return -EINVAL;
+
+ cfg->itr = AQ_CFG_INTERRUPT_MODERATION_ON;
+
+ cfg->rx_itr = coal->rx_coalesce_usecs;
+ cfg->tx_itr = coal->tx_coalesce_usecs;
+
+ return aq_nic_update_interrupt_moderation_settings(aq_nic);
+}
+
const struct ethtool_ops aq_ethtool_ops = {
.get_link = aq_ethtool_get_link,
.get_regs_len = aq_ethtool_get_regs_len,
@@ -235,4 +298,6 @@ const struct ethtool_ops aq_ethtool_ops = {
.get_ethtool_stats = aq_ethtool_stats,
.get_link_ksettings = aq_ethtool_get_link_ksettings,
.set_link_ksettings = aq_ethtool_set_link_ksettings,
+ .get_coalesce = aq_ethtool_get_coalesce,
+ .set_coalesce = aq_ethtool_set_coalesce,
};
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_hw.h b/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
index 3a8baae..0207927 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
@@ -151,8 +151,7 @@ struct aq_hw_ops {
[ETH_ALEN],
u32 count);
- int (*hw_interrupt_moderation_set)(struct aq_hw_s *self,
- bool itr_enabled);
+ int (*hw_interrupt_moderation_set)(struct aq_hw_s *self);
int (*hw_rss_set)(struct aq_hw_s *self,
struct aq_rss_parameters *rss_params);
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 9378b48..483e976 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -16,6 +16,7 @@
#include "aq_pci_func.h"
#include "aq_nic_internal.h"
+#include <linux/moduleparam.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/timer.h>
@@ -24,6 +25,18 @@
#include <linux/tcp.h>
#include <net/ip.h>
+static unsigned int aq_itr = AQ_CFG_INTERRUPT_MODERATION_AUTO;
+module_param_named(aq_itr, aq_itr, uint, 0644);
+MODULE_PARM_DESC(aq_itr, "Interrupt throttling mode");
+
+static unsigned int aq_itr_tx;
+module_param_named(aq_itr_tx, aq_itr_tx, uint, 0644);
+MODULE_PARM_DESC(aq_itr_tx, "TX interrupt throttle rate");
+
+static unsigned int aq_itr_rx;
+module_param_named(aq_itr_rx, aq_itr_rx, uint, 0644);
+MODULE_PARM_DESC(aq_itr_rx, "RX interrupt throttle rate");
+
static void aq_nic_rss_init(struct aq_nic_s *self, unsigned int num_rss_queues)
{
struct aq_nic_cfg_s *cfg = &self->aq_nic_cfg;
@@ -61,9 +74,9 @@ static void aq_nic_cfg_init_defaults(struct aq_nic_s *self)
cfg->is_polling = AQ_CFG_IS_POLLING_DEF;
- cfg->is_interrupt_moderation = AQ_CFG_IS_INTERRUPT_MODERATION_DEF;
- cfg->itr = cfg->is_interrupt_moderation ?
- AQ_CFG_INTERRUPT_MODERATION_RATE_DEF : 0U;
+ cfg->itr = aq_itr;
+ cfg->tx_itr = aq_itr_tx;
+ cfg->rx_itr = aq_itr_rx;
cfg->is_rss = AQ_CFG_IS_RSS_DEF;
cfg->num_rss_queues = AQ_CFG_NUM_RSS_QUEUES_DEF;
@@ -126,10 +139,12 @@ static int aq_nic_update_link_status(struct aq_nic_s *self)
if (err)
return err;
- if (self->link_status.mbps != self->aq_hw->aq_link_status.mbps)
+ if (self->link_status.mbps != self->aq_hw->aq_link_status.mbps) {
pr_info("%s: link change old %d new %d\n",
AQ_CFG_DRV_NAME, self->link_status.mbps,
self->aq_hw->aq_link_status.mbps);
+ aq_nic_update_interrupt_moderation_settings(self);
+ }
self->link_status = self->aq_hw->aq_link_status;
if (!netif_carrier_ok(self->ndev) && self->link_status.mbps) {
@@ -164,9 +179,6 @@ static void aq_nic_service_timer_cb(unsigned long param)
if (err)
goto err_exit;
- self->aq_hw_ops.hw_interrupt_moderation_set(self->aq_hw,
- self->aq_nic_cfg.is_interrupt_moderation);
-
if (self->aq_hw_ops.hw_update_stats)
self->aq_hw_ops.hw_update_stats(self->aq_hw);
@@ -425,9 +437,8 @@ int aq_nic_start(struct aq_nic_s *self)
if (err < 0)
goto err_exit;
- err = self->aq_hw_ops.hw_interrupt_moderation_set(self->aq_hw,
- self->aq_nic_cfg.is_interrupt_moderation);
- if (err < 0)
+ err = aq_nic_update_interrupt_moderation_settings(self);
+ if (err)
goto err_exit;
setup_timer(&self->service_timer, &aq_nic_service_timer_cb,
(unsigned long)self);
@@ -649,6 +660,11 @@ int aq_nic_xmit(struct aq_nic_s *self, struct sk_buff *skb)
return err;
}
+int aq_nic_update_interrupt_moderation_settings(struct aq_nic_s *self)
+{
+ return self->aq_hw_ops.hw_interrupt_moderation_set(self->aq_hw);
+}
+
int aq_nic_set_packet_filter(struct aq_nic_s *self, unsigned int flags)
{
int err = 0;
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.h b/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
index 0ddd556..4309983 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
@@ -40,6 +40,8 @@ struct aq_nic_cfg_s {
u32 vecs; /* vecs==allocated irqs */
u32 irq_type;
u32 itr;
+ u16 rx_itr;
+ u16 tx_itr;
u32 num_rss_queues;
u32 mtu;
u32 ucp_0x364;
@@ -49,7 +51,6 @@ struct aq_nic_cfg_s {
u16 is_mc_list_enabled;
u16 mc_list_count;
bool is_autoneg;
- bool is_interrupt_moderation;
bool is_polling;
bool is_rss;
bool is_lro;
@@ -104,5 +105,6 @@ int aq_nic_set_link_ksettings(struct aq_nic_s *self,
struct aq_nic_cfg_s *aq_nic_get_cfg(struct aq_nic_s *self);
u32 aq_nic_get_fw_version(struct aq_nic_s *self);
int aq_nic_change_pm_state(struct aq_nic_s *self, pm_message_t *pm_msg);
+int aq_nic_update_interrupt_moderation_settings(struct aq_nic_s *self);
#endif /* AQ_NIC_H */
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
index b0747b2..07b3c49 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
@@ -765,24 +765,23 @@ static int hw_atl_a0_hw_multicast_list_set(struct aq_hw_s *self,
return err;
}
-static int hw_atl_a0_hw_interrupt_moderation_set(struct aq_hw_s *self,
- bool itr_enabled)
+static int hw_atl_a0_hw_interrupt_moderation_set(struct aq_hw_s *self)
{
unsigned int i = 0U;
+ u32 itr_rx;
- if (itr_enabled && self->aq_nic_cfg->itr) {
- if (self->aq_nic_cfg->itr != 0xFFFFU) {
+ if (self->aq_nic_cfg->itr) {
+ if (self->aq_nic_cfg->itr != AQ_CFG_INTERRUPT_MODERATION_AUTO) {
u32 itr_ = (self->aq_nic_cfg->itr >> 1);
itr_ = min(AQ_CFG_IRQ_MASK, itr_);
- PHAL_ATLANTIC_A0->itr_rx = 0x80000000U |
- (itr_ << 0x10);
+ itr_rx = 0x80000000U | (itr_ << 0x10);
} else {
u32 n = 0xFFFFU & aq_hw_read_reg(self, 0x00002A00U);
if (n < self->aq_link_status.mbps) {
- PHAL_ATLANTIC_A0->itr_rx = 0U;
+ itr_rx = 0U;
} else {
static unsigned int hw_timers_tbl_[] = {
0x01CU, /* 10Gbit */
@@ -797,8 +796,7 @@ static int hw_atl_a0_hw_interrupt_moderation_set(struct aq_hw_s *self,
hw_atl_utils_mbps_2_speed_index(
self->aq_link_status.mbps);
- PHAL_ATLANTIC_A0->itr_rx =
- 0x80000000U |
+ itr_rx = 0x80000000U |
(hw_timers_tbl_[speed_index] << 0x10U);
}
@@ -806,11 +804,11 @@ static int hw_atl_a0_hw_interrupt_moderation_set(struct aq_hw_s *self,
aq_hw_write_reg(self, 0x00002A00U, 0x8D000000U);
}
} else {
- PHAL_ATLANTIC_A0->itr_rx = 0U;
+ itr_rx = 0U;
}
for (i = HW_ATL_A0_RINGS_MAX; i--;)
- reg_irq_thr_set(self, PHAL_ATLANTIC_A0->itr_rx, i);
+ reg_irq_thr_set(self, itr_rx, i);
return aq_hw_err_from_flags(self);
}
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index 6f6e70a..11f7e71 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -788,31 +788,37 @@ static int hw_atl_b0_hw_multicast_list_set(struct aq_hw_s *self,
return err;
}
-static int hw_atl_b0_hw_interrupt_moderation_set(struct aq_hw_s *self,
- bool itr_enabled)
+static int hw_atl_b0_hw_interrupt_moderation_set(struct aq_hw_s *self)
{
unsigned int i = 0U;
+ u32 itr_tx = 2U;
+ u32 itr_rx = 2U;
- if (itr_enabled && self->aq_nic_cfg->itr) {
+ switch (self->aq_nic_cfg->itr) {
+ case AQ_CFG_INTERRUPT_MODERATION_ON:
+ case AQ_CFG_INTERRUPT_MODERATION_AUTO:
tdm_tx_desc_wr_wb_irq_en_set(self, 0U);
tdm_tdm_intr_moder_en_set(self, 1U);
rdm_rx_desc_wr_wb_irq_en_set(self, 0U);
rdm_rdm_intr_moder_en_set(self, 1U);
- PHAL_ATLANTIC_B0->itr_tx = 2U;
- PHAL_ATLANTIC_B0->itr_rx = 2U;
+ if (self->aq_nic_cfg->itr == AQ_CFG_INTERRUPT_MODERATION_ON) {
+ /* HW timers are in 2us units */
+ int tx_max_timer = self->aq_nic_cfg->tx_itr / 2;
+ int tx_min_timer = tx_max_timer / 2;
- if (self->aq_nic_cfg->itr != 0xFFFFU) {
- unsigned int max_timer = self->aq_nic_cfg->itr / 2U;
- unsigned int min_timer = self->aq_nic_cfg->itr / 32U;
+ int rx_max_timer = self->aq_nic_cfg->rx_itr / 2;
+ int rx_min_timer = rx_max_timer / 2;
- max_timer = min(0x1FFU, max_timer);
- min_timer = min(0xFFU, min_timer);
+ tx_max_timer = min(HW_ATL_INTR_MODER_MAX, tx_max_timer);
+ tx_min_timer = min(HW_ATL_INTR_MODER_MIN, tx_min_timer);
+ rx_max_timer = min(HW_ATL_INTR_MODER_MAX, rx_max_timer);
+ rx_min_timer = min(HW_ATL_INTR_MODER_MIN, rx_min_timer);
- PHAL_ATLANTIC_B0->itr_tx |= min_timer << 0x8U;
- PHAL_ATLANTIC_B0->itr_tx |= max_timer << 0x10U;
- PHAL_ATLANTIC_B0->itr_rx |= min_timer << 0x8U;
- PHAL_ATLANTIC_B0->itr_rx |= max_timer << 0x10U;
+ itr_tx |= tx_min_timer << 0x8U;
+ itr_tx |= tx_max_timer << 0x10U;
+ itr_rx |= rx_min_timer << 0x8U;
+ itr_rx |= rx_max_timer << 0x10U;
} else {
static unsigned int hw_atl_b0_timers_table_tx_[][2] = {
{0xffU, 0xffU}, /* 10Gbit */
@@ -836,34 +842,36 @@ static int hw_atl_b0_hw_interrupt_moderation_set(struct aq_hw_s *self,
hw_atl_utils_mbps_2_speed_index(
self->aq_link_status.mbps);
- PHAL_ATLANTIC_B0->itr_tx |=
- hw_atl_b0_timers_table_tx_[speed_index]
- [0] << 0x8U; /* set min timer value */
- PHAL_ATLANTIC_B0->itr_tx |=
- hw_atl_b0_timers_table_tx_[speed_index]
- [1] << 0x10U; /* set max timer value */
-
- PHAL_ATLANTIC_B0->itr_rx |=
- hw_atl_b0_timers_table_rx_[speed_index]
- [0] << 0x8U; /* set min timer value */
- PHAL_ATLANTIC_B0->itr_rx |=
- hw_atl_b0_timers_table_rx_[speed_index]
- [1] << 0x10U; /* set max timer value */
+ /* Update user visible ITR settings */
+ self->aq_nic_cfg->tx_itr = hw_atl_b0_timers_table_tx_
+ [speed_index][1] * 2;
+ self->aq_nic_cfg->rx_itr = hw_atl_b0_timers_table_rx_
+ [speed_index][1] * 2;
+
+ itr_tx |= hw_atl_b0_timers_table_tx_
+ [speed_index][0] << 0x8U;
+ itr_tx |= hw_atl_b0_timers_table_tx_
+ [speed_index][1] << 0x10U;
+
+ itr_rx |= hw_atl_b0_timers_table_rx_
+ [speed_index][0] << 0x8U;
+ itr_rx |= hw_atl_b0_timers_table_rx_
+ [speed_index][1] << 0x10U;
}
- } else {
+ break;
+ case AQ_CFG_INTERRUPT_MODERATION_OFF:
tdm_tx_desc_wr_wb_irq_en_set(self, 1U);
tdm_tdm_intr_moder_en_set(self, 0U);
rdm_rx_desc_wr_wb_irq_en_set(self, 1U);
rdm_rdm_intr_moder_en_set(self, 0U);
- PHAL_ATLANTIC_B0->itr_tx = 0U;
- PHAL_ATLANTIC_B0->itr_rx = 0U;
+ itr_tx = 0U;
+ itr_rx = 0U;
+ break;
}
for (i = HW_ATL_B0_RINGS_MAX; i--;) {
- reg_tx_intr_moder_ctrl_set(self,
- PHAL_ATLANTIC_B0->itr_tx, i);
- reg_rx_intr_moder_ctrl_set(self,
- PHAL_ATLANTIC_B0->itr_rx, i);
+ reg_tx_intr_moder_ctrl_set(self, itr_tx, i);
+ reg_rx_intr_moder_ctrl_set(self, itr_rx, i);
}
return aq_hw_err_from_flags(self);
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0_internal.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0_internal.h
index fcf89e2..9aa2c6e 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0_internal.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0_internal.h
@@ -139,6 +139,9 @@
#define HW_ATL_B0_FW_VER_EXPECTED 0x01050006U
+#define HW_ATL_INTR_MODER_MAX 0x1FF
+#define HW_ATL_INTR_MODER_MIN 0xFF
+
/* Hardware tx descriptor */
struct __packed hw_atl_txd_s {
u64 buf_addr;
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
index 2218bdb..c99cc69 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
@@ -131,8 +131,6 @@ struct __packed hw_atl_s {
struct hw_atl_stats_s last_stats;
struct hw_atl_stats_s curr_stats;
u64 speed;
- u32 itr_tx;
- u32 itr_rx;
unsigned int chip_features;
u32 fw_ver_actual;
atomic_t dpc;
--
2.7.4
^ permalink raw reply related
* [PATCH net 5/7] net: aquantia: mmio unmap was not performed on driver removal
From: Igor Russkikh @ 2017-10-19 15:23 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, David Arcari, Pavel Belous, Nadezhda Krupnina,
Simon Edelhaus, Igor Russkikh
In-Reply-To: <cover.1508159232.git.igor.russkikh@aquantia.com>
That may lead to mmio resource leakage.
Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c b/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
index 727f0a4..cadaa64 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
@@ -268,6 +268,9 @@ void aq_pci_func_free(struct aq_pci_func_s *self)
aq_nic_ndev_free(self->port[port]);
}
+ if (self->mmio)
+ iounmap(self->mmio);
+
kfree(self);
err_exit:;
--
2.7.4
^ permalink raw reply related
* [PATCH net 4/7] net: aquantia: Limit number of MSIX irqs to the number of cpus
From: Igor Russkikh @ 2017-10-19 15:23 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, David Arcari, Pavel Belous, Nadezhda Krupnina,
Simon Edelhaus, Igor Russkikh
In-Reply-To: <cover.1508159232.git.igor.russkikh@aquantia.com>
There is no much practical use from having MSIX vectors more that number
of cpus, thus cap this first with preconfigured limit, then with number
of cpus online.
Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c b/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
index 4c6c882..727f0a4 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
@@ -85,6 +85,7 @@ int aq_pci_func_init(struct aq_pci_func_s *self)
int err = 0;
unsigned int bar = 0U;
unsigned int port = 0U;
+ unsigned int numvecs = 0U;
err = pci_enable_device(self->pdev);
if (err < 0)
@@ -142,10 +143,12 @@ int aq_pci_func_init(struct aq_pci_func_s *self)
}
}
- /*enable interrupts */
+ numvecs = min((u8)AQ_CFG_VECS_DEF, self->aq_hw_caps.msix_irqs);
+ numvecs = min(numvecs, num_online_cpus());
+
+ /* enable interrupts */
#if !AQ_CFG_FORCE_LEGACY_INT
- err = pci_alloc_irq_vectors(self->pdev, self->aq_hw_caps.msix_irqs,
- self->aq_hw_caps.msix_irqs, PCI_IRQ_MSIX);
+ err = pci_alloc_irq_vectors(self->pdev, numvecs, numvecs, PCI_IRQ_MSIX);
if (err < 0) {
err = pci_alloc_irq_vectors(self->pdev, 1, 1,
@@ -153,7 +156,7 @@ int aq_pci_func_init(struct aq_pci_func_s *self)
if (err < 0)
goto err_exit;
}
-#endif
+#endif /* AQ_CFG_FORCE_LEGACY_INT */
/* net device init */
for (port = 0; port < self->ports; ++port) {
--
2.7.4
^ permalink raw reply related
* [PATCH net 3/7] net: aquantia: Fixed transient link up/down/up notification
From: Igor Russkikh @ 2017-10-19 15:23 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, David Arcari, Pavel Belous, Nadezhda Krupnina,
Simon Edelhaus, Igor Russkikh
In-Reply-To: <cover.1508159232.git.igor.russkikh@aquantia.com>
When doing ifconfig down/up, driver did not reported carrier_off neither
in nic_stop nor in nic_start. That caused link to be visible as "up"
during couple of seconds immediately after "ifconfig up".
Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 6b49dd6..9378b48 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -337,6 +337,7 @@ struct aq_nic_s *aq_nic_alloc_hot(struct net_device *ndev)
}
if (netif_running(ndev))
netif_tx_disable(ndev);
+ netif_carrier_off(self->ndev);
for (self->aq_vecs = 0; self->aq_vecs < self->aq_nic_cfg.vecs;
self->aq_vecs++) {
@@ -902,6 +903,7 @@ int aq_nic_stop(struct aq_nic_s *self)
unsigned int i = 0U;
netif_tx_disable(self->ndev);
+ netif_carrier_off(self->ndev);
del_timer_sync(&self->service_timer);
--
2.7.4
^ permalink raw reply related
* [PATCH net 2/7] net: aquantia: Add queue restarts stats counter
From: Igor Russkikh @ 2017-10-19 15:23 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, David Arcari, Pavel Belous, Nadezhda Krupnina,
Simon Edelhaus, Igor Russkikh
In-Reply-To: <cover.1508159232.git.igor.russkikh@aquantia.com>
Queue stat strings are cleaned up, duplicate stat name strings removed,
queue restarts counter added
Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
.../net/ethernet/aquantia/atlantic/aq_ethtool.c | 92 ++++++++--------------
drivers/net/ethernet/aquantia/atlantic/aq_vec.c | 3 +
2 files changed, 37 insertions(+), 58 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
index a761e91..3eab408 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
@@ -56,10 +56,6 @@ aq_ethtool_set_link_ksettings(struct net_device *ndev,
return aq_nic_set_link_ksettings(aq_nic, cmd);
}
-/* there "5U" is number of queue[#] stats lines (InPackets+...+InErrors) */
-static const unsigned int aq_ethtool_stat_queue_lines = 5U;
-static const unsigned int aq_ethtool_stat_queue_chars =
- 5U * ETH_GSTRING_LEN;
static const char aq_ethtool_stat_names[][ETH_GSTRING_LEN] = {
"InPackets",
"InUCast",
@@ -83,56 +79,26 @@ static const char aq_ethtool_stat_names[][ETH_GSTRING_LEN] = {
"InOctetsDma",
"OutOctetsDma",
"InDroppedDma",
- "Queue[0] InPackets",
- "Queue[0] OutPackets",
- "Queue[0] InJumboPackets",
- "Queue[0] InLroPackets",
- "Queue[0] InErrors",
- "Queue[1] InPackets",
- "Queue[1] OutPackets",
- "Queue[1] InJumboPackets",
- "Queue[1] InLroPackets",
- "Queue[1] InErrors",
- "Queue[2] InPackets",
- "Queue[2] OutPackets",
- "Queue[2] InJumboPackets",
- "Queue[2] InLroPackets",
- "Queue[2] InErrors",
- "Queue[3] InPackets",
- "Queue[3] OutPackets",
- "Queue[3] InJumboPackets",
- "Queue[3] InLroPackets",
- "Queue[3] InErrors",
- "Queue[4] InPackets",
- "Queue[4] OutPackets",
- "Queue[4] InJumboPackets",
- "Queue[4] InLroPackets",
- "Queue[4] InErrors",
- "Queue[5] InPackets",
- "Queue[5] OutPackets",
- "Queue[5] InJumboPackets",
- "Queue[5] InLroPackets",
- "Queue[5] InErrors",
- "Queue[6] InPackets",
- "Queue[6] OutPackets",
- "Queue[6] InJumboPackets",
- "Queue[6] InLroPackets",
- "Queue[6] InErrors",
- "Queue[7] InPackets",
- "Queue[7] OutPackets",
- "Queue[7] InJumboPackets",
- "Queue[7] InLroPackets",
- "Queue[7] InErrors",
+};
+
+static const char aq_ethtool_queue_stat_names[][ETH_GSTRING_LEN] = {
+ "Queue[%d] InPackets",
+ "Queue[%d] OutPackets",
+ "Queue[%d] Restarts",
+ "Queue[%d] InJumboPackets",
+ "Queue[%d] InLroPackets",
+ "Queue[%d] InErrors",
};
static void aq_ethtool_stats(struct net_device *ndev,
struct ethtool_stats *stats, u64 *data)
{
struct aq_nic_s *aq_nic = netdev_priv(ndev);
+ struct aq_nic_cfg_s *cfg = aq_nic_get_cfg(aq_nic);
-/* ASSERT: Need add lines to aq_ethtool_stat_names if AQ_CFG_VECS_MAX > 8 */
- BUILD_BUG_ON(AQ_CFG_VECS_MAX > 8);
- memset(data, 0, ARRAY_SIZE(aq_ethtool_stat_names) * sizeof(u64));
+ memset(data, 0, (ARRAY_SIZE(aq_ethtool_stat_names) +
+ ARRAY_SIZE(aq_ethtool_queue_stat_names) *
+ cfg->vecs) * sizeof(u64));
aq_nic_get_stats(aq_nic, data);
}
@@ -154,8 +120,8 @@ static void aq_ethtool_get_drvinfo(struct net_device *ndev,
strlcpy(drvinfo->bus_info, pdev ? pci_name(pdev) : "",
sizeof(drvinfo->bus_info));
- drvinfo->n_stats = ARRAY_SIZE(aq_ethtool_stat_names) -
- (AQ_CFG_VECS_MAX - cfg->vecs) * aq_ethtool_stat_queue_lines;
+ drvinfo->n_stats = ARRAY_SIZE(aq_ethtool_stat_names) +
+ cfg->vecs * ARRAY_SIZE(aq_ethtool_queue_stat_names);
drvinfo->testinfo_len = 0;
drvinfo->regdump_len = regs_count;
drvinfo->eedump_len = 0;
@@ -164,14 +130,25 @@ static void aq_ethtool_get_drvinfo(struct net_device *ndev,
static void aq_ethtool_get_strings(struct net_device *ndev,
u32 stringset, u8 *data)
{
+ int i, si;
struct aq_nic_s *aq_nic = netdev_priv(ndev);
struct aq_nic_cfg_s *cfg = aq_nic_get_cfg(aq_nic);
-
- if (stringset == ETH_SS_STATS)
- memcpy(data, *aq_ethtool_stat_names,
- sizeof(aq_ethtool_stat_names) -
- (AQ_CFG_VECS_MAX - cfg->vecs) *
- aq_ethtool_stat_queue_chars);
+ u8 *p = data;
+
+ if (stringset == ETH_SS_STATS) {
+ memcpy(p, *aq_ethtool_stat_names,
+ sizeof(aq_ethtool_stat_names));
+ p = p + sizeof(aq_ethtool_stat_names);
+ for (i = 0; i < cfg->vecs; i++) {
+ for (si = 0;
+ si < ARRAY_SIZE(aq_ethtool_queue_stat_names);
+ si++) {
+ snprintf(p, ETH_GSTRING_LEN,
+ aq_ethtool_queue_stat_names[si], i);
+ p += ETH_GSTRING_LEN;
+ }
+ }
+ }
}
static int aq_ethtool_get_sset_count(struct net_device *ndev, int stringset)
@@ -182,9 +159,8 @@ static int aq_ethtool_get_sset_count(struct net_device *ndev, int stringset)
switch (stringset) {
case ETH_SS_STATS:
- ret = ARRAY_SIZE(aq_ethtool_stat_names) -
- (AQ_CFG_VECS_MAX - cfg->vecs) *
- aq_ethtool_stat_queue_lines;
+ ret = ARRAY_SIZE(aq_ethtool_stat_names) +
+ cfg->vecs * ARRAY_SIZE(aq_ethtool_queue_stat_names);
break;
default:
ret = -EOPNOTSUPP;
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
index 305ff8f..5fecc9a 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
@@ -373,8 +373,11 @@ int aq_vec_get_sw_stats(struct aq_vec_s *self, u64 *data, unsigned int *p_count)
memset(&stats_tx, 0U, sizeof(struct aq_ring_stats_tx_s));
aq_vec_add_stats(self, &stats_rx, &stats_tx);
+ /* This data should mimic aq_ethtool_queue_stat_names structure
+ */
data[count] += stats_rx.packets;
data[++count] += stats_tx.packets;
+ data[++count] += stats_tx.queue_restarts;
data[++count] += stats_rx.jumbo_packets;
data[++count] += stats_rx.lro_packets;
data[++count] += stats_rx.errors;
--
2.7.4
^ permalink raw reply related
* [PATCH net 1/7] net: aquantia: Reset nic statistics on interface up/down
From: Igor Russkikh @ 2017-10-19 15:23 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, David Arcari, Pavel Belous, Nadezhda Krupnina,
Simon Edelhaus, Igor Russkikh
In-Reply-To: <cover.1508159232.git.igor.russkikh@aquantia.com>
Internal statistics system on chip never gets reset until hardware
reboot. This is quite inconvenient in terms of ethtool statistics usage.
This patch implements incremental statistics update inside of
service callback.
Upon nic initialization, first request is done to fetch
initial stat data, current collected stat data gets cleared.
Internal statistics mailbox readout is improved to save space and
increase readability
Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
drivers/net/ethernet/aquantia/atlantic/aq_hw.h | 2 +
drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 +
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c | 1 +
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 1 +
.../aquantia/atlantic/hw_atl/hw_atl_utils.c | 69 +++++++++++++++++-----
.../aquantia/atlantic/hw_atl/hw_atl_utils.h | 16 ++++-
6 files changed, 75 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_hw.h b/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
index bf9b3f0..3a8baae 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_hw.h
@@ -163,6 +163,8 @@ struct aq_hw_ops {
int (*hw_get_regs)(struct aq_hw_s *self,
struct aq_hw_caps_s *aq_hw_caps, u32 *regs_buff);
+ int (*hw_update_stats)(struct aq_hw_s *self);
+
int (*hw_get_hw_stats)(struct aq_hw_s *self, u64 *data,
unsigned int *p_count);
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 0a5bb41..6b49dd6 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -167,6 +167,9 @@ static void aq_nic_service_timer_cb(unsigned long param)
self->aq_hw_ops.hw_interrupt_moderation_set(self->aq_hw,
self->aq_nic_cfg.is_interrupt_moderation);
+ if (self->aq_hw_ops.hw_update_stats)
+ self->aq_hw_ops.hw_update_stats(self->aq_hw);
+
memset(&stats_rx, 0U, sizeof(struct aq_ring_stats_rx_s));
memset(&stats_tx, 0U, sizeof(struct aq_ring_stats_tx_s));
for (i = AQ_DIMOF(self->aq_vec); i--;) {
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
index c5a02df..b0747b2 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
@@ -885,6 +885,7 @@ static struct aq_hw_ops hw_atl_ops_ = {
.hw_rss_set = hw_atl_a0_hw_rss_set,
.hw_rss_hash_set = hw_atl_a0_hw_rss_hash_set,
.hw_get_regs = hw_atl_utils_hw_get_regs,
+ .hw_update_stats = hw_atl_utils_update_stats,
.hw_get_hw_stats = hw_atl_utils_get_hw_stats,
.hw_get_fw_version = hw_atl_utils_get_fw_version,
};
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index 21784cc..6f6e70a 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -939,6 +939,7 @@ static struct aq_hw_ops hw_atl_ops_ = {
.hw_rss_set = hw_atl_b0_hw_rss_set,
.hw_rss_hash_set = hw_atl_b0_hw_rss_hash_set,
.hw_get_regs = hw_atl_utils_hw_get_regs,
+ .hw_update_stats = hw_atl_utils_update_stats,
.hw_get_hw_stats = hw_atl_utils_get_hw_stats,
.hw_get_fw_version = hw_atl_utils_get_fw_version,
};
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
index bf734b3..1fe016f 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
@@ -255,6 +255,15 @@ static int hw_atl_utils_mpi_create(struct aq_hw_s *self,
return err;
}
+int hw_atl_utils_mpi_read_mbox(struct aq_hw_s *self,
+ struct hw_aq_atl_utils_mbox_header *pmbox)
+{
+ return hw_atl_utils_fw_downld_dwords(self,
+ PHAL_ATLANTIC->mbox_addr,
+ (u32 *)(void *)pmbox,
+ sizeof(*pmbox) / sizeof(u32));
+}
+
void hw_atl_utils_mpi_read_stats(struct aq_hw_s *self,
struct hw_aq_atl_utils_mbox *pmbox)
{
@@ -267,9 +276,6 @@ void hw_atl_utils_mpi_read_stats(struct aq_hw_s *self,
if (err < 0)
goto err_exit;
- if (pmbox != &PHAL_ATLANTIC->mbox)
- memcpy(pmbox, &PHAL_ATLANTIC->mbox, sizeof(*pmbox));
-
if (IS_CHIP_FEATURE(REVISION_A0)) {
unsigned int mtu = self->aq_nic_cfg ?
self->aq_nic_cfg->mtu : 1514U;
@@ -299,17 +305,17 @@ void hw_atl_utils_mpi_set(struct aq_hw_s *self,
{
int err = 0;
u32 transaction_id = 0;
+ struct hw_aq_atl_utils_mbox_header mbox;
if (state == MPI_RESET) {
- hw_atl_utils_mpi_read_stats(self, &PHAL_ATLANTIC->mbox);
+ hw_atl_utils_mpi_read_mbox(self, &mbox);
- transaction_id = PHAL_ATLANTIC->mbox.transaction_id;
+ transaction_id = mbox.transaction_id;
AQ_HW_WAIT_FOR(transaction_id !=
- (hw_atl_utils_mpi_read_stats
- (self, &PHAL_ATLANTIC->mbox),
- PHAL_ATLANTIC->mbox.transaction_id),
- 1000U, 100U);
+ (hw_atl_utils_mpi_read_mbox(self, &mbox),
+ mbox.transaction_id),
+ 1000U, 100U);
if (err < 0)
goto err_exit;
}
@@ -492,16 +498,51 @@ int hw_atl_utils_hw_set_power(struct aq_hw_s *self,
return 0;
}
+int hw_atl_utils_update_stats(struct aq_hw_s *self)
+{
+ struct hw_atl_s *hw_self = PHAL_ATLANTIC;
+ struct hw_aq_atl_utils_mbox mbox;
+
+ if (!self->aq_link_status.mbps)
+ return 0;
+
+ hw_atl_utils_mpi_read_stats(self, &mbox);
+
+#define AQ_SDELTA(_N_) (hw_self->curr_stats._N_ += \
+ mbox.stats._N_ - hw_self->last_stats._N_)
+
+ AQ_SDELTA(uprc);
+ AQ_SDELTA(mprc);
+ AQ_SDELTA(bprc);
+ AQ_SDELTA(erpt);
+
+ AQ_SDELTA(uptc);
+ AQ_SDELTA(mptc);
+ AQ_SDELTA(bptc);
+ AQ_SDELTA(erpr);
+
+ AQ_SDELTA(ubrc);
+ AQ_SDELTA(ubtc);
+ AQ_SDELTA(mbrc);
+ AQ_SDELTA(mbtc);
+ AQ_SDELTA(bbrc);
+ AQ_SDELTA(bbtc);
+ AQ_SDELTA(dpc);
+
+#undef AQ_SDELTA
+
+ memcpy(&hw_self->last_stats, &mbox.stats, sizeof(mbox.stats));
+
+ return 0;
+}
+
int hw_atl_utils_get_hw_stats(struct aq_hw_s *self,
u64 *data, unsigned int *p_count)
{
- struct hw_atl_stats_s *stats = NULL;
+ struct hw_atl_s *hw_self = PHAL_ATLANTIC;
+ struct hw_atl_stats_s *stats = &hw_self->curr_stats;
int i = 0;
- hw_atl_utils_mpi_read_stats(self, &PHAL_ATLANTIC->mbox);
-
- stats = &PHAL_ATLANTIC->mbox.stats;
-
data[i] = stats->uprc + stats->mprc + stats->bprc;
data[++i] = stats->uprc;
data[++i] = stats->mprc;
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
index e0360a6..2218bdb 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
@@ -115,16 +115,21 @@ struct __packed hw_aq_atl_utils_fw_rpc {
};
};
-struct __packed hw_aq_atl_utils_mbox {
+struct __packed hw_aq_atl_utils_mbox_header {
u32 version;
u32 transaction_id;
- int error;
+ u32 error;
+};
+
+struct __packed hw_aq_atl_utils_mbox {
+ struct hw_aq_atl_utils_mbox_header header;
struct hw_atl_stats_s stats;
};
struct __packed hw_atl_s {
struct aq_hw_s base;
- struct hw_aq_atl_utils_mbox mbox;
+ struct hw_atl_stats_s last_stats;
+ struct hw_atl_stats_s curr_stats;
u64 speed;
u32 itr_tx;
u32 itr_rx;
@@ -170,6 +175,9 @@ enum hal_atl_utils_fw_state_e {
void hw_atl_utils_hw_chip_features_init(struct aq_hw_s *self, u32 *p);
+int hw_atl_utils_mpi_read_mbox(struct aq_hw_s *self,
+ struct hw_aq_atl_utils_mbox_header *pmbox);
+
void hw_atl_utils_mpi_read_stats(struct aq_hw_s *self,
struct hw_aq_atl_utils_mbox *pmbox);
@@ -199,6 +207,8 @@ int hw_atl_utils_hw_deinit(struct aq_hw_s *self);
int hw_atl_utils_get_fw_version(struct aq_hw_s *self, u32 *fw_version);
+int hw_atl_utils_update_stats(struct aq_hw_s *self);
+
int hw_atl_utils_get_hw_stats(struct aq_hw_s *self,
u64 *data,
unsigned int *p_count);
--
2.7.4
^ permalink raw reply related
* [PATCH net 0/7] net: aquantia: Atlantic driver 10/2017 updates
From: Igor Russkikh @ 2017-10-19 15:23 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, David Arcari, Pavel Belous, Nadezhda Krupnina,
Simon Edelhaus, Igor Russkikh
This patchset fixes various issues in driver,
improves parameters for better performance on 10Gbit link
Igor Russkikh (7):
net: aquantia: Reset nic statistics on interface up/down
net: aquantia: Add queue restarts stats counter
net: aquantia: Fixed transient link up/down/up notification
net: aquantia: Limit number of MSIX irqs to the number of cpus
net: aquantia: mmio unmap was not performed on driver removal
net: aquantia: Enable coalescing management via ethtool interface
net: aquantia: Bad udp rate on default interrupt coalescing
drivers/net/ethernet/aquantia/atlantic/aq_cfg.h | 8 +-
.../net/ethernet/aquantia/atlantic/aq_ethtool.c | 157 +++++++++++++--------
drivers/net/ethernet/aquantia/atlantic/aq_hw.h | 5 +-
drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 39 +++--
drivers/net/ethernet/aquantia/atlantic/aq_nic.h | 4 +-
.../net/ethernet/aquantia/atlantic/aq_pci_func.c | 14 +-
drivers/net/ethernet/aquantia/atlantic/aq_vec.c | 3 +
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c | 21 ++-
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 89 ++++++------
.../aquantia/atlantic/hw_atl/hw_atl_b0_internal.h | 3 +
.../aquantia/atlantic/hw_atl/hw_atl_utils.c | 69 +++++++--
.../aquantia/atlantic/hw_atl/hw_atl_utils.h | 18 ++-
12 files changed, 284 insertions(+), 146 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods
From: Egil Hjelmeland @ 2017-10-19 15:19 UTC (permalink / raw)
To: Vivien Didelot, andrew, f.fainelli, netdev, linux-kernel
In-Reply-To: <871slzmars.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
On 19. okt. 2017 17:12, Vivien Didelot wrote:
> Hi Egil,
>
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
>
>>> Why is there different spacing and prefix with these defines?
>>
>> The extra space is to set bit definitions apart from register offsets,
>> a convention that is used in the file. However, agree that the
>> bit defs should be prefixed with LAN9303_ to be consistent with
>> rest of the file.
>
> OK, I'm fine with this spacing then. The prefix would be nice though,
> thanks!
>
Prefix already done in my working version.
>
> If you cannot think about a comment text which brings value, it
> certainly means it isn't necessary. As you said the implicit "alr"
> namespace already helps here. I'd personally drop all section comments
> ;-)
>
Then I will just drop the section comments.
>
> Thank you,
>
> Vivien
>
Egil
^ permalink raw reply
* RE: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods
From: David Laight @ 2017-10-19 15:15 UTC (permalink / raw)
To: 'Andrew Lunn', Vivien Didelot
Cc: Egil Hjelmeland, f.fainelli@gmail.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20171019141505.GA14840@lunn.ch>
From: Andrew Lunn
> Sent: 19 October 2017 15:15
> > > +/* Clear learned (non-static) entry on given port */
> > > +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0,
> > > + u32 dat1, int portmap, void *ctx)
> > > +{
> > > + int *port = ctx;
> >
> > You can get the value directly to make the line below more readable:
> >
> > int port = *(int *)ctx;
>
> You have to be a bit careful with this. You often see people
> submitting patches taking away casts for void * pointers.
> If they do that here, it should at least not compile...
>
> So maybe do it in two steps?
>
> int * pport = ctx;
> int port = *pport;
IMHO it is best to define a struct for the 'ctx and then do:
..., void *v_ctx)
{
foo_ctx *ctx = v_ctx;
int port = ctx->port;
That stops anyone having to double-check that the *(int *)
is operating on a pointer to an integer of the correct size.
One of the syntax checkers probably ought to generate a warning
for *(integer_type *)foo since it is often a bug.
David
^ permalink raw reply
* Re: [PATCH 2/7] devlink: Adding NPAR permanent config parameters
From: Steve Lin @ 2017-10-19 15:12 UTC (permalink / raw)
To: Yuval Mintz
Cc: netdev@vger.kernel.org, Jiri Pirko, davem@davemloft.net,
michael.chan@broadcom.com, linville@tuxdriver.com,
gospo@broadcom.com
In-Reply-To: <AM0PR0502MB36839C516E78EF0C2C1E4B46BF420@AM0PR0502MB3683.eurprd05.prod.outlook.com>
On Thu, Oct 19, 2017 at 6:39 AM, Yuval Mintz <yuvalm@mellanox.com> wrote:
>> DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION_VALID: 1 to use
>> BW_RESERVATION setting, 0 to ignore.
>>
> ...
>> DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID: 1 to use BW_LIMIT
>> setting, 0 to ignore.
>
> While it probably ties to different fields in your NVM layout why would the user
> require specific attributes for these? Why not have values in the actual
> attributes indicating of this status?
Hi Yuval,
Does having the separate valid flag present any difficulties? There
are lots of implementation options here (a limit or reservation value
of 0 could mean invalid, or we could define (1 << 31) to be a valid
flag when setting the value, etc.), and I'm not necessarily tied to
doing it this way, but it seemed a straightforward way to represent
the validity of the other field.
Thanks again,
Steve
^ permalink raw reply
* Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods
From: Vivien Didelot @ 2017-10-19 15:12 UTC (permalink / raw)
To: Egil Hjelmeland, andrew, f.fainelli, netdev, linux-kernel
In-Reply-To: <1976baec-5ec3-9ec4-252b-743d9f1d9579@egil-hjelmeland.no>
Hi Egil,
Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
>>> #define LAN9303_MAC_RX_CFG_2 0x0c01
>>> #define LAN9303_MAC_TX_CFG_2 0x0c40
>>> #define LAN9303_SWE_ALR_CMD 0x1800
>>> +# define ALR_CMD_MAKE_ENTRY BIT(2)
>>> +# define ALR_CMD_GET_FIRST BIT(1)
>>> +# define ALR_CMD_GET_NEXT BIT(0)
>>> +#define LAN9303_SWE_ALR_WR_DAT_0 0x1801
>>> +#define LAN9303_SWE_ALR_WR_DAT_1 0x1802
>>> +# define ALR_DAT1_VALID BIT(26)
>>> +# define ALR_DAT1_END_OF_TABL BIT(25)
>>> +# define ALR_DAT1_AGE_OVERRID BIT(25)
>>> +# define ALR_DAT1_STATIC BIT(24)
>>> +# define ALR_DAT1_PORT_BITOFFS 16
>>> +# define ALR_DAT1_PORT_MASK (7 << ALR_DAT1_PORT_BITOFFS)
>>> +#define LAN9303_SWE_ALR_RD_DAT_0 0x1805
>>> +#define LAN9303_SWE_ALR_RD_DAT_1 0x1806
>>> +#define LAN9303_SWE_ALR_CMD_STS 0x1808
>>> +# define ALR_STS_MAKE_PEND BIT(0)
>>
>> Why is there different spacing and prefix with these defines?
>
> The extra space is to set bit definitions apart from register offsets,
> a convention that is used in the file. However, agree that the
> bit defs should be prefixed with LAN9303_ to be consistent with
> rest of the file.
OK, I'm fine with this spacing then. The prefix would be nice though,
thanks!
>>> +/* --------------------- Various chip setup ----------------------*/
>>> +
>>
>> This isn't a very useful comment, at least use an inline or block
>> comment if you want to keep it.
>>
>
> I put it to signify the end of the ALR section. But could not think of a
> better text... Perhaps "End of ALR functions" would be better?
> Or perhaps I should just drop the "section comments"? After all the
> functions in question has "alr" in their names.
If you cannot think about a comment text which brings value, it
certainly means it isn't necessary. As you said the implicit "alr"
namespace already helps here. I'd personally drop all section comments
;-)
A general thought against comments is that namespaced and readable code
must act as its own documentation. Explicit comments are only necessary
and appreciated for complex portion of code, for example when
performance is chosen over readability.
Thank you,
Vivien
^ permalink raw reply
* Re: [PATCH 4/7] devlink: Adding perm config of link settings
From: Steve Lin @ 2017-10-19 15:07 UTC (permalink / raw)
To: Yuval Mintz
Cc: netdev@vger.kernel.org, Jiri Pirko, davem@davemloft.net,
michael.chan@broadcom.com, linville@tuxdriver.com,
gospo@broadcom.com
In-Reply-To: <AM0PR0502MB36835F863A3E05376F3562B1BF420@AM0PR0502MB3683.eurprd05.prod.outlook.com>
On Thu, Oct 19, 2017 at 2:07 AM, Yuval Mintz <yuvalm@mellanox.com> wrote:
>> +enum devlink_autoneg_protocol {
>> + DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_BAM,
>> + DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_CONSORTIUM,
>> + DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY,
>> + DEVLINK_AUTONEG_PROTOCOL_BAM, /* Broadcom
>> Autoneg Mode */
>> + DEVLINK_AUTONEG_PROTOCOL_CONSORTIUM, /*
>> Consortium Autoneg Mode */
>> +};
>
> Wouldn't adding BAM as a 'generic' mode of operation be like adding
> non-consortium speeds to ethtool API?
> [I profess ignorance in this area; For all I know it can be a widely accepted
> industry standard]
>
Yuval, I'm glad to get input from other NIC vendors. The high-level
goal of this effort is to allow users of various vendors' NICs to be
able to configure these types of NVRAM/permanent/default settings
using an inbox tool, rather than the collection of vendor-specific
tools that is the status quo.
In order to provide that functionality, it seems like the
vendor-specific parameters and also the vendor-specific settings of
common parameters both need to be supported in this manner.
Ideally there will be much overlap in both the set of parameters
available as well as the options for each parameter, but in the real
world, there will always be differences between vendors and even
between different devices (drivers) from the same vendor. Despite
that reality, I think there is still great benefit in having a common
inbox tool that users can use for device configuration of this type.
It just means that not all drivers will support all parameters, nor
all options for each parameter that they do support.
Thanks,
Steve
^ permalink raw reply
* Re: [PATCH v2 02/15] usb: gadget: make config_item_type structures const
From: Laurent Pinchart @ 2017-10-19 15:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Bhumika Goyal, julia.lawall, rjw, lenb, alexander.shishkin, jic23,
knaack.h, lars, pmeerw, dledford, sean.hefty, hal.rosenstock,
sagi, kishon, bhelgaas, nab, balbi, gregkh, jlbec, ccaulfie,
teigland, mfasheh, linux-acpi, linux-kernel, linux-iio,
linux-rdma, netdev, linux-nvme, linux-pci, linux-scsi,
target-devel, linux-usb, cluster-devel, ocf
In-Reply-To: <20171019140657.GA22233@lst.de>
Hi Christoph,
On Thursday, 19 October 2017 17:06:57 EEST Christoph Hellwig wrote:
> > Now we have 9 const instances of the config_item_type structure that are
> > identical, with only the .ct_owner field set. Should they be all merged
> > into a single structure ?
>
> I think that's a good idea.
>
> But I'm about to slurp up this whole series into my tree, how about making
> that an incremental patch?
I'm fine with that.
Bhumika, would you like to submit an incremental patch, or should I do it ?
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
From: Marc Kleine-Budde @ 2017-10-19 14:55 UTC (permalink / raw)
To: Franklin S Cooper Jr, Sekhar Nori, Mario Hüttel,
Yang, Wenyou, wg, socketcan, quentin.schulz, edumazet, linux-can,
netdev, linux-kernel
Cc: Wenyou Yang, Dong Aisheng, Quadros, Roger
In-Reply-To: <74482aa8-7b65-bd3d-7160-0d515b8c7b27@ti.com>
[-- Attachment #1.1: Type: text/plain, Size: 6602 bytes --]
On 10/19/2017 03:54 PM, Franklin S Cooper Jr wrote:
> On 10/19/2017 06:32 AM, Marc Kleine-Budde wrote:
>> On 10/19/2017 01:09 PM, Sekhar Nori wrote:
>>> On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote:
>>>> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>>>>>> Sounds reasonable. What's the status of this series?
>>>>>>>
>>>>>>> I have had some offline discussions with Franklin on this, and I am not
>>>>>>> fully convinced that DT is the way to go here (although I don't have the
>>>>>>> agreement with Franklin there).
>>>>>>
>>>>>> Probably the fundamental area where we disagree is what "default" SSP
>>>>>> value should be used. Based on a short (< 1 ft) point to point test
>>>>>> using a SSP of 50% worked fine. However, I'm not convinced that this
>>>>>> default value of 50% will work in a more "traditional" CAN bus at higher
>>>>>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>>>>>> board in even the simplest test cases.
>>>>>>
>>>>>> I believe that this default SSP should be a DT property that allows any
>>>>>> board to determine what default value works best in general.
>>>>>
>>>>> With that, I think, we are taking DT from describing board/hardware
>>>>> characteristics to providing default values that software should use.
>>>>
>>>> If the default value is board specific and cannot be calculated in
>>>> general or from other values present in the DT, then it's from my point
>>>> of view describing the hardware.
>>>>
>>>>> In any case, if Marc and/or Wolfgang are okay with it, binding
>>>>> documentation for such a property should be sent to DT maintainers for
>>>>> review.
>>>>>
>>>>>>>
>>>>>>> There are two components in configuring the secondary sample point. It
>>>>>>> is the transceiver loopback delay and an offset (example half of the bit
>>>>>>> time in data phase).
>>>>>>>
>>>>>>> While the transceiver loopback delay is pretty board dependent (and thus
>>>>>>> amenable to DT encoding), I am not quite sure the offset can be
>>>>>>> configured in DT because its not really board dependent.
>>>>>>>
>>>>>>> Unfortunately, offset calculation does not seem to be an exact science.
>>>>>>> There are recommendations ranging from using 50% of bit time to making
>>>>>>> it same as the sample point configured. This means users who need to
>>>>>>> change the SSP due to offset variations need to change their DT even
>>>>>>> without anything changing on their board.
>>>>>>>
>>>>>>> Since we have a netlink socket interface to configure sample point, I
>>>>>>> wonder if that should be extended to configure SSP too (or at least the
>>>>>>> offset part of SSP)?
>>>>>>
>>>>>> Sekhar is right that ideally the user should be able to set the SSP at
>>>>>> runtime. However, my issue is that based on my experience CAN users
>>>>>> expect the driver to just work the majority of times. For unique use
>>>>>> cases where the driver calculated values don't work then the user should
>>>>>> be able to override it. This should only be done for a very small
>>>>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>>>>> many users of MCAN may find that the default SSP doesn't work and must
>>>>>> always use runtime overrides to get anything to work. I don't think that
>>>>>> is a good user experience which is why I don't like the idea.
>>>>>
>>>>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>>>>> work" without doing any bittiming related setup.
>>>>
>>>> From my point of view I'd rather buy a board from a HW vendor where
>>>> CAN-FD works, rather than a board where I have to tweak the bit-timing
>>>> for a simple CAN-FD test setup.
>>>>
>>>> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
>>>> MBit/s -> 50%". Do we need an array of tuples in general?
>
> Internally what I proposed was a binding that allowed you to pass in an
> array of a range of baud rates and then a SSP for that baud rate range.
> Therefore, if the baud rate being used impacted what SSP worked then it
> allows someone to provide a range of defaults. Of course a person also
> has the ability to use a single large range thus implementing a single
> default SSP value.
A single tuple is just a special case of a list of tuples :)
I was thinking of something like this:
First we need a struct defining the bitrate spp relationship.
The driver provides default values by a sorted array of these structs
together with array length.
During device registration we assign these default values to the actual
driver instance.
The netlink code can read and overwrite the current values. Maybe it can
read the default values.
The bitrate calculation code calculates the to be used spp value.
The driver sets the value during the open callback.
>> Do we need more than one tuple here?
>>
>>>> If good default values are transceiver and board specific, they can go
>>>> into the DT. We need a generic (this means driver agnostic) binding for
>>>> this. If this table needs to be tweaked for special purpose, then we can
>>>> add a netlink interface for this as well.
>>>>
>>>> Comments?
>>>
>>> I dont know how a good default (other than 50% as the starting point)
>>> can be arrived at without doing any actual measurements on the actual
>>> network. Since we do know that the value has to be tweaked, agree that
>>> netlink interface has to be provided.
>
> Now I have seen in non public documentations that setting SP to SSP also
> works.
This can already by done now, without the need for new interfaces.
> This makes a bit more sense to me and I'm alot more comfortable going
> with this. However, since its based on non public information I can't
> justify it beyond that "it works for me". But I'm alot more
> comfortable going with then saying "hey this default value works for
> TI's dra76 evm. Therefore, every MCAN board will be stuck by default
> for a value that works for us". So if there is no push back with
> going with SSP = db SP with no documentation to back up why that is
> being used then I will try that out and send patches.
This means we postpone the whole add-new-interface-dance until the
SPP=SP approach doesn't work for some usecase?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 net-next 2/2] net: dsa: lan9303: Add fdb/mdb manipulation
From: Vivien Didelot @ 2017-10-19 14:53 UTC (permalink / raw)
To: Egil Hjelmeland, andrew, f.fainelli, netdev, linux-kernel; +Cc: Egil Hjelmeland
In-Reply-To: <20171019121015.7395-3-privat@egil-hjelmeland.no>
Hi Egil,
Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> Add functions for managing the lan9303 ALR (Address Logic
> Resolution).
>
> Implement DSA methods: port_fdb_add, port_fdb_del, port_mdb_prepare,
> port_mdb_add and port_mdb_del.
>
> Since the lan9303 do not offer reading specific ALR entry, the driver
> caches all static entries - in a flat table.
>
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
The comments I made in patch 1/2 apply here as well, please consider
them. Other than that, the patch looks good to me:
Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
^ permalink raw reply
* Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods
From: Egil Hjelmeland @ 2017-10-19 14:51 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot; +Cc: f.fainelli, netdev, linux-kernel
In-Reply-To: <20171019141505.GA14840@lunn.ch>
On 19. okt. 2017 16:15, Andrew Lunn wrote:
>>> +/* Clear learned (non-static) entry on given port */
>>> +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0,
>>> + u32 dat1, int portmap, void *ctx)
>>> +{
>>> + int *port = ctx;
>>
>> You can get the value directly to make the line below more readable:
>>
>> int port = *(int *)ctx;
>
> You have to be a bit careful with this. You often see people
> submitting patches taking away casts for void * pointers.
> If they do that here, it should at least not compile...
>
> So maybe do it in two steps?
>
> int * pport = ctx;
> int port = *pport;
>
> ???
> Andrew
>
Without cast
int port = *ctx;
... I can not compile. Neither arm-v5te-linux-gnueabi-gcc 4.7.2 or
native 64bit gcc 6.3.0.
So I feel it is safe to do as Vivien suggest.
Egil
^ permalink raw reply
* Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods
From: Egil Hjelmeland @ 2017-10-19 14:46 UTC (permalink / raw)
To: Vivien Didelot, andrew, f.fainelli, netdev, linux-kernel
In-Reply-To: <87mv4nw869.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
On 19. okt. 2017 15:58, Vivien Didelot wrote:
> Hi Egil,
>
Hi Vivien
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
>
>> Add DSA method port_fast_age as a step to STP support.
>>
>> Add low level functions for accessing the lan9303 ALR (Address Logic
>> Resolution).
>>
>> Added DSA method port_fdb_dump
>>
>> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
>
> The patch looks good overall, a few nitpicks though.
>
>> ---
>> drivers/net/dsa/lan9303-core.c | 159 +++++++++++++++++++++++++++++++++++++++++
>> drivers/net/dsa/lan9303.h | 2 +
>> 2 files changed, 161 insertions(+)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
>> index 09a748327fc6..ae904242b001 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -124,6 +124,21 @@
>> #define LAN9303_MAC_RX_CFG_2 0x0c01
>> #define LAN9303_MAC_TX_CFG_2 0x0c40
>> #define LAN9303_SWE_ALR_CMD 0x1800
>> +# define ALR_CMD_MAKE_ENTRY BIT(2)
>> +# define ALR_CMD_GET_FIRST BIT(1)
>> +# define ALR_CMD_GET_NEXT BIT(0)
>> +#define LAN9303_SWE_ALR_WR_DAT_0 0x1801
>> +#define LAN9303_SWE_ALR_WR_DAT_1 0x1802
>> +# define ALR_DAT1_VALID BIT(26)
>> +# define ALR_DAT1_END_OF_TABL BIT(25)
>> +# define ALR_DAT1_AGE_OVERRID BIT(25)
>> +# define ALR_DAT1_STATIC BIT(24)
>> +# define ALR_DAT1_PORT_BITOFFS 16
>> +# define ALR_DAT1_PORT_MASK (7 << ALR_DAT1_PORT_BITOFFS)
>> +#define LAN9303_SWE_ALR_RD_DAT_0 0x1805
>> +#define LAN9303_SWE_ALR_RD_DAT_1 0x1806
>> +#define LAN9303_SWE_ALR_CMD_STS 0x1808
>> +# define ALR_STS_MAKE_PEND BIT(0)
>
> Why is there different spacing and prefix with these defines?
The extra space is to set bit definitions apart from register offsets,
a convention that is used in the file. However, agree that the
bit defs should be prefixed with LAN9303_ to be consistent with
rest of the file.
>
>> #define LAN9303_SWE_VLAN_CMD 0x180b
>> # define LAN9303_SWE_VLAN_CMD_RNW BIT(5)
>> # define LAN9303_SWE_VLAN_CMD_PVIDNVLAN BIT(4)
>> @@ -478,6 +493,125 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
>> return 0;
>> }
>>
>> +/* ----------------- Address Logic Resolution (ALR)------------------*/
>> +
>> +/* Map ALR-port bits to port bitmap, and back*/
>
> The (leading and trailing) spacing in your comments is often
> inconsistent. You can use simple inline or block comments, no need for
> fancy rules. Please refer to Documentation/networking/netdev-FAQ.txt for
> block comment style in netdev though, they are a bit different.
>
I can add an trailing space to the second comment.
The first comment is sort of "section" comment, so I wanted it to be
separate. But perhaps I should drop these "section" comments, see
further below.
>> +static const int alrport_2_portmap[] = {1, 2, 4, 0, 3, 5, 6, 7 };
>> +static const int portmap_2_alrport[] = {3, 0, 1, 4, 2, 5, 6, 7 };
>> +
>> +/* ALR: Actual register access functions */
>> +
>> +/* This function will wait a while until mask & reg == value */
>> +/* Otherwise, return timeout */
>
> Same, a single block comment will do the job.
>
See "section comment" comments...
>> +static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno,
>> + int mask, char value)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < 0x1000; i++) {
>> + u32 reg;
>> +
>> + lan9303_read_switch_reg(chip, regno, ®);
>> + if ((reg & mask) == value)
>> + return 0;
>> + usleep_range(1000, 2000);
>> + }
>> + return -ETIMEDOUT;
>> +}
>> +
>> +static int lan9303_alr_make_entry_raw(struct lan9303 *chip, u32 dat0, u32 dat1)
>> +{
>> + lan9303_write_switch_reg(
>> + chip, LAN9303_SWE_ALR_WR_DAT_0, dat0);
>> + lan9303_write_switch_reg(
>> + chip, LAN9303_SWE_ALR_WR_DAT_1, dat1);
>> + lan9303_write_switch_reg(
>> + chip, LAN9303_SWE_ALR_CMD, ALR_CMD_MAKE_ENTRY);
>> + lan9303_csr_reg_wait(
>> + chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND, 0);
>
> As I said in a previous series, please don't do this. Function arguments
> must be vertically aligned with the opening parenthesis.
>
Will fix
>> + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
>> + return 0;
>
> A newline before a return statement is appreciated.
>
OK
>> +}
>> +
>> +typedef void alr_loop_cb_t(struct lan9303 *chip, u32 dat0, u32 dat1,
>> + int portmap, void *ctx);
>> +
>> +static void lan9303_alr_loop(struct lan9303 *chip, alr_loop_cb_t *cb, void *ctx)
>> +{
>> + int i;
>> +
>> + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_FIRST);
>> + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
>> +
>> + for (i = 1; i < LAN9303_NUM_ALR_RECORDS; i++) {
>> + u32 dat0, dat1;
>> + int alrport, portmap;
>> +
>> + lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_0, &dat0);
>> + lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, &dat1);
>> + if (dat1 & ALR_DAT1_END_OF_TABL)
>> + break;
>> +
>> + alrport = (dat1 & ALR_DAT1_PORT_MASK) >> ALR_DAT1_PORT_BITOFFS;
>> + portmap = alrport_2_portmap[alrport];
>> +
>> + cb(chip, dat0, dat1, portmap, ctx);
>> +
>> + lan9303_write_switch_reg(
>> + chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_NEXT);
>
> Please align arguments with the opening parenthesis.
>
ok
>> + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
>> + }
>> +}
>> +
>> +/* ALR: lan9303_alr_loop callback functions */
>> +
> > No need for an extra newline if your comment refers directly to the
> function below. It will also be consistent with the rest of your patch.
>
The comment refer to several functions...
>> +static void alr_reg_to_mac(u32 dat0, u32 dat1, u8 mac[6])
>> +{
>> + mac[0] = (dat0 >> 0) & 0xff;
>> + mac[1] = (dat0 >> 8) & 0xff;
>> + mac[2] = (dat0 >> 16) & 0xff;
>> + mac[3] = (dat0 >> 24) & 0xff;
>> + mac[4] = (dat1 >> 0) & 0xff;
>> + mac[5] = (dat1 >> 8) & 0xff;
>> +}
>> +
>> +/* Clear learned (non-static) entry on given port */
>> +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0,
>> + u32 dat1, int portmap, void *ctx)
>> +{
>> + int *port = ctx;
>
> You can get the value directly to make the line below more readable:
>
> int port = *(int *)ctx;
>
Agree
>> +
>> + if (((BIT(*port) & portmap) == 0) || (dat1 & ALR_DAT1_STATIC))
>> + return;
>> +
>> + /* learned entries has only one port, we can just delete */
>> + dat1 &= ~ALR_DAT1_VALID; /* delete entry */
>> + lan9303_alr_make_entry_raw(chip, dat0, dat1);
>> +}
>> +
>> +struct port_fdb_dump_ctx {
>> + int port;
>> + void *data;
>> + dsa_fdb_dump_cb_t *cb;
>> +};
>> +
>> +static void alr_loop_cb_fdb_port_dump(struct lan9303 *chip, u32 dat0,
>> + u32 dat1, int portmap, void *ctx)
>> +{
>> + struct port_fdb_dump_ctx *dump_ctx = ctx;
>> + u8 mac[ETH_ALEN];
>> + bool is_static;
>> +
>> + if ((BIT(dump_ctx->port) & portmap) == 0)
>> + return;
>> +
>> + alr_reg_to_mac(dat0, dat1, mac);
>> + is_static = !!(dat1 & ALR_DAT1_STATIC);
>> + dump_ctx->cb(mac, 0, is_static, dump_ctx->data);
>> +}
>> +
>> +/* --------------------- Various chip setup ----------------------*/
>> +
>
> This isn't a very useful comment, at least use an inline or block
> comment if you want to keep it.
>
I put it to signify the end of the ALR section. But could not think of a
better text... Perhaps "End of ALR functions" would be better?
Or perhaps I should just drop the "section comments"? After all the
functions in question has "alr" in their names.
>> static int lan9303_disable_processing_port(struct lan9303 *chip,
>> unsigned int port)
>> {
>> @@ -923,6 +1057,29 @@ static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
>> /* else: touching SWE_PORT_STATE would break port separation */
>> }
>>
>> +static void lan9303_port_fast_age(struct dsa_switch *ds, int port)
>> +{
>> + struct lan9303 *chip = ds->priv;
>> +
>> + dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
>> + lan9303_alr_loop(chip, alr_loop_cb_del_port_learned, &port);
>> +}section
>> +
>> +static int lan9303_port_fdb_dump(struct dsa_switch *ds, int port,
>> + dsa_fdb_dump_cb_t *cb, void *data)
>> +{
>> + struct lan9303 *chip = ds->priv;
>> + struct port_fdb_dump_ctx dump_ctx = {
>> + .port = port,
>> + .data = data,
>> + .cb = cb,
>> + };
>> +
>> + dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
>> + lan9303_alr_loop(chip, alr_loop_cb_fdb_port_dump, &dump_ctx);
>> + return 0;
>
> A newline before the return statement is welcome.
>
ok
>> +}
>> +
>> static const struct dsa_switch_ops lan9303_switch_ops = {
>> .get_tag_protocol = lan9303_get_tag_protocol,
>> .setup = lan9303_setup,
>> @@ -937,6 +1094,8 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
>> .port_bridge_join = lan9303_port_bridge_join,
>> .port_bridge_leave = lan9303_port_bridge_leave,
>> .port_stp_state_set = lan9303_port_stp_state_set,
>> + .port_fast_age = lan9303_port_fast_age,
>> + .port_fdb_dump = lan9303_port_fdb_dump,
>> };
>>
>> static int lan9303_register_switch(struct lan9303 *chip)
>> diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
>> index 68ecd544b658..4db323d65741 100644
>> --- a/drivers/net/dsa/lan9303.h
>> +++ b/drivers/net/dsa/lan9303.h
>> @@ -11,6 +11,8 @@ struct lan9303_phy_ops {
>> int regnum, u16 val);
>> };
>>
>> +#define LAN9303_NUM_ALR_RECORDS 512
>> +
>> struct lan9303 {
>> struct device *dev;
>> struct regmap *regmap;
>> --
>> 2.11.0
>
Thanks
Egil
^ permalink raw reply
* [PATCH net-next] bnxt: Move generic devlink code to new file
From: Steve Lin @ 2017-10-19 14:45 UTC (permalink / raw)
To: netdev; +Cc: davem, michael.chan, gospo, Steve Lin
Moving generic devlink code (registration) out of VF-R code
into new bnxt_devlink file, in preparation for future work
to add additional devlink functionality to bnxt.
Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/Makefile | 2 +-
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 1 +
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 65 +++++++++++++++++++++++
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 39 ++++++++++++++
drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 53 ++----------------
drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h | 37 ++-----------
6 files changed, 112 insertions(+), 85 deletions(-)
create mode 100644 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
create mode 100644 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
diff --git a/drivers/net/ethernet/broadcom/bnxt/Makefile b/drivers/net/ethernet/broadcom/bnxt/Makefile
index 457201f..59c8ec9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/Makefile
+++ b/drivers/net/ethernet/broadcom/bnxt/Makefile
@@ -1,4 +1,4 @@
obj-$(CONFIG_BNXT) += bnxt_en.o
-bnxt_en-y := bnxt.o bnxt_sriov.o bnxt_ethtool.o bnxt_dcb.o bnxt_ulp.o bnxt_xdp.o bnxt_vfr.o
+bnxt_en-y := bnxt.o bnxt_sriov.o bnxt_ethtool.o bnxt_dcb.o bnxt_ulp.o bnxt_xdp.o bnxt_vfr.o bnxt_devlink.o
bnxt_en-$(CONFIG_BNXT_FLOWER_OFFLOAD) += bnxt_tc.o
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 5ba4993..52cc38d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -61,6 +61,7 @@
#include "bnxt_xdp.h"
#include "bnxt_vfr.h"
#include "bnxt_tc.h"
+#include "bnxt_devlink.h"
#define BNXT_TX_TIMEOUT (5 * HZ)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
new file mode 100644
index 0000000..f3f6aa8
--- /dev/null
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -0,0 +1,65 @@
+/* Broadcom NetXtreme-C/E network driver.
+ *
+ * Copyright (c) 2017 Broadcom Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/pci.h>
+#include <linux/netdevice.h>
+#include "bnxt_hsi.h"
+#include "bnxt.h"
+#include "bnxt_vfr.h"
+#include "bnxt_devlink.h"
+
+static const struct devlink_ops bnxt_dl_ops = {
+#ifdef CONFIG_BNXT_SRIOV
+ .eswitch_mode_set = bnxt_dl_eswitch_mode_set,
+ .eswitch_mode_get = bnxt_dl_eswitch_mode_get,
+#endif /* CONFIG_BNXT_SRIOV */
+};
+
+int bnxt_dl_register(struct bnxt *bp)
+{
+ struct devlink *dl;
+ int rc;
+
+ if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
+ return 0;
+
+ if (bp->hwrm_spec_code < 0x10800) {
+ netdev_warn(bp->dev, "Firmware does not support SR-IOV E-Switch SWITCHDEV mode.\n");
+ return -ENOTSUPP;
+ }
+
+ dl = devlink_alloc(&bnxt_dl_ops, sizeof(struct bnxt_dl));
+ if (!dl) {
+ netdev_warn(bp->dev, "devlink_alloc failed");
+ return -ENOMEM;
+ }
+
+ bnxt_link_bp_to_dl(bp, dl);
+ bp->eswitch_mode = DEVLINK_ESWITCH_MODE_LEGACY;
+ rc = devlink_register(dl, &bp->pdev->dev);
+ if (rc) {
+ bnxt_link_bp_to_dl(bp, NULL);
+ devlink_free(dl);
+ netdev_warn(bp->dev, "devlink_register failed. rc=%d", rc);
+ return rc;
+ }
+
+ return 0;
+}
+
+void bnxt_dl_unregister(struct bnxt *bp)
+{
+ struct devlink *dl = bp->dl;
+
+ if (!dl)
+ return;
+
+ devlink_unregister(dl);
+ devlink_free(dl);
+}
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
new file mode 100644
index 0000000..e92a35d
--- /dev/null
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -0,0 +1,39 @@
+/* Broadcom NetXtreme-C/E network driver.
+ *
+ * Copyright (c) 2017 Broadcom Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#ifndef BNXT_DEVLINK_H
+#define BNXT_DEVLINK_H
+
+/* Struct to hold housekeeping info needed by devlink interface */
+struct bnxt_dl {
+ struct bnxt *bp; /* back ptr to the controlling dev */
+};
+
+static inline struct bnxt *bnxt_get_bp_from_dl(struct devlink *dl)
+{
+ return ((struct bnxt_dl *)devlink_priv(dl))->bp;
+}
+
+/* To clear devlink pointer from bp, pass NULL dl */
+static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
+{
+ bp->dl = dl;
+
+ /* add a back pointer in dl to bp */
+ if (dl) {
+ struct bnxt_dl *bp_dl = devlink_priv(dl);
+
+ bp_dl->bp = bp;
+ }
+}
+
+int bnxt_dl_register(struct bnxt *bp);
+void bnxt_dl_unregister(struct bnxt *bp);
+
+#endif /* BNXT_DEVLINK_H */
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index e75db04..b76849f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -16,6 +16,7 @@
#include "bnxt_hsi.h"
#include "bnxt.h"
#include "bnxt_vfr.h"
+#include "bnxt_devlink.h"
#include "bnxt_tc.h"
#ifdef CONFIG_BNXT_SRIOV
@@ -416,7 +417,7 @@ static int bnxt_vf_reps_create(struct bnxt *bp)
}
/* Devlink related routines */
-static int bnxt_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode)
+int bnxt_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode)
{
struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
@@ -424,7 +425,7 @@ static int bnxt_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode)
return 0;
}
-static int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode)
+int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode)
{
struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
int rc = 0;
@@ -462,52 +463,4 @@ static int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode)
return rc;
}
-static const struct devlink_ops bnxt_dl_ops = {
- .eswitch_mode_set = bnxt_dl_eswitch_mode_set,
- .eswitch_mode_get = bnxt_dl_eswitch_mode_get
-};
-
-int bnxt_dl_register(struct bnxt *bp)
-{
- struct devlink *dl;
- int rc;
-
- if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
- return 0;
-
- if (bp->hwrm_spec_code < 0x10800) {
- netdev_warn(bp->dev, "Firmware does not support SR-IOV E-Switch SWITCHDEV mode.\n");
- return -ENOTSUPP;
- }
-
- dl = devlink_alloc(&bnxt_dl_ops, sizeof(struct bnxt_dl));
- if (!dl) {
- netdev_warn(bp->dev, "devlink_alloc failed");
- return -ENOMEM;
- }
-
- bnxt_link_bp_to_dl(bp, dl);
- bp->eswitch_mode = DEVLINK_ESWITCH_MODE_LEGACY;
- rc = devlink_register(dl, &bp->pdev->dev);
- if (rc) {
- bnxt_link_bp_to_dl(bp, NULL);
- devlink_free(dl);
- netdev_warn(bp->dev, "devlink_register failed. rc=%d", rc);
- return rc;
- }
-
- return 0;
-}
-
-void bnxt_dl_unregister(struct bnxt *bp)
-{
- struct devlink *dl = bp->dl;
-
- if (!dl)
- return;
-
- devlink_unregister(dl);
- devlink_free(dl);
-}
-
#endif
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h
index 7787cd24..fb06bbe 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h
@@ -14,31 +14,6 @@
#define MAX_CFA_CODE 65536
-/* Struct to hold housekeeping info needed by devlink interface */
-struct bnxt_dl {
- struct bnxt *bp; /* back ptr to the controlling dev */
-};
-
-static inline struct bnxt *bnxt_get_bp_from_dl(struct devlink *dl)
-{
- return ((struct bnxt_dl *)devlink_priv(dl))->bp;
-}
-
-/* To clear devlink pointer from bp, pass NULL dl */
-static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
-{
- bp->dl = dl;
-
- /* add a back pointer in dl to bp */
- if (dl) {
- struct bnxt_dl *bp_dl = devlink_priv(dl);
-
- bp_dl->bp = bp;
- }
-}
-
-int bnxt_dl_register(struct bnxt *bp);
-void bnxt_dl_unregister(struct bnxt *bp);
void bnxt_vf_reps_destroy(struct bnxt *bp);
void bnxt_vf_reps_close(struct bnxt *bp);
void bnxt_vf_reps_open(struct bnxt *bp);
@@ -53,16 +28,10 @@ static inline u16 bnxt_vf_rep_get_fid(struct net_device *dev)
return bp->pf.vf[vf_rep->vf_idx].fw_fid;
}
-#else
-
-static inline int bnxt_dl_register(struct bnxt *bp)
-{
- return 0;
-}
+int bnxt_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode);
+int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode);
-static inline void bnxt_dl_unregister(struct bnxt *bp)
-{
-}
+#else
static inline void bnxt_vf_reps_close(struct bnxt *bp)
{
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox