Re: [PATCH rdma-core 2/4] mlx5: Implement ibv_query_qp_data_in_order() verb

[Leon Romanovsky <leon@kernel.org>]

On Thu, Jun 10, 2021 at 08:42:24AM -0300, Jason Gunthorpe wrote:
> On Thu, Jun 10, 2021 at 08:50:59AM +0300, Leon Romanovsky wrote:
> > On Wed, Jun 09, 2021 at 06:59:30PM +0300, Yishai Hadas wrote:
> > > From: Patrisious Haddad <phaddad@nvidia.com>
> > > 
> > > Implement the ibv_query_qp_data_in_order() verb by using DEVX to read
> > > from firmware the 'in_order_data' capability.
> > > 
> > > Signed-off-by: Patrisious Haddad <phaddad@nvidia.com>
> > > Reviewed-by: Maor Gottlieb <maorg@nvidia.com>
> > > Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> > >  providers/mlx5/mlx5.c     |  1 +
> > >  providers/mlx5/mlx5.h     |  3 +++
> > >  providers/mlx5/mlx5_ifc.h | 39 +++++++++++++++++++++++++++++++--
> > >  providers/mlx5/verbs.c    | 55 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 96 insertions(+), 2 deletions(-)
> > 
> > <...>
> > 
> > > +int mlx5_query_qp_data_in_order(struct ibv_qp *qp, enum ibv_wr_opcode op,
> > > +				uint32_t flags)
> > > +{
> > > +	uint32_t in_qp[DEVX_ST_SZ_DW(query_qp_in)] = {};
> > > +	uint32_t out_qp[DEVX_ST_SZ_DW(query_qp_out)] = {};
> > > +	struct mlx5_context *mctx = to_mctx(qp->context);
> > > +	struct mlx5_qp *mqp = to_mqp(qp);
> > > +	int ret;
> > > +
> > > +/* Currently this API is only supported for x86 architectures since most
> > > + * non-x86 platforms are known to be OOO and need to do a per-platform study.
> > > + */
> > > +#if !defined(__i386__) && !defined(__x86_64__)
> > > +	return 0;
> > > +#endif
> > 
> > Does it compile without warnings/errors on such platforms?
> > You have "return 0;" in the middle of function, so the right thing to do
> > it is to write with "#if ..." over function or inside like below, as
> > long as "#else" exists.
> > 
> > int mlx5_query_qp_data_in_order(struct ibv_qp *qp, enum ibv_wr_opcode op,
> > 				uint32_t flags)
> > {
> > #if !defined(__i386__) && !defined(__x86_64__)
> > 	/* Currently this API is only supported for x86 architectures since most
> > 	 * non-x86 platforms are known to be OOO and need to do a per-platform study.
> > 	 */
> > 	 return 0;
> > #else
> > .....
> > #endif
> 
> We should probably put the above in the core code anyhow

Agree, it makes sense.

Thanks

> 
> Jason