[PATCH v1] IB/ipoib: Report SG feature regardless of HW UD CSUM capability

[PATCH v1] IB/ipoib: Report SG feature regardless of HW UD CSUM capability

[Yuval Shaia]

Decouple SG support from HW ability to do UD checksum.
This coupling is for historical reasons and removed with 'commit
ec5f06156423 ("net: Kill link between CSUM and SG features.")'

Signed-off-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
v0 -> v1:
	* Fix patch title name
	* Move settings to ipoib_setup instead of ipoib_set_dev_features
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 5f58c41..de7b010 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1763,8 +1763,8 @@ void ipoib_setup(struct net_device *dev)
 	dev->addr_len		 = INFINIBAND_ALEN;
 	dev->type		 = ARPHRD_INFINIBAND;
 	dev->tx_queue_len	 = ipoib_sendq_size * 2;
-	dev->features		 = (NETIF_F_VLAN_CHALLENGED	|
-				    NETIF_F_HIGHDMA);
+	dev->features		 = NETIF_F_VLAN_CHALLENGED | NETIF_F_HIGHDMA |
+				   NETIF_F_SG;
 	netif_keep_dst(dev);
 
 	memcpy(dev->broadcast, ipv4_bcast_addr, INFINIBAND_ALEN);
@@ -1967,8 +1967,7 @@ int ipoib_set_dev_features(struct ipoib_dev_priv *priv, struct ib_device *hca)
 	priv->hca_caps = hca->attrs.device_cap_flags;
 
 	if (priv->hca_caps & IB_DEVICE_UD_IP_CSUM) {
-		priv->dev->hw_features = NETIF_F_SG |
-			NETIF_F_IP_CSUM | NETIF_F_RXCSUM;
+		priv->dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM;
 
 		if (priv->hca_caps & IB_DEVICE_UD_TSO)
 			priv->dev->hw_features |= NETIF_F_TSO;
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] IB/ipoib: Report SG feature regardless of HW UD CSUM capability

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote:
> Decouple SG support from HW ability to do UD checksum.
> This coupling is for historical reasons and removed with 'commit
> ec5f06156423 ("net: Kill link between CSUM and SG features.")'
> 
> Signed-off-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
> v0 -> v1:
> 	* Fix patch title name
> 	* Move settings to ipoib_setup instead of ipoib_set_dev_features

Thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1] IB/ipoib: Report SG feature regardless of HW UD CSUM capability

[Jason Gunthorpe]

On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote:
> Decouple SG support from HW ability to do UD checksum.
> This coupling is for historical reasons and removed with 'commit
> ec5f06156423 ("net: Kill link between CSUM and SG features.")'

Shouldn't this also check that the QP's sg limit is something
appropriate? At least > 1 ..

Presumably things still work if the skb needs more sg's than are
available?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] IB/ipoib: Report SG feature regardless of HW UD CSUM capability

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

On Mon, Jul 18, 2016 at 11:19:21AM -0600, Jason Gunthorpe wrote:
> On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote:
> > Decouple SG support from HW ability to do UD checksum.
> > This coupling is for historical reasons and removed with 'commit
> > ec5f06156423 ("net: Kill link between CSUM and SG features.")'
> 
> Shouldn't this also check that the QP's sg limit is something
> appropriate? At least > 1 ..
> 
> Presumably things still work if the skb needs more sg's than are
> available?

IMHO, this question is relevant for the code before change as well.

> 
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1] IB/ipoib: Report SG feature regardless of HW UD CSUM capability

[Yuval Shaia]

On Mon, Jul 18, 2016 at 11:19:21AM -0600, Jason Gunthorpe wrote:
> On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote:
> > Decouple SG support from HW ability to do UD checksum.
> > This coupling is for historical reasons and removed with 'commit
> > ec5f06156423 ("net: Kill link between CSUM and SG features.")'
> 
> Shouldn't this also check that the QP's sg limit is something
> appropriate? At least > 1 ..

What is special with 1?

Network stack can pass MAX_SKB_FRAGS frags so even if QP's SG limit is 3 we
are still in problem.
Fortunately this was addressed in 'commit 78a50a5 ("Add handling for
sending of skb with many frags").

> 
> Presumably things still work if the skb needs more sg's than are
> available?
> 
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] IB/ipoib: Report SG feature regardless of HW UD CSUM capability

[Jason Gunthorpe]

