* Re: [patch net-next RFC 00/12] devlink: introduce port flavours and common phys_port_name generation
From: Andrew Lunn @ 2018-03-24 14:40 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, idosch, jakub.kicinski, mlxsw, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, saeedm, simon.horman,
pieter.jansenvanvuuren, john.hurley, dirk.vandermerwe,
alexander.h.duyck, ogerlitz, dsahern, vijaya.guvva,
satananda.burla, raghu.vatsavayi, felix.manlunas, gospo,
sathya.perla, vasundhara-v.volam, tariqt, eranbe,
jeffrey.t.kirsher
In-Reply-To: <20180324074551.GD1891@nanopsycho>
> >The hardware and mechanical engineer is free to wire switch ports to
> >the front panel however they want. That is why we put the netdev name
> >in device tree.
>
> Got it. Hmm, so I think that the port number can be made optional and
> when it is present, it would be used to generate phys_port_name. If
> not, perhaps devlink port index could be used instead.
>
> So iiuc, you don't really need phys_port_name in dsa, as it provides
> misleading names, right? Why is it implemented then?
Hi Jiri
Isn't the same true for all devices? It is not just DSA devices where
the hardware engineer is free to wire up the front panel however they
want, it can happen for any device.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 6/8] MIPS: mscc: Add switch to ocelot
From: Andrew Lunn @ 2018-03-24 14:48 UTC (permalink / raw)
To: Florian Fainelli
Cc: Alexandre Belloni, David S . Miller, Allan Nielsen,
razvan.stefanescu, po.liu, Thomas Petazzoni, netdev, devicetree,
linux-kernel, linux-mips, James Hogan
In-Reply-To: <171fb3db-70f4-4818-9390-8164fab5adca@gmail.com>
On Fri, Mar 23, 2018 at 03:11:23PM -0700, Florian Fainelli wrote:
> On 03/23/2018 03:06 PM, Andrew Lunn wrote:
> >>> That is the trade off of having a standalone MDIO bus driver. Maybe
> >>> add a phandle to the internal MDIO bus? The switch driver could then
> >>> follow the phandle, and direct connect the internal PHYs?
> >>
> >> This is more or less what patch 7 does, right?
> >
> > Patch 7 does it in DT. I'm suggesting it could be done in C. It is
> > hard wired, so there is no need to describe it in DT. Use the phandle
> > to get the mdio bus, mdiobus_get_phy(, port) to get the phydev and
> > then use phy_connect().
>
> That does not sound like a great idea. And to go back to your example
> about DSA, it is partially true, you will see some switch bindings
> defining the internal PHYs (e.g: qca8k), and most not doing it (b53,
> mv88e6xxx, etc.). In either case, this resolves to the same thing
> though. Being able to parse a phy-handle property is a lot more
> flexible, and if it does matter that the PHY truly is internal, then the
> 'phy-mode' property can help reflect that.
Hi Florian
With DSA, you can always provide a phy-handle. It is only when there
is nothing specified that the fallback case is used to map internal
PHYs to ports.
Putting internal PHYs in DT is fine, but it is a nice simplification
if it is not needed.
Andrew
^ permalink raw reply
* Re: [net-next 02/15] ethtool: Add support for configuring PFC stall prevention in ethtool
From: Andrew Lunn @ 2018-03-24 14:57 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: David S. Miller, netdev, Inbar Karmy, Michal Kubecek
In-Reply-To: <20180323223925.21678-3-saeedm@mellanox.com>
On Fri, Mar 23, 2018 at 03:39:12PM -0700, Saeed Mahameed wrote:
> From: Inbar Karmy <inbark@mellanox.com>
>
> In the event where the device unexpectedly becomes unresponsive
> for a long period of time, flow control mechanism may propagate
> pause frames which will cause congestion spreading to the entire
> network.
> To prevent this scenario, when the device is stalled for a period
> longer than a pre-configured timeout, flow control mechanisms are
> automatically disabled.
>
> This patch adds support for the ETHTOOL_PFC_STALL_PREVENTION
> as a tunable.
> This API provides support for configuring flow control storm prevention
> timeout (msec).
>
> Signed-off-by: Inbar Karmy <inbark@mellanox.com>
> Cc: Michal Kubecek <mkubecek@suse.cz>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
> include/uapi/linux/ethtool.h | 4 ++++
> net/core/ethtool.c | 6 ++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 20da156aaf64..9dc63a14a747 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -217,10 +217,14 @@ struct ethtool_value {
> __u32 data;
> };
>
> +#define PFC_STORM_PREVENTION_AUTO 0xffff
> +#define PFC_STORM_PREVENTION_DISABLE 0
> +
> enum tunable_id {
> ETHTOOL_ID_UNSPEC,
> ETHTOOL_RX_COPYBREAK,
> ETHTOOL_TX_COPYBREAK,
> + ETHTOOL_PFC_PREVENTION_TOUT,
Hi Inbar
Please could you add a comment here about the units. Ideally we want
this file to be self documenting.
Andrew
^ permalink raw reply
* Re: [PATCH v5 3/5] bnx2x: Eliminate duplicate barriers on weakly-ordered archs
From: Sinan Kaya @ 2018-03-24 14:57 UTC (permalink / raw)
To: Chopra, Manish, David Miller
Cc: netdev@vger.kernel.org, timur@codeaurora.org,
sulrich@codeaurora.org, linux-arm-msm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Elior, Ariel,
Dept-Eng Everest Linux L2, linux-kernel@vger.kernel.org
In-Reply-To: <DM2PR07MB685FD0A0877865A41EE723889AF0@DM2PR07MB685.namprd07.prod.outlook.com>
On 3/24/2018 10:30 AM, Chopra, Manish wrote:
>> -----Original Message-----
>> From: Sinan Kaya [mailto:okaya@codeaurora.org]
>> Sent: Friday, March 23, 2018 10:44 PM
>> To: David Miller <davem@davemloft.net>
>> Cc: netdev@vger.kernel.org; timur@codeaurora.org; sulrich@codeaurora.org;
>> linux-arm-msm@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Elior,
>> Ariel <Ariel.Elior@cavium.com>; Dept-Eng Everest Linux L2 <Dept-
>> EngEverestLinuxL2@cavium.com>; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v5 3/5] bnx2x: Eliminate duplicate barriers on weakly-
>> ordered archs
>>
>> On 3/23/2018 1:04 PM, David Miller wrote:
>>> From: Sinan Kaya <okaya@codeaurora.org>
>>> Date: Fri, 23 Mar 2018 12:51:47 -0400
>>>
>>>> It could if txdata->tx_db was not a union. There is a data dependency
>>>> between txdata->tx_db.data.prod and txdata->tx_db.raw.
>>>>
>>>> So, no reordering.
>>>
>>> I don't see it that way, the code requires that:
>>>
>>> txdata->tx_db.data.prod += nbd;
>>>
>>> is visible before the doorbell update.>
>>> barrier() doesn't provide that.
>>>
>>> Neither does writel_relaxed(). However plain writel() does.
>>
>> Correct for some architectures including ARM but not correct universally.
>>
>> writel() just guarantees register read/writes before and after to be ordered
>> when HW observes it.
>>
>> writel() doesn't guarantee that the memory update is visible to the HW on all
>> architectures.
>>
>> If you need memory update visibility, that barrier() should have been a
>> wmb()
>>
>> A correct multi-arch pattern is
>>
>> wmb()
>> writel_relaxed()
>> mmiowb()
>>
>
> Sinan, Since you have mentioned the use of mmiowb() here after writel_relaxed().
> I believe this is not always correct for all types of IO mapped memory [Specially if IO memory is mapped using write combined (for ex. Ioremap_wc())].
> We have a current issue on our NIC (qede) driver on x86 for which the patch is already been sent more than a week ago [Still awaiting to hear from David on that].
> where mmiowb() seems to be useless since we use write combined mapped doorbell and mmiowb() just seems to be a compiler barrier() there.
> So in order to flush write combined buffer we really need writel_relaxed() followed by a wmb() to synchronize writes among CPU cores.
> I think the correct pattern in such cases (for write combined IO) would have been like below -
>
> wmb();
> writel_relaxed();
> wmb(); -> To flush the writes actually.
You actually have good points. It is the same problem with barrier() description above.
The answer really depends on what you are doing/expecting after mmiowb(). If you expect
that some memory content to be observed by HW, you definitely need a wmb() like
you mentioned.
If you just want writes to be flushed but you don't expect any memory content to be
updated, you need a mmiowb().
https://lwn.net/Articles/198988/
"There is mmiowb(), but its real purpose is to enforce ordering between MMIO operations only."
>
> Thanks.
>
>
>
>
>
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
From: David Ahern @ 2018-03-24 15:02 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, davem, roopa, shm, jiri, idosch, David Ahern
In-Reply-To: <20180323204749.52d30129@cakuba.netronome.com>
On 3/23/18 9:47 PM, Jakub Kicinski wrote:
> On Thu, 22 Mar 2018 15:57:57 -0700, David Ahern wrote:
>> From: David Ahern <dsahern@gmail.com>
>>
>> Add devlink support to netdevsim and use it to implement a simple,
>> profile based resource controller. Only one controller is needed
>> per namespace, so the first netdevsim netdevice in a namespace
>> registers with devlink. If that device is deleted, the resource
>> settings are deleted.
>
> FWIW some nits from me blow.
>
>> diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
>> index 09388c06171d..449b2a1a1800 100644
>> --- a/drivers/net/netdevsim/Makefile
>> +++ b/drivers/net/netdevsim/Makefile
>> @@ -9,3 +9,7 @@ ifeq ($(CONFIG_BPF_SYSCALL),y)
>> netdevsim-objs += \
>> bpf.o
>> endif
>> +
>> +ifneq ($(CONFIG_NET_DEVLINK),)
>
> Hm. Don't you need MAY_USE_DEVLINK dependency perhaps?
mlxsw uses CONFIG_NET_DEVLINK in its Makefile.
MAY_USE_DEVLINK seems to only be used in Kconfig files. Not clear to me
why it is needed at all.
>
>> +netdevsim-objs += devlink.o fib.o
>> +endif
>> diff --git a/drivers/net/netdevsim/devlink.c b/drivers/net/netdevsim/devlink.c
>> new file mode 100644
>> index 000000000000..d10558e1b022
>> --- /dev/null
>> +++ b/drivers/net/netdevsim/devlink.c
>
>> +static int devlink_resources_register(struct devlink *devlink)
>> +{
>> + struct devlink_resource_size_params params = {
>> + .size_max = (u64)-1,
>> + .size_granularity = 1,
>> + .unit = DEVLINK_RESOURCE_UNIT_ENTRY
>> + };
>> + struct net *net = devlink_net(devlink);
>> + int err;
>> + u64 n;
>> +
>> + /* Resources for IPv4 */
>> + err = devlink_resource_register(devlink, "IPv4", (u64)-1,
>> + NSIM_RESOURCE_IPV4,
>> + DEVLINK_RESOURCE_ID_PARENT_TOP,
>> + ¶ms, NULL);
>> + if (err) {
>> + pr_err("Failed to register IPv4 top resource\n");
>> + goto out;
>
> nit: why goto out here and return err everywhere else? If I was to
> choose I'd rather see returns. goto X; X: return; is less
> obviously correct IMHO. Besides labels should be called by the
> action they perform/undo, so goto err_return? :S
Will fix. Just got lost in the many iterations leading up to the RFC.
>
>> + }
>> +
>> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB, true);
>> + err = devlink_resource_register(devlink, "fib", n,
>> + NSIM_RESOURCE_IPV4_FIB,
>> + NSIM_RESOURCE_IPV4,
>> + ¶ms, &nsim_ipv4_fib_res_ops);
>> + if (err) {
>> + pr_err("Failed to register IPv4 FIB resource\n");
>> + return err;
>> + }
>> +
>> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB_RULES, true);
>> + err = devlink_resource_register(devlink, "fib-rules", n,
>> + NSIM_RESOURCE_IPV4_FIB_RULES,
>> + NSIM_RESOURCE_IPV4,
>> + ¶ms, &nsim_ipv4_fib_rules_res_ops);
>> + if (err) {
>> + pr_err("Failed to register IPv4 FIB rules resource\n");
>> + return err;
>> + }
>> +
>> + /* Resources for IPv6 */
>> + err = devlink_resource_register(devlink, "IPv6", (u64)-1,
>> + NSIM_RESOURCE_IPV6,
>> + DEVLINK_RESOURCE_ID_PARENT_TOP,
>> + ¶ms, NULL);
>> + if (err) {
>> + pr_err("Failed to register IPv6 top resource\n");
>> + goto out;
>> + }
>> +
>> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB, true);
>> + err = devlink_resource_register(devlink, "fib", n,
>> + NSIM_RESOURCE_IPV6_FIB,
>> + NSIM_RESOURCE_IPV6,
>> + ¶ms, &nsim_ipv6_fib_res_ops);
>> + if (err) {
>> + pr_err("Failed to register IPv6 FIB resource\n");
>> + return err;
>> + }
>> +
>> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB_RULES, true);
>> + err = devlink_resource_register(devlink, "fib-rules", n,
>> + NSIM_RESOURCE_IPV6_FIB_RULES,
>> + NSIM_RESOURCE_IPV6,
>> + ¶ms, &nsim_ipv6_fib_rules_res_ops);
>> + if (err) {
>> + pr_err("Failed to register IPv6 FIB rules resource\n");
>> + return err;
>> + }
>> +out:
>> + return err;
>> +}
>
>> +void nsim_devlink_teardown(struct netdevsim *ns)
>> +{
>> + if (ns->devlink) {
>> + struct net *net = dev_net(ns->netdev);
>> + bool *reg_devlink = net_generic(net, nsim_devlink_id);
>
> nit: reverse xmas tree
reg_devlink uses net, so the order can not be changed
>
>> + devlink_unregister(ns->devlink);
>> + devlink_free(ns->devlink);
>> + ns->devlink = NULL;
>> +
>> + nsim_devlink_net_reset(net);
>> + *reg_devlink = true;
>> + }
>> +}
>
>> diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
>> new file mode 100644
>> index 000000000000..b77dcafc7158
>> --- /dev/null
>> +++ b/drivers/net/netdevsim/fib.c
>
>> +static int nsim_fib_event_nb(struct notifier_block *nb, unsigned long event,
>> + void *ptr)
>> +{
>> + struct fib_notifier_info *info = ptr;
>> + int err;
>> +
>> + switch (event) {
>> + case FIB_EVENT_RULE_ADD: /* fall through */
>
> nit: I don't think fall through comment is needed for back-to-back
> cases.
ok.
Thanks for the review.
>
>> + case FIB_EVENT_RULE_DEL:
>> + err = nsim_fib_rule_event(info, event == FIB_EVENT_RULE_ADD);
>> + break;
>> +
>> + case FIB_EVENT_ENTRY_ADD: /* fall through */
>> + case FIB_EVENT_ENTRY_DEL:
>> + err = nsim_fib_event(info, event == FIB_EVENT_ENTRY_ADD);
>> + break;
>> + }
>> +
>> + return notifier_from_errno(err);
>> +}
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 00/21] net/ipv6: Separate data structures for FIB and data path
From: Ido Schimmel @ 2018-03-24 15:05 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, roopa, eric.dumazet, weiwan, kafai, yoshfuji
In-Reply-To: <20180319033622.16693-1-dsahern@gmail.com>
On Sun, Mar 18, 2018 at 08:36:01PM -0700, David Ahern wrote:
> This set is the first of many changes to improve the scalability of the
> IPv6 code. Follow on changes include:
> - consolidating duplicate fib6_info references like IPv4 does with
> duplicate fib_info
Before that you need to further break fib6_info into fib6_info and
fib6_alias, or I misunderstood you?
^ permalink raw reply
* Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
From: David Ahern @ 2018-03-24 15:05 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, roopa, shm, jiri, idosch, jakub.kicinski,
David Ahern
In-Reply-To: <20180324072621.GA1891@nanopsycho>
On 3/24/18 1:26 AM, Jiri Pirko wrote:
> Fri, Mar 23, 2018 at 04:13:14PM CET, dsa@cumulusnetworks.com wrote:
>> On 3/23/18 9:05 AM, Jiri Pirko wrote:
>>> Fri, Mar 23, 2018 at 04:03:40PM CET, dsa@cumulusnetworks.com wrote:
>>>> On 3/23/18 9:01 AM, Jiri Pirko wrote:
>>>>> Fri, Mar 23, 2018 at 03:31:02PM CET, dsa@cumulusnetworks.com wrote:
>>>>>> On 3/23/18 12:50 AM, Jiri Pirko wrote:
>>>>>>>> +void nsim_devlink_setup(struct netdevsim *ns)
>>>>>>>> +{
>>>>>>>> + struct net *net = dev_net(ns->netdev);
>>>>>>>> + bool *reg_devlink = net_generic(net, nsim_devlink_id);
>>>>>>>> + struct devlink *devlink;
>>>>>>>> + int err = -ENOMEM;
>>>>>>>> +
>>>>>>>> + /* only one device per namespace controls devlink */
>>>>>>>> + if (!*reg_devlink) {
>>>>>>>> + ns->devlink = NULL;
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + devlink = devlink_alloc(&nsim_devlink_ops, 0);
>>>>>>>> + if (!devlink)
>>>>>>>> + return;
>>>>>>>> +
>>>>>>>> + devlink_net_set(devlink, net);
>>>>>>>> + err = devlink_register(devlink, &ns->dev);
>>>>>>>
>>>>>>> This reg_devlink construct looks odd. Why don't you leave the devlink
>>>>>>> instance in init_ns?
>>>>>>
>>>>>> It is a per-network namespace resource controller. Since struct devlink
>>>>>
>>>>> Wait a second. What do you mean by "per-network namespace"? Devlink
>>>>> instance is always associated with one physical device. Like an ASIC.
>>>>>
>>>>>
>>>>>> has a net entry, the simplest design is to put it into the namespace of
>>>>>> the controller. Without it, controlling resource sizes in namespace
>>>>>> 'foobar' has to be done from init_net, which is just wrong.
>>>>
>>>> you need to look at how netdevsim creates a device per netdevice.
>>>
>>> That means one devlink instance for each netdevsim device, doesn't it?
>>>
>>
>> yes.
>
> Still not sure how to handle namespaces in devlink. Originally, I
> thought it would be okay to leave all devlink instances in init_ns.
> Because what happens if you move netdev to another namespace? Should the
> devlink move as well? What if you have multiple ports, each in different
> namespace. Can user move devlink instance to another namespace? Etc.
>
The devlink instance is associated with a 'struct device' and those do
not change namespaces AFAIK.
^ permalink raw reply
* Re: [net-next 03/15] net/mlx5e: PFC stall prevention support
From: Andrew Lunn @ 2018-03-24 15:07 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: David S. Miller, netdev, Inbar Karmy
In-Reply-To: <20180323223925.21678-4-saeedm@mellanox.com>
On Fri, Mar 23, 2018 at 03:39:13PM -0700, Saeed Mahameed wrote:
> From: Inbar Karmy <inbark@mellanox.com>
>
> Implement set/get functions to configure PFC stall prevention
> timeout by tunables api through ethtool.
> By default the stall prevention timeout is configured to 8 sec.
> Timeout range is: 80-8000 msec.
> Enabling stall prevention without a specific timeout will set
> the timeout to 100 msec.
Hi Inbar
I think this last sentence should be talking about auto, not without a
specific timeout.
>
> Signed-off-by: Inbar Karmy <inbark@mellanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 57 +++++++++++++++++++
> drivers/net/ethernet/mellanox/mlx5/core/port.c | 64 +++++++++++++++++++---
> include/linux/mlx5/mlx5_ifc.h | 17 ++++--
> include/linux/mlx5/port.h | 6 ++
> 4 files changed, 132 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index cc8048f68f11..62061fd23143 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -1066,6 +1066,57 @@ static int mlx5e_get_rxnfc(struct net_device *netdev,
> return err;
> }
>
> +#define MLX5E_PFC_PREVEN_AUTO_TOUT_MSEC 100
> +#define MLX5E_PFC_PREVEN_TOUT_MAX_MSEC 8000
> +#define MLX5E_PFC_PREVEN_MINOR_PRECENT 85
> +#define MLX5E_PFC_PREVEN_TOUT_MIN_MSEC 80
> +#define MLX5E_DEVICE_STALL_MINOR_WATERMARK(critical_tout) \
> + max_t(u16, MLX5E_PFC_PREVEN_TOUT_MIN_MSEC, \
> + (critical_tout * MLX5E_PFC_PREVEN_MINOR_PRECENT) / 100)
> +
> +static int mlx5e_get_pfc_prevention_tout(struct net_device *netdev,
> + u16 *pfc_prevention_tout)
> +{
> + struct mlx5e_priv *priv = netdev_priv(netdev);
> + struct mlx5_core_dev *mdev = priv->mdev;
> +
> + if (!MLX5_CAP_PCAM_FEATURE((priv)->mdev, pfcc_mask) ||
> + !MLX5_CAP_DEBUG((priv)->mdev, stall_detect))
> + return -EOPNOTSUPP;
> +
> + return mlx5_query_port_stall_watermark(mdev, pfc_prevention_tout, NULL);
Shouldn't you map a value of MLX5E_PFC_PREVEN_AUTO_TOUT_MSEC back to
PFC_STORM_PREVENTION_AUTO?
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] cxgb4: collect hardware dump in second kernel
From: Andrew Lunn @ 2018-03-24 15:18 UTC (permalink / raw)
To: Rahul Lakkireddy
Cc: netdev, linux-fsdevel, kexec, linux-kernel, davem, viro, ebiederm,
stephen, akpm, torvalds, ganeshgr, nirranjan, indranil
In-Reply-To: <f06cca21650affa10d83a28dbd7fb577792e35a9.1521888444.git.rahul.lakkireddy@chelsio.com>
On Sat, Mar 24, 2018 at 04:26:34PM +0530, Rahul Lakkireddy wrote:
> Register callback to collect hardware/firmware dumps in second kernel
> before hardware/firmware is initialized. The dumps for each device
> will be available under /sys/kernel/crashdd/cxgb4/ directory in second
> kernel.
>
> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> Signed-off-by: Ganesh Goudar <ganeshgr@chelsio.com>
Thanks for adding generic documentation about the files in sysfs.
However, you don't add any specific documentation here about the cxgb4
crash dump. How am i supposed to interpret it? Does it follow any of
the standard core dump formats? How do i use gdb with it? Are there
some specific tools i should use to analyse it?
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 0/2] kernel: add support to collect hardware logs in crash recovery kernel
From: Eric W. Biederman @ 2018-03-24 15:20 UTC (permalink / raw)
To: Rahul Lakkireddy
Cc: netdev, linux-fsdevel, kexec, linux-kernel, davem, viro, stephen,
akpm, torvalds, ganeshgr, nirranjan, indranil
In-Reply-To: <cover.1521888444.git.rahul.lakkireddy@chelsio.com>
Rahul Lakkireddy <rahul.lakkireddy@chelsio.com> writes:
> On production servers running variety of workloads over time, kernel
> panic can happen sporadically after days or even months. It is
> important to collect as much debug logs as possible to root cause
> and fix the problem, that may not be easy to reproduce. Snapshot of
> underlying hardware/firmware state (like register dump, firmware
> logs, adapter memory, etc.), at the time of kernel panic will be very
> helpful while debugging the culprit device driver.
>
> This series of patches add new generic framework that enable device
> drivers to collect device specific snapshot of the hardware/firmware
> state of the underlying device in the crash recovery kernel. In crash
> recovery kernel, the collected logs are exposed via /sys/kernel/crashdd/
> directory, which is copied by user space scripts for post-analysis.
>
> A kernel module crashdd is newly added. In crash recovery kernel,
> crashdd exposes /sys/kernel/crashdd/ directory containing device
> specific hardware/firmware logs.
Have you looked at instead of adding a sysfs file adding the dumps
as additional elf notes in /proc/vmcore?
That should allow existing tools to capture your extended dump
information with no code changes, and it will allow having a single file
core dump for storing the information.
Both of which should mean something that will integrate better into
existing flows.
The interface logic of the driver should be essentially the same.
Also have you tested this and seen how well your current logic captures
the device information?
>
> The sequence of actions done by device drivers to append their device
> specific hardware/firmware logs to /sys/kernel/crashdd/ directory are
> as follows:
>
> 1. During probe (before hardware is initialized), device drivers
> register to the crashdd module (via crashdd_add_dump()), with
> callback function, along with buffer size and log name needed for
> firmware/hardware log collection.
>
> 2. Crashdd creates a driver's directory under /sys/kernel/crashdd/<driver>.
> Then, it allocates the buffer with requested size and invokes the
> device driver's registered callback function.
>
> 3. Device driver collects all hardware/firmware logs into the buffer
> and returns control back to crashdd.
>
> 4. Crashdd exposes the buffer as a file via
> /sys/kernel/crashdd/<driver>/<dump_file>.
>
> 5. User space script (/usr/lib/kdump/kdump-lib-initramfs.sh) copies
> the entire /sys/kernel/crashdd/ directory to /var/crash/ directory.
>
> Patch 1 adds crashdd module to allow drivers to register callback to
> collect the device specific hardware/firmware logs. The module also
> exports /sys/kernel/crashdd/ directory containing the hardware/firmware
> logs.
>
> Patch 2 shows a cxgb4 driver example using the API to collect
> hardware/firmware logs in crash recovery kernel, before hardware is
> initialized. The logs for the devices are made available under
> /sys/kernel/crashdd/cxgb4/ directory.
>
> Thanks,
> Rahul
>
> RFC v1: https://lkml.org/lkml/2018/3/2/542
> RFC v2: https://lkml.org/lkml/2018/3/16/326
>
> ---
> v2:
> - Added ABI Documentation for crashdd.
> - Directly use octal permission instead of macro.
>
> Changes since rfc v2:
> - Moved exporting crashdd from procfs to sysfs. Suggested by
> Stephen Hemminger <stephen@networkplumber.org>
> - Moved code from fs/proc/crashdd.c to fs/crashdd/ directory.
> - Replaced all proc API with sysfs API and updated comments.
> - Calling driver callback before creating the binary file under
> crashdd sysfs.
> - Changed binary dump file permission from S_IRUSR to S_IRUGO.
> - Changed module name from CRASH_DRIVER_DUMP to CRASH_DEVICE_DUMP.
>
> rfc v2:
> - Collecting logs in 2nd kernel instead of during kernel panic.
> Suggested by Eric Biederman <ebiederm@xmission.com>.
> - Added new crashdd module that exports /proc/crashdd/ containing
> driver's registered hardware/firmware logs in patch 1.
> - Replaced the API to allow drivers to register their hardware/firmware
> log collect routine in crash recovery kernel in patch 1.
> - Updated patch 2 to use the new API in patch 1.
>
>
> Rahul Lakkireddy (2):
> fs/crashdd: add API to collect hardware dump in second kernel
> cxgb4: collect hardware dump in second kernel
>
> Documentation/ABI/testing/sysfs-kernel-crashdd | 34 ++++
> drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 4 +
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_cudbg.c | 25 +++
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_cudbg.h | 3 +
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 12 ++
> fs/Kconfig | 1 +
> fs/Makefile | 1 +
> fs/crashdd/Kconfig | 10 +
> fs/crashdd/Makefile | 3 +
> fs/crashdd/crashdd.c | 233 +++++++++++++++++++++++
> fs/crashdd/crashdd_internal.h | 24 +++
> include/linux/crashdd.h | 24 +++
> 12 files changed, 374 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-kernel-crashdd
> create mode 100644 fs/crashdd/Kconfig
> create mode 100644 fs/crashdd/Makefile
> create mode 100644 fs/crashdd/crashdd.c
> create mode 100644 fs/crashdd/crashdd_internal.h
> create mode 100644 include/linux/crashdd.h
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 00/21] net/ipv6: Separate data structures for FIB and data path
From: David Ahern @ 2018-03-24 15:28 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, davem, roopa, eric.dumazet, weiwan, kafai, yoshfuji
In-Reply-To: <20180324150516.GA8940@splinter>
On 3/24/18 9:05 AM, Ido Schimmel wrote:
> On Sun, Mar 18, 2018 at 08:36:01PM -0700, David Ahern wrote:
>> This set is the first of many changes to improve the scalability of the
>> IPv6 code. Follow on changes include:
>> - consolidating duplicate fib6_info references like IPv4 does with
>> duplicate fib_info
>
> Before that you need to further break fib6_info into fib6_info and
> fib6_alias, or I misunderstood you?
>
I was only listing the high level intent - to consolidate duplication. I
have not looked into that step yet, but I suspect that split can be done
in the same patch set as consolidating duplicate fib6_info.
As you know, my preference is to move to nexthop objects (makes fib6_nh
optional). I have IPv4 done; IPv6 requires this patch set. To get there
the patch sequence is:
1. separate data structures for fib and dst paths (this set),
2. cleanup set on top of this one (separate set only because this one is
at 21 patches), and
3. nexthop API.
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 19/21] net/ipv6: separate handling of FIB entries from dst based routes
From: David Ahern @ 2018-03-24 15:31 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, davem, roopa, eric.dumazet, weiwan, kafai, yoshfuji
In-Reply-To: <20180324143100.GA19895@splinter>
On 3/24/18 8:31 AM, Ido Schimmel wrote:
> On Sun, Mar 18, 2018 at 08:36:20PM -0700, David Ahern wrote:
>> @@ -405,18 +383,9 @@ static void ip6_dst_destroy(struct dst_entry *dst)
>> rt->rt6i_idev = NULL;
>> in6_dev_put(idev);
>> }
>> - bucket = rcu_dereference_protected(rt->rt6i_exception_bucket, 1);
>> - if (bucket) {
>> - rt->rt6i_exception_bucket = NULL;
>> - kfree(bucket);
>> - }
>> -
>> - m = rt->fib6_metrics;
>> - if (m != &dst_default_metrics && refcount_dec_and_test(&m->refcnt))
>> - kfree(m);
>
> You remove this...
>
>>
>> rt->from = NULL;
>> - dst_release(&from->dst);
>> + fib6_info_release(from);
>
> Yet fib6_info_release() doesn't take care of it (unlike the IPv4
> equivalent), which means you're leaking the metrics.
>
>> }
ok, I'll take a look. I thought I verified both paths (fib6_info and
dst) were freeing the metrics.
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 00/21] net/ipv6: Separate data structures for FIB and data path
From: Ido Schimmel @ 2018-03-24 15:59 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, roopa, eric.dumazet, weiwan, kafai, yoshfuji
In-Reply-To: <aee428c3-9c13-3cb8-0c3f-c69a7345e562@gmail.com>
On Sat, Mar 24, 2018 at 09:28:01AM -0600, David Ahern wrote:
> On 3/24/18 9:05 AM, Ido Schimmel wrote:
> > On Sun, Mar 18, 2018 at 08:36:01PM -0700, David Ahern wrote:
> >> This set is the first of many changes to improve the scalability of the
> >> IPv6 code. Follow on changes include:
> >> - consolidating duplicate fib6_info references like IPv4 does with
> >> duplicate fib_info
> >
> > Before that you need to further break fib6_info into fib6_info and
> > fib6_alias, or I misunderstood you?
> >
>
> I was only listing the high level intent - to consolidate duplication. I
> have not looked into that step yet, but I suspect that split can be done
> in the same patch set as consolidating duplicate fib6_info.
I don't think you can perform consolidation of fib6_info as long as it
describes both the route and the nexthop info.
> As you know, my preference is to move to nexthop objects (makes fib6_nh
> optional). I have IPv4 done; IPv6 requires this patch set.
After going over your presentation [1] I was under the impression that
the fib6_info will be optional, not fib6_nh: "Idea is similar to adding
id to fib_info that is exposed to userspace. Subsequent routes pass id
to avoid fib_info overhead".
But I think misunderstood you. You want to introduce the nexthop API
that will allow you to have multiple fib6_info pointing to the same
fib6_nh?
1. http://vger.kernel.org/netconf2017_files/nexthop-objects.pdf
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 19/21] net/ipv6: separate handling of FIB entries from dst based routes
From: Ido Schimmel @ 2018-03-24 16:02 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, roopa, eric.dumazet, weiwan, kafai, yoshfuji
In-Reply-To: <b5912ae8-0495-3df9-177d-ddd83d5248aa@gmail.com>
On Sat, Mar 24, 2018 at 09:31:02AM -0600, David Ahern wrote:
> On 3/24/18 8:31 AM, Ido Schimmel wrote:
> > On Sun, Mar 18, 2018 at 08:36:20PM -0700, David Ahern wrote:
> >> @@ -405,18 +383,9 @@ static void ip6_dst_destroy(struct dst_entry *dst)
> >> rt->rt6i_idev = NULL;
> >> in6_dev_put(idev);
> >> }
> >> - bucket = rcu_dereference_protected(rt->rt6i_exception_bucket, 1);
> >> - if (bucket) {
> >> - rt->rt6i_exception_bucket = NULL;
> >> - kfree(bucket);
> >> - }
> >> -
> >> - m = rt->fib6_metrics;
> >> - if (m != &dst_default_metrics && refcount_dec_and_test(&m->refcnt))
> >> - kfree(m);
> >
> > You remove this...
> >
> >>
> >> rt->from = NULL;
> >> - dst_release(&from->dst);
> >> + fib6_info_release(from);
> >
> > Yet fib6_info_release() doesn't take care of it (unlike the IPv4
> > equivalent), which means you're leaking the metrics.
> >
> >> }
>
> ok, I'll take a look. I thought I verified both paths (fib6_info and
> dst) were freeing the metrics.
I get this from kmemleak (applied your patchset on top of fe2d55d295cf):
unreferenced object 0xffff88004e2c16c8 (size 96):
comm "systemd-network", pid 1255, jiffies 4295166424 (age 957.858s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
ip6_route_info_create (/net/ipv6/route.c:2849)
ip6_route_add (/net/ipv6/route.c:2975)
inet6_rtm_newroute (/net/ipv6/route.c:4357)
rtnetlink_rcv_msg (/net/core/rtnetlink.c:4643)
netlink_rcv_skb (/net/netlink/af_netlink.c:2445)
netlink_unicast (/net/netlink/af_netlink.c:1309 /net/netlink/af_netlink.c:1334)
netlink_sendmsg (/net/netlink/af_netlink.c:1897)
sock_sendmsg (/net/socket.c:630 /net/socket.c:639)
SYSC_sendto (/net/socket.c:1748)
do_syscall_64 (/arch/x86/entry/common.c:287)
entry_SYSCALL_64_after_hwframe (/arch/x86/entry/entry_64.S:239)
0xffffffffffffffff (/./include/asm-generic/sections.h:42)
^ permalink raw reply
* Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
From: Jiri Pirko @ 2018-03-24 16:02 UTC (permalink / raw)
To: David Ahern
Cc: netdev, davem, roopa, shm, jiri, idosch, jakub.kicinski,
David Ahern
In-Reply-To: <2eadcfb9-6dd2-d533-7ae3-c9299a70a172@cumulusnetworks.com>
Sat, Mar 24, 2018 at 04:05:38PM CET, dsa@cumulusnetworks.com wrote:
>On 3/24/18 1:26 AM, Jiri Pirko wrote:
>> Fri, Mar 23, 2018 at 04:13:14PM CET, dsa@cumulusnetworks.com wrote:
>>> On 3/23/18 9:05 AM, Jiri Pirko wrote:
>>>> Fri, Mar 23, 2018 at 04:03:40PM CET, dsa@cumulusnetworks.com wrote:
>>>>> On 3/23/18 9:01 AM, Jiri Pirko wrote:
>>>>>> Fri, Mar 23, 2018 at 03:31:02PM CET, dsa@cumulusnetworks.com wrote:
>>>>>>> On 3/23/18 12:50 AM, Jiri Pirko wrote:
>>>>>>>>> +void nsim_devlink_setup(struct netdevsim *ns)
>>>>>>>>> +{
>>>>>>>>> + struct net *net = dev_net(ns->netdev);
>>>>>>>>> + bool *reg_devlink = net_generic(net, nsim_devlink_id);
>>>>>>>>> + struct devlink *devlink;
>>>>>>>>> + int err = -ENOMEM;
>>>>>>>>> +
>>>>>>>>> + /* only one device per namespace controls devlink */
>>>>>>>>> + if (!*reg_devlink) {
>>>>>>>>> + ns->devlink = NULL;
>>>>>>>>> + return;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + devlink = devlink_alloc(&nsim_devlink_ops, 0);
>>>>>>>>> + if (!devlink)
>>>>>>>>> + return;
>>>>>>>>> +
>>>>>>>>> + devlink_net_set(devlink, net);
>>>>>>>>> + err = devlink_register(devlink, &ns->dev);
>>>>>>>>
>>>>>>>> This reg_devlink construct looks odd. Why don't you leave the devlink
>>>>>>>> instance in init_ns?
>>>>>>>
>>>>>>> It is a per-network namespace resource controller. Since struct devlink
>>>>>>
>>>>>> Wait a second. What do you mean by "per-network namespace"? Devlink
>>>>>> instance is always associated with one physical device. Like an ASIC.
>>>>>>
>>>>>>
>>>>>>> has a net entry, the simplest design is to put it into the namespace of
>>>>>>> the controller. Without it, controlling resource sizes in namespace
>>>>>>> 'foobar' has to be done from init_net, which is just wrong.
>>>>>
>>>>> you need to look at how netdevsim creates a device per netdevice.
>>>>
>>>> That means one devlink instance for each netdevsim device, doesn't it?
>>>>
>>>
>>> yes.
>>
>> Still not sure how to handle namespaces in devlink. Originally, I
>> thought it would be okay to leave all devlink instances in init_ns.
>> Because what happens if you move netdev to another namespace? Should the
>> devlink move as well? What if you have multiple ports, each in different
>> namespace. Can user move devlink instance to another namespace? Etc.
>>
>
>The devlink instance is associated with a 'struct device' and those do
>not change namespaces AFAIK.
Yeah. But you put devlink instance into namespace according to struct
net_device. That is mismatch.
^ permalink raw reply
* Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper
From: Mike Looijmans @ 2018-03-24 16:03 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, linux-kernel, devicetree, f.fainelli, robh+dt,
frowand.list
In-Reply-To: <20180323194259.GP24361@lunn.ch>
On 23-03-18 20:42, Andrew Lunn wrote:
>> Indeed. I'll add my settings as an example. Where should I put this
>> documentation, in the commit comment or somewhere in
>> Documents/devicetree/bindings?
>
> Documention/devicetree/bindings/net/ethernet.txt
Ok
>> It's what I intended to do, but there were two problems with that:
>> - of_get_mac_address() returns a pointer to constant data in memory, but the
>> nvmem functions return an allocated memory object that must be freed after
>> use. This changes the way the call is to be made.
>> - The nvmem functions need the "struct device" pointer as well, while
>> of_get_mac_address() only gets passed the DT node.
>
> Does of_nvmem_cell_get() help?
Yes, looking at the interface, it would.
--
Mike Looijmans
^ permalink raw reply
* Re: [patch net-next RFC 00/12] devlink: introduce port flavours and common phys_port_name generation
From: Jiri Pirko @ 2018-03-24 16:04 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, davem, idosch, jakub.kicinski, mlxsw, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, saeedm, simon.horman,
pieter.jansenvanvuuren, john.hurley, dirk.vandermerwe,
alexander.h.duyck, ogerlitz, dsahern, vijaya.guvva,
satananda.burla, raghu.vatsavayi, felix.manlunas, gospo,
sathya.perla, vasundhara-v.volam, tariqt, eranbe,
jeffrey.t.kirsher
In-Reply-To: <20180324144002.GA31941@lunn.ch>
Sat, Mar 24, 2018 at 03:40:02PM CET, andrew@lunn.ch wrote:
>> >The hardware and mechanical engineer is free to wire switch ports to
>> >the front panel however they want. That is why we put the netdev name
>> >in device tree.
>>
>> Got it. Hmm, so I think that the port number can be made optional and
>> when it is present, it would be used to generate phys_port_name. If
>> not, perhaps devlink port index could be used instead.
>>
>> So iiuc, you don't really need phys_port_name in dsa, as it provides
>> misleading names, right? Why is it implemented then?
>
>Hi Jiri
>
>Isn't the same true for all devices? It is not just DSA devices where
>the hardware engineer is free to wire up the front panel however they
>want, it can happen for any device.
In mlxsw, driver queries the FW to get this info.
^ permalink raw reply
* Re: [patch net-next RFC 00/12] devlink: introduce port flavours and common phys_port_name generation
From: Jiri Pirko @ 2018-03-24 16:07 UTC (permalink / raw)
To: Florian Fainelli
Cc: Andrew Lunn, netdev, davem, idosch, jakub.kicinski, mlxsw,
vivien.didelot, michael.chan, ganeshgr, saeedm, simon.horman,
pieter.jansenvanvuuren, john.hurley, dirk.vandermerwe,
alexander.h.duyck, ogerlitz, dsahern, vijaya.guvva,
satananda.burla, raghu.vatsavayi, felix.manlunas, gospo,
sathya.perla, vasundhara-v.volam, tariqt, eranbe,
jeffrey.t.kirsher
In-Reply-To: <0B8BC202-9067-4D34-AC4F-79DC33B4E6F9@gmail.com>
Sat, Mar 24, 2018 at 03:35:09PM CET, f.fainelli@gmail.com wrote:
>On March 24, 2018 12:45:51 AM PDT, Jiri Pirko <jiri@resnulli.us> wrote:
>>Fri, Mar 23, 2018 at 04:24:12PM CET, andrew@lunn.ch wrote:
>>>On Fri, Mar 23, 2018 at 03:59:35PM +0100, Jiri Pirko wrote:
>>>> Fri, Mar 23, 2018 at 02:43:57PM CET, andrew@lunn.ch wrote:
>>>> >> I tested this for mlxsw and nfp. I have no way to test this on
>>DSA hw,
>>>> >> I would really appretiate DSA guys to test this.
>>>> >
>>>> >Hi Jiri
>>>> >
>>>> >With the missing break added, i get:
>>>> >
>>>> >root@zii-devel-b:~# ./iproute2/devlink/devlink port
>>>> >mdio_bus/0.1:00/0: type eth netdev lan0 flavour physical number 0
>>>> >mdio_bus/0.1:00/1: type eth netdev lan1 flavour physical number 1
>>>> >mdio_bus/0.1:00/2: type eth netdev lan2 flavour physical number 2
>>>> >mdio_bus/0.1:00/3: type notset
>>>> >mdio_bus/0.1:00/4: type notset
>>>> >mdio_bus/0.1:00/5: type notset flavour dsa number 5
>>>> >mdio_bus/0.1:00/6: type notset flavour cpu number 6
>>>> >mdio_bus/0.2:00/0: type eth netdev lan3 flavour physical number 0
>>>> >mdio_bus/0.2:00/1: type eth netdev lan4 flavour physical number 1
>>>> >mdio_bus/0.2:00/2: type eth netdev lan5 flavour physical number 2
>>>> >mdio_bus/0.2:00/3: type notset
>>>> >mdio_bus/0.2:00/4: type notset
>>>> >mdio_bus/0.2:00/5: type notset flavour dsa number 5
>>>> >mdio_bus/0.2:00/6: type notset flavour dsa number 6
>>>> >mdio_bus/0.4:00/0: type eth netdev lan6 flavour physical number 0
>>>> >mdio_bus/0.4:00/1: type eth netdev lan7 flavour physical number 1
>>>> >mdio_bus/0.4:00/2: type eth netdev lan8 flavour physical number 2
>>>> >mdio_bus/0.4:00/3: type eth netdev optical3 flavour physical number
>>3
>>>> >mdio_bus/0.4:00/4: type eth netdev optical4 flavour physical number
>>4
>>>> >mdio_bus/0.4:00/5: type notset
>>>> >mdio_bus/0.4:00/6: type notset
>>>> >mdio_bus/0.4:00/7: type notset
>>>> >mdio_bus/0.4:00/8: type notset
>>>> >mdio_bus/0.4:00/9: type notset flavour dsa number 9
>>>
>>>> That is basically front panel number for physical ports.
>>>
>>>You cannot make that assumption. As you can see here, we have 3 ports
>>>with the number 0.
>>>
>>>Look at clearfog, armada-388-clearfog.dts. port 0=lan5, port 1=lan4
>>>port 2=lan3, port 3=lan2, port 4=lan1, port 5=cpu, port 6=lan6.
>>>
>>>The hardware and mechanical engineer is free to wire switch ports to
>>>the front panel however they want. That is why we put the netdev name
>>>in device tree.
>>
>>Got it. Hmm, so I think that the port number can be made optional and
>>when it is present, it would be used to generate phys_port_name. If
>>not, perhaps devlink port index could be used instead.
>>
>>So iiuc, you don't really need phys_port_name in dsa, as it provides
>>misleading names, right? Why is it implemented then?
>
>We do need phys_port_name because there are switch configuration operations, e.g: ethtool::rxnfc which take a port number and queue number as part of the action to redirect a packet for instance. There is no way to obtain this physical port number other than either knowing it and hard coding it (not great) or scanning the device tree and look for the "reg" property value. phys_port_name gets you that and it is easy for an application to scan /sys/class/net/ on startup to get to know that (and other stuff as well).
Hmm. That sounds like misuse of phys_port_name. The original purpose was
to provide names as they are actually written on the front panel.
>
>--
>Florian
^ permalink raw reply
* switchdev and dsa
From: Ran Shalit @ 2018-03-24 16:12 UTC (permalink / raw)
To: netdev
Hello,
I am new with switchdev and dsa.
I would please like to ask if configuring a device tree/board file
with a switch (for example marvell switch) will provide all ports
behins the switch as valid interface (ethX) ports in userspace
ifconfig command ?
Thank you,
ranran
^ permalink raw reply
* Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper
From: Mike Looijmans @ 2018-03-24 16:17 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn
Cc: netdev, linux-kernel, devicetree, robh+dt, frowand.list
In-Reply-To: <886c94ef-e2c8-2afa-307d-1924c74c6dc0@gmail.com>
On 23-03-18 20:33, Florian Fainelli wrote:
> On 03/23/2018 12:20 PM, Mike Looijmans wrote:
>> On 23-3-2018 16:11, Andrew Lunn wrote:
>>> On Fri, Mar 23, 2018 at 03:24:34PM +0100, Mike Looijmans wrote:
>>>> It's common practice to store MAC addresses for network interfaces into
>>>> nvmem devices. However the code to actually do this in the kernel lacks,
>>>> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
>>>> address from an nvmem cell provider.
>>>>
>>>> This is particulary useful on devices where the ethernet interface
>>>> cannot
>>>> be configured by the bootloader, for example because it's in an FPGA.
>>>>
>>>> Tested by adapting the cadence macb driver to call this instead of
>>>> of_get_mac_address().
>>>
>>> Hi Mike
>>>
>>> Please can you document the device tree binding. I assume you are
>>> adding a nvmen-cells and nvmem-cell-names to the Ethernet node in
>>> device tree.
>>
>> Indeed. I'll add my settings as an example. Where should I put this
>> documentation, in the commit comment or somewhere in
>> Documents/devicetree/bindings?
>>
>>>> +/**
>>>> + * Search the device tree for a MAC address, by calling
>>>> of_get_mac_address
>>>> + * and if that doesn't provide an address, fetch it from an nvmem
>>>> provider
>>>> + * using the name 'mac-address'.
>>>> + * On success, copies the new address is into memory pointed to by
>>>> addr and
>>>> + * returns 0. Returns a negative error code otherwise.
>>>> + * @dev: Pointer to the device containing the device_node
>>>> + * @addr: Pointer to receive the MAC address using ether_addr_copy()
>>>> + */
>>>> +int of_get_nvmem_mac_address(struct device *dev, char *addr)
>>>> +{
>>>> + const char *mac;
>>>> + struct nvmem_cell *cell;
>>>> + size_t len;
>>>> + int ret;
>>>> +
>>>> + mac = of_get_mac_address(dev->of_node);
>>>> + if (mac) {
>>>> + ether_addr_copy(addr, mac);
>>>> + return 0;
>>>> + }
>>>
>>> Is there a need to add a new API? Could of_get_mac_address() be
>>> extended to look in NVMEM? The MAC driver does not care. It is saying,
>>> using OF get me a MAC address. One API seems sufficient, and would
>>> mean you don't need to change the MAC drivers.
>>
>> It's what I intended to do, but there were two problems with that:
>> - of_get_mac_address() returns a pointer to constant data in memory, but
>> the nvmem functions return an allocated memory object that must be freed
>> after use. This changes the way the call is to be made.
>
> Yeah...
>
>> - The nvmem functions need the "struct device" pointer as well, while
>> of_get_mac_address() only gets passed the DT node.
>
> Bummer, you can't assume there is always a struct device associated with
> a struct device_node. Also, bigger question is, how can we make this
> work, for e.g: ACPI systems and therefore use an abstract fw_node handle?
>
Andrew Lunn's suggestion of using "of_nvmem_cell_get()" should solve this.
>> One approach would be to deprecate the of_get_mac_address() interface
>> and migrate existing drivers to the of_get_nvmem_mac_address() interface.
>
> Humm maybe, but clearly making of_get_mac_address() look for a nvmem is
> less error prone and does not require people to opt-in for the new
> helper, that seems beneficial to me.
Totally agree. But I can't think of a way that doesn't change the
method's signature. At some point the allocated nvmem buffer must be freed.
A quick survey for the of_get_mac_address users learns that most of them
do a memcpy (or similar) right after it, so for these drivers the
"of_get_nvmem_mac_address" style signature that performs the memcpy (or
better, ether_addr_copy) is a better fit, e.g.:
int of_get_mac_address(struct device_node *np, void *addr)
--
Mike Looijmans
^ permalink raw reply
* RE: [PATCH net-next,1/2] hv_netvsc: Fix the return status in RX path
From: Michael Kelley (EOSG) @ 2018-03-24 16:48 UTC (permalink / raw)
To: Haiyang Zhang, davem@davemloft.net, netdev@vger.kernel.org
Cc: KY Srinivasan, Stephen Hemminger, olaf@aepfle.de,
vkuznets@redhat.com, devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20180322190114.25596-2-haiyangz@linuxonhyperv.com>
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf
> Of Haiyang Zhang
> Sent: Thursday, March 22, 2018 12:01 PM
> To: davem@davemloft.net; netdev@vger.kernel.org
> Cc: Haiyang Zhang <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets@redhat.com;
> devel@linuxdriverproject.org; linux-kernel@vger.kernel.org
> Subject: [PATCH net-next,1/2] hv_netvsc: Fix the return status in RX path
>
> From: Haiyang Zhang <haiyangz@microsoft.com>
>
> As defined in hyperv_net.h, the NVSP_STAT_SUCCESS is one not zero.
> Some functions returns 0 when it actually means NVSP_STAT_SUCCESS.
> This patch fixes them.
>
> In netvsc_receive(), it puts the last RNDIS packet's receive status
> for all packets in a vmxferpage which may contain multiple RNDIS
> packets.
> This patch puts NVSP_STAT_FAIL in the receive completion if one of
> the packets in a vmxferpage fails.
This patch changes the status field that is being reported back to
the Hyper-V host in the receive completion message in
enq_receive_complete(). The current code reports 0 on success,
and with the patch, it will report 1 on success. So does this change
affect anything on the Hyper-V side? Or is Hyper-V just ignoring
the value? If this change doesn't have any impact on the
interactions with Hyper-V, perhaps it would be good to explain
why in the commit message.
Michael
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/net/hyperv/netvsc.c | 8 ++++++--
> drivers/net/hyperv/netvsc_drv.c | 2 +-
> drivers/net/hyperv/rndis_filter.c | 4 ++--
> 3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index aa95e81af6e5..1ddb2c39b6e4 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -1098,12 +1098,16 @@ static int netvsc_receive(struct net_device *ndev,
> void *data = recv_buf
> + vmxferpage_packet->ranges[i].byte_offset;
> u32 buflen = vmxferpage_packet->ranges[i].byte_count;
> + int ret;
>
> trace_rndis_recv(ndev, q_idx, data);
>
> /* Pass it to the upper layer */
> - status = rndis_filter_receive(ndev, net_device,
> - channel, data, buflen);
> + ret = rndis_filter_receive(ndev, net_device,
> + channel, data, buflen);
> +
> + if (unlikely(ret != NVSP_STAT_SUCCESS))
> + status = NVSP_STAT_FAIL;
> }
>
> enq_receive_complete(ndev, net_device, q_idx,
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index cdb78eefab67..33607995be62 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -818,7 +818,7 @@ int netvsc_recv_callback(struct net_device *net,
> u64_stats_update_end(&rx_stats->syncp);
>
> napi_gro_receive(&nvchan->napi, skb);
> - return 0;
> + return NVSP_STAT_SUCCESS;
> }
>
> static void netvsc_get_drvinfo(struct net_device *net,
> diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
> index 2dc00f714482..591fb8080f11 100644
> --- a/drivers/net/hyperv/rndis_filter.c
> +++ b/drivers/net/hyperv/rndis_filter.c
> @@ -443,10 +443,10 @@ int rndis_filter_receive(struct net_device *ndev,
> "unhandled rndis message (type %u len %u)\n",
> rndis_msg->ndis_msg_type,
> rndis_msg->msg_len);
> - break;
> + return NVSP_STAT_FAIL;
> }
>
> - return 0;
> + return NVSP_STAT_SUCCESS;
> }
>
> static int rndis_filter_query_device(struct rndis_device *dev,
> --
> 2.15.1
^ permalink raw reply
* Re: [PATCH net-next RFC V1 2/5] net: phy: Move time stamping interface into the generic mdio layer.
From: Richard Cochran @ 2018-03-24 16:59 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, devicetree, Andrew Lunn, David Miller, Mark Rutland,
Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20180321214513.5kkw4qpbhp3juxur@localhost>
On Wed, Mar 21, 2018 at 02:45:13PM -0700, Richard Cochran wrote:
> On Wed, Mar 21, 2018 at 12:10:07PM -0700, Florian Fainelli wrote:
> > > + phydev->mdio.ts_info = dp83640_ts_info;
> > > + phydev->mdio.hwtstamp = dp83640_hwtstamp;
> > > + phydev->mdio.rxtstamp = dp83640_rxtstamp;
> > > + phydev->mdio.txtstamp = dp83640_txtstamp;
> >
> > Why is this implemented a the mdio_device level and not at the
> > mdio_driver level? This looks like the wrong level at which this is done.
>
> The question could be asked of:
>
> struct mdio_device {
> int (*bus_match)(struct device *dev, struct device_driver *drv);
> void (*device_free)(struct mdio_device *mdiodev);
> void (*device_remove)(struct mdio_device *mdiodev);
> }
>
> I saw how this is done for the phy, etc, but I don't see any benefit
> of doing it that way. It would add an extra layer (or two) of
> indirection and save the space four pointer functions. Is that
> trade-off worth it?
Actually, there was another reason not to put these methods into the
driver structure. In contrast to phy_device, mdio_device doesn't have
a pointer to the driver structure. It looks like there is no way to
start from a mdio_device and get to a mdio_driver. Please correct me
if I'm wrong.
I propose keeping those methods in the mdio_device, wrapping them in
#ifdef CONFIG_NETWORK_PHY_TIMESTAMPING. That way, they do not add any
burden to the vast majority of users.
Thoughts?
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH net-next RFC V1 3/5] net: Introduce field for the MII time stamper.
From: Richard Cochran @ 2018-03-24 17:01 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, devicetree, Andrew Lunn, David Miller, Mark Rutland,
Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <cbab700a-8a4f-8c4a-b959-e356de006f6e@gmail.com>
On Wed, Mar 21, 2018 at 12:12:00PM -0700, Florian Fainelli wrote:
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 5fbb9f1da7fd..223d691aa0b0 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1943,6 +1943,7 @@ struct net_device {
> > struct netprio_map __rcu *priomap;
> > #endif
> > struct phy_device *phydev;
> > + struct mdio_device *mdiots;
>
> phy_device embedds a mdio_device, can you find a way to rework the PHY
> PTP code to utilize the phy_device's mdio instance so do not introduce
> yet another pointer in that big structure that net_device already is?
You are right in that this field is wasted space for most users.
In V2 this will be inside #ifdef CONFIG_NETWORK_PHY_TIMESTAMPING.
Thanks,
Richard
^ permalink raw reply
* Re: [patch net-next RFC 00/12] devlink: introduce port flavours and common phys_port_name generation
From: Florian Fainelli @ 2018-03-24 17:07 UTC (permalink / raw)
To: Jiri Pirko
Cc: Andrew Lunn, netdev, davem, idosch, jakub.kicinski, mlxsw,
vivien.didelot, michael.chan, ganeshgr, saeedm, simon.horman,
pieter.jansenvanvuuren, john.hurley, dirk.vandermerwe,
alexander.h.duyck, ogerlitz, dsahern, vijaya.guvva,
satananda.burla, raghu.vatsavayi, felix.manlunas, gospo,
sathya.perla, vasundhara-v.volam, tariqt, eranbe,
jeffrey.t.kirsher
In-Reply-To: <20180324160749.GG1891@nanopsycho>
On March 24, 2018 9:07:49 AM PDT, Jiri Pirko <jiri@resnulli.us> wrote:
>Sat, Mar 24, 2018 at 03:35:09PM CET, f.fainelli@gmail.com wrote:
>>On March 24, 2018 12:45:51 AM PDT, Jiri Pirko <jiri@resnulli.us>
>wrote:
>>>Fri, Mar 23, 2018 at 04:24:12PM CET, andrew@lunn.ch wrote:
>>>>On Fri, Mar 23, 2018 at 03:59:35PM +0100, Jiri Pirko wrote:
>>>>> Fri, Mar 23, 2018 at 02:43:57PM CET, andrew@lunn.ch wrote:
>>>>> >> I tested this for mlxsw and nfp. I have no way to test this on
>>>DSA hw,
>>>>> >> I would really appretiate DSA guys to test this.
>>>>> >
>>>>> >Hi Jiri
>>>>> >
>>>>> >With the missing break added, i get:
>>>>> >
>>>>> >root@zii-devel-b:~# ./iproute2/devlink/devlink port
>>>>> >mdio_bus/0.1:00/0: type eth netdev lan0 flavour physical number 0
>>>>> >mdio_bus/0.1:00/1: type eth netdev lan1 flavour physical number 1
>>>>> >mdio_bus/0.1:00/2: type eth netdev lan2 flavour physical number 2
>>>>> >mdio_bus/0.1:00/3: type notset
>>>>> >mdio_bus/0.1:00/4: type notset
>>>>> >mdio_bus/0.1:00/5: type notset flavour dsa number 5
>>>>> >mdio_bus/0.1:00/6: type notset flavour cpu number 6
>>>>> >mdio_bus/0.2:00/0: type eth netdev lan3 flavour physical number 0
>>>>> >mdio_bus/0.2:00/1: type eth netdev lan4 flavour physical number 1
>>>>> >mdio_bus/0.2:00/2: type eth netdev lan5 flavour physical number 2
>>>>> >mdio_bus/0.2:00/3: type notset
>>>>> >mdio_bus/0.2:00/4: type notset
>>>>> >mdio_bus/0.2:00/5: type notset flavour dsa number 5
>>>>> >mdio_bus/0.2:00/6: type notset flavour dsa number 6
>>>>> >mdio_bus/0.4:00/0: type eth netdev lan6 flavour physical number 0
>>>>> >mdio_bus/0.4:00/1: type eth netdev lan7 flavour physical number 1
>>>>> >mdio_bus/0.4:00/2: type eth netdev lan8 flavour physical number 2
>>>>> >mdio_bus/0.4:00/3: type eth netdev optical3 flavour physical
>number
>>>3
>>>>> >mdio_bus/0.4:00/4: type eth netdev optical4 flavour physical
>number
>>>4
>>>>> >mdio_bus/0.4:00/5: type notset
>>>>> >mdio_bus/0.4:00/6: type notset
>>>>> >mdio_bus/0.4:00/7: type notset
>>>>> >mdio_bus/0.4:00/8: type notset
>>>>> >mdio_bus/0.4:00/9: type notset flavour dsa number 9
>>>>
>>>>> That is basically front panel number for physical ports.
>>>>
>>>>You cannot make that assumption. As you can see here, we have 3
>ports
>>>>with the number 0.
>>>>
>>>>Look at clearfog, armada-388-clearfog.dts. port 0=lan5, port 1=lan4
>>>>port 2=lan3, port 3=lan2, port 4=lan1, port 5=cpu, port 6=lan6.
>>>>
>>>>The hardware and mechanical engineer is free to wire switch ports to
>>>>the front panel however they want. That is why we put the netdev
>name
>>>>in device tree.
>>>
>>>Got it. Hmm, so I think that the port number can be made optional and
>>>when it is present, it would be used to generate phys_port_name. If
>>>not, perhaps devlink port index could be used instead.
>>>
>>>So iiuc, you don't really need phys_port_name in dsa, as it provides
>>>misleading names, right? Why is it implemented then?
>>
>>We do need phys_port_name because there are switch configuration
>operations, e.g: ethtool::rxnfc which take a port number and queue
>number as part of the action to redirect a packet for instance. There
>is no way to obtain this physical port number other than either knowing
>it and hard coding it (not great) or scanning the device tree and look
>for the "reg" property value. phys_port_name gets you that and it is
>easy for an application to scan /sys/class/net/ on startup to get to
>know that (and other stuff as well).
>
>Hmm. That sounds like misuse of phys_port_name. The original purpose
>was
>to provide names as they are actually written on the front panel.
Ok, if we look back at the history of the changes I had implemented ndo_phys_port_id() to return the port number initially, but this was wrong and reverted based on your feedback, and then ndo_phys_port_name() was implemented with your reviewed-by tag:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/dsa/slave.c?id=3a543ef479868e36c95935de320608a7e41466ca
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/dsa/slave.c?id=592050b2541407d033da18226d3644644832d082
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/dsa/slave.c?id=592050b2541407d033da18226d3644644832d082
Now that this is reported through sysfs it unfortunately becomes ABI and we should not be breaking user applications relying on that and there is at least one I know of...
What is an appropriate attribute to use to return the physical port number within a given switch?
--
Florian
^ permalink raw reply
* Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.
From: Richard Cochran @ 2018-03-24 17:12 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, devicetree, David Miller, Florian Fainelli, Mark Rutland,
Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20180321235007.GA28402@lunn.ch>
On Thu, Mar 22, 2018 at 12:50:07AM +0100, Andrew Lunn wrote:
> How clever is this device? Can it tell the difference between
> 1000Base-X and SGMII? Can it figure out that the MAC is repeating
> every bit 100 times and so has dropped to 10Mbits? Does it understand
> EEE? Does it need to know if RGMII or RGMII-ID is being used?
This device isn't configurable at run time for any of those AFAICT.
Those decisions are made when the IP core is synthesized as part of
the HW design.
> Can such a device really operation without the MAC being involved? My
> feeling is it needs to understand how the MII bus is being used. It
> might also be that the device is less capable than the MAC, so you
> need to turn off some of the MAC features. I think you are going to
> need the MAC actively involved in this.
You are right in that this particular device *does* need to know the
link speed. I have neglected that part for this RFC. I'm looking for
a notification based method of informing the device of link speed
changes, but without hacking any MAC driver.
In general, we might see devices one day that care about things like
EEE for example, but let's cross that bridge when we come to it. In
the case of EEE, when the user enables it via ethtool we can tell the
time stamping device directly without hacking the MAC driver.
Thanks,
Richard
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox