| Message ID | 20160713100825.GD29468@mwanda |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 13/07/2016 13:08, Dan Carpenter wrote: > We accidentally return success when we had intended to return an error > code. > > Fixes: 69697b6e2086 ('net/mlx5: E-Switch, Add support for the sriov offloads mode') > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c > index 1842dfb..7d982cf 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c > @@ -183,6 +183,7 @@ static int esw_create_offloads_fdb_table(struct mlx5_eswitch *esw, int nvports) > > root_ns = mlx5_get_flow_namespace(dev, MLX5_FLOW_NAMESPACE_FDB); > if (!root_ns) { > + err = -EINVAL; > esw_warn(dev, "Failed to get FDB flow namespace\n"); > goto ns_err; > } > Hi, Thanks for the patch. I'm not sure EINVAL is the right error here though. Maybe -ENOTSUPP is a bit more appropriate here. Regards, Matan
On Wed, Jul 13, 2016 at 01:08:25PM +0300, Dan Carpenter wrote: > We accidentally return success when we had intended to return an error > code. > > Fixes: 69697b6e2086 ('net/mlx5: E-Switch, Add support for the sriov offloads mode') > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Thanks Dan, Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On 7/13/2016 2:19 PM, Matan Barak wrote: > I'm not sure EINVAL is the right error here though. > Maybe -ENOTSUPP is a bit more appropriate here. I agree, Dan, can you please change to be along Matan's suggestion? Or.
On 13/07/2016 14:48, Dan Carpenter wrote: > We accidentally return success when we had intended to return an error > code. > > Fixes: 69697b6e2086 ('net/mlx5: E-Switch, Add support for the sriov offloads mode') > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > v2: return -ENOTSUPP instead --EINVAL > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c > index 1842dfb..7d982cf 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c > @@ -183,6 +183,7 @@ static int esw_create_offloads_fdb_table(struct mlx5_eswitch *esw, int nvports) > > root_ns = mlx5_get_flow_namespace(dev, MLX5_FLOW_NAMESPACE_FDB); > if (!root_ns) { > + err = -ENOTSUPP; > esw_warn(dev, "Failed to get FDB flow namespace\n"); > goto ns_err; > } > Thanks. Reviewed-by: Matan Barak <matanb@mellanox.com>
On Wed, Jul 13, 2016 at 02:48:44PM +0300, Dan Carpenter wrote: > We accidentally return success when we had intended to return an error > code. > > Fixes: 69697b6e2086 ('net/mlx5: E-Switch, Add support for the sriov offloads mode') > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > v2: return -ENOTSUPP instead --EINVAL I'm a little bit confused. Why did you prefer ENOTSUPP over EOPNOTSUPP? Thanks.
On 13/07/2016 16:04, Leon Romanovsky wrote: > On Wed, Jul 13, 2016 at 02:48:44PM +0300, Dan Carpenter wrote: >> We accidentally return success when we had intended to return an error >> code. >> >> Fixes: 69697b6e2086 ('net/mlx5: E-Switch, Add support for the sriov offloads mode') >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> --- >> v2: return -ENOTSUPP instead --EINVAL > > I'm a little bit confused. Why did you prefer ENOTSUPP over EOPNOTSUPP? According to [1], it fits our case better - operation is valid and make sense, but isn't supported. [1] https://lists.gnu.org/archive/html/bug-glibc/2002-08/msg00017.html > > Thanks. >
On Wed, Jul 13, 2016 at 02:48:44PM +0300, Dan Carpenter wrote: > We accidentally return success when we had intended to return an error > code. > > Fixes: 69697b6e2086 ('net/mlx5: E-Switch, Add support for the sriov offloads mode') > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > v2: return -ENOTSUPP instead --EINVAL > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c > index 1842dfb..7d982cf 100644 > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c > @@ -183,6 +183,7 @@ static int esw_create_offloads_fdb_table(struct mlx5_eswitch *esw, int nvports) > > root_ns = mlx5_get_flow_namespace(dev, MLX5_FLOW_NAMESPACE_FDB); > if (!root_ns) { > + err = -ENOTSUPP; Did you mean ENOTSUP? I thought ENOTSUPP was not to be used outside NFS, and isn't properly exported to userspace.. $ find /usr/include -name "*errno*" | xargs grep 524 Jason
On Wed, Jul 13, 2016 at 11:54:54AM -0600, Jason Gunthorpe wrote: > On Wed, Jul 13, 2016 at 02:48:44PM +0300, Dan Carpenter wrote: > > We accidentally return success when we had intended to return an error > > code. > > > > Fixes: 69697b6e2086 ('net/mlx5: E-Switch, Add support for the sriov offloads mode') > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > v2: return -ENOTSUPP instead --EINVAL > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c > > index 1842dfb..7d982cf 100644 > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c > > @@ -183,6 +183,7 @@ static int esw_create_offloads_fdb_table(struct mlx5_eswitch *esw, int nvports) > > > > root_ns = mlx5_get_flow_namespace(dev, MLX5_FLOW_NAMESPACE_FDB); > > if (!root_ns) { > > + err = -ENOTSUPP; > > Did you mean ENOTSUP? > > I thought ENOTSUPP was not to be used outside NFS, and isn't properly > exported to userspace.. > > $ find /usr/include -name "*errno*" | xargs grep 524 I asked similar question [1] with different return value in reply to this patch. [1] http://marc.info/?l=linux-netdev&m=146843171508230&w=2 > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 13, 2016 at 02:40:26PM +0300, Or Gerlitz wrote: > On 7/13/2016 2:19 PM, Matan Barak wrote: > >I'm not sure EINVAL is the right error here though. > >Maybe -ENOTSUPP is a bit more appropriate here. > > I agree, Dan, can you please change to be along Matan's suggestion? Or, Dan already did it before Matan's response and we have very vivid discussion about it [1]. [1] http://marc.info/?t=146841063000002&r=1&w=2 > > Or. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c index 1842dfb..7d982cf 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c @@ -183,6 +183,7 @@ static int esw_create_offloads_fdb_table(struct mlx5_eswitch *esw, int nvports) root_ns = mlx5_get_flow_namespace(dev, MLX5_FLOW_NAMESPACE_FDB); if (!root_ns) { + err = -EINVAL; esw_warn(dev, "Failed to get FDB flow namespace\n"); goto ns_err; }
We accidentally return success when we had intended to return an error code. Fixes: 69697b6e2086 ('net/mlx5: E-Switch, Add support for the sriov offloads mode') Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>