On Wed, Oct 10, 2018 at 02:39:31PM -0400, Dennis Dalessandro wrote:
> On 10/7/2018 5:03 AM, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> > The ucma users supply timeout in u32 format, it means that any number
> > with most significant bit set will be converted to negative value
> > by various rdma_*, cma_* and sa_query functions, which treat timeout
> > as int.
> >
> > In the lowest level, the timeout is converted back to be unsigned long.
> > Remove this ambiguous conversion by updating all function signatures to
> > receive unsigned long.
> >
> > Reported-by: Noa Osherovich <noaos@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > ---
> > drivers/infiniband/core/addr.c | 5 ++---
> > drivers/infiniband/core/cma.c | 15 ++++++++-------
> > drivers/infiniband/core/mad.c | 2 +-
> > drivers/infiniband/core/mad_priv.h | 2 +-
> > drivers/infiniband/core/sa.h | 8 +++-----
> > drivers/infiniband/core/sa_query.c | 13 +++++++------
> > include/rdma/ib_addr.h | 5 ++---
> > include/rdma/ib_cm.h | 2 +-
> > include/rdma/ib_sa.h | 38 ++++++++++++++++----------------------
> > include/rdma/rdma_cm.h | 5 +++--
> > 10 files changed, 44 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
> > index 07e0ffe74a8a..0dce94e3c495 100644
> > --- a/drivers/infiniband/core/addr.c
> > +++ b/drivers/infiniband/core/addr.c
> > @@ -659,11 +659,10 @@ static void process_one_req(struct work_struct *_work)
> > }
> > int rdma_resolve_ip(struct sockaddr *src_addr, const struct sockaddr *dst_addr,
> > - struct rdma_dev_addr *addr, int timeout_ms,
> > + struct rdma_dev_addr *addr, unsigned long timeout_ms,
> > void (*callback)(int status, struct sockaddr *src_addr,
> > struct rdma_dev_addr *addr, void *context),
> > - bool resolve_by_gid_attr,
> > - void *context)
> > + bool resolve_by_gid_attr, void *context)
>
> Doesn't seem necessary for the patch.
>
>
> > -static int cma_resolve_iw_route(struct rdma_id_private *id_priv, int timeout_ms)
> > +static int cma_resolve_iw_route(struct rdma_id_private *id_priv)
>
> Agree with the change but doesn't belong in this patch.
>
> > -int rdma_resolve_route(struct rdma_cm_id *id, int timeout_ms)
> > +int rdma_resolve_route(struct rdma_cm_id *id, unsigned long timeout_ms)
> > {
> > struct rdma_id_private *id_priv;
> > int ret;
> > @@ -2865,7 +2866,7 @@ int rdma_resolve_route(struct rdma_cm_id *id, int timeout_ms)
> > else if (rdma_protocol_roce(id->device, id->port_num))
> > ret = cma_resolve_iboe_route(id_priv);
> > else if (rdma_protocol_iwarp(id->device, id->port_num))
> > - ret = cma_resolve_iw_route(id_priv, timeout_ms);
> > + ret = cma_resolve_iw_route(id_priv);
>
> This goes with the comment above as well.
>
>
> > diff --git a/drivers/infiniband/core/sa.h b/drivers/infiniband/core/sa.h
> > index b1d4bbf4ce5c..cbaaaa92fff3 100644
> > --- a/drivers/infiniband/core/sa.h
> > +++ b/drivers/infiniband/core/sa.h
> > @@ -49,16 +49,14 @@ static inline void ib_sa_client_put(struct ib_sa_client *client)
> > }
> > int ib_sa_mcmember_rec_query(struct ib_sa_client *client,
> > - struct ib_device *device, u8 port_num,
> > - u8 method,
> > + struct ib_device *device, u8 port_num, u8 method,
>
> Another random clean up that changes context unnecessarily.
>
> > - void *context,
> > - struct ib_sa_query **sa_query);
> > + void *context, struct ib_sa_query **sa_query);
>
> And another.
>
>
> > -static int send_mad(struct ib_sa_query *query, int timeout_ms, gfp_t gfp_mask)
> > +static int send_mad(struct ib_sa_query *query, unsigned long timeout_ms,
> > + gfp_t gfp_mask)
>
> Should probably align with parentheses.
>
> > diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h
> > index e09eca91eb18..2734c895c1bf 100644
> > --- a/include/rdma/ib_addr.h
> > +++ b/include/rdma/ib_addr.h
> > @@ -99,11 +99,10 @@ int rdma_translate_ip(const struct sockaddr *addr,
> > * @context: User-specified context associated with the call.
> > */
> > int rdma_resolve_ip(struct sockaddr *src_addr, const struct sockaddr *dst_addr,
> > - struct rdma_dev_addr *addr, int timeout_ms,
> > + struct rdma_dev_addr *addr, unsigned long timeout_ms,
> > void (*callback)(int status, struct sockaddr *src_addr,
> > struct rdma_dev_addr *addr, void *context),
> > - bool resolve_by_gid_attr,
> > - void *context);
> > + bool resolve_by_gid_attr, void *context);
>
> Unrelated.
>
>
> > diff --git a/include/rdma/ib_sa.h b/include/rdma/ib_sa.h
> > index b6ddf2a1b9d8..19520979b84c 100644
> > --- a/include/rdma/ib_sa.h
> > +++ b/include/rdma/ib_sa.h
> > @@ -449,28 +449,23 @@ struct ib_sa_query;
> > void ib_sa_cancel_query(int id, struct ib_sa_query *query);
> > -int ib_sa_path_rec_get(struct ib_sa_client *client,
> > - struct ib_device *device, u8 port_num,
> > - struct sa_path_rec *rec,
> > - ib_sa_comp_mask comp_mask,
> > - int timeout_ms, gfp_t gfp_mask,
> > - void (*callback)(int status,
> > - struct sa_path_rec *resp,
> > +int ib_sa_path_rec_get(struct ib_sa_client *client, struct ib_device *device,
> > + u8 port_num, struct sa_path_rec *rec,
> > + ib_sa_comp_mask comp_mask, unsigned long timeout_ms,
> > + gfp_t gfp_mask,
> > + void (*callback)(int status, struct sa_path_rec *resp,
> > void *context),
> > - void *context,
> > - struct ib_sa_query **query);
> > + void *context, struct ib_sa_query **query);
> > int ib_sa_service_rec_query(struct ib_sa_client *client,
> > - struct ib_device *device, u8 port_num,
> > - u8 method,
> > - struct ib_sa_service_rec *rec,
> > - ib_sa_comp_mask comp_mask,
> > - int timeout_ms, gfp_t gfp_mask,
> > - void (*callback)(int status,
> > - struct ib_sa_service_rec *resp,
> > - void *context),
> > - void *context,
> > - struct ib_sa_query **sa_query);
> > + struct ib_device *device, u8 port_num, u8 method,
> > + struct ib_sa_service_rec *rec,
> > + ib_sa_comp_mask comp_mask, unsigned long timeout_ms,
> > + gfp_t gfp_mask,
> > + void (*callback)(int status,
> > + struct ib_sa_service_rec *resp,
> > + void *context),
> > + void *context, struct ib_sa_query **sa_query);
>
> Again more complicated than just annotating with unsigned long.
>
>
> > struct ib_sa_multicast {
> > struct ib_sa_mcmember_rec rec;
> > @@ -573,12 +568,11 @@ int ib_sa_guid_info_rec_query(struct ib_sa_client *client,
> > struct ib_device *device, u8 port_num,
> > struct ib_sa_guidinfo_rec *rec,
> > ib_sa_comp_mask comp_mask, u8 method,
> > - int timeout_ms, gfp_t gfp_mask,
> > + unsigned long timeout_ms, gfp_t gfp_mask,
> > void (*callback)(int status,
> > struct ib_sa_guidinfo_rec *resp,
> > void *context),
> > - void *context,
> > - struct ib_sa_query **sa_query);
> > + void *context, struct ib_sa_query **sa_query);
>
> Another one.
>
> Overall, while I agree with the changes, there are a number here which
> are not germane to the patch. Why not submit the clean-ups separately?
The immediate reason - simplicity. I found unused variables while
changed timeout_ms type, but definitely will separate this patch to
extra pieces.
Thanks
>
> -Denny
Attachment:
signature.asc
Description: PGP signature
