[ofa-general][PATCH 8/12 v1] mlx4: Dynamic port configuration

[Roland Dreier]

 > +static int mlx4_change_port_types(struct mlx4_dev *dev,
 > +				  enum mlx4_port_type *port_types)
 > +{
 > +	int i;
 > +	int err = 0;
 > +	int change = 0;
 > +	int port;
 > +
 > +	for (i = 0; i <  MLX4_MAX_PORTS; i++) {
 > +		if (port_types[i] != dev->caps.port_type[i + 1]) {
 > +			change = 1;
 > +			dev->caps.port_type[i + 1] = port_types[i];
 > +		}
 > +	}
 > +	if (change) {
 > +		mlx4_unregister_device(dev);
 > +		for (port = 1; port <= dev->caps.num_ports; port++) {
 > +			mlx4_CLOSE_PORT(dev, port);
 > +			err = mlx4_SET_PORT(dev, port);
 > +			if (err) {
 > +				mlx4_err(dev, "Failed to set port %d, "
 > +					      "aborting\n", port);
 > +				return err;
 > +			}
 > +		}
 > +		err = mlx4_register_device(dev);
 > +	}
 > +	return err;
 > +}

If I read the code correctly, there is no locking around this, so
multiple processes could race and cause all sorts of problems.  And also
you do the assignment

 > +			dev->caps.port_type[i + 1] = port_types[i];

before unregistering the device -- so there is a window where
caps.port_type has the wrong data -- not sure if this is a real issue.

 > +static ssize_t show_port_type(struct device *dev,
 > +			      struct device_attribute *attr,
 > +			      char *buf)
 > +{
 > +	struct pci_dev *pdev = to_pci_dev(dev);
 > +	struct mlx4_dev *mdev = pci_get_drvdata(pdev);
 > +	int i;
 > +
 > +	sprintf(buf, "Current port types:\n");
 > +	for (i = 1; i <= MLX4_MAX_PORTS; i++) {
 > +		sprintf(buf, "%sPort%d: %s\n", buf, i,
 > +			(mdev->caps.port_type[i] == MLX4_PORT_TYPE_IB)?
 > +			"ib": "eth");
 > +	}
 > +	return strlen(buf);
 > +}

This violates the one-value-per-file rule for sysfs if I am reading the
code correctly.

 - R.