| Message ID | 1457461000-24088-2-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 03/08/2016 10:16 AM, Christoph Hellwig wrote: > The new RW API will need this. Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com> -- 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 Tue, Mar 08, 2016 at 07:16:32PM +0100, Christoph Hellwig wrote: > The new RW API will need this. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > Tested-by: Steve Wise <swise@opengridcomputing.com> > --- > drivers/infiniband/core/cma.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index 9729639..a791522 100644 > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -800,6 +800,7 @@ int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, > if (id->device != pd->device) > return -EINVAL; > > + qp_init_attr->port_num = id->port_num; I doubt whether this is the right place to update qp_init_attr structure. Maybe the better solution will be to update the callers of rdma_create_qp? It will place all qp_init_attr assignments in one place. > qp = ib_create_qp(pd, qp_init_attr); > if (IS_ERR(qp)) > return PTR_ERR(qp); > -- > 2.1.4 > > -- > 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 -- 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 Tue, Mar 08, 2016 at 07:16:32PM +0100, Christoph Hellwig wrote: > > The new RW API will need this. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Tested-by: Steve Wise <swise@opengridcomputing.com> > > --- > > drivers/infiniband/core/cma.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > > index 9729639..a791522 100644 > > --- a/drivers/infiniband/core/cma.c > > +++ b/drivers/infiniband/core/cma.c > > @@ -800,6 +800,7 @@ int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, > > if (id->device != pd->device) > > return -EINVAL; > > > > + qp_init_attr->port_num = id->port_num; > > I doubt whether this is the right place to update qp_init_attr structure. > Maybe the better solution will be to update the callers of rdma_create_qp? > It will place all qp_init_attr assignments in one place. At the expense of replicating this code and forcing all users to remember to set this. -- 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 Tue, Mar 08, 2016 at 01:38:20PM -0600, Steve Wise wrote: > > On Tue, Mar 08, 2016 at 07:16:32PM +0100, Christoph Hellwig wrote: > > > The new RW API will need this. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > Tested-by: Steve Wise <swise@opengridcomputing.com> > > > --- > > > drivers/infiniband/core/cma.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > > > index 9729639..a791522 100644 > > > --- a/drivers/infiniband/core/cma.c > > > +++ b/drivers/infiniband/core/cma.c > > > @@ -800,6 +800,7 @@ int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, > > > if (id->device != pd->device) > > > return -EINVAL; > > > > > > + qp_init_attr->port_num = id->port_num; > > > > I doubt whether this is the right place to update qp_init_attr structure. > > Maybe the better solution will be to update the callers of rdma_create_qp? > > It will place all qp_init_attr assignments in one place. > > At the expense of replicating this code and forcing all users to remember to set this. There are no much such users to update, these users need to set qp_init_attr structure anyway. > > > -- 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 Tue, Mar 08, 2016 at 09:55:14PM +0200, Leon Romanovsky wrote: > > At the expense of replicating this code and forcing all users to remember to set this. > > There are no much such users to update, these users need to set > qp_init_attr structure anyway. But they need to extract the port_num first, while we already get the cm_id that has the right port_id passed to this function. Not setting it in the qp_init_attr changes the interface from one that just works to one that is arcane, and prone to generate hard to detect errors (passing a 0 port_num will just work for all current drivers, but if someone at some point actually introduces different capabilities for differnet ports it will break for just that case!) -- 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, Mar 09, 2016 at 02:58:04PM +0100, 'Christoph Hellwig' wrote: > On Tue, Mar 08, 2016 at 09:55:14PM +0200, Leon Romanovsky wrote: > > > At the expense of replicating this code and forcing all users to remember to set this. > > > > There are no much such users to update, these users need to set > > qp_init_attr structure anyway. > > But they need to extract the port_num first, while we already get > the cm_id that has the right port_id passed to this function. Not > setting it in the qp_init_attr changes the interface from one that > just works to one that is arcane, and prone to generate hard to > detect errors (passing a 0 port_num will just work for all current > drivers, but if someone at some point actually introduces different > capabilities for differnet ports it will break for just that case!) I understand your points and my claim is similar to yours, but from different side. I think that setting qp_init_attr field separately from the place there all variables were set is prone to errors. > -- > 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 -- 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 Tue, Mar 08, 2016 at 09:55:14PM +0200, Leon Romanovsky wrote: > > > At the expense of replicating this code and forcing all users to > remember to set this. > > > > There are no much such users to update, these users need to set > > qp_init_attr structure anyway. > > But they need to extract the port_num first, while we already get > the cm_id that has the right port_id passed to this function. Not > setting it in the qp_init_attr changes the interface from one that > just works to one that is arcane, and prone to generate hard to > detect errors (passing a 0 port_num will just work for all current > drivers, but if someone at some point actually introduces different > capabilities for differnet ports it will break for just that case!) While I'm for keeping your change as-is, it turns out the port numbers are 1 relative, so 0 could be treated as an error. From read_port_immutable(): /** * device->port_immutable is indexed directly by the port number to make * access to this data as efficient as possible. * * Therefore port_immutable is declared as a 1 based array with * potential empty slots at the beginning. */ device->port_immutable = kzalloc(sizeof(*device->port_immutable) * (end_port + 1), GFP_KERNEL); -- 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, Mar 09, 2016 at 09:51:25AM -0600, Steve Wise wrote: > While I'm for keeping your change as-is, it turns out the port numbers > are 1 relative, so 0 could be treated as an error. From > read_port_immutable(): I'll add a check for 0, but will keep things as-is otherwise. -- 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/core/cma.c b/drivers/infiniband/core/cma.c index 9729639..a791522 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -800,6 +800,7 @@ int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, if (id->device != pd->device) return -EINVAL; + qp_init_attr->port_num = id->port_num; qp = ib_create_qp(pd, qp_init_attr); if (IS_ERR(qp)) return PTR_ERR(qp);