[ofa-general] [PATCH] IB/ipoib: refresh paths instead of fushing them on SM change event

[Roland Dreier]

Please run your patches through scripts/checkpatch.pl and consider the
output before sending them... it's easy to do and saves everyone time
fixing trivial style mistakes.

More substantive comments:

 > +static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv, int level)

Instead of "int level" here, I would prefer to see something like

enum ipoib_flush_level {
	IPOIB_FLUSH_LIGHT,
	IPOIB_FLUSH_NORMAL,
	IPOIB_FLUSH_HEAVY,
};

so that code like

 > -	if (pkey_event) {
 > +	if (level == 2) {

remains at somewhat self-documenting.

 > +static void path_refresh(struct net_device *dev, struct ipoib_path *path)
 > +{
 > +	struct ipoib_dev_priv *priv = netdev_priv(dev);
 > +	ipoib_dbg(priv, "mark path LID 0x%04x GID " IPOIB_GID_FMT " invalid\n",
 > +		  be16_to_cpu(path->pathrec.dlid), IPOIB_GID_ARG(path->pathrec.dgid));
 > +	path->valid =  0;
 > +}

I'm not sure this wrapper makes sense, because it is so trivial and it
doesn't even do what the name suggests -- it doesn't refresh the path,
it just marks it as invalid.

 > +void ipoib_refresh_paths(struct net_device *dev)
 > +{
 > +	struct ipoib_dev_priv *priv = netdev_priv(dev);
 > +	struct ipoib_path *path, *tp;
 > +	spin_lock_irq(&priv->tx_lock);
 > +	spin_lock(&priv->lock);
 > +
 > +	list_for_each_entry_safe(path, tp, &priv->path_list, list) {
 > +		if (path->query)
 > +			ib_sa_cancel_query(path->query_id, path->query);
 > +		spin_unlock(&priv->lock);
 > +		spin_unlock_irq(&priv->tx_lock);
 > +		wait_for_completion(&path->done);
 > +		path_refresh(dev, path);
 > +		spin_lock_irq(&priv->tx_lock);
 > +		spin_lock(&priv->lock);
 > +	}
 > +	spin_unlock(&priv->lock);
 > +	spin_unlock_irq(&priv->tx_lock);
 > +}

This looks broken for a couple of reasons:

 - What protects against paths being added to the path list while this
   loop is running?  ipoib_flush_paths() moves the list to a new place
   to avoid this problem.

 - What happens if you mark a path as invalid and do
   wait_for_completion(), and then ipoib_flush_paths() runs before
   another path record query is done?  It seems that the second
   wait_for_completion() will wait forever.

 - R.