Re: [PATCH v2 03/13] Attach/detach SoftiWarp to/from network and RDMA subsystem

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2349 bytes --]

On Sat, Oct 14, 2017 at 01:28:43AM +0000, Bernard Metzler wrote:
> -----Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote: -----
>
> >> +
> >> +#define SIW_MAX_IF 12
> >> +static int if_cnt;
> >> +static char *iface_list[SIW_MAX_IF] = {[0 ... (SIW_MAX_IF-1)] =
> >'\0'};
> >> +module_param_array(iface_list, charp, &if_cnt, 0444);
> >> +MODULE_PARM_DESC(iface_list, "Interface list siw attaches to if
> >present");
> >> +
> >> +static bool loopback_enabled = 1;
> >> +module_param(loopback_enabled, bool, 0644);
> >> +MODULE_PARM_DESC(loopback_enabled, "enable_loopback");
> >> +
> >> +LIST_HEAD(siw_devlist);
> >> +
> >> +static int cpu_cnt;
> >> +static char *tx_cpu_list[MAX_CPU] = {[0 ... (MAX_CPU-1)] = '\0'};
> >> +module_param_array(tx_cpu_list, charp, &cpu_cnt, 0444);
> >> +MODULE_PARM_DESC(tx_cpu_list, "List of CPUs siw TX thread shall be
> >bound to");
> >
> >No module parameters please.
> OK. Would you have a pointer which sheds some light on that
> rule? Thank you!
>

It is not rule, but common knowledge exactly as BUG_ON which is not
prohibited, but makes no sense in low level driver code.

The module parameters sometimes make sense, for example in subsystem
level where they can apply to whole devices underneath.

But most of the time, they indicate complete ignorance of users in favor
of easy developer's life.

For people, like me, who doesn't run modules at all, the change in
module parameters require rebuild of initramfs and in more extreme
cases rebuild of SELinux labels.

> >
> >> +
> >> +int default_tx_cpu = -1;
> >> +struct task_struct *qp_tx_thread[MAX_CPU];
> >> +struct crypto_shash *siw_crypto_shash;
> >> +
> >> +static ssize_t show_sw_version(struct device *dev,
> >> +			       struct device_attribute *attr, char *buf)
> >> +{
> >> +	struct siw_dev *sdev = container_of(dev, struct siw_dev,
> >ofa_dev.dev);
> >
> >Please remove "ofa_*" from this code, upstream has nothing to do with
> >OFA.
> OK, sure.
> That came from the fact it felt wired to write an iWarp driver
> and use ib_*. Shall we stick to history and name those things ib
> even if we have now 4 transports, or can we come up with something
> more generic? ofa obviously is not a good idea.

We already have rdma_* notation for not-IB protocols, in long run,
we will remove drivers/infiniband to be drivers/rdma.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]