Re: [PATCH] net/mlx5: fix kfree mismatch in indir_table.c
On Mon, 2021-04-05 at 07:56 +0300, Leon Romanovsky wrote:
> On Mon, Apr 05, 2021 at 10:53:39AM +0800, Xiaoming Ni wrote:
> > Memory allocated by kvzalloc() should be freed by kvfree().
> >
> > Fixes: 34ca65352ddf2 ("net/mlx5: E-Switch, Indirect table
> > infrastructur")
> > Signed-off-by: Xiaoming Ni
> > ---
> > .../net/ethernet/mellanox/mlx5/core/esw/indir_table.c | 10 +-
> >
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
>
> Thanks,
> Reviewed-by: Leon Romanovsky
Applied to net-mlx5.
Thanks,
Saeed.
Re: [PATCH][next] net/mlx5: Fix spelling mistakes in mlx5_core_info message
On Mon, 2021-03-15 at 12:30 +, Colin King wrote: > From: Colin Ian King > > There are two spelling mistakes in a mlx5_core_info message. Fix > them. > > Signed-off-by: Colin Ian King > --- > drivers/net/ethernet/mellanox/mlx5/core/health.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/health.c > b/drivers/net/ethernet/mellanox/mlx5/core/health.c > index a0a851640804..9ff163c5bcde 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/health.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/health.c > @@ -340,7 +340,7 @@ static int mlx5_health_try_recover(struct > mlx5_core_dev *dev) > return -EIO; > } > > - mlx5_core_info(dev, "health revovery succeded\n"); > + mlx5_core_info(dev, "health recovery succeeded\n"); > return 0; > } > Applied to net-next-mlx5, sorry for the delay.
Re: [PATCH] net/mlx5: use kvfree() for memory allocated with kvzalloc()
On Wed, 2021-03-03 at 09:54 +0200, Roi Dayan wrote:
>
>
> On 2021-03-03 4:40 AM, angkery wrote:
> > From: Junlin Yang
> >
> > It is allocated with kvzalloc(), the corresponding release function
> > should not be kfree(), use kvfree() instead.
> >
> > Generated by: scripts/coccinelle/api/kfree_mismatch.cocci
> >
> > Signed-off-by: Junlin Yang
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/esw/indir_table.c | 10
> > +-
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git
> > a/drivers/net/ethernet/mellanox/mlx5/core/esw/indir_table.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/esw/indir_table.c
> > index 6f6772b..3da7bec 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/esw/indir_table.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/indir_table.c
> > @@ -248,7 +248,7 @@ static int mlx5_esw_indir_table_rule_get(struct
> > mlx5_eswitch *esw,
> > err_ethertype:
> > kfree(rule);
> > out:
> > - kfree(rule_spec);
> > + kvfree(rule_spec);
> > return err;
> > }
> >
> > @@ -328,7 +328,7 @@ static int
> > mlx5_create_indir_recirc_group(struct mlx5_eswitch *esw,
> > e->recirc_cnt = 0;
> >
> > out:
> > - kfree(in);
> > + kvfree(in);
> > return err;
> > }
> >
> > @@ -347,7 +347,7 @@ static int mlx5_create_indir_fwd_group(struct
> > mlx5_eswitch *esw,
> >
> > spec = kvzalloc(sizeof(*spec), GFP_KERNEL);
> > if (!spec) {
> > - kfree(in);
> > + kvfree(in);
> > return -ENOMEM;
> > }
> >
> > @@ -371,8 +371,8 @@ static int mlx5_create_indir_fwd_group(struct
> > mlx5_eswitch *esw,
> > }
> >
> > err_out:
> > - kfree(spec);
> > - kfree(in);
> > + kvfree(spec);
> > + kvfree(in);
> > return err;
> > }
> >
> >
>
> thanks!
>
> Reviewed-by: Roi Dayan
>
applied to net-next-mlx5
Re: [PATCH] net/mlx5: remove unneeded semicolon
On Wed, 2021-03-03 at 08:52 +, Parav Pandit wrote: > Hi Saeed, > > > From: Parav Pandit > > Sent: Monday, February 22, 2021 3:32 PM > > > > > > > From: Jiapeng Chong > > > Sent: Monday, February 22, 2021 3:27 PM > > > > > > Fix the following coccicheck warnings: > > > > > > ./drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c:495:2-3: > > > Unneeded semicolon. > > > > > > Reported-by: Abaci Robot > > > Signed-off-by: Jiapeng Chong > > > --- > > > drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > > > b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > > > index c2ba41b..60a6328 100644 > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > > > @@ -492,7 +492,7 @@ static int mlx5_sf_esw_event(struct > > > notifier_block > > > *nb, unsigned long event, voi > > > break; > > > default: > > > break; > > > - }; > > > + } > > > > > > return 0; > > > } > > > -- > > > 1.8.3.1 > > > > Reviewed-by: Parav Pandit > > Will you take this patch [1] to your queue? > > [1] > https://lore.kernel.org/linux-rdma/1613987819-43161-1-git-send-email-jiapeng.ch...@linux.alibaba.com/ Applied to net-next-mlx5. Thanks, Saeed.
Re: [PATCH] net: mellanox: mlx5: fix error return code in mlx5_fpga_device_start()
On Sun, 2021-03-07 at 10:50 +0200, Leon Romanovsky wrote:
> On Thu, Mar 04, 2021 at 06:18:14AM -0800, Jia-Ju Bai wrote:
> > When mlx5_is_fpga_lookaside() returns a non-zero value, no error
> > return code is assigned.
> > To fix this bug, err is assigned with -EINVAL as error return code.
> >
> > Reported-by: TOTE Robot
> > Signed-off-by: Jia-Ju Bai
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Like Heiner said, the current code has correct behavior.
> The mlx5_fpga_device_load_check() has same mlx5_is_fpga_lookaside()
> check and it is not an error if it returns true.
>
> NAK: Leon Romanovsky
>
> Thanks
Agreed, apparently this robot is looking for "goto {out|*err*}"
statements and treats all of them as errors, this is very unreliable,
Re: [PATCH] net/mlx5e: allocate 'indirection_rqt' buffer dynamically
On Mon, 2021-03-08 at 18:28 +0200, Tariq Toukan wrote:
>
>
> On 3/8/2021 5:32 PM, Arnd Bergmann wrote:
> > From: Arnd Bergmann
> >
> > Increasing the size of the indirection_rqt array from 128 to 256
> > bytes
> > pushed the stack usage of the mlx5e_hairpin_fill_rqt_rqns()
> > function
> > over the warning limit when building with clang and CONFIG_KASAN:
> >
> > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:970:1: error: stack
> > frame size of 1180 bytes in function 'mlx5e_tc_add_nic_flow' [-
> > Werror,-Wframe-larger-than=]
> >
> > Using dynamic allocation here is safe because the caller does the
> > same, and it reduces the stack usage of the function to just a few
> > bytes.
> >
> > Fixes: 1dd55ba2fb70 ("net/mlx5e: Increase indirection RQ table size
> > to 256")
> > Signed-off-by: Arnd Bergmann
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 16
> > +---
> > 1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > index 0da69b98f38f..66f98618dc13 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > @@ -445,12 +445,16 @@ static void
> > mlx5e_hairpin_destroy_transport(struct mlx5e_hairpin *hp)
> > mlx5_core_dealloc_transport_domain(hp->func_mdev, hp->tdn);
> > }
> >
> > -static void mlx5e_hairpin_fill_rqt_rqns(struct mlx5e_hairpin *hp,
> > void *rqtc)
> > +static int mlx5e_hairpin_fill_rqt_rqns(struct mlx5e_hairpin *hp,
> > void *rqtc)
> > {
> > - u32 indirection_rqt[MLX5E_INDIR_RQT_SIZE], rqn;
> > + u32 *indirection_rqt, rqn;
> > struct mlx5e_priv *priv = hp->func_priv;
> > int i, ix, sz = MLX5E_INDIR_RQT_SIZE;
> >
> > + indirection_rqt = kzalloc(sz, GFP_KERNEL);
> > + if (!indirection_rqt)
> > + return -ENOMEM;
> > +
> > mlx5e_build_default_indir_rqt(indirection_rqt, sz,
> > hp->num_channels);
> >
> > @@ -462,6 +466,9 @@ static void mlx5e_hairpin_fill_rqt_rqns(struct
> > mlx5e_hairpin *hp, void *rqtc)
> > rqn = hp->pair->rqn[ix];
> > MLX5_SET(rqtc, rqtc, rq_num[i], rqn);
> > }
> > +
> > + kfree(indirection_rqt);
> > + return 0;
> > }
> >
> > static int mlx5e_hairpin_create_indirect_rqt(struct mlx5e_hairpin
> > *hp)
> > @@ -482,12 +489,15 @@ static int
> > mlx5e_hairpin_create_indirect_rqt(struct mlx5e_hairpin *hp)
> > MLX5_SET(rqtc, rqtc, rqt_actual_size, sz);
> > MLX5_SET(rqtc, rqtc, rqt_max_size, sz);
> >
> > - mlx5e_hairpin_fill_rqt_rqns(hp, rqtc);
> > + err = mlx5e_hairpin_fill_rqt_rqns(hp, rqtc);
> > + if (err)
> > + goto out;
> >
> > err = mlx5_core_create_rqt(mdev, in, inlen,
> > >indir_rqt.rqtn);
> > if (!err)
> > hp->indir_rqt.enabled = true;
> >
> > +out:
> > kvfree(in);
> > return err;
> > }
> >
>
> Reviewed-by: Tariq Toukan
> Thanks for your patch.
>
> Tariq
Applied to net-next-mlx5
Thanks!
Re: [PATCH] net/mlx5e: include net/nexthop.h where needed
On Mon, 2021-03-08 at 20:23 +0200, Roi Dayan wrote:
>
>
> On 2021-03-08 5:31 PM, Arnd Bergmann wrote:
> > From: Arnd Bergmann
> >
> > drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c:1510:12:
> > error: implicit declaration of function 'fib_info_nh' [-Werror,-
> > Wimplicit-function-declaration]
> > fib_dev = fib_info_nh(fen_info->fi, 0)->fib_nh_dev;
> > ^
> > drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c:1510:12:
> > note: did you mean 'fib_info_put'?
> > include/net/ip_fib.h:529:20: note: 'fib_info_put' declared here
> > static inline void fib_info_put(struct fib_info *fi)
> > ^
> >
> > Fixes: 8914add2c9e5 ("net/mlx5e: Handle FIB events to update tunnel
> > endpoint device")
> > Signed-off-by: Arnd Bergmann
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git
> > a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
> > index 6a116335bb21..32d06fe94acc 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
> > @@ -2,6 +2,7 @@
> > /* Copyright (c) 2021 Mellanox Technologies. */
> >
> > #include
> > +#include
> > #include "tc_tun_encap.h"
> > #include "en_tc.h"
> > #include "tc_tun.h"
> >
>
> Hi,
>
> I see internally we have a pending commit from Vlad fixing this
> already,
> with few more fixes. "net/mlx5e: Add missing include"
>
> I'll check why it's being held.
Just submitted to net-next.
Thanks!
Re: [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower()
On Tue, 2021-03-09 at 11:44 +0200, Roi Dayan wrote:
>
>
> On 2021-03-09 10:32 AM, Jia-Ju Bai wrote:
> >
> >
> > On 2021/3/9 16:24, Roi Dayan wrote:
> > >
> > >
> > > On 2021-03-09 10:20 AM, Roi Dayan wrote:
> > > >
> > > >
> > > > On 2021-03-06 3:47 PM, Jia-Ju Bai wrote:
> > > > > When mlx5e_tc_get_counter() returns NULL to counter or
> > > > > mlx5_devcom_get_peer_data() returns NULL to peer_esw, no
> > > > > error return
> > > > > code of mlx5e_stats_flower() is assigned.
> > > > > To fix this bug, err is assigned with -EINVAL in these cases.
> > > > >
> > > > > Reported-by: TOTE Robot
Hey Jia-Ju, What are the conditions for this robot to raise a flag?
sometimes it is totally normal to abort a function and return 0.. i am
just curious to know ?
> > > > > Signed-off-by: Jia-Ju Bai
> > > > > ---
> > > > > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12
> > > > > +---
> > > > > 1 file changed, 9 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > > > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > > > index 0da69b98f38f..1f2c9da7bd35 100644
> > > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > > > @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct
> > > > > net_device
> > > > > *dev, struct mlx5e_priv *priv,
> > > > > if (mlx5e_is_offloaded_flow(flow) ||
> > > > > flow_flag_test(flow, CT)) {
> > > > > counter = mlx5e_tc_get_counter(flow);
> > > > > - if (!counter)
> > > > > + if (!counter) {
> > > > > + err = -EINVAL;
> > > > > goto errout;
> > > > > + }
> > > > > mlx5_fc_query_cached(counter, , ,
> > > > > );
> > > > > }
> > > > > @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct
> > > > > net_device
> > > > > *dev, struct mlx5e_priv *priv,
> > > > > * un-offloaded while the other rule is offloaded.
> > > > > */
> > > > > peer_esw = mlx5_devcom_get_peer_data(devcom,
> > > > > MLX5_DEVCOM_ESW_OFFLOADS);
> > > > > - if (!peer_esw)
> > > > > + if (!peer_esw) {
> > > > > + err = -EINVAL;
> > > >
This is not an error flow, i am curious what are the thresholds of this
robot ?
> > > > note here it's not an error. it could be there is no peer esw
> > > > so just continue with the stats update.
> > > >
> > > > > goto out;
> > > > > + }
> > > > > if (flow_flag_test(flow, DUP) &&
> > > > > flow_flag_test(flow->peer_flow, OFFLOADED)) {
> > > > > @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct
> > > > > net_device
> > > > > *dev, struct mlx5e_priv *priv,
> > > > > u64 lastuse2;
> > > > > counter = mlx5e_tc_get_counter(flow->peer_flow);
> > > > > - if (!counter)
> > > > > + if (!counter) {
> > > > > + err = -EINVAL;
> > >
> > > this change is problematic. the current goto is to do stats
> > > update with
> > > the first counter stats we got but if you now want to return an
> > > error
> > > then you probably should not do any update at all.
> >
> > Thanks for your reply :)
> > I am not sure whether an error code should be returned here?
> > If so, flow_stats_update(...) should not be called here?
> >
> >
> > Best wishes,
> > Jia-Ju Bai
> >
>
> basically flow and peer_flow should be valid and protected from
> changes,
> and counter should be valid.
> it looks like the check here is more of a sanity check if something
> goes
> wrong but shouldn't. you can just let it be, update the stats from
> the
> first queried counter.
>
Roi, let's consider returning an error code here, we shouldn't be
silently returning if we are not expecting these errors,
why would mlx5e_stats_flower() be called if stats are not offloaded ?
Thanks,
Saeed.
Re: [PATCH] net/mlx5e: fix mlx5e_tc_tun_update_header_ipv6 dummy definition
On Mon, 2021-03-01 at 11:57 +0200, Vlad Buslov wrote:
> On Thu 25 Feb 2021 at 14:54, Arnd Bergmann wrote:
> > From: Arnd Bergmann
> >
> > The alternative implementation of this function in a header file
> > is declared as a global symbol, and gets added to every .c file
> > that includes it, which leads to a link error:
> >
> > arm-linux-gnueabi-ld:
> > drivers/net/ethernet/mellanox/mlx5/core/en_rx.o: in function
> > `mlx5e_tc_tun_update_header_ipv6':
> > en_rx.c:(.text+0x0): multiple definition of
> > `mlx5e_tc_tun_update_header_ipv6';
> > drivers/net/ethernet/mellanox/mlx5/core/en_main.o:en_main.c:(.text+
> > 0x0): first defined here
> >
> > Mark it 'static inline' like the other functions here.
> >
> > Fixes: c7b9038d8af6 ("net/mlx5e: TC preparation refactoring for
> > routing update event")
> > Signed-off-by: Arnd Bergmann
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h | 10 ++---
> > -
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
> > b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
> > index 67de2bf36861..89d5ca91566e 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
> > @@ -76,10 +76,12 @@ int mlx5e_tc_tun_update_header_ipv6(struct
> > mlx5e_priv *priv,
> > static inline int
> > mlx5e_tc_tun_create_header_ipv6(struct mlx5e_priv *priv,
> > struct net_device *mirred_dev,
> > - struct mlx5e_encap_entry *e) {
> > return -EOPNOTSUPP; }
> > -int mlx5e_tc_tun_update_header_ipv6(struct mlx5e_priv *priv,
> > - struct net_device *mirred_dev,
> > - struct mlx5e_encap_entry *e)
> > + struct mlx5e_encap_entry *e)
> > +{ return -EOPNOTSUPP; }
> > +static inline int
> > +mlx5e_tc_tun_update_header_ipv6(struct mlx5e_priv *priv,
> > + struct net_device *mirred_dev,
> > + struct mlx5e_encap_entry *e)
> > { return -EOPNOTSUPP; }
> > #endif
> > int mlx5e_tc_tun_route_lookup(struct mlx5e_priv *priv,
>
> Thanks Arnd!
>
> Reviewed-by: Vlad Buslov
Applied to net-mlx5,
Thanks.
Re: mlx5 HW crypto offload support
On Mon, 2021-02-15 at 23:17 +0530, James Spader wrote: > Hi All, > > Does HW crypto offload support for mlx5 work under virtualized > environment? > For e.g with PF (Physical Function) driver and VF(Virtual Function) > driver. > > If yes, then how does the information that is required to create > security association get passed to the PF driver? > > For example in rx full offload the FDB table needs to match the spi. > As the spi is created by the VF driver, how does its value get > communicated to PF driver... > Hi James, please find this guide [1] for a step by step configuration, i hope it will answer your questions. the solution is only in OFED package right now, Huy and Raed "CCed" are working on the upstream submission. The link below discusses the configuration for Bluefield smart device, but the concept should be the same for Native SRIOV. [1] https://community.mellanox.com/s/article/ConnectX-6DX-Bluefield-2-IPsec-HW-Full-Offload-Configuration-Guide
Re: linux-next: manual merge of the net-next tree with the net tree
On Mon, 2021-02-15 at 11:52 +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the net-next tree got conflicts in:
>
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>
> between commit:
>
> e4484d9df500 ("net/mlx5e: Enable striding RQ for Connect-X IPsec
> capable devices")
>
> from the net tree and commits:
>
> 224169d2a32b ("net/mlx5e: IPsec, Remove unnecessary config flag
> usage")
> 70038b73e40e ("net/mlx5e: Add listener to trap event")
> 214baf22870c ("net/mlx5e: Support HTB offload")
>
> from the net-next tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your
> tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any
> particularly
> complex conflicts.
>
Resolution looks correct.
Thanks,
Saeed.
Re: [PATCH] net/mlx5: docs: correct section reference in table of contents
On Fri, 2021-02-05 at 10:55 +0100, Lukas Bulwahn wrote:
> Commit 142d93d12dc1 ("net/mlx5: Add devlink subfunction port
> documentation") refers to a section 'mlx5 port function' in the table
> of
> contents, but includes a section 'mlx5 function attributes' instead.
>
> Hence, make htmldocs warns:
>
> mlx5.rst:16: WARNING: Unknown target name: "mlx5 port function".
>
> Correct the section reference in table of contents to the actual name
> of
> section in the documentation.
>
> Also, tune another section underline while visiting this document.
>
> Signed-off-by: Lukas Bulwahn
> ---
> Saeed, please pick this patch for your -next tree on top of the
> commit above.
Applied to net-next-mlx5,
Thanks,
Saeed.
Re: [next] [s390 ] net: mlx5: tc_tun.h:24:29: error: field 'match_level' has incomplete type
On Wed, 2021-02-10 at 10:50 +0530, Naresh Kamboju wrote: > While building Linux next tag 20210209 s390 (defconfig) with gcc-9 > make modules failed. > ... > Reported-by: Naresh Kamboju > Thanks for the report a patch was already posted earlier today https://patchwork.kernel.org/project/netdevbpf/patch/20210209203722.12387-1-sa...@kernel.org/ >
Re: [PATCH][next] net/mlx5e: Fix spelling mistake "Unknouwn" -> "Unknown"
On Wed, 2021-02-03 at 14:57 -0800, Jesse Brandeburg wrote: > Colin King wrote: > > > From: Colin Ian King > > > > There is a spelling mistake in a netdev_warn message. Fix it. > > > > Signed-off-by: Colin Ian King > > Trivial patch, looks fine! > > Reviewed-by: Jesse Brandeburg Applied to net-next-mlx5. Thanks!
Re: [PATCH][next] net/mlx5e: Fix spelling mistake "channles" -> "channels"
On Thu, 2021-02-04 at 09:32 +, Colin King wrote: > From: Colin Ian King > > There is a spelling mistake in a netdev_warn message. Fix it. > > Signed-off-by: Colin Ian King > --- Applied to net-next-mlx5 thanks!
Re: [PATCH] net/mlx5e: free page before return
On Thu, 2021-01-21 at 19:49 +0200, Leon Romanovsky wrote:
> On Wed, Jan 20, 2021 at 08:58:30PM -0800, Pan Bian wrote:
> > Instead of directly return, goto the error handling label to free
> > allocated page.
> >
> > Fixes: 5f29458b77d5 ("net/mlx5e: Support dump callback in TX
> > reporter")
> > Signed-off-by: Pan Bian
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en/health.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
>
> Thanks,
> Reviewed-by: Leon Romanovsky
Applied to net-mlx5,
Thanks!
Re: [PATCH net-next 0/6] net: ipa: GSI interrupt updates
On Wed, 2021-01-13 at 11:15 -0600, Alex Elder wrote: > This series implements some updates for the GSI interrupt code, > buliding on some bug fixes implemented last month. > > The first two are simple changes made to improve readability and > consistency. The third replaces all msleep() calls with comparable > usleep_range() calls. > > The remainder make some more substantive changes to make the code > align with recommendations from Qualcomm. The fourth implements a > much shorter timeout for completion GSI commands, and the fifth > implements a longer delay between retries of the STOP channel > command. Finally, the last implements retries for stopping TX > channels (in addition to RX channels). > > -Alex > A minor thing that bothers me about this series is that it looks like it is based on magic numbers and some redefined constant values according to some mysterious sources ;-) .. It would be nice to have some wording in the commit messages explaining reasoning and maybe "semi-official" sources behind the changes. LGMT code style wise :) Reviewed-by: Saeed Mahameed > Alex Elder (6): > net: ipa: a few simple renames > net: ipa: introduce some interrupt helpers > net: ipa: use usleep_range() > net: ipa: change GSI command timeout > net: ipa: change stop channel retry delay > net: ipa: retry TX channel stop commands > > drivers/net/ipa/gsi.c | 140 +++-- > > drivers/net/ipa/ipa_endpoint.c | 4 +- > 2 files changed, 83 insertions(+), 61 deletions(-) >
Re: [PATCv4 net-next] octeontx2-pf: Add RSS multi group support
On Tue, 2021-01-12 at 15:16 -0800, Saeed Mahameed wrote: > On Mon, 2021-01-04 at 12:50 +0530, Geetha sowjanya wrote: > > Hardware supports 8 RSS groups per interface. Currently we are > > using > > only group '0'. This patch allows user to create new RSS > > groups/contexts > > and use the same as destination for flow steering rules. > > > > usage: > > To steer the traffic to RQ 2,3 > > > > ethtool -X eth0 weight 0 0 1 1 context new > > (It will print the allocated context id number) > > New RSS context is 1 > > > > ethtool -N eth0 flow-type tcp4 dst-port 80 context 1 loc 1 > > > > To delete the context > > ethtool -X eth0 context 1 delete > > > > When an RSS context is removed, the active classification > > rules using this context are also removed. > > > > Change-log: > > > > v4 > > - Fixed compiletime warning. > > - Address Saeed's comments on v3. > > > > This patch is marked as accepted in patchwork > https://patchwork.kernel.org/project/netdevbpf/patch/20210104072039.27297-1-gak...@marvell.com/ > > but it is not actually applied, maybe resend.. > > > you can add: > Reviewed-by: Saeed Mahameed > > I missed Jakub's comment, The patch is already applied, i looked at the wrong tree. Thanks.
Re: [PATCv4 net-next] octeontx2-pf: Add RSS multi group support
On Mon, 2021-01-04 at 12:50 +0530, Geetha sowjanya wrote: > Hardware supports 8 RSS groups per interface. Currently we are using > only group '0'. This patch allows user to create new RSS > groups/contexts > and use the same as destination for flow steering rules. > > usage: > To steer the traffic to RQ 2,3 > > ethtool -X eth0 weight 0 0 1 1 context new > (It will print the allocated context id number) > New RSS context is 1 > > ethtool -N eth0 flow-type tcp4 dst-port 80 context 1 loc 1 > > To delete the context > ethtool -X eth0 context 1 delete > > When an RSS context is removed, the active classification > rules using this context are also removed. > > Change-log: > > v4 > - Fixed compiletime warning. > - Address Saeed's comments on v3. > This patch is marked as accepted in patchwork https://patchwork.kernel.org/project/netdevbpf/patch/20210104072039.27297-1-gak...@marvell.com/ but it is not actually applied, maybe resend.. you can add: Reviewed-by: Saeed Mahameed
Re: [PATCH] [v2] net/mlx5e: Fix two double free cases
On Tue, 2021-01-05 at 13:02 -0800, Saeed Mahameed wrote:
> On Mon, 2020-12-28 at 16:48 +0800, Dinghao Liu wrote:
> > mlx5e_create_ttc_table_groups() frees ft->g on failure of
> > kvzalloc(), but such failure will be caught by its caller
> > in mlx5e_create_ttc_table() and ft->g will be freed again
> > in mlx5e_destroy_flow_table(). The same issue also occurs
> > in mlx5e_create_ttc_table_groups(). Set ft->g to NULL after
> > kfree() to avoid double free.
> >
> > Fixes: 7b3722fa9ef64 ("net/mlx5e: Support RSS for GRE tunneled
^ this is one digit too much..
Fixes line must start with a 12 char SHA and not 13 :).
I fixed this up, no need to do anything but just FYI.
> > packets")
> > Fixes: 33cfaaa8f36ff ("net/mlx5e: Split the main flow steering
> > table")
> > Signed-off-by: Dinghao Liu
> > ---
> >
> > Changelog:
> >
> > v2: - Set ft->g to NULL after kfree() instead of removing kfree().
> > Refine commit message.
> >
>
> applied to net-next-mlx5,
> Thanks!
>
Re: [PATCH] net/mlx5: fix spelling mistake in Kconfig "accelaration" -> "acceleration"
On Tue, 2020-12-15 at 14:49 +, Colin King wrote: > From: Colin Ian King > > There are some spelling mistakes in the Kconfig. Fix these. > > Signed-off-by: Colin Ian King > applied to net-next-mlx5, Thanks!
Re: [PATCH] net/mlx5e: remove h from printk format specifier
On Wed, 2020-12-23 at 11:45 -0800, t...@redhat.com wrote:
> From: Tom Rix
>
> This change fixes the checkpatch warning described in this commit
> commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> unnecessary %h[xudi] and %hh[xudi]")
>
> Standard integer promotion is already done and %hx and %hhx is
> useless
> so do not encourage the use of %hh[xudi] or %h[xudi].
>
> Signed-off-by: Tom Rix
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en/params.c | 2 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
applied to net-next-mlx5,
Thanks
Re: [PATCH] net/mlx5e: Fix memleak in mlx5e_create_l2_table_groups
On Sun, 2020-12-27 at 10:33 +0200, Leon Romanovsky wrote:
> On Mon, Dec 21, 2020 at 07:27:31PM +0800, Dinghao Liu wrote:
> > When mlx5_create_flow_group() fails, ft->g should be
> > freed just like when kvzalloc() fails. The caller of
> > mlx5e_create_l2_table_groups() does not catch this
> > issue on failure, which leads to memleak.
> >
> > Signed-off-by: Dinghao Liu
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en_fs.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
>
> Fixes: 33cfaaa8f36f ("net/mlx5e: Split the main flow steering table")
>
Added
> Thanks,
> Reviewed-by: Leon Romanovsky
Applied to net-mlx5
Thanks
Re: [PATCH] [v2] net/mlx5e: Fix two double free cases
On Mon, 2020-12-28 at 16:48 +0800, Dinghao Liu wrote:
> mlx5e_create_ttc_table_groups() frees ft->g on failure of
> kvzalloc(), but such failure will be caught by its caller
> in mlx5e_create_ttc_table() and ft->g will be freed again
> in mlx5e_destroy_flow_table(). The same issue also occurs
> in mlx5e_create_ttc_table_groups(). Set ft->g to NULL after
> kfree() to avoid double free.
>
> Fixes: 7b3722fa9ef64 ("net/mlx5e: Support RSS for GRE tunneled
> packets")
> Fixes: 33cfaaa8f36ff ("net/mlx5e: Split the main flow steering
> table")
> Signed-off-by: Dinghao Liu
> ---
>
> Changelog:
>
> v2: - Set ft->g to NULL after kfree() instead of removing kfree().
> Refine commit message.
>
applied to net-next-mlx5,
Thanks!
Re: [PATCH -next v2] net/mlx5_core: remove unused including
On Wed, 2020-12-09 at 15:01 +0800, Zou Wei wrote:
> Remove including that don't need it.
>
> Fixes: 17a7612b99e6 ("net/mlx5_core: Clean driver version and name")
> Signed-off-by: Zou Wei
> ---
Applied to net-next-mlx5.
Thanks!
Re: [PATCH net-next] net/mlx5: simplify the return expression of mlx5_esw_offloads_pair()
On Tue, 2020-12-08 at 16:25 -0800, David Miller wrote: > From: Zheng Yongjun > Date: Tue, 8 Dec 2020 21:56:25 +0800 > > > Simplify the return expression. > > > > Signed-off-by: Zheng Yongjun > > Applied. > Hey Dave, it's great to have you back! I still don't see this patch in net-next, i will take it to my tree and submit it in my next pr. Thanks, Saeed.
Re: [patch 22/30] net/mlx5: Replace irq_to_desc() abuse
On Thu, 2020-12-10 at 20:25 +0100, Thomas Gleixner wrote:
> No driver has any business with the internals of an interrupt
> descriptor. Storing a pointer to it just to use yet another helper at
> the
> actual usage site to retrieve the affinity mask is creative at best.
> Just
> because C does not allow encapsulation does not mean that the kernel
> has no
> limits.
>
you can't blame the developers for using stuff from include/linux/
Not all developers are the same, and sometime we don't read in between
the lines, you can't assume all driver developers to be expert on irq
APIs disciplines.
your rules must be programmatically expressed, for instance,
you can just hide struct irq_desc and irq_to_desc() in kernel/irq/ and
remove them from include/linux/ header files, if you want privacy in
your subsystem, don't put all your header files on display under
include/linux.
> Retrieve a pointer to the affinity mask itself and use that. It's
> still
> using an interface which is usually not for random drivers, but
> definitely
> less hideous than the previous hack.
>
> Signed-off-by: Thomas Gleixner
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en.h |2 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c |2 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c |6 +-
> 3 files changed, 3 insertions(+), 7 deletions(-)
>
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> @@ -669,7 +669,7 @@ struct mlx5e_channel {
> spinlock_t async_icosq_lock;
>
> /* data path - accessed per napi poll */
> - struct irq_desc *irq_desc;
> + const struct cpumask *aff_mask;
> struct mlx5e_ch_stats *stats;
>
> /* control */
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -1998,7 +1998,7 @@ static int mlx5e_open_channel(struct mlx
> c->num_tc = params->num_tc;
> c->xdp = !!params->xdp_prog;
> c->stats= >channel_stats[ix].ch;
> - c->irq_desc = irq_to_desc(irq);
> + c->aff_mask = irq_get_affinity_mask(irq);
as long as the affinity mask pointer stays the same for the lifetime of
the irq vector.
Assuming that:
Acked-by: Saeed Mahameed
Re: [patch 23/30] net/mlx5: Use effective interrupt affinity
On Thu, 2020-12-10 at 20:25 +0100, Thomas Gleixner wrote: > Using the interrupt affinity mask for checking locality is not really > working well on architectures which support effective affinity masks. > > The affinity mask is either the system wide default or set by user > space, > but the architecture can or even must reduce the mask to the > effective set, > which means that checking the affinity mask itself does not really > tell > about the actual target CPUs. > > Signed-off-by: Thomas Gleixner > Cc: Saeed Mahameed > Cc: Leon Romanovsky > Cc: "David S. Miller" > Cc: Jakub Kicinski > Cc: net...@vger.kernel.org > Cc: linux-r...@vger.kernel.org > Acked-by: Saeed Mahameed
Re: [PATCH net-next 3/7] net: hns3: add support for forwarding packet to queues of specified TC when flow director rule hit
On Thu, 2020-12-10 at 20:24 +0800, tanhuazhong wrote:
>
> On 2020/12/10 13:40, Saeed Mahameed wrote:
> > On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:
> > > From: Jian Shen
> > >
> > > For some new device, it supports forwarding packet to queues
> > > of specified TC when flow director rule hit. So extend the
> > > command handle to support it.
> > >
> >
> > ...
> >
> > > static int hclge_config_action(struct hclge_dev *hdev, u8
> > > stage,
> > > struct hclge_fd_rule *rule)
> > > {
> > > + struct hclge_vport *vport = hdev->vport;
> > > + struct hnae3_knic_private_info *kinfo = >nic.kinfo;
> > > struct hclge_fd_ad_data ad_data;
> > >
> > > + memset(_data, 0, sizeof(struct hclge_fd_ad_data));
> > > ad_data.ad_id = rule->location;
> > >
> > > if (rule->action == HCLGE_FD_ACTION_DROP_PACKET) {
> > > ad_data.drop_packet = true;
> > > - ad_data.forward_to_direct_queue = false;
> > > - ad_data.queue_id = 0;
> > > + } else if (rule->action == HCLGE_FD_ACTION_SELECT_TC) {
> > > + ad_data.override_tc = true;
> > > + ad_data.queue_id =
> > > + kinfo->tc_info.tqp_offset[rule->tc];
> > > + ad_data.tc_size =
> > > + ilog2(kinfo->tc_info.tqp_count[rule->tc]);
> >
> > In the previous patch you copied this info from mqprio, which is an
> > egress qdisc feature, this patch is clearly about rx flow director,
> > I
> > think the patch is missing some context otherwise it doesn't make
> > any
> > sense.
> >
>
> Since tx and rx are in the same tqp, what we do here is to make tx
> and
> rx in the same tc when rule is hit.
>
this needs more clarification, even if tx and rx are the same hw
object, AFAIK there is not correlation between mqprio and tc rx
classifiers.
> > > } else {
> > > - ad_data.drop_packet = false;
> > > ad_data.forward_to_direct_queue = true;
> > > ad_data.queue_id = rule->queue_id;
> > > }
> > > @@ -5937,7 +5950,7 @@ static int hclge_add_fd_entry(struct
> > > hnae3_handle *handle,
> > > return -EINVAL;
> > > }
> > >
> > > - action = HCLGE_FD_ACTION_ACCEPT_PACKET;
> > > + action = HCLGE_FD_ACTION_SELECT_QUEUE;
> > > q_index = ring;
> > > }
> > >
> > > diff --git
> > > a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> > > b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> > > index b3c1301..a481064 100644
> > > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> > > @@ -572,8 +572,9 @@ enum HCLGE_FD_PACKET_TYPE {
> > > };
> > >
> > > enum HCLGE_FD_ACTION {
> > > - HCLGE_FD_ACTION_ACCEPT_PACKET,
> > > + HCLGE_FD_ACTION_SELECT_QUEUE,
> > > HCLGE_FD_ACTION_DROP_PACKET,
> > > + HCLGE_FD_ACTION_SELECT_TC,
> >
> > what is SELECT_TC ? you never actually write this value
> > anywhere in
> > this patch.
> >
>
> HCLGE_FD_ACTION_SELECT_TC means that the packet will be forwarded
> into
> the queue of specified TC when rule is hit.
>
what is "specified TC" in this context ?
Are we talking about ethtool nfc steering here ? because clearly this
was the purpose of HCLGE_FD_ACTION_ACCEPT_PACKET before it got removed.
> the assignment is in the next patch, maybe these two patch should be
> merged for making it more readable.
>
>
> Thanks.
> Huazhong.
>
> >
> > .
> >
Re: [PATCH net-next 2/7] net: hns3: add support for tc mqprio offload
On Thu, 2020-12-10 at 20:27 +0800, tanhuazhong wrote:
>
> On 2020/12/10 12:50, Saeed Mahameed wrote:
> > On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:
> > > From: Jian Shen
> > >
> > > Currently, the HNS3 driver only supports offload for tc number
> > > and prio_tc. This patch adds support for other qopts, including
> > > queues count and offset for each tc.
> > >
> > > When enable tc mqprio offload, it's not allowed to change
> > > queue numbers by ethtool. For hardware limitation, the queue
> > > number of each tc should be power of 2.
> > >
> > > For the queues is not assigned to each tc by average, so it's
> > > should return vport->alloc_tqps for hclge_get_max_channels().
> > >
> >
> > The commit message needs some improvements, it is not really clear
> > what
> > the last two sentences are about.
> >
>
> The hclge_get_max_channels() returns the max queue number of each TC
> for
> user can set by command ethool -L. In previous implement, the queues
> are
> assigned to each TC by average, so we return it by vport-:
> alloc_tqps / num_tc. And now we can assign differrent queue number
> for
> each TC, so it shouldn't be divided by num_tc.
What do you mean by "queues assigned to each tc by average" ?
[...]
>
> > > + }
> > > + if (hdev->vport[0].alloc_tqps < queue_sum) {
> >
> > can't you just allocate new tqps according to the new mqprio input
> > like
> > other drivers do ? how the user allocates those tqps ?
> >
>
> maybe the name of 'alloc_tqps' is a little bit misleading, the
> meaning
> of this field is the total number of the available tqps in this
> vport.
>
from your driver code it seems alloc_tqps is number of rings allocated
via ethool -L.
My point is, it seems like in this patch you demand for the queues to
be preallocated, but what other drivers do on setup tc, they just
duplicate what ever number of queues was configured prior to setup tc,
num_tc times.
> > > + dev_err(>pdev->dev,
> > > + "qopt queue count sum should be less than
> > > %u\n",
> > > + hdev->vport[0].alloc_tqps);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void hclge_sync_mqprio_qopt(struct hnae3_tc_info
> > > *tc_info,
> > > +struct tc_mqprio_qopt_offload
> > > *mqprio_qopt)
> > > +{
> > > + int i;
> > > +
> > > + memset(tc_info, 0, sizeof(*tc_info));
> > > + tc_info->num_tc = mqprio_qopt->qopt.num_tc;
> > > + memcpy(tc_info->prio_tc, mqprio_qopt->qopt.prio_tc_map,
> > > +sizeof_field(struct hnae3_tc_info, prio_tc));
> > > + memcpy(tc_info->tqp_count, mqprio_qopt->qopt.count,
> > > +sizeof_field(struct hnae3_tc_info, tqp_count));
> > > + memcpy(tc_info->tqp_offset, mqprio_qopt->qopt.offset,
> > > +sizeof_field(struct hnae3_tc_info, tqp_offset));
> > > +
> >
> > isn't it much easier to just store a copy of tc_mqprio_qopt in you
> > tc_info and then just:
> > tc_info->qopt = mqprio->qopt;
> >
> > [...]
>
> The tc_mqprio_qopt_offload still contains a lot of opt hns3 driver
> does
> not use yet, even if the hns3 use all the opt, I still think it is
> better to create our own struct, if struct tc_mqprio_qopt_offload
> changes in the future, we can limit the change to the
> tc_mqprio_qopt_offload convertion.
>
ok.
Re: [PATCH net-next 3/7] net: hns3: add support for forwarding packet to queues of specified TC when flow director rule hit
On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:
> From: Jian Shen
>
> For some new device, it supports forwarding packet to queues
> of specified TC when flow director rule hit. So extend the
> command handle to support it.
>
...
> static int hclge_config_action(struct hclge_dev *hdev, u8 stage,
> struct hclge_fd_rule *rule)
> {
> + struct hclge_vport *vport = hdev->vport;
> + struct hnae3_knic_private_info *kinfo = >nic.kinfo;
> struct hclge_fd_ad_data ad_data;
>
> + memset(_data, 0, sizeof(struct hclge_fd_ad_data));
> ad_data.ad_id = rule->location;
>
> if (rule->action == HCLGE_FD_ACTION_DROP_PACKET) {
> ad_data.drop_packet = true;
> - ad_data.forward_to_direct_queue = false;
> - ad_data.queue_id = 0;
> + } else if (rule->action == HCLGE_FD_ACTION_SELECT_TC) {
> + ad_data.override_tc = true;
> + ad_data.queue_id =
> + kinfo->tc_info.tqp_offset[rule->tc];
> + ad_data.tc_size =
> + ilog2(kinfo->tc_info.tqp_count[rule->tc]);
In the previous patch you copied this info from mqprio, which is an
egress qdisc feature, this patch is clearly about rx flow director, I
think the patch is missing some context otherwise it doesn't make any
sense.
> } else {
> - ad_data.drop_packet = false;
> ad_data.forward_to_direct_queue = true;
> ad_data.queue_id = rule->queue_id;
> }
> @@ -5937,7 +5950,7 @@ static int hclge_add_fd_entry(struct
> hnae3_handle *handle,
> return -EINVAL;
> }
>
> - action = HCLGE_FD_ACTION_ACCEPT_PACKET;
> + action = HCLGE_FD_ACTION_SELECT_QUEUE;
> q_index = ring;
> }
>
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> index b3c1301..a481064 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> @@ -572,8 +572,9 @@ enum HCLGE_FD_PACKET_TYPE {
> };
>
> enum HCLGE_FD_ACTION {
> - HCLGE_FD_ACTION_ACCEPT_PACKET,
> + HCLGE_FD_ACTION_SELECT_QUEUE,
> HCLGE_FD_ACTION_DROP_PACKET,
> + HCLGE_FD_ACTION_SELECT_TC,
what is SELECT_TC ? you never actually write this value anywhere in
this patch.
Re: [PATCH net-next 2/7] net: hns3: add support for tc mqprio offload
On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:
> From: Jian Shen
>
> Currently, the HNS3 driver only supports offload for tc number
> and prio_tc. This patch adds support for other qopts, including
> queues count and offset for each tc.
>
> When enable tc mqprio offload, it's not allowed to change
> queue numbers by ethtool. For hardware limitation, the queue
> number of each tc should be power of 2.
>
> For the queues is not assigned to each tc by average, so it's
> should return vport->alloc_tqps for hclge_get_max_channels().
>
The commit message needs some improvements, it is not really clear what
the last two sentences are about.
> Signed-off-by: Jian Shen
> Signed-off-by: Huazhong Tan
> ---
>
...
>
> + if (kinfo->tc_info.mqprio_active) {
> + dev_err(>dev,
why not use netdev_err() and friends ?
anyway I see your driver is using dev_err(>dev, ...)
intensively,
maybe submit a follow up patch to fix all your prints ?
...]
>
> +static int hclge_mqprio_qopt_check(struct hclge_dev *hdev,
> +struct tc_mqprio_qopt_offload
> *mqprio_qopt)
> +{
> + u16 queue_sum = 0;
> + int ret;
> + int i;
> +
> + if (!mqprio_qopt->qopt.num_tc) {
> + mqprio_qopt->qopt.num_tc = 1;
> + return 0;
> + }
> +
> + ret = hclge_dcb_common_validate(hdev, mqprio_qopt->qopt.num_tc,
> + mqprio_qopt->qopt.prio_tc_map);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < mqprio_qopt->qopt.num_tc; i++) {
> + if (!is_power_of_2(mqprio_qopt->qopt.count[i])) {
> + dev_err(>pdev->dev,
> + "qopt queue count must be power of
> 2\n");
> + return -EINVAL;
> + }
> +
> + if (mqprio_qopt->qopt.count[i] > hdev->rss_size_max) {
> + dev_err(>pdev->dev,
> + "qopt queue count should be no more
> than %u\n",
> + hdev->rss_size_max);
> + return -EINVAL;
> + }
> +
> + if (mqprio_qopt->qopt.offset[i] != queue_sum) {
> + dev_err(>pdev->dev,
> + "qopt queue offset must start from 0,
> and being continuous\n");
> + return -EINVAL;
> + }
> +
> + if (mqprio_qopt->min_rate[i] || mqprio_qopt-
> >max_rate[i]) {
> + dev_err(>pdev->dev,
> + "qopt tx_rate is not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + queue_sum = mqprio_qopt->qopt.offset[i];
> + queue_sum += mqprio_qopt->qopt.count[i];
it will make more sense if you moved this queue summing outside of the
loop
> + }
> + if (hdev->vport[0].alloc_tqps < queue_sum) {
can't you just allocate new tqps according to the new mqprio input like
other drivers do ? how the user allocates those tqps ?
> + dev_err(>pdev->dev,
> + "qopt queue count sum should be less than
> %u\n",
> + hdev->vport[0].alloc_tqps);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static void hclge_sync_mqprio_qopt(struct hnae3_tc_info *tc_info,
> +struct tc_mqprio_qopt_offload
> *mqprio_qopt)
> +{
> + int i;
> +
> + memset(tc_info, 0, sizeof(*tc_info));
> + tc_info->num_tc = mqprio_qopt->qopt.num_tc;
> + memcpy(tc_info->prio_tc, mqprio_qopt->qopt.prio_tc_map,
> +sizeof_field(struct hnae3_tc_info, prio_tc));
> + memcpy(tc_info->tqp_count, mqprio_qopt->qopt.count,
> +sizeof_field(struct hnae3_tc_info, tqp_count));
> + memcpy(tc_info->tqp_offset, mqprio_qopt->qopt.offset,
> +sizeof_field(struct hnae3_tc_info, tqp_offset));
> +
isn't it much easier to just store a copy of tc_mqprio_qopt in you
tc_info and then just:
tc_info->qopt = mqprio->qopt;
[...]
> - hclge_tm_schd_info_update(hdev, tc);
> - hclge_tm_prio_tc_info_update(hdev, prio_tc);
> -
> - ret = hclge_tm_init_hw(hdev, false);
> - if (ret)
> - goto err_out;
> + kinfo = >nic.kinfo;
> + memcpy(_tc_info, >tc_info, sizeof(old_tc_info));
if those are of the same kind, just normal assignment would be much
cleaner.
> + hclge_sync_mqprio_qopt(>tc_info, mqprio_qopt);
> + kinfo->tc_info.mqprio_active = tc > 0;
>
> - ret = hclge_client_setup_tc(hdev);
> + ret = hclge_config_tc(hdev, >tc_info);
> if (ret)
> goto err_out;
>
> @@ -436,6 +534,12 @@ static int hclge_setup_tc(struct hnae3_handle
> *h, u8 tc, u8 *prio_tc)
> return hclge_notify_init_up(hdev);
>
> err_out:
> + /* roll-back */
> + memcpy(>tc_info, _tc_info, sizeof(old_tc_info));
same.
Re: [PATCHv3 net-next] octeontx2-pf: Add RSS multi group support
On Wed, 2020-12-09 at 22:39 +0530, Geetha sowjanya wrote:
> Hardware supports 8 RSS groups per interface. Currently we are using
> only group '0'. This patch allows user to create new RSS
> groups/contexts
> and use the same as destination for flow steering rules.
>
> usage:
> To steer the traffic to RQ 2,3
>
> ethtool -X eth0 weight 0 0 1 1 context new
> (It will print the allocated context id number)
> New RSS context is 1
>
> ethtool -N eth0 flow-type tcp4 dst-port 80 context 1 loc 1
>
> To delete the context
> ethtool -X eth0 context 1 delete
>
> When an RSS context is removed, the active classification
> rules using this context are also removed.
>
> Change-log:
> v2
> - Removed unrelated whitespace
> - Coverted otx2_get_rxfh() to use new function.
>
> v3
> - Coverted otx2_set_rxfh() to use new function.
>
> Signed-off-by: Sunil Kovvuri Goutham
> Signed-off-by: Geetha sowjanya
> ---
...
> -/* Configure RSS table and hash key */
> -static int otx2_set_rxfh(struct net_device *dev, const u32 *indir,
> - const u8 *hkey, const u8 hfunc)
> +static int otx2_get_rxfh_context(struct net_device *dev, u32 *indir,
> + u8 *hkey, u8 *hfunc, u32 rss_context)
> {
> struct otx2_nic *pfvf = netdev_priv(dev);
> + struct otx2_rss_ctx *rss_ctx;
> struct otx2_rss_info *rss;
> int idx;
>
> - if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc !=
> ETH_RSS_HASH_TOP)
> - return -EOPNOTSUPP;
> -
> rss = >hw.rss_info;
>
> if (!rss->enable) {
> - netdev_err(dev, "RSS is disabled, cannot change
> settings\n");
> + netdev_err(dev, "RSS is disabled\n");
> return -EIO;
> }
I see that you init/enable rss on open, is this is your way to block
getting rss info if device is not open ? why do you need to report an
error anyway, why not just report whatever default config you will be
setting up on next open ?
to me reporting errors to ethtool queries when device is down is a bad
user experience.
> + if (rss_context >= MAX_RSS_GROUPS)
> + return -EINVAL;
> +
-ENOENT
> + rss_ctx = rss->rss_ctx[rss_context];
> + if (!rss_ctx)
> + return -EINVAL;
>
-ENOENT
Re: [resend/standalone PATCH v4] Add auxiliary bus support
On Fri, 2020-12-04 at 08:25 -0800, Jakub Kicinski wrote: > On Fri, 4 Dec 2020 14:54:55 +0200 Leon Romanovsky wrote: > > Thanks, pulled to mlx5-next > > > > Jason, Jakob, > > > > Can you please pull that mlx5-next branch to your trees? > > git://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.git > > Could you post a PR with a proper description and so on? > > Thanks! I will do that. Thanks!
Re: [PATCH net] net/mlx5: fix error return code in mlx5e_tc_nic_init()
On Sat, 2020-11-14 at 19:52 +0800, Wang Hai wrote:
> Fix to return a negative error code from the error handling
> case instead of 0, as done elsewhere in this function.
>
> Fixes: aedd133d17bc ("net/mlx5e: Support CT offload for tc nic
> flows")
> Reported-by: Hulk Robot
> Signed-off-by: Wang Hai
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index e3a968e9e2a0..c7ad5db84f78 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -5227,8 +5227,10 @@ int mlx5e_tc_nic_init(struct mlx5e_priv *priv)
>
> tc->ct = mlx5_tc_ct_init(priv, tc->chains,
> >fs.tc.mod_hdr,
>MLX5_FLOW_NAMESPACE_KERNEL);
> - if (IS_ERR(tc->ct))
> + if (IS_ERR(tc->ct)) {
> + err = PTR_ERR(tc->ct);
> goto err_ct;
> + }
>
> tc->netdevice_nb.notifier_call = mlx5e_tc_netdev_event;
> err = register_netdevice_notifier_dev_net(priv->netdev,
Applied to net-mlx5
Thanks !
Re: [PATCH net-next] net/mlx5: Fix passing zero to 'PTR_ERR'
On Thu, 2020-11-12 at 22:28 +0800, YueHaibing wrote: > Fix smatch warnings: > > drivers/net/ethernet/mellanox/mlx5/core/esw/acl/egress_lgcy.c:105 > esw_acl_egress_lgcy_setup() warn: passing zero to 'PTR_ERR' > drivers/net/ethernet/mellanox/mlx5/core/esw/acl/egress_ofld.c:177 > esw_acl_egress_ofld_setup() warn: passing zero to 'PTR_ERR' > drivers/net/ethernet/mellanox/mlx5/core/esw/acl/ingress_lgcy.c:184 > esw_acl_ingress_lgcy_setup() warn: passing zero to 'PTR_ERR' > drivers/net/ethernet/mellanox/mlx5/core/esw/acl/ingress_ofld.c:262 > esw_acl_ingress_ofld_setup() warn: passing zero to 'PTR_ERR' > > esw_acl_table_create() never returns NULL, so > NULL test should be removed. > > Signed-off-by: YueHaibing > Applied to net-next-mlx5 thanks
Re: [PATCH v3 net-next 00/13] Add ethtool ntuple filters support
On Wed, 2020-11-11 at 12:43 +0530, Naveen Mamindlapalli wrote: > This patch series adds support for ethtool ntuple filters, unicast > address filtering, VLAN offload and SR-IOV ndo handlers. All of the > above features are based on the Admin Function(AF) driver support to > install and delete the low level MCAM entries. Each MCAM entry is > programmed with the packet fields to match and what actions to take > if the match succeeds. The PF driver requests AF driver to allocate > set of MCAM entries to be used to install the flows by that PF. The > entries will be freed when the PF driver is unloaded. > > * The patches 1 to 4 adds AF driver infrastructure to install and > delete the low level MCAM flow entries. > * Patch 5 adds ethtool ntuple filter support. > * Patch 6 adds unicast MAC address filtering. > * Patch 7 adds support for dumping the MCAM entries via debugfs. > * Patches 8 to 10 adds support for VLAN offload. > * Patch 10 to 11 adds support for SR-IOV ndo handlers. > * Patch 12 adds support to read the MCAM entries. > > Misc: > * Removed redundant mailbox NIX_RXVLAN_ALLOC. > > Change-log: > v3: > - Fixed Saeed's review comments on v2. > - Fixed modifying the netdev->flags from driver. > - Fixed modifying the netdev features and hw_features after > register_netdev. > - Removed unwanted ndo_features_check callback. > v2: > - Fixed the sparse issues reported by Jakub. > Reviewed-by: Saeed Mahameed
Re: [PATCH v3 net-next 01/13] octeontx2-af: Modify default KEX profile to extract TX packet fields
On Wed, 2020-11-11 at 12:43 +0530, Naveen Mamindlapalli wrote: > From: Stanislaw Kardach > > The current default Key Extraction(KEX) profile can only use RX > packet fields while generating the MCAM search key. The profile > can't be used for matching TX packet fields. This patch modifies > the default KEX profile to add support for extracting TX packet > fields into MCAM search key. Enabled Tx KPU packet parsing by > configuring TX PKIND in tx_parse_cfg. > > Also modified the default KEX profile to extract VLAN TCI from > the LB_PTR and exact byte offset of VLAN header. The NPC KPU > parser was modified to point LB_PTR to the starting byte offset > of VLAN header which points to the tpid field. > > Signed-off-by: Stanislaw Kardach > Signed-off-by: Sunil Goutham > Signed-off-by: Naveen Mamindlapalli > Reviewed-by: Saeed Mahameed
Re: [PATCH v3 net-next 07/13] octeontx2-af: Add debugfs entry to dump the MCAM rules
On Wed, 2020-11-11 at 12:43 +0530, Naveen Mamindlapalli wrote: > From: Subbaraya Sundeep > > Add debugfs support to dump the MCAM rules installed using > NPC_INSTALL_FLOW mbox message. Debugfs file can display mcam > entry, counter if any, flow type and counter hits. > > Ethtool will dump the ntuple flows related to the PF only. > The debugfs file gives systemwide view of the MCAM rules > installed by all the PF's. > > Below is the example output when the debugfs file is read: > ~ # mount -t debugfs none /sys/kernel/debug > ~ # cat /sys/kernel/debug/octeontx2/npc/mcam_rules > > Installed by: PF1 > direction: RX > mcam entry: 227 > udp source port 23 mask 0x > Forward to: PF1 VF0 > action: Direct to queue 0 > enabled: yes > counter: 1 > hits: 0 > I don't want to block this series or anything, but you might want to use devlink dpipe interface for this: https://www.kernel.org/doc/html/latest/networking/devlink/devlink-dpipe.html As a future patch of course. Thanks, Saeed.
Re: [PATCH v2 net-next 09/13] octeontx2-pf: Implement ingress/egress VLAN offload
On Thu, 2020-11-05 at 14:58 +0530, Naveen Mamindlapalli wrote:
> From: Hariprasad Kelam
>
> This patch implements egress VLAN offload by appending NIX_SEND_EXT_S
> header to NIX_SEND_HDR_S. The VLAN TCI information is specified
> in the NIX_SEND_EXT_S. The VLAN offload in the ingress path is
> implemented by configuring the NIX_RX_VTAG_ACTION_S to strip and
> capture the outer vlan fields. The NIX PF allocates one MCAM entry
> for Rx VLAN offload.
>
> Signed-off-by: Hariprasad Kelam
> Signed-off-by: Sunil Goutham
> Signed-off-by: Naveen Mamindlapalli
> ---
..
> @@ -56,6 +58,8 @@ void otx2_mcam_flow_del(struct otx2_nic *pf)
> int otx2_alloc_mcam_entries(struct otx2_nic *pfvf)
> {
> struct otx2_flow_config *flow_cfg = pfvf->flow_cfg;
> + netdev_features_t wanted = NETIF_F_HW_VLAN_STAG_RX |
> +NETIF_F_HW_VLAN_CTAG_RX;
> struct npc_mcam_alloc_entry_req *req;
> struct npc_mcam_alloc_entry_rsp *rsp;
> int i;
> @@ -88,15 +92,22 @@ int otx2_alloc_mcam_entries(struct otx2_nic
> *pfvf)
> if (rsp->count != req->count) {
> netdev_info(pfvf->netdev, "number of rules truncated to
> %d\n",
> rsp->count);
> + netdev_info(pfvf->netdev,
> + "Disabling RX VLAN offload due to non-
> availability of MCAM space\n");
> /* support only ntuples here */
> flow_cfg->ntuple_max_flows = rsp->count;
> flow_cfg->ntuple_offset = 0;
> pfvf->netdev->priv_flags &= ~IFF_UNICAST_FLT;
> pfvf->flags &= ~OTX2_FLAG_UCAST_FLTR_SUPPORT;
> + pfvf->flags &= ~OTX2_FLAG_RX_VLAN_SUPPORT;
> + pfvf->netdev->features &= ~wanted;
> + pfvf->netdev->hw_features &= ~wanted;
Drivers are not allowed to change own features dynamically.
please see:
https://www.kernel.org/doc/html/latest/networking/netdev-features.html
Features dependencies must be resolved via:
ndo_fix_features() and netdev_update_features();
> +static netdev_features_t
> +otx2_features_check(struct sk_buff *skb, struct net_device *dev,
> + netdev_features_t features)
> +{
> + return features;
> +}
> +
what is the point of no-op features_check ?
Re: [PATCH v2 net-next 06/13] octeontx2-pf: Add support for unicast MAC address filtering
On Thu, 2020-11-05 at 14:58 +0530, Naveen Mamindlapalli wrote:
> From: Hariprasad Kelam
>
> Add unicast MAC address filtering support using install flow
> message. Total of 8 MCAM entries are allocated for adding
> unicast mac filtering rules. If the MCAM allocation fails,
> the unicast filtering support will not be advertised.
>
> Signed-off-by: Hariprasad Kelam
> Signed-off-by: Sunil Goutham
> Signed-off-by: Naveen Mamindlapalli
> ---
> .../ethernet/marvell/octeontx2/nic/otx2_common.h | 10 ++
> .../ethernet/marvell/octeontx2/nic/otx2_flows.c| 138
> +++--
> .../net/ethernet/marvell/octeontx2/nic/otx2_pf.c | 5 +
> 3 files changed, 146 insertions(+), 7 deletions(-)
>
> +int otx2_add_macfilter(struct net_device *netdev, const u8 *mac)
> +{
> + struct otx2_nic *pf = netdev_priv(netdev);
> + int err;
> +
> + err = otx2_do_add_macfilter(pf, mac);
> + if (err) {
> + netdev->flags |= IFF_PROMISC;
I don't think you are allowed to change netdev->flags inside the driver
like this, this can easily conflict with other users of this netdev;
netdev promiscuity is managed by the stack via refcount Please see:
__dev_set_promiscuity() and dev_set_promiscuity()
And you will need to notify stack and userspace of flags changes.
Re: [Linux-kernel-mentees] [PATCH v2 net] rose: Fix Null pointer dereference in rose_send_frame()
On Thu, 2020-11-05 at 21:26 +0530, Anmol Karn wrote:
> rose_send_frame() dereferences `neigh->dev` when called from
> rose_transmit_clear_request(), and the first occurance of the `neigh`
> is in rose_loopback_timer() as `rose_loopback_neigh`, and it is
> initialized
> in rose_add_loopback_neigh() as NULL. i.e when `rose_loopback_neigh`
> used in
> rose_loopback_timer() its `->dev` was still NULL and
> rose_loopback_timer()
> was calling rose_rx_call_request() without checking for NULL.
>
> - net/rose/rose_link.c
> This bug seems to get triggered in this line:
>
> rose_call = (ax25_address *)neigh->dev->dev_addr;
>
> Fix it by adding NULL checking for `rose_loopback_neigh->dev` in
> rose_loopback_timer().
>
> Reported-and-tested-by:
> syzbot+a1c743815982d9496...@syzkaller.appspotmail.com
> Link:
> https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
>
> Signed-off-by: Anmol Karn
missing proper fixes tag.
> ---
> net/rose/rose_loopback.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
> index 7b094275ea8b..cd7774cb1d07 100644
> --- a/net/rose/rose_loopback.c
> +++ b/net/rose/rose_loopback.c
> @@ -96,7 +96,7 @@ static void rose_loopback_timer(struct timer_list
> *unused)
> }
>
> if (frametype == ROSE_CALL_REQUEST) {
> - if ((dev = rose_dev_get(dest)) != NULL) {
> + if (rose_loopback_neigh->dev && (dev =
> rose_dev_get(dest)) != NULL) {
> if (rose_rx_call_request(skb, dev,
> rose_loopback_neigh, lci_o) == 0)
> kfree_skb(skb);
> } else {
check patch is not happy:
WARNING:TYPO_SPELLING: 'occurance' may be misspelled - perhaps
'occurrence'?
#7:
rose_transmit_clear_request(), and the first occurance of the `neigh`
ERROR:ASSIGN_IN_IF: do not use assignment in if condition
#36: FILE: net/rose/rose_loopback.c:99:
+ if (rose_loopback_neigh->dev && (dev =
rose_dev_get(dest)) != NULL) {
total: 1 errors, 1 warnings, 0 checks, 8 lines checked
Re: [PATCH v2 net-next 3/3] octeontx2-af: Add devlink health reporters for NIX
On Fri, 2020-11-06 at 00:59 +0530, Sunil Kovvuri wrote: > > > > > Output: > > > > > # ./devlink health > > > > > pci/0002:01:00.0: > > > > >reporter npa > > > > > state healthy error 0 recover 0 > > > > >reporter nix > > > > > state healthy error 0 recover 0 > > > > > # ./devlink health dump show pci/0002:01:00.0 reporter nix > > > > > NIX_AF_GENERAL: > > > > > Memory Fault on NIX_AQ_INST_S read: 0 > > > > > Memory Fault on NIX_AQ_RES_S write: 0 > > > > > AQ Doorbell error: 0 > > > > > Rx on unmapped PF_FUNC: 0 > > > > > Rx multicast replication error: 0 > > > > > Memory fault on NIX_RX_MCE_S read: 0 > > > > > Memory fault on multicast WQE read: 0 > > > > > Memory fault on mirror WQE read: 0 > > > > > Memory fault on mirror pkt write: 0 > > > > > Memory fault on multicast pkt write: 0 > > > > >NIX_AF_RAS: > > > > > Poisoned data on NIX_AQ_INST_S read: 0 > > > > > Poisoned data on NIX_AQ_RES_S write: 0 > > > > > Poisoned data on HW context read: 0 > > > > > Poisoned data on packet read from mirror buffer: 0 > > > > > Poisoned data on packet read from mcast buffer: 0 > > > > > Poisoned data on WQE read from mirror buffer: 0 > > > > > Poisoned data on WQE read from multicast buffer: 0 > > > > > Poisoned data on NIX_RX_MCE_S read: 0 > > > > >NIX_AF_RVU: > > > > > Unmap Slot Error: 0 > > > > > > > > > > > > > Now i am a little bit skeptic here, devlink health reporter > > > > infrastructure was > > > > never meant to deal with dump op only, the main purpose is to > > > > diagnose/dump and recover. > > > > > > > > especially in your use case where you only report counters, i > > > > don't > > > > believe > > > > devlink health dump is a proper interface for this. > > > These are not counters. These are error interrupts raised by HW > > > blocks. > > > The count is provided to understand on how frequently the errors > > > are > > > seen. > > > Error recovery for some of the blocks happen internally. That is > > > the > > > reason, > > > Currently only dump op is added. > > > > So you are counting these events in driver, sounds like a counter > > to > > me, i really think this shouldn't belong to devlink, unless you > > really > > utilize devlink health ops for actual reporting and recovery. > > > > what's wrong with just dumping these counters to ethtool ? > > This driver is a administrative driver which handles all the > resources > in the system and doesn't do any IO. > NIX and NPA are key co-processor blocks which this driver handles. > With NIX and NPA, there are pieces > which gets attached to a PCI device to make it a networking device. > We > have netdev drivers registered to this > networking device. Some more information about the drivers is > available at > https://www.kernel.org/doc/html/latest/networking/device_drivers/ethernet/marvell/octeontx2.html > > So we don't have a netdev here to report these co-processor block > level errors over ethtool. > but AF driver can't be standalone to operate your hw, it must have a PF/VF with netdev interface to do io, so even if your model is modular, a common user of this driver will always see a netdev.
Re: [PATCH v2 net-next 3/3] octeontx2-af: Add devlink health reporters for NIX
On Thu, 2020-11-05 at 12:42 -0800, Jakub Kicinski wrote:
> On Thu, 05 Nov 2020 11:23:54 -0800 Saeed Mahameed wrote:
> > If you report an error without recovering, devlink health will
> > report a
> > bad device state
> >
> > $ ./devlink health
> >pci/0002:01:00.0:
> > reporter npa
> >state error error 1 recover 0
>
> Actually, the counter in the driver is unnecessary, right? Devlink
> counts errors.
>
if you mean error and recover counters, then yes. they are managed by
devlink health
every call to dl-health-report will do:
devlink_health_report(reporter, err_ctx, msg)
{
reproter.error++;
devlink_trigger_event(reporter, msg);
reporter.dump(err_ctx, msg);
reporter.diag(err_ctx);
if (!reporter.recover(err_ctx))
reporter.recover++;
}
so dl-health reports without a recover op will confuse the user if user
sees error count > recover count.
error count should only be grater than recover count when recover
procedure fails which now will indicate the device is not in a healthy
state.
also i want to clarify one small note about devlink dump.
devlink health dump semantics:
on devlink health dump, the devlink health will check if previous dump
exists and will just return it without actually calling the driver, if
not then it will call the driver to perform a new dump and will cache
it.
user has to explicitly clear the devlink health dump of that reporter
in order to allow for newer dump to get generated.
this is done this way because we want the driver to store the dump of
the previously reported errors at the moment the erorrs are reported by
driver, so when a user issue a dump command the dump of the previous
error will be reported to user form memory without the need to access
driver/hw who might be in a bad state.
so this is why using devlink dump for reporting counters doesn't really
work, it will only report the first time the counters are accessed via
devlink health dump, after that it will report the same cached values
over and over until the user clears it up.
> > So you will need to implement an empty recover op.
> > so if these events are informational only and they don't indicate
> > device health issues, why would you report them via devlink health
> > ?
>
> I see devlink health reporters a way of collecting errors reports
> which
> for the most part are just shared with the vendor. IOW firmware (or
> hardware) bugs.
>
> Obviously as you say without recover and additional context in the
> report the value is quite diminished. But _if_ these are indeed
> "report
> me to the vendor" kind of events then at least they should use our
> current mechanics for such reports - which is dl-health.
>
> Without knowing what these events are it's quite hard to tell if
> devlink health is an overkill or counter is sufficient.
>
> Either way - printing these to the logs is definitely the worst
> choice
> :)
Sure, I don't mind using devlink health for dump only, I don't really
have strong feelings against this, they can always extend it in the
future.
it just doesn't make sense to me to have it mainly used for dumping
counters and without using devlik helath utilities, like events,
reports and recover.
so maybe Sunil et al. could polish this patchset and provide more
devlink health support, like diagnose for these errors, dump HW
information and contexts related to these errors so they could debug
root causes, etc ..
Then the use for dl health in this series can be truly justified.
Re: [PATCH mlx5-next v1 05/11] net/mlx5: Register mlx5 devices to auxiliary virtual bus
On Sun, 2020-11-01 at 22:15 +0200, Leon Romanovsky wrote: > From: Leon Romanovsky > > Create auxiliary devices under new virtual bus. This will replace > the custom-made mlx5 ->add()/->remove() interfaces and next patches > will fill the missing callback and remove the old interface logic. > > The attachment of auxiliary drivers to the devices is possible in > 1-to-1 manner only and it requires us to create device for every > protocol, > so that device (module) will be able to connect to it. > > System with 2 IB and 1 RoCE cards: > [leonro@vm ~]$ lspci |grep nox > 00:09.0 Ethernet controller: Mellanox Technologies MT27800 Family > [ConnectX-5] > 00:0a.0 Ethernet controller: Mellanox Technologies MT28908 Family > [ConnectX-6] > 00:0b.0 Ethernet controller: Mellanox Technologies MT2910 Family > [ConnectX-7] > [leonro@vm ~]$ ls -l /sys/bus/auxiliary/devices/ > mlx5_core.eth.2 -> > ../../../devices/pci:00/:00:0b.0/mlx5_core.eth.2 > mlx5_core.rdma.0 -> > ../../../devices/pci:00/:00:09.0/mlx5_core.rdma.0 > mlx5_core.rdma.1 -> > ../../../devices/pci:00/:00:0a.0/mlx5_core.rdma.1 > mlx5_core.rdma.2 -> > ../../../devices/pci:00/:00:0b.0/mlx5_core.rdma.2 > mlx5_core.vdpa.1 -> > ../../../devices/pci:00/:00:0a.0/mlx5_core.vdpa.1 > mlx5_core.vdpa.2 -> > ../../../devices/pci:00/:00:0b.0/mlx5_core.vdpa.2 > [leonro@vm ~]$ rdma dev > 0: ibp0s9: node_type ca fw 4.6. node_guid 5254:00c0:fe12:3455 > sys_image_guid 5254:00c0:fe12:3455 > 1: ibp0s10: node_type ca fw 4.6. node_guid 5254:00c0:fe12:3456 > sys_image_guid 5254:00c0:fe12:3456 > 2: rdmap0s11: node_type ca fw 4.6. node_guid 5254:00c0:fe12:3457 > sys_image_guid 5254:00c0:fe12:3457 > > System with RoCE SR-IOV card with 4 VFs: > [leonro@vm ~]$ lspci |grep nox > 01:00.0 Ethernet controller: Mellanox Technologies MT28908 Family > [ConnectX-6] > 01:00.1 Ethernet controller: Mellanox Technologies MT28908 Family > [ConnectX-6 Virtual Function] > 01:00.2 Ethernet controller: Mellanox Technologies MT28908 Family > [ConnectX-6 Virtual Function] > 01:00.3 Ethernet controller: Mellanox Technologies MT28908 Family > [ConnectX-6 Virtual Function] > 01:00.4 Ethernet controller: Mellanox Technologies MT28908 Family > [ConnectX-6 Virtual Function] > [leonro@vm ~]$ ls -l /sys/bus/auxiliary/devices/ > mlx5_core.eth.0 -> > ../../../devices/pci:00/:00:09.0/:01:00.0/mlx5_core.eth.0 > mlx5_core.eth.1 -> > ../../../devices/pci:00/:00:09.0/:01:00.1/mlx5_core.eth.1 > mlx5_core.eth.2 -> > ../../../devices/pci:00/:00:09.0/:01:00.2/mlx5_core.eth.2 > mlx5_core.eth.3 -> > ../../../devices/pci:00/:00:09.0/:01:00.3/mlx5_core.eth.3 > mlx5_core.eth.4 -> > ../../../devices/pci:00/:00:09.0/:01:00.4/mlx5_core.eth.4 > mlx5_core.rdma.0 -> > ../../../devices/pci:00/:00:09.0/:01:00.0/mlx5_core.rdma. > 0 > mlx5_core.rdma.1 -> > ../../../devices/pci:00/:00:09.0/:01:00.1/mlx5_core.rdma. > 1 > mlx5_core.rdma.2 -> > ../../../devices/pci:00/:00:09.0/:01:00.2/mlx5_core.rdma. > 2 > mlx5_core.rdma.3 -> > ../../../devices/pci:00/:00:09.0/:01:00.3/mlx5_core.rdma. > 3 > mlx5_core.rdma.4 -> > ../../../devices/pci:00/:00:09.0/:01:00.4/mlx5_core.rdma. > 4 > mlx5_core.vdpa.1 -> > ../../../devices/pci:00/:00:09.0/:01:00.1/mlx5_core.vdpa. > 1 > mlx5_core.vdpa.2 -> > ../../../devices/pci:00/:00:09.0/:01:00.2/mlx5_core.vdpa. > 2 > mlx5_core.vdpa.3 -> > ../../../devices/pci:00/:00:09.0/:01:00.3/mlx5_core.vdpa. > 3 > mlx5_core.vdpa.4 -> > ../../../devices/pci:00/:00:09.0/:01:00.4/mlx5_core.vdpa. > 4 > [leonro@vm ~]$ rdma dev > 0: rocep1s0f0: node_type ca fw 4.6. node_guid 5254:00c0:fe12:3455 > sys_image_guid 5254:00c0:fe12:3455 > 1: rocep1s0f0v0: node_type ca fw 4.6. node_guid > ::: sys_image_guid 5254:00c0:fe12:3456 > 2: rocep1s0f0v1: node_type ca fw 4.6. node_guid > ::: sys_image_guid 5254:00c0:fe12:3457 > 3: rocep1s0f0v2: node_type ca fw 4.6. node_guid > ::: sys_image_guid 5254:00c0:fe12:3458 > 4: rocep1s0f0v3: node_type ca fw 4.6. node_guid > ::: sys_image_guid 5254:00c0:fe12:3459 > > Signed-off-by: Leon Romanovsky > --- > .../net/ethernet/mellanox/mlx5/core/Kconfig | 1 + > drivers/net/ethernet/mellanox/mlx5/core/dev.c | 265 > +- > .../net/ethernet/mellanox/mlx5/core/main.c| 24 +- > .../ethernet/mellanox/mlx5/core/mlx5_core.h | 20 +- > include/linux/mlx5/driver.h | 26 +- > 5 files changed, 325 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig > b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig > index 99f1ec3b2575..485478979b1a 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig > +++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig > @@ -6,6 +6,7 @@ > config MLX5_CORE > tristate
Re: [PATCH mlx5-next v1 04/11] vdpa/mlx5: Make hardware definitions visible to all mlx5 devices
On Sun, 2020-11-01 at 22:15 +0200, Leon Romanovsky wrote: > From: Leon Romanovsky > > Move mlx5_vdpa IFC header file to the general include folder, so > mlx5_core will be able to reuse it to check if VDPA is supported > prior to creating an auxiliary device. > I don't really like this, the whole idea of aux devices is that they get to do own logic and hide details, now we are exposing aux specific stuff to the bus .. let's figure a way to avoid such exposure as we discussed yesterday. is_supported check shouldn't belong to mlx5_core and each aux device (en/ib/vdpa) should implement own is_supported op and keep the details hidden in the aux driver like it was before this patch.
Re: [PATCH v2 net-next 3/3] octeontx2-af: Add devlink health reporters for NIX
On Thu, 2020-11-05 at 09:07 -0800, Jakub Kicinski wrote: > On Thu, 5 Nov 2020 13:36:56 + George Cherian wrote: > > > Now i am a little bit skeptic here, devlink health reporter > > > infrastructure was > > > never meant to deal with dump op only, the main purpose is to > > > diagnose/dump and recover. > > > > > > especially in your use case where you only report counters, i > > > don't believe > > > devlink health dump is a proper interface for this. > > These are not counters. These are error interrupts raised by HW > > blocks. > > The count is provided to understand on how frequently the errors > > are seen. > > Error recovery for some of the blocks happen internally. That is > > the reason, > > Currently only dump op is added. > > The previous incarnation was printing messages to logs, so I assume > these errors are expected to be relatively low rate. > > The point of using devlink health was that you can generate a netlink > notification when the error happens. IOW you need some calls to > devlink_health_report() or such. > > At least that's my thinking, others may disagree. If you report an error without recovering, devlink health will report a bad device state $ ./devlink health pci/0002:01:00.0: reporter npa state error error 1 recover 0 So you will need to implement an empty recover op. so if these events are informational only and they don't indicate device health issues, why would you report them via devlink health ?
Re: [PATCH v2 net-next 3/3] octeontx2-af: Add devlink health reporters for NIX
On Thu, 2020-11-05 at 13:36 +, George Cherian wrote: > Hi Saeed, > > Thanks for the review. > > > -Original Message----- > > From: Saeed Mahameed > > Sent: Thursday, November 5, 2020 10:39 AM > > To: George Cherian ; net...@vger.kernel.org; > > linux-kernel@vger.kernel.org; Jiri Pirko > > Cc: k...@kernel.org; da...@davemloft.net; Sunil Kovvuri Goutham > > ; Linu Cherian ; > > Geethasowjanya Akula ; masahi...@kernel.org; > > willemdebruijn.ker...@gmail.com > > Subject: Re: [PATCH v2 net-next 3/3] octeontx2-af: Add devlink > > health > > reporters for NIX > > > > On Wed, 2020-11-04 at 17:57 +0530, George Cherian wrote: > > > Add health reporters for RVU NPA block. > >^^^ NIX ? > > > Yes, it's NIX. > > > Cc: Jiri > > > > Anyway, could you please spare some words on what is NPA and what > > is > > NIX? > > > > Regarding the reporters names, all drivers register well known > > generic names > > such as (fw,hw,rx,tx), I don't know if it is a good idea to use > > vendor specific > > names, if you are reporting for hw/fw units then just use "hw" or > > "fw" as the > > reporter name and append the unit NPA/NIX to the counter/error > > names. > Okay. These are hw units, I will rename them as hw_npa/hw_nix. What i meant is have one reporter named "hw" and inside it report counters with their unit name appended to the counter name. ./devlink health pci/0002:01:00.0: reporter hw state healthy error 0 recover 0 ./devlink health dump show pci/0002:01:00.0 reporter hw NIX: nix_counter_a: 0 ... NPA: npa_counter_a: 0 ... > > > Only reporter dump is supported. > > > > > > Output: > > > # ./devlink health > > > pci/0002:01:00.0: > > >reporter npa > > > state healthy error 0 recover 0 > > >reporter nix > > > state healthy error 0 recover 0 > > > # ./devlink health dump show pci/0002:01:00.0 reporter nix > > > NIX_AF_GENERAL: > > > Memory Fault on NIX_AQ_INST_S read: 0 > > > Memory Fault on NIX_AQ_RES_S write: 0 > > > AQ Doorbell error: 0 > > > Rx on unmapped PF_FUNC: 0 > > > Rx multicast replication error: 0 > > > Memory fault on NIX_RX_MCE_S read: 0 > > > Memory fault on multicast WQE read: 0 > > > Memory fault on mirror WQE read: 0 > > > Memory fault on mirror pkt write: 0 > > > Memory fault on multicast pkt write: 0 > > >NIX_AF_RAS: > > > Poisoned data on NIX_AQ_INST_S read: 0 > > > Poisoned data on NIX_AQ_RES_S write: 0 > > > Poisoned data on HW context read: 0 > > > Poisoned data on packet read from mirror buffer: 0 > > > Poisoned data on packet read from mcast buffer: 0 > > > Poisoned data on WQE read from mirror buffer: 0 > > > Poisoned data on WQE read from multicast buffer: 0 > > > Poisoned data on NIX_RX_MCE_S read: 0 > > >NIX_AF_RVU: > > > Unmap Slot Error: 0 > > > > > > > Now i am a little bit skeptic here, devlink health reporter > > infrastructure was > > never meant to deal with dump op only, the main purpose is to > > diagnose/dump and recover. > > > > especially in your use case where you only report counters, i don't > > believe > > devlink health dump is a proper interface for this. > These are not counters. These are error interrupts raised by HW > blocks. > The count is provided to understand on how frequently the errors are > seen. > Error recovery for some of the blocks happen internally. That is the > reason, > Currently only dump op is added. So you are counting these events in driver, sounds like a counter to me, i really think this shouldn't belong to devlink, unless you really utilize devlink health ops for actual reporting and recovery. what's wrong with just dumping these counters to ethtool ? > > Many of these counters if not most are data path packet based and > > maybe > > they should belong to ethtool. > > Regards, > -George > > >
Re: [PATCH v2 net-next 3/3] octeontx2-af: Add devlink health reporters for NIX
On Wed, 2020-11-04 at 17:57 +0530, George Cherian wrote: > Add health reporters for RVU NPA block. ^^^ NIX ? Cc: Jiri Anyway, could you please spare some words on what is NPA and what is NIX? Regarding the reporters names, all drivers register well known generic names such as (fw,hw,rx,tx), I don't know if it is a good idea to use vendor specific names, if you are reporting for hw/fw units then just use "hw" or "fw" as the reporter name and append the unit NPA/NIX to the counter/error names. > Only reporter dump is supported. > > Output: > # ./devlink health > pci/0002:01:00.0: >reporter npa > state healthy error 0 recover 0 >reporter nix > state healthy error 0 recover 0 > # ./devlink health dump show pci/0002:01:00.0 reporter nix > NIX_AF_GENERAL: > Memory Fault on NIX_AQ_INST_S read: 0 > Memory Fault on NIX_AQ_RES_S write: 0 > AQ Doorbell error: 0 > Rx on unmapped PF_FUNC: 0 > Rx multicast replication error: 0 > Memory fault on NIX_RX_MCE_S read: 0 > Memory fault on multicast WQE read: 0 > Memory fault on mirror WQE read: 0 > Memory fault on mirror pkt write: 0 > Memory fault on multicast pkt write: 0 >NIX_AF_RAS: > Poisoned data on NIX_AQ_INST_S read: 0 > Poisoned data on NIX_AQ_RES_S write: 0 > Poisoned data on HW context read: 0 > Poisoned data on packet read from mirror buffer: 0 > Poisoned data on packet read from mcast buffer: 0 > Poisoned data on WQE read from mirror buffer: 0 > Poisoned data on WQE read from multicast buffer: 0 > Poisoned data on NIX_RX_MCE_S read: 0 >NIX_AF_RVU: > Unmap Slot Error: 0 > Now i am a little bit skeptic here, devlink health reporter infrastructure was never meant to deal with dump op only, the main purpose is to diagnose/dump and recover. especially in your use case where you only report counters, i don't believe devlink health dump is a proper interface for this. Many of these counters if not most are data path packet based and maybe they should belong to ethtool.
Re: [PATCH net-next] net/mlx5e: Remove duplicated include
On Sat, 2020-10-31 at 10:50 +0800, YueHaibing wrote: > Remove duplicated include. > > Signed-off-by: YueHaibing > --- > drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > index 599f5b5ebc97..58c177756dc4 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > @@ -52,7 +52,6 @@ > #include "en/xsk/rx.h" > #include "en/health.h" > #include "en/params.h" > -#include "en/txrx.h" Applied to net-next-mlx5, Thanks !
Re: [PATCH] vdpa/mlx5: Fix dependency on MLX5_CORE
On Wed, 2020-10-07 at 09:40 +0300, Eli Cohen wrote:
> Remove propmt for selecting MLX5_VDPA by the user and modify
> MLX5_VDPA_NET to select MLX5_VDPA. Also modify MLX5_VDPA_NET to
> depend
> on mlx5_core.
>
> This fixes an issue where configuration sets 'y' for MLX5_VDPA_NET
> while
> MLX5_CORE is compiled as a module causing link errors.
>
> Reported-by: kernel test robot
> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5
> device")s
> Signed-off-by: Eli Cohen
Reviewed-by: Saeed Mahameed
Re: [PATCH] net/mlx5e: Fix freeing of unassigned pointer
On Sat, 2020-10-03 at 12:10 +0100, Alex Dewar wrote:
> Commit ff7ea04ad579 ("net/mlx5e: Fix potential null pointer
> dereference")
> added some missing null checks but the error handling in
> mlx5e_alloc_flow() was left broken: the variable attr is passed to
> kfree
> although it is never assigned to and never needs to be freed in this
> function. Fix this.
>
> Addresses-Coverity-ID: 1497536 ("Memory - illegal accesses")
> Fixes: ff7ea04ad579 ("net/mlx5e: Fix potential null pointer
> dereference")
> Signed-off-by: Alex Dewar
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 17 +
>
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
Hi Alex, thanks for the patch,
Colin submitted a one liner patch that I already picked up.
I hope you are ok with this.
Thanks,
Saeed.
Re: [PATCH][next] net/mlx5: Fix uininitialized pointer read on pointer attr
On Tue, 2020-10-06 at 19:12 +0100, Colin King wrote:
> From: Colin Ian King
>
> Currently the error exit path err_free kfree's attr. In the case
> where
> flow and parse_attr failed to be allocated this return path will free
> the uninitialized pointer attr, which is not correct. In the other
> case where attr fails to allocate attr does not need to be freed. So
> in both error exits via err_free attr should not be freed, so remove
> it.
>
> Addresses-Coverity: ("Uninitialized pointer read")
> Fixes: ff7ea04ad579 ("net/mlx5e: Fix potential null pointer
> dereference")
> Signed-off-by: Colin Ian King
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index a0c356987e1a..e3a968e9e2a0 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -4569,7 +4569,6 @@ mlx5e_alloc_flow(struct mlx5e_priv *priv, int
> attr_size,
> err_free:
> kfree(flow);
> kvfree(parse_attr);
> - kfree(attr);
> return err;
> }
>
Applied to net-next-mlx5,
Thanks.
Re: net/mlx5: Refactor tc flow attributes structure
On Mon, 2020-09-28 at 17:06 +0100, Colin Ian King wrote: > Hi, > > static analysis with Coverity has found a null pointer dereference > issue > with the following commit: > > commit c620b772152b8274031083bdb2e11c963e596c5c > Author: Ariel Levkovich > Date: Thu Apr 30 05:54:08 2020 +0300 > > net/mlx5: Refactor tc flow attributes structure > > The analysis is as follows: > > 1240slow_attr = > mlx5_alloc_flow_attr(MLX5_FLOW_NAMESPACE_FDB); > > 1. Condition !slow_attr, taking true branch. > 2. var_compare_op: Comparing slow_attr to null implies that > slow_attr might be null. > > 1241if (!slow_attr) > 1242mlx5_core_warn(flow->priv->mdev, "Unable to > unoffload slow path rule\n"); > 1243 > 1244memcpy(slow_attr, flow->attr, ESW_FLOW_ATTR_SZ); > > Dereference after null check (FORWARD_NULL) > 3. var_deref_op: Dereferencing null pointer slow_attr. > > 1245slow_attr->action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST; > 1246slow_attr->esw_attr->split_count = 0; > 1247slow_attr->flags |= MLX5_ESW_ATTR_FLAG_SLOW_PATH; > 1248mlx5e_tc_unoffload_fdb_rules(esw, flow, slow_attr); > 1249flow_flag_clear(flow, SLOW); > 1250kfree(slow_attr); > > there is a !slow_attr check but if it slow_attr is null the code then > dereferences it multiple times afterwards. > > Colin Thanks Colin for the Report, Ariel is handling this internally and we will be posting the patch soon.
Re: [PATCH][next] net/mlx5e: Fix potential null pointer dereference
On Fri, 2020-09-25 at 11:49 -0500, Gustavo A. R. Silva wrote:
> Calls to kzalloc() and kvzalloc() should be null-checked
> in order to avoid any potential failures. In this case,
> a potential null pointer dereference.
>
> Fix this by adding null checks for _parse_attr_ and _flow_
> right after allocation.
>
> Addresses-Coverity-ID: 1497154 ("Dereference before null check")
> Fixes: c620b772152b ("net/mlx5: Refactor tc flow attributes
> structure")
> Signed-off-by: Gustavo A. R. Silva
> ---
>
Applied to net-next-mlx5.
Thanks.
Re: [PATCH] Revert "net: linkwatch: add check for netdevice being present to linkwatch_do_dev"
On Thu, 2020-09-24 at 09:03 -0700, Jakub Kicinski wrote: > On Wed, 23 Sep 2020 22:49:37 -0700 Saeed Mahameed wrote: > > 2) Another problematic scenario which i see is repeated in many > > drivers: > > > > shutdown/suspend() > > rtnl_lock() > > netif_device_detach()//Mark !present; > > stop()->carrier_off()->linkwatch_event() > > // at this point device is still IFF_UP and !present > > // due to the early detach above.. > > rtnl_unlock(); > > Maybe we can solve this by providing drivers with a better helper for > the suspend use case? > > AFAIU netif_device_detach() is used by both IO errors and drivers > willingly detaching the device during normal operation (e.g. for > suspend). > > Since the suspend path can sleep if we have a separate helper perhaps > we could fire off the appropriate events synchronously, and > quiescence > the stack properly? I was thinking something similar, a more heavy weight netif_device_detach(), which will be used in all drivers suspend flows. 1) clear IFF_UP 2) ndo_stop() 3) fire events 4) mark !present ... 5) suspend device but went and sampled some drivers and found there are many variations for using netif_device_detach it is not going to be a simple task.
Re: [PATCH] Revert "net: linkwatch: add check for netdevice being present to linkwatch_do_dev"
On Wed, 2020-09-23 at 17:23 -0700, David Miller wrote: > From: David Miller > Date: Wed, 23 Sep 2020 17:21:25 -0700 (PDT) > > > If an async code path tests 'present', gets true, and then the RTNL > > holding synchronous code path puts the device into D3hot > immediately > > afterwards, the async code path will still continue and access the > > chips registers and fault. > > Wait, is the sequence: > > ->ndo_stop() > mark device not present and put into D3hot > triggers linkwatch event > ... > ->ndo_get_stats64() > > ??? > I assume it is, since normally device drivers do carrier_off() on ndo_stop() 1) One problematic sequence would be (for drivers doing D3hot on ndo_stop()) __dev_close_many() ->ndo_stop() netif_device_detach() //Mark !present; ... D3hot carrier_off()->linkwatch_event() ... // !present && IFF_UP 2) Another problematic scenario which i see is repeated in many drivers: shutdown/suspend() rtnl_lock() netif_device_detach()//Mark !present; stop()->carrier_off()->linkwatch_event() // at this point device is still IFF_UP and !present // due to the early detach above.. rtnl_unlock(); For scenario 1) we can fix by marking IFF_UP at the beginning, but for 2), i think we need to fix the drivers to detach only after stop :( > Then yeah we might have to clear IFF_UP at the beginning of taking > a netdev down.
Re: [PATCH] Revert "net: linkwatch: add check for netdevice being present to linkwatch_do_dev"
On Wed, 2020-09-23 at 22:44 +0200, Heiner Kallweit wrote: > On 23.09.2020 22:15, David Miller wrote: > > From: Heiner Kallweit > > Date: Wed, 23 Sep 2020 21:58:59 +0200 > > > > > On 23.09.2020 20:35, Saeed Mahameed wrote: > > > > Why would a driver detach the device on ndo_stop() ? > > > > seems like this is the bug you need to be chasing .. > > > > which driver is doing this ? > > > > > > > Some drivers set the device to PCI D3hot at the end of ndo_stop() > > > to save power (using e.g. Runtime PM). Marking the device as > > > detached > > > makes clear to to the net core that the device isn't accessible > > > any > > > longer. > > > > That being the case, the problem is that IFF_UP+!present is not a > > valid netdev state. > > > If this combination is invalid, then netif_device_detach() should > clear IFF_UP? At a first glance this should be sufficient to avoid > the issue I was dealing with. > Feels like a work around and would conflict with the assumption that netif_device_detach() should only be called when !IFF_UP Maybe we need to clear IFF_UP before calling ops->ndo_stop(dev), instead of after on __dev_close_many(). Assuming no driver is checking IFF_UP state on its own ndo_stop(), other than this, the order shouldn't really matter, since clearing the flag and calling ndo_stop() should be considered as one atomic operation. > > Is it simply the issue that, upon resume, IFF_UP is marked true > > before > > the device is brought out from D3hot state and thus marked as > > present > > again? > > > I can't really comment on that. The issue I was dealing with at the > time I submitted this change was about an async linkwatch event > (caused by powering down the PHY in ndo_stop) trying to access the > device when it was powered down already.
Re: [PATCH] Revert "net: linkwatch: add check for netdevice being present to linkwatch_do_dev"
On Wed, 2020-09-23 at 13:49 +0200, Heiner Kallweit wrote:
> On 18.09.2020 19:58, Saeed Mahameed wrote:
> > On Tue, 2020-09-01 at 17:02 +0200, Geert Uytterhoeven wrote:
> > > This reverts commit 124eee3f6955f7aa19b9e6ff5c9b6d37cb3d1e2c.
> > >
> > > Inami-san reported that this commit breaks bridge support in a
> > > Xen
> > > environment, and that reverting it fixes this.
> > >
> > > During system resume, bridge ports are no longer enabled, as that
> > > relies
> > > on the receipt of the NETDEV_CHANGE notification. This
> > > notification
> > > is
> > > not sent, as netdev_state_change() is no longer called.
> > >
> > > Note that the condition this commit intended to fix never existed
> > > upstream, as the patch triggering it and referenced in the commit
> > > was
> > > never applied upstream. Hence I can confirm s2ram on
> > > r8a73a4/ape6evm
> > > and sh73a0/kzm9g works fine before/after this revert.
> > >
> > > Reported-by Gaku Inami
> > > Signed-off-by: Geert Uytterhoeven
> > > ---
> > > net/core/link_watch.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/core/link_watch.c b/net/core/link_watch.c
> > > index 75431ca9300fb9c4..c24574493ecf95e6 100644
> > > --- a/net/core/link_watch.c
> > > +++ b/net/core/link_watch.c
> > > @@ -158,7 +158,7 @@ static void linkwatch_do_dev(struct
> > > net_device
> > > *dev)
> > > clear_bit(__LINK_STATE_LINKWATCH_PENDING, >state);
> > >
> > > rfc2863_policy(dev);
> > > - if (dev->flags & IFF_UP && netif_device_present(dev)) {
> > > + if (dev->flags & IFF_UP) {
> >
> > So with your issue the devices is both IFF_UP and !present ? how so
> > ?
> > I think you should look into that.
> >
> > I am ok with removing the "dev present" check from here just
> > because we
> > shouldn't be expecting IFF_UP && !present .. such thing must be a
> > bug
> > somewhere else.
> >
> > > if (netif_carrier_ok(dev))
> > > dev_activate(dev);
> > > else
>
> In __dev_close_many() we call ndo_stop() whilst IFF_UP is still set.
> ndo_stop() may detach the device and bring down the PHY, resulting in
> an
Why would a driver detach the device on ndo_stop() ?
seems like this is the bug you need to be chasing ..
which driver is doing this ?
> async link change event that calls dev_get_stats(). The latter call
> may
> have a problem if the device is detached. In a first place I'd
> consider
> such a case a network driver bug (ndo_get_stats/64 should check for
> device presence if depending on it).
Device drivers should avoid presence check as much as possible
especially in ndo, this check must be performed by the stack.
> The additional check in linkwatch_do_dev() was meant to protect from
> such
> driver issues.
Re: [PATCH] net/mlx5: remove unreachable return
On Mon, 2020-09-21 at 22:54 -0700, Saeed Mahameed wrote: > On Mon, 2020-09-21 at 13:41 +0200, Pavel Machek wrote: > > The last return statement is unreachable code. I'm not sure if it > > will > > provoke any warnings, but it looks ugly. > > > > Signed-off-by: Pavel Machek (CIP) > > > > > > Applied to net-next-mlx5. > > Thanks, > Saeed. > Actually checkpatch reports this issue: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Pavel Machek ' Do you want me to override the Signed-off-by tag with the above email ? Thanks, Saeed.
Re: [PATCH -next] net/mlx5: simplify the return expression of mlx5_ec_init()
On Tue, 2020-09-22 at 08:04 -0700, Jakub Kicinski wrote: > On Mon, 21 Sep 2020 22:52:30 -0700 Saeed Mahameed wrote: > > On Mon, 2020-09-21 at 21:10 +0800, Qinglang Miao wrote: > > > Simplify the return expression. > > > > > > Signed-off-by: Qinglang Miao > > > --- > > > drivers/net/ethernet/mellanox/mlx5/core/ecpf.c | 6 +- > > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > > > > > > > Applied to net-next-mlx5. > > Beware: > > drivers/net/ethernet/mellanox/mlx5/core/ecpf.c: In function > ‘mlx5_ec_init’: > drivers/net/ethernet/mellanox/mlx5/core/ecpf.c:46:6: warning: unused > variable ‘err’ [-Wunused-variable] > 46 | int err = 0; > | ^~~ Thanks Jakub Yes, Saw this in my CI as well, will resolve this one myself. and for next time I will wait for the CI result before replying ;)
Re: [PATCH] net/mlx5: remove unreachable return
On Mon, 2020-09-21 at 13:41 +0200, Pavel Machek wrote: > The last return statement is unreachable code. I'm not sure if it > will > provoke any warnings, but it looks ugly. > > Signed-off-by: Pavel Machek (CIP) > > Applied to net-next-mlx5. Thanks, Saeed.
Re: [PATCH -next] net/mlx5: simplify the return expression of mlx5_ec_init()
On Mon, 2020-09-21 at 21:10 +0800, Qinglang Miao wrote: > Simplify the return expression. > > Signed-off-by: Qinglang Miao > --- > drivers/net/ethernet/mellanox/mlx5/core/ecpf.c | 6 +- > 1 file changed, 1 insertion(+), 5 deletions(-) > > Applied to net-next-mlx5. Thanks.
Re: [PATCH 2/2] net/mlx5e: Use kfree() to free fd->g in accel_fs_tcp_create_groups()
On Mon, 2020-09-21 at 19:23 +0300, Denis Efremov wrote: > Memory ft->g in accel_fs_tcp_create_groups() is allocaed with > kcalloc(). > It's excessive to free ft->g with kvfree(). Use kfree() instead. > > Signed-off-by: Denis Efremov > --- > series applied to net-next-mlx5
Re: [PATCH net-next] hinic: modify irq name
On Fri, 2020-09-18 at 17:23 +0800, Luo bin wrote: > Make a distinction between different irqs by netdev name or pci name. > > Signed-off-by: Luo bin > --- > Reviewed-by: Saeed Mahameed
Re: [PATCH] Revert "net: linkwatch: add check for netdevice being present to linkwatch_do_dev"
On Tue, 2020-09-01 at 17:02 +0200, Geert Uytterhoeven wrote:
> This reverts commit 124eee3f6955f7aa19b9e6ff5c9b6d37cb3d1e2c.
>
> Inami-san reported that this commit breaks bridge support in a Xen
> environment, and that reverting it fixes this.
>
> During system resume, bridge ports are no longer enabled, as that
> relies
> on the receipt of the NETDEV_CHANGE notification. This notification
> is
> not sent, as netdev_state_change() is no longer called.
>
> Note that the condition this commit intended to fix never existed
> upstream, as the patch triggering it and referenced in the commit was
> never applied upstream. Hence I can confirm s2ram on r8a73a4/ape6evm
> and sh73a0/kzm9g works fine before/after this revert.
>
> Reported-by Gaku Inami
> Signed-off-by: Geert Uytterhoeven
> ---
> net/core/link_watch.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/link_watch.c b/net/core/link_watch.c
> index 75431ca9300fb9c4..c24574493ecf95e6 100644
> --- a/net/core/link_watch.c
> +++ b/net/core/link_watch.c
> @@ -158,7 +158,7 @@ static void linkwatch_do_dev(struct net_device
> *dev)
> clear_bit(__LINK_STATE_LINKWATCH_PENDING, >state);
>
> rfc2863_policy(dev);
> - if (dev->flags & IFF_UP && netif_device_present(dev)) {
> + if (dev->flags & IFF_UP) {
So with your issue the devices is both IFF_UP and !present ? how so ?
I think you should look into that.
I am ok with removing the "dev present" check from here just because we
shouldn't be expecting IFF_UP && !present .. such thing must be a bug
somewhere else.
> if (netif_carrier_ok(dev))
> dev_activate(dev);
> else
Re: [PATCH net-next] liquidio: Fix -Wmissing-prototypes warnings for liquidio
On Fri, 2020-09-18 at 21:02 +0800, Wang Hai wrote: > If the header file containing a function's prototype isn't included > by > the sourcefile containing the associated function, the build system > complains of missing prototypes. > > Fixes the following W=1 kernel build warning(s): > > drivers/net/ethernet/cavium/liquidio/cn68xx_device.c:124:5: warning: > no previous prototype for ‘lio_setup_cn68xx_octeon_device’ [- > Wmissing-prototypes] > drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:159:1: warning: > no previous prototype for ‘octeon_pci_read_core_mem’ [-Wmissing- > prototypes] > drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:168:1: warning: > no previous prototype for ‘octeon_pci_write_core_mem’ [-Wmissing- > prototypes] > drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:176:5: warning: > no previous prototype for ‘octeon_read_device_mem64’ [-Wmissing- > prototypes] > drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:185:5: warning: > no previous prototype for ‘octeon_read_device_mem32’ [-Wmissing- > prototypes] > drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:194:6: warning: > no previous prototype for ‘octeon_write_device_mem32’ [-Wmissing- > prototypes] > > Signed-off-by: Wang Hai > Reviewed-by: Saeed Mahameed
Re: [PATCH net-next] net: hns3: Supply missing hclge_dcb.h include file
On Fri, 2020-09-18 at 21:06 +0800, Wang Hai wrote: > If the header file containing a function's prototype isn't included > by > the sourcefile containing the associated function, the build system > complains of missing prototypes. > > Fixes the following W=1 kernel build warning(s): > > drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c:453:6: > warning: no previous prototype for ‘hclge_dcb_ops_set’ [-Wmissing- > prototypes] > > Signed-off-by: Wang Hai > --- Reviewed-by: Saeed Mahameed
Re: [PATCH -next] vdpa: mlx5: select VHOST to fix build errors
On Thu, 2020-09-17 at 11:58 -0700, Randy Dunlap wrote: > From: Randy Dunlap > > drivers/vdpa/mlx5/ uses vhost_iotlb*() interfaces, so select > VHOST to eliminate build errors. > > ld: drivers/vdpa/mlx5/core/mr.o: in function `add_direct_chain': > mr.c:(.text+0x106): undefined reference to `vhost_iotlb_itree_first' > ld: mr.c:(.text+0x1cf): undefined reference to > `vhost_iotlb_itree_next' > ld: mr.c:(.text+0x30d): undefined reference to > `vhost_iotlb_itree_first' > ld: mr.c:(.text+0x3e8): undefined reference to > `vhost_iotlb_itree_next' > ld: drivers/vdpa/mlx5/core/mr.o: in function `_mlx5_vdpa_create_mr': > mr.c:(.text+0x908): undefined reference to `vhost_iotlb_itree_first' > ld: mr.c:(.text+0x9e6): undefined reference to > `vhost_iotlb_itree_next' > ld: drivers/vdpa/mlx5/core/mr.o: in function > `mlx5_vdpa_handle_set_map': > mr.c:(.text+0xf1d): undefined reference to `vhost_iotlb_itree_first' > > Signed-off-by: Randy Dunlap > Cc: "Michael S. Tsirkin" > Cc: Jason Wang > Cc: virtualizat...@lists.linux-foundation.org > Cc: Saeed Mahameed > Cc: Leon Romanovsky > Cc: net...@vger.kernel.org > --- > Note: This patch may not be the right thing, but it fixes the build > errors. > > drivers/vdpa/Kconfig |1 + > 1 file changed, 1 insertion(+) > > --- linux-next-20200917.orig/drivers/vdpa/Kconfig > +++ linux-next-20200917/drivers/vdpa/Kconfig > @@ -32,6 +32,7 @@ config IFCVF > config MLX5_VDPA > bool "MLX5 VDPA support library for ConnectX devices" > depends on MLX5_CORE > + select VHOST select keyword usually complicates things. It is better if you add a dependency rather than forcing VHOST. Just do: depends on VHOST & MLX5_CORE > default n > help > Support library for Mellanox VDPA drivers. Provides code that > is >
Re: linux-next: Signed-off-by missing for commit in the net-next tree
On Thu, 2020-09-17 at 08:31 +1000, Stephen Rothwell wrote:
> Hi all,
>
> Commits
>
> 0d2ffdc8d400 ("net/mlx5: Don't call timecounter cyc2time directly
> from 1PPS flow")
> 87f3495cbe8d ("net/mlx5: Release clock lock before scheduling a PPS
> work")
> aac2df7f022e ("net/mlx5: Rename ptp clock info")
> fb609b5112bd ("net/mlx5: Always use container_of to find mdev
> pointer from clock struct")
> ec529b44abfe ("net/mlx5: remove erroneous fallthrough")
>
> are missing a Signed-off-by from their committer.
>
Sorry for the mix-up, i overwrote the old mlnx email address with the
new nvidia one but didn't consider the committer email :S.
Re: [PATCH V2 net-next 0/6] net: hns3: updates for -next
On Wed, 2020-09-16 at 17:33 +0800, Huazhong Tan wrote: > There are some optimizations related to IO path. > > Change since V1: > - fixes a unsuitable handling in hns3_lb_clear_tx_ring() of #6 which > pointed out by Saeed Mahameed. > > previous version: > V1: > https://patchwork.ozlabs.org/project/netdev/cover/1600085217-26245-1-git-send-email-tanhuazh...@huawei.com/ > > Yunsheng Lin (6): > net: hns3: batch the page reference count updates > net: hns3: batch tx doorbell operation > net: hns3: optimize the tx clean process > net: hns3: optimize the rx clean process > net: hns3: use writel() to optimize the barrier operation > net: hns3: use napi_consume_skb() when cleaning tx desc > > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c| 225 > - > drivers/net/ethernet/hisilicon/hns3/hns3_enet.h| 20 +- > drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 6 +- > 3 files changed, 140 insertions(+), 111 deletions(-) > Reviewed-by: Saeed Mahameed
Re: [PATCH net-next] netdev: Remove unused funtions
On Wed, 2020-09-16 at 22:18 +0800, YueHaibing wrote:
> There is no callers in tree, so can remove it.
>
You have a typo in the patch title:
funtions -> functions
> Signed-off-by: YueHaibing
Please feel free to add my R.B tag after on V2.
Reviewed-by: Saeed Mahameed
And by the way, you have 3 patches doing similar things, please
consider submitting them as one series on V2:
$ git format-patch --cover-letter \
--subject-prefix="PATCH net-next" HEAD~3.. -o patches/
> ---
> include/linux/netdevice.h | 10 --
> 1 file changed, 10 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 157e0242e9ee..909b1fbb0481 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4677,16 +4677,6 @@ int netdev_class_create_file_ns(const struct
> class_attribute *class_attr,
> void netdev_class_remove_file_ns(const struct class_attribute
> *class_attr,
>const void *ns);
>
> -static inline int netdev_class_create_file(const struct
> class_attribute *class_attr)
> -{
> - return netdev_class_create_file_ns(class_attr, NULL);
> -}
> -
> -static inline void netdev_class_remove_file(const struct
> class_attribute *class_attr)
> -{
> - netdev_class_remove_file_ns(class_attr, NULL);
> -}
> -
> extern const struct kobj_ns_type_operations net_ns_type_operations;
>
> const char *netdev_drivername(const struct net_device *dev);
Re: [PATCH net-next 6/6] net: hns3: use napi_consume_skb() when cleaning tx desc
On Tue, 2020-09-15 at 15:04 +0800, Yunsheng Lin wrote:
> On 2020/9/15 13:09, Saeed Mahameed wrote:
> > On Mon, 2020-09-14 at 20:06 +0800, Huazhong Tan wrote:
> > > From: Yunsheng Lin
> > >
> > > Use napi_consume_skb() to batch consuming skb when cleaning
> > > tx desc in NAPI polling.
> > >
> > > Signed-off-by: Yunsheng Lin
> > > Signed-off-by: Huazhong Tan
> > > ---
> > > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c| 27
> > > +++-
> > > --
> > > drivers/net/ethernet/hisilicon/hns3/hns3_enet.h| 2 +-
> > > drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 4 ++--
> > > 3 files changed, 17 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> > > b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> > > index 4a49a76..feeaf75 100644
> > > --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> > > @@ -2333,10 +2333,10 @@ static int hns3_alloc_buffer(struct
> > > hns3_enet_ring *ring,
> > > }
> > >
> > > static void hns3_free_buffer(struct hns3_enet_ring *ring,
> > > - struct hns3_desc_cb *cb)
> > > + struct hns3_desc_cb *cb, int budget)
> > > {
> > > if (cb->type == DESC_TYPE_SKB)
> > > - dev_kfree_skb_any((struct sk_buff *)cb->priv);
> > > + napi_consume_skb(cb->priv, budget);
> >
> > This code can be reached from hns3_lb_clear_tx_ring() below which
> > is
> > your loopback test and called with non-zero budget, I am not sure
> > you
> > are allowed to call napi_consume_skb() with non-zero budget outside
> > napi context, perhaps the cb->type for loopback test is different
> > in lb
> > test case ? Idk.. , please double check other code paths.
>
> Yes, loopback test may call napi_consume_skb() with non-zero budget
> outside
> napi context. Thanks for pointing out this case.
>
> How about add the below WARN_ONCE() in napi_consume_skb() to catch
> this
> kind of error?
>
> WARN_ONCE(!in_serving_softirq(), "napi_consume_skb() is called with
> non-zero budget outside napi context");
>
Cc: Eric
I don't know, need to check performance impact.
And in_serving_softirq() doesn't necessarily mean in napi
but looking at _kfree_skb_defer(), i think it shouldn't care if napi or
not as long as it runs in soft irq it will push the skb to that
particular cpu napi_alloc_cache, which should be fine.
Maybe instead of the WARN_ONCE just remove the budget condition and
replace it with
if (!in_serving_softirq())
dev_consume_skb_any(skb);
Re: [PATCH net-next 6/6] net: hns3: use napi_consume_skb() when cleaning tx desc
On Mon, 2020-09-14 at 20:06 +0800, Huazhong Tan wrote:
> From: Yunsheng Lin
>
> Use napi_consume_skb() to batch consuming skb when cleaning
> tx desc in NAPI polling.
>
> Signed-off-by: Yunsheng Lin
> Signed-off-by: Huazhong Tan
> ---
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c| 27 +++-
> --
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.h| 2 +-
> drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 4 ++--
> 3 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 4a49a76..feeaf75 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -2333,10 +2333,10 @@ static int hns3_alloc_buffer(struct
> hns3_enet_ring *ring,
> }
>
> static void hns3_free_buffer(struct hns3_enet_ring *ring,
> - struct hns3_desc_cb *cb)
> + struct hns3_desc_cb *cb, int budget)
> {
> if (cb->type == DESC_TYPE_SKB)
> - dev_kfree_skb_any((struct sk_buff *)cb->priv);
> + napi_consume_skb(cb->priv, budget);
This code can be reached from hns3_lb_clear_tx_ring() below which is
your loopback test and called with non-zero budget, I am not sure you
are allowed to call napi_consume_skb() with non-zero budget outside
napi context, perhaps the cb->type for loopback test is different in lb
test case ? Idk.. , please double check other code paths.
[...]
> static void hns3_lb_clear_tx_ring(struct hns3_nic_priv *priv, u32
> start_ringid,
> - u32 end_ringid, u32 budget)
> + u32 end_ringid, int budget)
> {
> u32 i;
>
> for (i = start_ringid; i <= end_ringid; i++) {
> struct hns3_enet_ring *ring = >ring[i];
>
> - hns3_clean_tx_ring(ring);
> + hns3_clean_tx_ring(ring, budget);
> }
> }
>
Re: [PATCH] ath10k: sdio: remove reduntant check in for loop
On Mon, 2020-09-14 at 20:19 +0100, Alex Dewar wrote:
> The for loop checks whether cur_section is NULL on every iteration,
> but
> we know it can never be NULL as there is another check towards the
> bottom of the loop body. Remove this unnecessary check.
>
> Also change i to start at 1, so that we don't need an extra +1 wheno
> we
> use it.
>
> Addresses-Coverity: 1496984 ("Null pointer dereferences)
> Signed-off-by: Alex Dewar
> ---
> drivers/net/wireless/ath/ath10k/sdio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c
> b/drivers/net/wireless/ath/ath10k/sdio.c
> index 81ddaafb6721..f31ab2ec2c48 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -2308,7 +2308,7 @@ static int
> ath10k_sdio_dump_memory_section(struct ath10k *ar,
>
> count = 0;
>
> - for (i = 0; cur_section; i++) {
> + for (i = 1; ; i++) {
'i' is only referenced once inside the loop to check boundary,
the loop is actually iterating over cur_section, so i would make it
clear in the loop statement, e.g.:
Remove the break condition and the cur_section assignment at the end of
the loop and use the loop statement to do it for you
for (; cur_section; cur_section = next_section)
> section_size = cur_section->end - cur_section->start;
>
> if (section_size <= 0) {
> @@ -2318,7 +2318,7 @@ static int
> ath10k_sdio_dump_memory_section(struct ath10k *ar,
> break;
> }
>
> - if ((i + 1) == mem_region->section_table.size) {
And for i you can just increment it inline:
if (++i == ...)
> + if (i == mem_region->section_table.size) {
> /* last section */
> next_section = NULL;
> skip_size = 0;
Re: [PATCH 4.19] net/mlx5e: Don't support phys switch id if not in switchdev mode
On Fri, 2020-08-07 at 15:13 +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 06, 2020 at 07:05:42PM -0700, Saeed Mahameed wrote:
> > From: Roi Dayan
> >
> > Support for phys switch id ndo added for representors and if
> > we do not have representors there is no need to support it.
> > Since each port return different switch id supporting this
> > block support for creating bond over PFs and attaching to bridge
> > in legacy mode.
> >
> > This bug doesn't exist upstream as the code got refactored and the
> > netdev api is totally different.
> >
> > Fixes: cb67b832921c ("net/mlx5e: Introduce SRIOV VF representors")
> > Signed-off-by: Roi Dayan
> > Signed-off-by: Saeed Mahameed
> > ---
> > Hi Greg,
> >
> > Sorry for submitting a non upstream patch, but this bug is
> > bothering some users on 4.19-stable kernels and it doesn't exist
> > upstream, so i hope you are ok with backporting this one liner
> > patch.
>
> Also queued up to 4.9.y and 4.14.y.
>
Hi Greg, the request was originally made for 4.19.y kernel,
I see the patch in 4.9 and 4.14 but not in 4.19 can we push it to 4.19
as well ?
Thanks,
Saeed.
Re: [PATCH net-next] net/mlx5e: kTLS, Avoid kzalloc(GFP_KERNEL) under spinlock
On Tue, 2020-09-01 at 22:35 +0800, YueHaibing wrote:
> A spin lock is held before kzalloc, it may sleep with holding
> the spinlock, so we should use GFP_ATOMIC instead.
>
> This is detected by coccinelle.
>
> Fixes: 0419d8c9d8f8 ("net/mlx5e: kTLS, Add kTLS RX resync support")
> Signed-off-by: YueHaibing
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git
> a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
> index acf6d80a6bb7..1a32435acac3 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
> @@ -247,7 +247,7 @@ resync_post_get_progress_params(struct
> mlx5e_icosq *sq,
> int err;
> u16 pi;
>
> - buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> + buf = kzalloc(sizeof(*buf), GFP_ATOMIC);
> if (unlikely(!buf)) {
> err = -ENOMEM;
> goto err_out;
Thanks for the patch, the kzalloc can move outside the spinlock.
This patch should also go to net.
I will provide a newer version of the patch to deal with this and with
a missing kfree on error handling i found while looking at the code.
Thanks,
Saeed.
[PATCH 4.19] net/mlx5e: Don't support phys switch id if not in switchdev mode
From: Roi Dayan
Support for phys switch id ndo added for representors and if
we do not have representors there is no need to support it.
Since each port return different switch id supporting this
block support for creating bond over PFs and attaching to bridge
in legacy mode.
This bug doesn't exist upstream as the code got refactored and the
netdev api is totally different.
Fixes: cb67b832921c ("net/mlx5e: Introduce SRIOV VF representors")
Signed-off-by: Roi Dayan
Signed-off-by: Saeed Mahameed
---
Hi Greg,
Sorry for submitting a non upstream patch, but this bug is
bothering some users on 4.19-stable kernels and it doesn't exist
upstream, so i hope you are ok with backporting this one liner patch.
Thanks !!
drivers/net/ethernet/mellanox/mlx5/core/en_rep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index 701624a63d2f..1ab40d622ae1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -198,7 +198,7 @@ int mlx5e_attr_get(struct net_device *dev, struct
switchdev_attr *attr)
struct mlx5_eswitch_rep *rep = rpriv->rep;
struct mlx5_eswitch *esw = priv->mdev->priv.eswitch;
- if (esw->mode == SRIOV_NONE)
+ if (esw->mode != SRIOV_OFFLOADS)
return -EOPNOTSUPP;
switch (attr->id) {
--
2.8.4
Re: [PATCH V4 linux-next 00/12] VDPA support for Mellanox ConnectX devices
On Wed, 2020-08-05 at 09:12 -0400, Michael S. Tsirkin wrote: > On Wed, Aug 05, 2020 at 04:01:58PM +0300, Eli Cohen wrote: > > On Wed, Aug 05, 2020 at 08:48:52AM -0400, Michael S. Tsirkin wrote: > > > > Did you merge this?: > > > > git pull > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.gi > > > > t mlx5-next > > > > > > I can only merge this tree if no one else will. Linus does not > > > like > > > getting same patches through two trees. > > > > > > Is this the case? Is mlx5-next going to be merged through > > > my tree in this cycle? > > > > > > > Saeed Mahameed from Mellanox (located in California) usuaally sends > > out > > net patches. So he's supposed to send that to Dave Miller. > > > > I think Saeed should answer this. Let's wait a few more hours till > > he > > wakes up. > > Alternatives: > - merge vdpa through Saeed's tree. I can ack that, we'll need to > resolve any conflicts by merging the two trees and show the > result to Linus so he can resolve the merge in the same way. > - extract just the necessary patches that are needed for vdpa and > merge through my tree. > - if Saeed sends his pull today, it's likely it will be merged > early next week. Then I can rebase and send a pull with your > patches > on top. A bit risky. > - do some tricks with build. E.g. disable build of your code, > and enable in Saeed's tree when everything is merged together. > Can be somewhat hard. > Hi Michael, We do this all the time with net-next and rdma, mlx5-next is a very small branch based on a very early rc that includes mlx5 shared stuff between rdma and net-next, and now virtio as well. we send pull requests of mlx5-next to both rdma and net-next with the respective features, exactly as we did here, and it works nicely, since we reduce the number of conflicts to 0 between different subsystems that rely on mlx5 core. all the alternative you suggested have never been tried before :), net-next is Closed, so i can't do further submissions.
Re: [PATCH v2] net/mlx5e: fix bpf_prog reference count leaks in mlx5e_alloc_rq
On Thu, 2020-07-30 at 18:29 +0800, Xin Xiong wrote:
> The function invokes bpf_prog_inc(), which increases the reference
> count of a bpf_prog object "rq->xdp_prog" if the object isn't NULL.
>
> The refcount leak issues take place in two error handling paths. When
> either mlx5_wq_ll_create() or mlx5_wq_cyc_create() fails, the
> function
> simply returns the error code and forgets to drop the reference count
> increased earlier, causing a reference count leak of "rq->xdp_prog".
>
> Fix this issue by jumping to the error handling path
> err_rq_wq_destroy
> while either function fails.
>
> Fixes: 422d4c401edd ("net/mlx5e: RX, Split WQ objects for different
> RQ
> types")
>
Please don't break the line of the fixes tag.
I will fix this up.
> Signed-off-by: Xin Xiong
> Signed-off-by: Xiyu Yang
> Signed-off-by: Xin Tan
> ---
> v1 -> v2:
> - Amended parts of wording to be better understood
> - Added Fixes tag
> ---
Applied to net-mlx5, Thanks !
Re: [PATCH] net/mlx5e: fix bpf_prog refcnt leaks in mlx5e_alloc_rq
On Wed, 2020-07-29 at 13:28 -0700, David Miller wrote:
> From: Saeed Mahameed
> Date: Wed, 29 Jul 2020 19:02:15 +
>
> >> Fix this issue by jumping to the error handling path
> >> err_rq_wq_destroy
> >> when either function fails.
> >>
> >
> > Fixes: 422d4c401edd ("net/mlx5e: RX, Split WQ objects for different
> RQ
> > types")
>
> Saeed, are you going to take this into your tree or would you like me
> to
> apply it directly?
>
> Thanks.
I will take this to my tree once Xin adds the missing Fixes tag.
Thanks.
Re: [PATCH] net/mlx5e: fix bpf_prog refcnt leaks in mlx5e_alloc_rq
On Wed, 2020-07-29 at 20:33 +0800, Xin Xiong wrote:
> The function invokes bpf_prog_inc(), which increases the refcount of
> a
> bpf_prog object "rq->xdp_prog" if the object isn't NULL.
>
> The refcount leak issues take place in two error handling paths. When
> mlx5_wq_ll_create() or mlx5_wq_cyc_create() fails, the function
> simply
> returns the error code and forgets to drop the refcount increased
> earlier, causing a refcount leak of "rq->xdp_prog".
>
> Fix this issue by jumping to the error handling path
> err_rq_wq_destroy
> when either function fails.
>
Fixes: 422d4c401edd ("net/mlx5e: RX, Split WQ objects for different RQ
types")
>
> Signed-off-by: Xin Xiong
> Signed-off-by: Xiyu Yang
> Signed-off-by: Xin Tan
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index a836a02a2116..8e1b1ab416d8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -419,7 +419,7 @@ static int mlx5e_alloc_rq(struct mlx5e_channel
> *c,
> err = mlx5_wq_ll_create(mdev, >wq, rqc_wq,
> >mpwqe.wq,
> >wq_ctrl);
> if (err)
> - return err;
> + goto err_rq_wq_destroy;
>
> rq->mpwqe.wq.db = >mpwqe.wq.db[MLX5_RCV_DBR];
>
> @@ -470,7 +470,7 @@ static int mlx5e_alloc_rq(struct mlx5e_channel
> *c,
> err = mlx5_wq_cyc_create(mdev, >wq, rqc_wq,
> >wqe.wq,
>>wq_ctrl);
> if (err)
> - return err;
> + goto err_rq_wq_destroy;
>
> rq->wqe.wq.db = >wqe.wq.db[MLX5_RCV_DBR];
>
Re: [PATCH][next] net/mlx5: Use fallthrough pseudo-keyword
On Mon, 2020-07-27 at 13:24 -0700, David Miller wrote: > From: "Gustavo A. R. Silva" > Date: Mon, 27 Jul 2020 13:03:56 -0500 > > > Replace the existing /* fall through */ comments and its variants > with > > the new pseudo-keyword macro fallthrough[1]. Also, remove > unnecessary > > fall-through markings when it is the case. > > > > [1] > https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through > > > > Signed-off-by: Gustavo A. R. Silva > > Saeed, please pick this up. > > Thank you. Applied to net-next-mlx5. Thanks.
Re: [PATCH 4/7] net/mlx5: drop unnecessary list_empty
On Mon, 2020-07-27 at 10:10 -0700, David Miller wrote: > From: Julia Lawall > Date: Sun, 26 Jul 2020 12:58:29 +0200 > > > list_for_each_entry is able to handle an empty list. > > The only effect of avoiding the loop is not initializing the > > index variable. > > Drop list_empty tests in cases where these variables are not > > used. > > > > Note that list_for_each_entry is defined in terms of > list_first_entry, > > which indicates that it should not be used on an empty list. But > in > > list_for_each_entry, the element obtained by list_first_entry is > not > > really accessed, only the address of its list_head field is > compared > > to the address of the list head, so the list_first_entry is safe. > > > > The semantic patch that makes this change is as follows (with > another > > variant for the no brace case): (http://coccinelle.lip6.fr/) > ... > > Signed-off-by: Julia Lawall > > Saeed, please pick this up. > > Thank you. Applied to net-next-mlx5. Thanks !
Re: mmotm 2020-07-09-21-00 uploaded (drivers/net/ethernet/mellanox/mlx5/core/en_main.c)
On Sun, 2020-07-12 at 21:17 -0700, Randy Dunlap wrote:
> On 7/12/20 9:02 PM, Stephen Rothwell wrote:
> > Hi Randy,
> >
> > On Fri, 10 Jul 2020 10:40:29 -0700 Randy Dunlap <
> > rdun...@infradead.org> wrote:
> > > on i386:
> > >
> > > In file included from
> > > ../drivers/net/ethernet/mellanox/mlx5/core/en_main.c:49:0:
> > > ../drivers/net/ethernet/mellanox/mlx5/core/en_accel/en_accel.h:
> > > In function ‘mlx5e_accel_sk_get_rxq’:
> > > ../drivers/net/ethernet/mellanox/mlx5/core/en_accel/en_accel.h:15
> > > 3:12: error: implicit declaration of function ‘sk_rx_queue_get’;
> > > did you mean ‘sk_rx_queue_set’? [-Werror=implicit-function-
> > > declaration]
> > > int rxq = sk_rx_queue_get(sk);
> > > ^~~
> > > sk_rx_queue_set
> >
> > Caused by commit
> >
> > 1182f3659357 ("net/mlx5e: kTLS, Add kTLS RX HW offload support")
> >
> > from the net-next tree. Presumably CONFIG_XPS is not set.
>
> Yes, that's correct.
>
Thanks for the report, we will handle and submit a patch to net-next.
Re: [PATCH net-next v2 3/4] mlx5: become aware of when running as a bonding slave
On Sun, 2020-06-21 at 16:25 -0400, Jarod Wilson wrote: > On Thu, Jun 11, 2020 at 5:51 PM Saeed Mahameed > wrote: > > On Wed, 2020-06-10 at 14:59 -0400, Jarod Wilson wrote: > > > I've been unable to get my hands on suitable supported hardware > > > to > > > date, > > > but I believe this ought to be all that is needed to enable the > > > mlx5 > > > driver to also work with bonding active-backup crypto offload > > > passthru. > > > > > > CC: Boris Pismenny > > > CC: Saeed Mahameed > > > CC: Leon Romanovsky > > > CC: Jay Vosburgh > > > CC: Veaceslav Falico > > > CC: Andy Gospodarek > > > CC: "David S. Miller" > > > CC: Jeff Kirsher > > > CC: Jakub Kicinski > > > CC: Steffen Klassert > > > CC: Herbert Xu > > > CC: net...@vger.kernel.org > > > Signed-off-by: Jarod Wilson > > > --- > > > drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c | 6 > > > ++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git > > > a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c > > > b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c > > > index 92eb3bad4acd..72ad6664bd73 100644 > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c > > > @@ -210,6 +210,9 @@ static inline int > > > mlx5e_xfrm_validate_state(struct xfrm_state *x) > > > struct net_device *netdev = x->xso.dev; > > > struct mlx5e_priv *priv; > > > > > > + if (x->xso.slave_dev) > > > + netdev = x->xso.slave_dev; > > > + > > > > Do we really need to repeat this per driver ? > > why not just setup xso.real_dev, in xfrm layer once and for all > > before > > calling device drivers ? > > > > Device drivers will use xso.real_dev blindly. > > > > Will be useful in the future when you add vlan support, etc.. > > Apologies, I didn't catch your reply until just recently. Yeah, that > sounds like a better approach, if I can work it out cleanly. We just > init xso.real_dev to the same thing as xso.dev, then overwrite it in > the upper layer drivers (bonding, vlan, etc), while device drivers > just always use xso.real_dev, if I'm understanding your suggestion. > I'll see what I can come up with. > > Yes, exactly what i meant, Thanks !
Re: [REGRESSION] mlx5: Driver remove during hot unplug is broken
On Fri, 2020-06-12 at 15:09 +0200, Niklas Schnelle wrote:
> Hello Parav, Hello Saeed,
>
> our CI system for IBM Z Linux found a hang[0] when hot unplugging a
> ConnectX-4 Lx VF from a z/VM guest
> in Linus' current tree and added during the merge window.
> Sadly it didn't happen all the time which sent me on the wrong path
> for two full git bisects.
>
> Anyway, I've now tracked this down to the following commit which when
> reverted
> fixes the issue:
>
> 41798df9bfca ("net/mlx5: Drain wq first during PCI device removal")
>
> Looking at the diff I'd say the likely culprit is that before
> the commit the order of calls was:
>
> mlx5_unregister_device(dev)
> mlx5_drain_health_wq(dev)
>
> But with the commit it becomes
>
> mlx5_drain_health_wq(dev)
> mlx5_unregister_device(dev)
>
> So without really knowing anything about these functions I would
> guess that with the device still registered the drained
> queue does not remain empty as new entries are added.
> Does that sound plausible to you?
>
I don't think it is related, maybe this is similar to some issues
addressed lately by Shay's patches:
https://patchwork.ozlabs.org/project/netdev/patch/20200611224708.235014-2-sae...@mellanox.com/
https://patchwork.ozlabs.org/project/netdev/patch/20200611224708.235014-3-sae...@mellanox.com/
net/mlx5: drain health workqueue in case of driver load error
net/mlx5: Fix fatal error handling during device load
> Best regards,
> Niklas Schnelle
>
> [0] dmesg output:
> [ 36.447442] mlx5_core :00:00.0: poll_health:694:(pid 0): Fatal
> error 1 detected
> [ 36.447450] mlx5_core :00:00.0: print_health_info:372:(pid 0):
> assert_var[0] 0x
> [ 36.447453] mlx5_core :00:00.0: print_health_info:372:(pid 0):
> assert_var[1] 0x
> [ 36.447455] mlx5_core :00:00.0: print_health_info:372:(pid 0):
> assert_var[2] 0x
> [ 36.447458] mlx5_core :00:00.0: print_health_info:372:(pid 0):
> assert_var[3] 0x
> [ 36.447461] mlx5_core :00:00.0: print_health_info:372:(pid 0):
> assert_var[4] 0x
> [ 36.447463] mlx5_core :00:00.0: print_health_info:375:(pid 0):
> assert_exit_ptr 0x
> [ 36.447467] mlx5_core :00:00.0: print_health_info:377:(pid 0):
> assert_callra 0x
> [ 36.447471] mlx5_core :00:00.0: print_health_info:380:(pid 0):
> fw_ver 65535.65535.65535
> [ 36.447475] mlx5_core :00:00.0: print_health_info:381:(pid 0):
> hw_id 0x
> [ 36.447478] mlx5_core :00:00.0: print_health_info:382:(pid 0):
> irisc_index 255
> [ 36.447492] mlx5_core :00:00.0: print_health_info:383:(pid 0):
> synd 0xff: unrecognized error
> [ 36.447621] mlx5_core :00:00.0: print_health_info:385:(pid 0):
> ext_synd 0x
> [ 36.447624] mlx5_core :00:00.0: print_health_info:387:(pid 0):
> raw fw_ver 0x
> [ 36.447885] crw_info : CRW reports slct=0, oflw=0, chn=0, rsc=B,
> anc=0, erc=0, rsid=0
> [ 36.447897] zpci: :00:00.0: Event 0x303 reconfigured PCI
> function 0x514
> [ 47.099220] mlx5_core :00:00.0: poll_health:709:(pid 0):
> device's health compromised - reached miss count
> [ 47.099228] mlx5_core :00:00.0: print_health_info:372:(pid 0):
> assert_var[0] 0x
> [ 47.099231] mlx5_core :00:00.0: print_health_info:372:(pid 0):
> assert_var[1] 0x
> [ 47.099234] mlx5_core :00:00.0: print_health_info:372:(pid 0):
> assert_var[2] 0x
> [ 47.099236] mlx5_core :00:00.0: print_health_info:372:(pid 0):
> assert_var[3] 0x
> [ 47.099239] mlx5_core :00:00.0: print_health_info:372:(pid 0):
> assert_var[4] 0x
> [ 47.099241] mlx5_core :00:00.0: print_health_info:375:(pid 0):
> assert_exit_ptr 0x
> [ 47.099245] mlx5_core :00:00.0: print_health_info:377:(pid 0):
> assert_callra 0x
> [ 47.099249] mlx5_core :00:00.0: print_health_info:380:(pid 0):
> fw_ver 65535.65535.65535
> [ 47.099253] mlx5_core :00:00.0: print_health_info:381:(pid 0):
> hw_id 0x
> [ 47.099256] mlx5_core :00:00.0: print_health_info:382:(pid 0):
> irisc_index 255
> [ 47.099327] mlx5_core :00:00.0: print_health_info:383:(pid 0):
> synd 0xff: unrecognized error
> [ 47.099329] mlx5_core :00:00.0: print_health_info:385:(pid 0):
> ext_synd 0x
> [ 47.099330] mlx5_core :00:00.0: print_health_info:387:(pid 0):
> raw fw_ver 0x
> [ 100.539106] mlx5_core :00:00.0: wait_func:991:(pid 121):
> 2RST_QP(0x50a) timeout. Will cause a leak of a command resource
> [ 100.539118] infiniband mlx5_0: destroy_qp_common:2525:(pid 121):
> mlx5_ib: modify QP 0x00072c to RESET failed
> [ 141.499325] mlx5_core :00:00.0: wait_func:991:(pid 32):
> QUERY_VPORT_COUNTER(0x770) timeout. Will cause a leak of a command
> resource
> [ 161.978957] mlx5_core :00:00.0: wait_func:991:(pid 121):
> DESTROY_QP(0x501) timeout. Will cause a leak of a command resource
Shay's patches also came to avoid such command timeouts.
Re: [PATCH 4.19 25/25] Revert "net/mlx5: Annotate mutex destroy for root ns"
On Tue, 2020-06-09 at 19:45 +0200, Greg Kroah-Hartman wrote: > From: Greg Kroah-Hartman > > This reverts commit 95fde2e46860c183f6f47a99381a3b9bff488bd5 which is > commit 9ca415399dae133b00273a4283ef31d003a6818d upstream. > > It was backported incorrectly, Paul writes at: > https://lore.kernel.org/r/20200607203425.gd23...@windriver.com > > I happened to notice this commit: > > 9ca415399dae - "net/mlx5: Annotate mutex destroy for root ns" > > ...was backported to 4.19 and 5.4 and v5.6 in linux-stable. > > It patches del_sw_root_ns() - which only exists after v5.7-rc7 > from: > > 6eb7a268a99b - "net/mlx5: Don't maintain a case of del_sw_func > being > null" > > which creates the one line del_sw_root_ns stub function around > kfree(node) by breaking it out of tree_put_node(). > > In the absense of del_sw_root_ns - the backport finds an > identical one > line kfree stub fcn - named del_sw_prio from this earlier > commit: > > 139ed6c6c46a - "net/mlx5: Fix steering memory leak" [in v4.15- > rc5] > > and then puts the mutex_destroy() into that (wrong) function, > instead of > putting it into tree_put_node where the root ns case used to be > hand > > Reported-by: Paul Gortmaker > Cc: Roi Dayan > Cc: Mark Bloch > Cc: Saeed Mahameed > Signed-off-by: Greg Kroah-Hartman Acked-by: Saeed Mahameed I don't know if this was due to something wrong in my backporting process or AUTOSEL/wrong Fixes tag related. I will check later and will try my best to avoid this in the future. Thanks, Saeed.
Re: [PATCH] net/mlx5: Use kfree(ft->g) in arfs_create_groups()
On Fri, 2020-06-05 at 12:57 -0700, Eric Dumazet wrote:
>
> On 6/5/20 12:22 PM, Denis Efremov wrote:
> > Use kfree() instead of kvfree() on ft->g in arfs_create_groups()
> > because
> > the memory is allocated with kcalloc().
> >
> > Signed-off-by: Denis Efremov
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
> > index 014639ea06e3..c4c9d6cda7e6 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
> > @@ -220,7 +220,7 @@ static int arfs_create_groups(struct
> > mlx5e_flow_table *ft,
> > sizeof(*ft->g), GFP_KERNEL);
> > in = kvzalloc(inlen, GFP_KERNEL);
> > if (!in || !ft->g) {
> > - kvfree(ft->g);
> > + kfree(ft->g);
> > kvfree(in);
> > return -ENOMEM;
> > }
> >
>
> This is slow path, kvfree() is perfectly able to free memory that was
> kmalloc()ed
>
> net-next is closed, can we avoid these patches during merge window ?
I agree, with Eric, better if we wait for net-next to open.
I will apply this once net-next is open.
Thanks,
Saeed.
Re: [PATCH net-next v2 3/4] mlx5: become aware of when running as a bonding slave
On Wed, 2020-06-10 at 14:59 -0400, Jarod Wilson wrote:
> I've been unable to get my hands on suitable supported hardware to
> date,
> but I believe this ought to be all that is needed to enable the mlx5
> driver to also work with bonding active-backup crypto offload
> passthru.
>
> CC: Boris Pismenny
> CC: Saeed Mahameed
> CC: Leon Romanovsky
> CC: Jay Vosburgh
> CC: Veaceslav Falico
> CC: Andy Gospodarek
> CC: "David S. Miller"
> CC: Jeff Kirsher
> CC: Jakub Kicinski
> CC: Steffen Klassert
> CC: Herbert Xu
> CC: net...@vger.kernel.org
> Signed-off-by: Jarod Wilson
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> index 92eb3bad4acd..72ad6664bd73 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> @@ -210,6 +210,9 @@ static inline int
> mlx5e_xfrm_validate_state(struct xfrm_state *x)
> struct net_device *netdev = x->xso.dev;
> struct mlx5e_priv *priv;
>
> + if (x->xso.slave_dev)
> + netdev = x->xso.slave_dev;
> +
Do we really need to repeat this per driver ?
why not just setup xso.real_dev, in xfrm layer once and for all before
calling device drivers ?
Device drivers will use xso.real_dev blindly.
Will be useful in the future when you add vlan support, etc..
> priv = netdev_priv(netdev);
>
> if (x->props.aalgo != SADB_AALG_NONE) {
> @@ -291,6 +294,9 @@ static int mlx5e_xfrm_add_state(struct xfrm_state
> *x)
> unsigned int sa_handle;
> int err;
>
> + if (x->xso.slave_dev)
> + netdev = x->xso.slave_dev;
> +
> priv = netdev_priv(netdev);
>
> err = mlx5e_xfrm_validate_state(x);
Re: [PATCH] net/mlx5: DR, Fix freeing in dr_create_rc_qp()
On Mon, 2020-06-01 at 19:45 +0300, Denis Efremov wrote:
> Variable "in" in dr_create_rc_qp() is allocated with kvzalloc() and
> should be freed with kvfree().
>
> Fixes: 297cccebdc5a ("net/mlx5: DR, Expose an internal API to issue
> RDMA operations")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Denis Efremov
>
Applied to net-mlx5,
Thanks,
Saeed.
Re: [PATCH] net/mlx5e: Don't use err uninitialized in mlx5e_attach_decap
On Wed, 2020-05-27 at 00:50 -0700, Nathan Chancellor wrote:
> Clang warns:
>
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:3712:6: warning:
> variable 'err' is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
> if (IS_ERR(d->pkt_reformat)) {
> ^~~
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:3718:6: note:
> uninitialized use occurs here
> if (err)
> ^~~
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:3712:2: note: remove
> the
> 'if' if its condition is always true
> if (IS_ERR(d->pkt_reformat)) {
> ^
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:3670:9: note:
> initialize
> the variable 'err' to silence this warning
> int err;
>^
> = 0
> 1 warning generated.
>
> It is not wrong, err is only ever initialized in if statements but
> this
> one is not in one. Initialize err to 0 to fix this.
>
> Fixes: 14e6b038afa0 ("net/mlx5e: Add support for hw decapsulation of
> MPLS over UDP")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1037
> Signed-off-by: Nathan Chancellor
> ---
Applied to net-next-mlx5, will send shortly to net-next
Thanks,
Saeed.
Re: [PATCH v2] net/mlx5e: Use IS_ERR() to check and simplify code
On Sat, 2020-05-16 at 07:06 +0800, Tang Bin wrote: > Use IS_ERR() and PTR_ERR() instead of PTR_ERR_OR_ZERO() to > simplify code, avoid redundant judgements. > > Signed-off-by: Zhang Shengju > Signed-off-by: Tang Bin > Reviewed-by: Leon Romanovsky > --- > Changes from v1 > - fix the commit message for typo. > --- > drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > Applied to net-next-mlx5 Thanks, Saeed.
Re: [PATCH] net/mlx5e: Use IS_ERR() to check and simplify code
On Wed, 2020-05-13 at 17:48 +0800, Tang Bin wrote: > Hi David: > > On 2020/5/8 4:18, David Miller wrote: > > From: Tang Bin > > Date: Thu, 7 May 2020 19:50:10 +0800 > > > > > Use IS_ERR() and PTR_ERR() instead of PTR_ZRR_OR_ZERO() ^^^ typo > > > to simplify code, avoid redundant judgements. > > > > > > Signed-off-by: Zhang Shengju > > > Signed-off-by: Tang Bin > > Saeed, please pick this up. > > Does this mean the patch has been received and I just have to wait? > no, mlx5 patches normally go to net-next-mlx5 branch and usually pulled into net-next once a week when i send my pull requests. i will reply with "applied" when i apply this patch, but for now please fix the typo. Thanks, Saeed
Re: [PATCH] net/mlx5: Replace zero-length array with flexible-array
On Thu, 2020-05-07 at 13:59 -0500, Gustavo A. R. Silva wrote:
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array
> member[1][2],
> introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure,
> which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
>
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
>
> "Flexible array members have incomplete type, and so the sizeof
> operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
>
> sizeof(flexible-array-member) triggers a warning because flexible
> array
> members have incomplete type[1]. There are some instances of code in
> which the sizeof operator is being incorrectly/erroneously applied to
> zero-length arrays and the result is zero. Such instances may be
> hiding
> some bugs. So, this work (flexible-array member conversions) will
> also
> help to get completely rid of those sorts of issues.
>
> This issue was found with the help of Coccinelle.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva
>
Applied to mlx5-next, thanks !
Re: [PATCH] net/mlx5: Replace zero-length array with flexible-array
On Thu, 2020-05-07 at 13:59 -0500, Gustavo A. R. Silva wrote:
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array
> member[1][2],
> introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure,
> which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
>
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
>
> "Flexible array members have incomplete type, and so the sizeof
> operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
>
> sizeof(flexible-array-member) triggers a warning because flexible
> array
> members have incomplete type[1]. There are some instances of code in
> which the sizeof operator is being incorrectly/erroneously applied to
> zero-length arrays and the result is zero. Such instances may be
> hiding
hmmm, we actually have some tooling that rely on this to identify such
0 length fields .. since the structs in this file are usually auto-
generated from the hw sepcs .. now i see that these tools are broken in
our CI with this patch applied.
I guess we will need to fix them, and fix our code auto-generation
tools.
overall i am ok with this patch. I will apply it to mlx5-next.
and submit it upstream soom.
> some bugs. So, this work (flexible-array member conversions) will
> also
> help to get completely rid of those sorts of issues.
>
> This issue was found with the help of Coccinelle.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> include/linux/mlx5/driver.h |2 -
> include/linux/mlx5/mlx5_ifc.h | 66 +
> -
> include/linux/mlx5/qp.h |2 -
> 3 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/include/linux/mlx5/driver.h
> b/include/linux/mlx5/driver.h
> index 6f8f79ef829b..1a4ba36275de 100644
> --- a/include/linux/mlx5/driver.h
> +++ b/include/linux/mlx5/driver.h
> @@ -200,7 +200,7 @@ struct mlx5_rsc_debug {
> void *object;
> enum dbg_rsc_type type;
> struct dentry *root;
> - struct mlx5_field_desc fields[0];
> + struct mlx5_field_desc fields[];
> };
>
> enum mlx5_dev_event {
> diff --git a/include/linux/mlx5/mlx5_ifc.h
> b/include/linux/mlx5/mlx5_ifc.h
> index 69b27c7dfc3e..c55686ff6504 100644
> --- a/include/linux/mlx5/mlx5_ifc.h
> +++ b/include/linux/mlx5/mlx5_ifc.h
> @@ -1677,7 +1677,7 @@ struct mlx5_ifc_wq_bits {
>
> u8 reserved_at_140[0x4c0];
>
> - struct mlx5_ifc_cmd_pas_bits pas[0];
> + struct mlx5_ifc_cmd_pas_bits pas[];
> };
>
> struct mlx5_ifc_rq_num_bits {
> @@ -1895,7 +1895,7 @@ struct mlx5_ifc_resource_dump_menu_segment_bits
> {
> u8 reserved_at_20[0x10];
> u8 num_of_records[0x10];
>
> - struct mlx5_ifc_resource_dump_menu_record_bits record[0];
> + struct mlx5_ifc_resource_dump_menu_record_bits record[];
> };
>
> struct mlx5_ifc_resource_dump_resource_segment_bits {
> @@ -1907,7 +1907,7 @@ struct
> mlx5_ifc_resource_dump_resource_segment_bits {
>
> u8 index2[0x20];
>
> - u8 payload[0][0x20];
> + u8 payload[][0x20];
> };
>
> struct mlx5_ifc_resource_dump_terminate_segment_bits {
> @@ -2984,7 +2984,7 @@ struct mlx5_ifc_flow_context_bits {
>
> u8 reserved_at_1200[0x600];
>
> - union mlx5_ifc_dest_format_struct_flow_counter_list_auto_bits
> destination[0];
> + union mlx5_ifc_dest_format_struct_flow_counter_list_auto_bits
> destination[];
> };
>
> enum {
> @@ -3276,7 +3276,7 @@ struct mlx5_ifc_rqtc_bits {
>
> u8 reserved_at_e0[0x6a0];
>
> - struct mlx5_ifc_rq_num_bits rq_num[0];
> + struct mlx5_ifc_rq_num_bits rq_num[];
> };
>
> enum {
> @@ -3388,7 +3388,7 @@ struct mlx5_ifc_nic_vport_context_bits {
>
> u8 reserved_at_7e0[0x20];
>
> - u8 current_uc_mac_address[0][0x40];
> + u8 current_uc_mac_address[][0x40];
> };
>
> enum {
> @@ -4310,7 +4310,7 @@ struct mlx5_ifc_query_xrc_srq_out_bits {
>
> u8 reserved_at_280[0x600];
>
> - u8 pas[0][0x40];
> + u8 pas[][0x40];
> };
>
> struct mlx5_ifc_query_xrc_srq_in_bits {
> @@ -4588,7 +4588,7 @@ struct mlx5_ifc_query_srq_out_bits {
>
> u8 reserved_at_280[0x600];
>
> - u8 pas[0][0x40];
> + u8 pas[][0x40];
> };
>
> struct
Re: [PATCH] net/mlx5: Replace zero-length array with flexible-array
On Fri, 2020-05-08 at 16:36 -0700, Jakub Kicinski wrote:
> On Thu, 7 May 2020 13:59:35 -0500 Gustavo A. R. Silva wrote:
> > The current codebase makes use of the zero-length array language
> > extension to the C90 standard, but the preferred mechanism to
> > declare
> > variable-length types such as these ones is a flexible array
> > member[1][2],
> > introduced in C99:
> >
> > struct foo {
> > int stuff;
> > struct boo array[];
> > };
>
> Saeed, I'm expecting you to take this and the mlx4 patch via your
> trees.
Yes for the mlx5 patch, but usually Dave takes mlx4 patches directly.
since the volume of mlx4 patches is very small, let's apply them
directly to net-next, unless you want me to handle them from now on and
make your life easier, then i don't have any issue with that.
Saeed.
Re: linux-next: manual merge of the net-next tree with the rdma tree
On Fri, 2020-05-08 at 09:35 -0300, Jason Gunthorpe wrote:
> On Fri, May 08, 2020 at 01:18:51PM +1000, Stephen Rothwell wrote:
> > Hi all,
> >
> > Today's linux-next merge of the net-next tree got a conflict in:
> >
> > drivers/net/bonding/bond_main.c
> >
> > between commits:
> >
> > ed7d4f023b1a ("bonding: Rename slave_arr to usable_slaves")
> > c071d91d2a89 ("bonding: Add helper function to get the xmit slave
> > based on hash")
> > 29d5bbccb3a1 ("bonding: Add helper function to get the xmit slave
> > in rr mode")
> >
> > from the rdma and mlx5-next trees and commit:
> >
> > ae46f184bc1f ("bonding: propagate transmit status")
> >
> > from the net-next tree.
>
> Saeed? These patches in the shared branch were supposed to be a PR to
> net-net? I see it hasn't happened yet and now we have conflicts??
>
Yes, I don't usually send standalone PRs of mlx5-next, and I only do it
with the corresponding (depending on) patches from net-next-mlx5, but I
agree this one was different I should have submitted it .. anyway the
conflict is minor, i already fixed it up and will submit soon..
Thanks,
Saeed.
Re: linux-next: manual merge of the net-next tree with the rdma tree
On Fri, 2020-05-08 at 13:18 +1000, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the net-next tree got a conflict in:
>
> drivers/net/bonding/bond_main.c
>
> between commits:
>
> ed7d4f023b1a ("bonding: Rename slave_arr to usable_slaves")
> c071d91d2a89 ("bonding: Add helper function to get the xmit slave
> based on hash")
> 29d5bbccb3a1 ("bonding: Add helper function to get the xmit slave
> in rr mode")
>
> from the rdma and mlx5-next trees and commit:
>
> ae46f184bc1f ("bonding: propagate transmit status")
>
> from the net-next tree.
>
> I fixed it up (I think - see below) and can carry the fix as
> necessary. This is now fixed as far as linux-next is concerned, but
> any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging. You may also want to
> consider
> cooperating with the maintainer of the conflicting tree to minimise
> any
> particularly complex conflicts.
>
Hi Stephen and thanks for the report.
Your fix seems to be ok, i think it is missing some hunks for
bond_get_slave_by_id function and some "likely" directives are missing,
which were added by Maor's or Eric's patches.
Anyway this is already fixed up in my net-next-mlx5 tree and will be
submitted very soon to net-next with the conflict fixup ..
Thanks,
Saeed.
Re: [PATCH 0/1] net/mlx5: Call pci_disable_sriov() on remove
On Thu, 2020-04-30 at 14:03 +0200, Niklas Schnelle wrote: > Hello, > > I'm currently working on improvements in PF-VF handling on s390. One > thing that > may be a bit special for us is that the s390 hotplug code supports > shutting > down a single PF of a multi-function device such as a ConnectX-5. > During testing I found that the mlx5_core driver does not call > pci_disable_sriov() in its remove handler which is called on the PF > via > pci_stop_and_remove_bus_device() on an orderly hot unplug. Actually what happens if you call pci_disable_sriov() when there are VFs attached to VMs ? > > Following is a patch to fix this, I want to point out however that > I'm not > entirely sure about the effect of clear_vfs that distinguishes > mlx5_sriov_detach() from mlx5_sriov_disable() only that the former is > called > from the remove handler and it doesn't call pci_disable_sriov(). > That however is required according to Documentation/PCI/pci-iov- > howto.rst > The Doc doesn't say "required", so the question is, is it really required ? because the way i see it, it is the admin responsibility to clear out vfs before shutting down the PF driver. as explained in the patch review, mlx5 vf driver is resilient of such behavior and once the device is back online the vf driver quickly recovers, so it is actually a feature and not a bug :) There are many reasons why the admin would want to do this: 1) Fast Firmware upgrade 2) Fast Hyper-visor upgrade/refresh 3) PF debugging So you can quickly reset the PF driver without the need to wait for all VFs or manually detach-attach them. > I've only tested this on top of my currently still internal PF-VF > rework so > I might also be totally missing something here in that case excuse my > ignorance. > > Best regards, > Niklas Schnelle > > Niklas Schnelle (1): > net/mlx5: Call pci_disable_sriov() on remove > > drivers/net/ethernet/mellanox/mlx5/core/sriov.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) >
