[openib-general] Re: [PATCH] Allow setting of NodeDescription

[Michael S. Tsirkin]

Quoting r. Roland Dreier <rolandd at cisco.com>:
> Subject: [PATCH] Allow setting of NodeDescription
> 
> This patch does a few things:
>  - Adds node_guid and node_desc fields to struct ib_device
>  - Has mthca set these fields on startup
>  - Extends modify_device method to handle setting node_desc
>  - Exposes node_desc in sysfs
>  - Allows userspace to set node_desc by writing into sysfs file, eg.
>      echo -n `hostname` >> /sys/class/infiniband/mthca0/node_desc
> 
> This should probably be combined with Sean's work to get rid of
> node_guid queries in ULPs.
> 
> Comments?
> 
>  - R.


Good stuff.

I think 
echo -n `hostname` mthca0 >> /sys/class/infiniband/mthca0/node_desc
is even more useful, but thats now up to the user, isnt it?

> +static int mthca_init_node_data(struct mthca_dev *dev)
> +{
> +	struct ib_smp *in_mad  = NULL;
> +	struct ib_smp *out_mad = NULL;
> +	int err = -ENOMEM;
> +	u8 status;
> +
> +	in_mad  = kzalloc(sizeof *in_mad, GFP_KERNEL);
> +	out_mad = kmalloc(sizeof *out_mad, GFP_KERNEL);
> +	if (!in_mad || !out_mad)
> +		goto out;
> +
> +	init_query_mad(in_mad);
> +	in_mad->attr_id = IB_SMP_ATTR_NODE_DESC;
> +
> +	err = mthca_MAD_IFC(dev, 1, 1,
> +			    1, NULL, NULL, in_mad, out_mad,
> +			    &status);
> +	if (err)
> +		goto out;
> +	if (status) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	memcpy(dev->ib_dev.node_desc, out_mad->data, 64);


Would make more sense to initialize node_desc to an empty string
instead of whatever is programmed in the device flash?
This creates a way for the remote user to find out that the "echo"
script above did not run yet, and try again later.

> @@ -1064,6 +1120,7 @@ int mthca_register_device(struct mthca_d
>  	dev->ib_dev.class_dev.dev        = &dev->pdev->dev;
>  	dev->ib_dev.query_device         = mthca_query_device;
>  	dev->ib_dev.query_port           = mthca_query_port;
> +	dev->ib_dev.modify_device        = mthca_modify_device;
>  	dev->ib_dev.modify_port          = mthca_modify_port;
>  	dev->ib_dev.query_pkey           = mthca_query_pkey;
>  	dev->ib_dev.query_gid            = mthca_query_gid;

By the way, why do we need ib_modify_device in ib_verbs, at all?
The code basically does memcpy from ibdev, in addition to some locking.
This seems something that belongs in ib_mad, the mad snooping logic
would be exactly the same for any provider. No?

-- 
MST