| Message ID | 552BB5AC.6050101@profitbricks.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Mon, Apr 13, 2015 at 02:25:16PM +0200, Michael Wang wrote: > > Use raw management helpers to reform IB-ulp ipoib. > > Cc: Steve Wise <swise@opengridcomputing.com> > Cc: Tom Talpey <tom@talpey.com> > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Cc: Doug Ledford <dledford@redhat.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Sean Hefty <sean.hefty@intel.com> > Signed-off-by: Michael Wang <yun.wang@profitbricks.com> > --- > drivers/infiniband/ulp/ipoib/ipoib_main.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c > index 58b5aa3..97372b1 100644 > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c > @@ -1654,9 +1654,7 @@ static void ipoib_add_one(struct ib_device *device) > struct net_device *dev; > struct ipoib_dev_priv *priv; > int s, e, p; > - > - if (rdma_node_get_transport(device->node_type) != RDMA_TRANSPORT_IB) > - return; > + int count = 0; Same comment as before. rdma_tech_ib_dev(device) Ira -- 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 Mon, Apr 13, 2015 at 02:25:16PM +0200, Michael Wang wrote: > dev_list = kmalloc(sizeof *dev_list, GFP_KERNEL); > if (!dev_list) > @@ -1673,13 +1671,19 @@ static void ipoib_add_one(struct ib_device *device) > } > > for (p = s; p <= e; ++p) { > - if (rdma_port_get_link_layer(device, p) != IB_LINK_LAYER_INFINIBAND) > + if (!rdma_tech_ib(device, p)) > continue; > dev = ipoib_add_port("ib%d", device, p); > if (!IS_ERR(dev)) { > priv = netdev_priv(dev); > list_add_tail(&priv->list, dev_list); > } > + count++; > + } > + > + if (!count) { > + kfree(dev_list); > + return; > } This doesn't quite look right, it should be 'goto error1' But then I read 'goto error1' and it doesn't look like it can handle cm_dev->port being NULL, so more fixing is needed. Ditto for cm_remove_one Should audit all uses of cm_dev->port[] to make sure they can all handle NULL. 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 Mon, Apr 13, 2015 at 01:27:01PM -0600, Jason Gunthorpe wrote: > On Mon, Apr 13, 2015 at 02:25:16PM +0200, Michael Wang wrote: > > dev_list = kmalloc(sizeof *dev_list, GFP_KERNEL); > > if (!dev_list) > > @@ -1673,13 +1671,19 @@ static void ipoib_add_one(struct ib_device *device) > > } > > > > for (p = s; p <= e; ++p) { > > - if (rdma_port_get_link_layer(device, p) != IB_LINK_LAYER_INFINIBAND) > > + if (!rdma_tech_ib(device, p)) > > continue; > > dev = ipoib_add_port("ib%d", device, p); > > if (!IS_ERR(dev)) { > > priv = netdev_priv(dev); > > list_add_tail(&priv->list, dev_list); > > } > > + count++; > > + } > > + > > + if (!count) { > > + kfree(dev_list); > > + return; > > } > > This doesn't quite look right, it should be 'goto error1' Looks like you replied to the wrong patch. ?? I don't see error1 in ipoib_add_one. For the ib_cm module... Yes I think it should go to "error1". However, see below... This is the type of clean up error which would be avoided if a call to cap_ib_cm_dev() were done at the top of the function. > > But then I read 'goto error1' and it doesn't look like it can handle > cm_dev->port being NULL, so more fixing is needed. I think this is highlighting an existing dependency which we don't want but exists. It appears the code currently does not support this situation. Dev port 1 : cap_is_cm == true port 2 : cap_is_cm == false port 3 : cap_is_cm == true Because the error handling bails when port 2 fails... Leaving both port 1 and port 3 uninitialized. :-( Ira > > Ditto for cm_remove_one > > Should audit all uses of cm_dev->port[] to make sure they can all > handle NULL. > > 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 Mon, Apr 13, 2015 at 03:46:03PM -0400, ira.weiny wrote: > > This doesn't quite look right, it should be 'goto error1' > > Looks like you replied to the wrong patch. ?? I don't see error1 in ipoib_add_one. > For the ib_cm module... Right, sorry. > Yes I think it should go to "error1". However, see below... > > This is the type of clean up error which would be avoided if a call to > cap_ib_cm_dev() were done at the top of the function. So what does cap_ib_cm_dev return in your example: > Dev > port 1 : cap_is_cm == true > port 2 : cap_is_cm == false > port 3 : cap_is_cm == true True? Then the code is still broken, having cap_ib_cm_dev doesn't help anything. If we make it possible to be per port then it has to be fixed. If you want to argue the above example is illegal and port 2 has to be on a different device, I'd be interested to see what that looks like. Thinking about it some more, cap_foo_dev only makes sense if all ports are either true or false. Mixed is a BUG. That seems reasonable, and solves the #10 problem, but we should enforce this invariant during device register. Typically the ports seem to be completely orthogonal (like SA), so in those cases the _dev and restriction makes no sense. CM seems to be different, so it should probably enforce its rules 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 04/13/2015 09:27 PM, Jason Gunthorpe wrote: > On Mon, Apr 13, 2015 at 02:25:16PM +0200, Michael Wang wrote: >> dev_list = kmalloc(sizeof *dev_list, GFP_KERNEL); >> if (!dev_list) >> @@ -1673,13 +1671,19 @@ static void ipoib_add_one(struct ib_device *device) >> } >> >> for (p = s; p <= e; ++p) { >> - if (rdma_port_get_link_layer(device, p) != IB_LINK_LAYER_INFINIBAND) >> + if (!rdma_tech_ib(device, p)) >> continue; >> dev = ipoib_add_port("ib%d", device, p); >> if (!IS_ERR(dev)) { >> priv = netdev_priv(dev); >> list_add_tail(&priv->list, dev_list); >> } >> + count++; >> + } >> + >> + if (!count) { >> + kfree(dev_list); >> + return; >> } > > This doesn't quite look right, it should be 'goto error1' > > But then I read 'goto error1' and it doesn't look like it can handle > cm_dev->port being NULL, so more fixing is needed. Nice catch ;-) I guess the 'count++' should be inside 'if (!IS_ERR(dev))' after link the node. Regards, Michael Wang > > Ditto for cm_remove_one > > Should audit all uses of cm_dev->port[] to make sure they can all > handle NULL. > > 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
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c index 58b5aa3..97372b1 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c @@ -1654,9 +1654,7 @@ static void ipoib_add_one(struct ib_device *device) struct net_device *dev; struct ipoib_dev_priv *priv; int s, e, p; - - if (rdma_node_get_transport(device->node_type) != RDMA_TRANSPORT_IB) - return; + int count = 0; dev_list = kmalloc(sizeof *dev_list, GFP_KERNEL); if (!dev_list) @@ -1673,13 +1671,19 @@ static void ipoib_add_one(struct ib_device *device) } for (p = s; p <= e; ++p) { - if (rdma_port_get_link_layer(device, p) != IB_LINK_LAYER_INFINIBAND) + if (!rdma_tech_ib(device, p)) continue; dev = ipoib_add_port("ib%d", device, p); if (!IS_ERR(dev)) { priv = netdev_priv(dev); list_add_tail(&priv->list, dev_list); } + count++; + } + + if (!count) { + kfree(dev_list); + return; } ib_set_client_data(device, &ipoib_client, dev_list); @@ -1690,9 +1694,6 @@ static void ipoib_remove_one(struct ib_device *device) struct ipoib_dev_priv *priv, *tmp; struct list_head *dev_list; - if (rdma_node_get_transport(device->node_type) != RDMA_TRANSPORT_IB) - return; - dev_list = ib_get_client_data(device, &ipoib_client); if (!dev_list) return;
Use raw management helpers to reform IB-ulp ipoib. Cc: Steve Wise <swise@opengridcomputing.com> Cc: Tom Talpey <tom@talpey.com> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Cc: Doug Ledford <dledford@redhat.com> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Sean Hefty <sean.hefty@intel.com> Signed-off-by: Michael Wang <yun.wang@profitbricks.com> --- drivers/infiniband/ulp/ipoib/ipoib_main.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)