> On 10 Jun 2019, at 15:55, Honggang Li <honli@xxxxxxxxxx> wrote:
>
> The low 64 bits of cxgb3 and cxgb4 devices' GID are zeros. If the
> "provider" was set in the option file, ibacm will fail with
> segment fault.
>
> $ sed -i -e 's/# provider ibacmp 0xFE80000000000000/provider ibacmp 0xFE80000000000000/g' /etc/rdma/ibacm_opts.cfg
> $ /usr/sbin/ibacm --systemd
> Segmentation fault (core dumped)
> $ gdb /usr/sbin/ibacm core.ibacm
> (gdb) bt
> 0 0x00005625a4809217 in acm_assign_provider (port=0x5625a4bc6f28) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2285
> 1 acm_port_up (port=0x5625a4bc6f28) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2372
> 2 0x00005625a48073d2 in acm_activate_devices () at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2564
> 3 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:3270
>
> Note: The rpm was built with tarball generated from upstream repo. The last
> commit is aa41a65ec86bdb9c1c86e57885ee588b39558238.
>
> acm_open_dev function should not open an umad port for iWARP or RoCE devices.
It is "a umad port" (as suggested), even though the "u" is a vowel, it is pronounced as a consonant, hence "a umad port".
> Signed-off-by: Honggang Li <honli@xxxxxxxxxx>
> ---
> ibacm/src/acm.c | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/ibacm/src/acm.c b/ibacm/src/acm.c
> index a21069d4..5c8a5d3c 100644
> --- a/ibacm/src/acm.c
> +++ b/ibacm/src/acm.c
> @@ -2600,9 +2600,11 @@ static void acm_open_dev(struct ibv_device *ibdev)
> {
> struct acmc_device *dev;
> struct ibv_device_attr attr;
> + struct ibv_port_attr port_attr;
> struct ibv_context *verbs;
> size_t size;
> int i, ret;
> + unsigned int opened_ib_port_cnt = 0;
>
> acm_log(1, "%s\n", ibdev->name);
> verbs = ibv_open_device(ibdev);
> @@ -2628,13 +2630,29 @@ static void acm_open_dev(struct ibv_device *ibdev)
> list_head_init(&dev->prov_dev_context_list);
>
> for (i = 0; i < dev->port_cnt; i++) {
> + acm_log(1, "%s port %d\n", ibdev->name, i + 1);
> + ret = ibv_query_port(dev->device.verbs, i + 1, &port_attr);
> + if (ret) {
> + acm_log(0, "ERROR - ibv_query_port failed\n");
With the richness below when printing port or ports, may be add the value of ret here as well?
> + continue;
> + }
> + if (port_attr.link_layer != IBV_LINK_LAYER_INFINIBAND) {
> + acm_log(1, "not an InfiniBand port\n");
> + continue;
> + }
> +
> acm_open_port(&dev->port[i], dev, i + 1);
> + opened_ib_port_cnt++;
> }
>
> - list_add(&dev_list, &dev->entry);
> -
> - acm_log(1, "%s opened\n", ibdev->name);
> - return;
> + if (opened_ib_port_cnt > 0) {
or simpler, if (opened_ib_port_cnt) {
> + list_add(&dev_list, &dev->entry);
> + acm_log(1, "%d InfiniBand %s opened for %s\n",
> + opened_ib_port_cnt,
> + opened_ib_port_cnt == 1 ? "port":"ports",
Spaces around ":". I am also OK with the literal "port(s)" as well.
Otherwise, LGTM,
Signed-off-by: Håkon Bugge <haakon.bugge@xxxxxxxxxx>
Thxs, Håkon
> + ibdev->name);
> + return;
> + }
>
> err1:
> ibv_close_device(verbs);
> --
> 2.20.1
>
