On Sun, Jul 24, 2011 at 9:43 PM, <[email protected]> wrote:
> module that implements a soft IB device using ib_rxe in loopback.
> +MODULE_DESCRIPTION("RDMA transport over Converged Enhanced Ethernet");
That's the same description as for the ib_rxe_net kernel module, so
one of the two probably should be updated.
> +MODULE_LICENSE("Dual BSD/GPL");
> +
> +static __be64 node_guid(struct rxe_dev *rxe)
> +{
> + return cpu_to_be64(0x3333333333333333ULL);
> +}
In the OUI part of the above EUI-64 identifier the "M" (multicast) and
L ("locally administered") bits are set. I'm not sure that's fine.
Also, has it been considered to use __constant_cpu_to_be64() instead
of cpu_to_be64() ?
> +static __be64 port_guid(struct rxe_dev *rxe, unsigned int port_num)
> +{
> + return cpu_to_be64(0x4444444444444444ULL);
> +}
Same remark here about __constant_cpu_to_be64().
> +/*
> + * the ofed core requires that we provide a valid device
> + * object for registration
> + */
> +static struct class *my_class;
> +static struct device *my_dev;
Probably the above comment should be updated since this driver is
being submitted for inclusion in the mainline Linux kernel ?
> + if (av->attr.ah_flags & IB_AH_GRH)
> + pkt->mask |= RXE_GRH_MASK;
Please use a space instead of a tab before "|=".
> +static struct rxe_dev *rxe_sample;
> +
> +static int rxe_sample_add(void)
> +{
> + int err;
> + struct rxe_port *port;
> +
> + rxe_sample = (struct rxe_dev *)ib_alloc_device(sizeof(*rxe_sample));
> + if (!rxe_sample) {
> + err = -ENOMEM;
> + goto err1;
> + }
Since calling rxe_sample_add() twice in a row is an error a check for
that condition might be appropriate, e.g. by inserting a
WARN_ON(rxe_sample) before the allocation.
> +static void rxe_sample_remove(void)
> +{
> + if (!rxe_sample)
> + goto done;
> +
> + rxe_remove(rxe_sample);
> +
> + pr_info("rxe_sample: removed %s\n",
> + rxe_sample->ib_dev.name);
> +done:
> + return;
> +}
Setting rxe_sample to NULL after the rxe_remove() call can be a help
to catch potential dangling pointer dereferences.
> +
> +static int __init rxe_sample_init(void)
> +{
> + int err;
> +
> + rxe_crc_disable = 1;
> +
> + my_class = class_create(THIS_MODULE, "foo");
> + my_dev = device_create(my_class, NULL, 0, NULL, "bar");
> +
> + err = rxe_sample_add();
> + return err;
> +}
Sorry, but it's not clear to me why a new class has to be created this
sample driver. Also, one should be aware that class and device names
are globally visible and hence should be meaningful - names like "foo"
and "bar" are not acceptable.
# find /sys -name foo -o -name bar
/sys/devices/virtual/foo
/sys/devices/virtual/foo/bar
/sys/class/foo
/sys/class/foo/bar
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
