Re: [PATCH rdma-core 3/8] mlx4: Add sparse annotations

Christoph Hellwig
2017-07-13T08:07:52Z

> > case MLX4_RECV_OPCODE_SEND_INVAL:
> > - return be32toh(cq->cqe->immed_rss_invalid);
> > + /* This is returning invalidate_rkey which is in host order, see
> > + * ibv_wc_read_invalidated_rkey */
> > + return (__force __be32)be32toh(cq->cqe->immed_rss_invalid);
>
> Jason,
> It is insane construction, convert to host-> force to be32 -> use as uint32_t.

Yes, this doesn't make sense to me - we are swapping it but pretending
it's native? There's certainly something very odd going on here.

It seems like the ->read_imm_data needs to be split and/or have a
prototype change. The sad part is that it is an exported ABI, although
one that is almost undocumented.

> > }
> >
> > -uint32_t *mlx4_alloc_db(struct mlx4_context *context, enum mlx4_db_type type)
> > +__be32 *mlx4_alloc_db(struct mlx4_context *context, enum mlx4_db_type type)
> > {
> > struct mlx4_db_page *page;
> > uint32_t *db = NULL;
> > @@ -113,10 +113,10 @@ found:
> > out:
> > pthread_mutex_unlock(&context->db_list_mutex);
> >
> > - return db;
> > + return (__force __be32 *)db;
> > }
>
> I see that librdmacm/rsocket.c full of these __force annotations.
> I would be very happy to see it fixed rather suppressed, but don't know
> if it is realistic goal or not.

The db local variable in mlx4_alloc_db should be easily changed to a
__be32 pointer - it is derived from a void pointer using pointer
arithmetics.

In general a __foce for endian annotations is a GIANT WARNING sign.
Don't ever add one without a long discussion first, and even then only
in a well documented helper and not randomly all over code.