On Tue, Jul 19, 2016 at 03:55:57PM +0300, Yuval Shaia wrote:
> On Mon, Jul 18, 2016 at 11:19:21AM -0600, Jason Gunthorpe wrote:
> > On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote:
> > > Decouple SG support from HW ability to do UD checksum.
> > > This coupling is for historical reasons and removed with 'commit
> > > ec5f06156423 ("net: Kill link between CSUM and SG features.")'
> > 
> > Shouldn't this also check that the QP's sg limit is something
> > appropriate? At least > 1 ..
> 
> What is special with 1?

One means the hardware doesn't support SG at all.

Is there a reason to sg_linearize down to ipoib if we know the
hardware doesn't support SG at all? Presumably it is better to let the
netstack deal with that case.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] IB/ipoib: Report SG feature regardless of HW UD CSUM capability

[Yuval Shaia]

On Tue, Jul 19, 2016 at 01:34:53PM -0600, Jason Gunthorpe wrote:
> On Tue, Jul 19, 2016 at 03:55:57PM +0300, Yuval Shaia wrote:
> > On Mon, Jul 18, 2016 at 11:19:21AM -0600, Jason Gunthorpe wrote:
> > > On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote:
> > > > Decouple SG support from HW ability to do UD checksum.
> > > > This coupling is for historical reasons and removed with 'commit
> > > > ec5f06156423 ("net: Kill link between CSUM and SG features.")'
> > > 
> > > Shouldn't this also check that the QP's sg limit is something
> > > appropriate? At least > 1 ..
> > 
> > What is special with 1?
> 
> One means the hardware doesn't support SG at all.
> 
> Is there a reason to sg_linearize down to ipoib if we know the
> hardware doesn't support SG at all? Presumably it is better to let the
> netstack deal with that case.

ok, i see the point but as Leon Romanovsky mentioned - this is not an issue
with the patch itself, right? i.e. the original code relays on the fact
that HW supports UD-CSUM and did not checked QP's sg limit (we even do not
have any QP at this stage yet).

> 
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] IB/ipoib: Report SG feature regardless of HW UD CSUM capability

[Jason Gunthorpe]

On Tue, Jul 19, 2016 at 11:02:32PM +0300, Yuval Shaia wrote:

> ok, i see the point but as Leon Romanovsky mentioned - this is not an issue
> with the patch itself, right? i.e. the original code relays on the fact
> that HW supports UD-CSUM and did not checked QP's sg limit (we even do not
> have any QP at this stage yet).

ud-csum was only supported by one IB vendor and that vendor supports
SG.

You are changing things to enable SG on hardware that has never had SG
enabled, so you need to make sure it works right..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] IB/ipoib: Report SG feature regardless of HW UD CSUM capability

[ira.weiny]

On Tue, Jul 19, 2016 at 02:11:13PM -0600, Jason Gunthorpe wrote:
> On Tue, Jul 19, 2016 at 11:02:32PM +0300, Yuval Shaia wrote:
> 
> > ok, i see the point but as Leon Romanovsky mentioned - this is not an issue
> > with the patch itself, right? i.e. the original code relays on the fact
> > that HW supports UD-CSUM and did not checked QP's sg limit (we even do not
> > have any QP at this stage yet).
> 
> ud-csum was only supported by one IB vendor and that vendor supports
> SG.
> 
> You are changing things to enable SG on hardware that has never had SG
> enabled, so you need to make sure it works right..

My thoughts exactly.
Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] IB/ipoib: Report SG feature regardless of HW UD CSUM capability

[Yuval Shaia]

On Thu, Sep 01, 2016 at 09:10:22PM -0400, ira.weiny wrote:
> On Tue, Jul 19, 2016 at 02:11:13PM -0600, Jason Gunthorpe wrote:
> > On Tue, Jul 19, 2016 at 11:02:32PM +0300, Yuval Shaia wrote:
> > 
> > > ok, i see the point but as Leon Romanovsky mentioned - this is not an issue
> > > with the patch itself, right? i.e. the original code relays on the fact
> > > that HW supports UD-CSUM and did not checked QP's sg limit (we even do not
> > > have any QP at this stage yet).
> > 
> > ud-csum was only supported by one IB vendor and that vendor supports
> > SG.
> > 
> > You are changing things to enable SG on hardware that has never had SG
> > enabled, so you need to make sure it works right..
> 
> My thoughts exactly.
> Ira

Already posted v2.

> 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html