On Mon, Aug 24, 2020 at 09:41:14AM -0700, Bart Van Assche wrote:
> On 8/24/20 8:52 AM, Kamal Heib wrote:
> > +bool rxe_is_loaded;
>
> The name of this variable seems wrong to me. My understanding is that rxe_module_init() is
> called whether or not rxe has been built as a module. Consider renaming this variable into
> e.g. "rxe_initialized".
>
OK, I'll change it.
> > diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > index ccda5f5a3bc0..12c7ca0764d5 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > @@ -61,6 +61,11 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
> > struct net_device *ndev;
> > struct rxe_dev *exists;
> > + if (!rxe_is_loaded) {
> > + pr_err("Please make sure to load the rdma_rxe module first\n");
> > + return -EINVAL;
> > + }
> > +
> > len = sanitize_arg(val, intf, sizeof(intf));
> > if (!len) {
> > pr_err("add: invalid interface name\n");
>
> The above message is misleading. Consider changing it into e.g. the following:
>
> Please wait until initialization of the rdma_rxe module has finished.
>
> Additionally, how about returning -EAGAIN instead of -EINVAL?
>
Yes, this makes more sense, I'll change it.
Thanks,
Kamal
> Thanks,
>
> Bart.
>
>
