Re: [PATCH] mlx4: prevent the device from being removed concurrently

Jack Morgenstein
2012-03-01T07:51:58Z

Mr Cascardo,

If this is at a customer site, there is a workaround available for you.

In file /etc/modprobe.d/mlx4_core.conf, enter a line:
options mlx4_core internal_err_reset=0

(do "modinfo mlx4_core" to see a description of the module parameter).

Setting this parameter to 0 in the modprobe conf file will cause the driver to simply report the
internal error failure (in /var/log/messages), but the device will not be restarted.

This will avoid your Oops -- the EEH flow will then be the only flow restarting the device.

(Note that even if you implement my suggestion, you will still have a situation where the device
will be reset TWICE).

Again, this is a workaround for current users. The issue needs to be fixed so that even when the
internal error does cause a reset, the kernel will not crash.

Unfortunately, I do not have the time now to generate a patch.

-Jack

On Wednesday 29 February 2012 17:19, Jack Morgenstein wrote:
> On Wednesday 29 February 2012 16:47, Jack Morgenstein wrote:
> > Some comments.
> >
> > 1. Mr Cascardo's solution is only partial, and does not cover all the problem cases. He
> > simply uncovered one of several examples of what lack-of-sync will do when removing a device.
> > Mr. Cascardo found the kernel Oops that happens when a catastrophic error occurs during device
> > removal. What if we receive a catas error while doing "init_one"? What if we are in the middle
> > of catas error recovery (in the init_one stage), and we get a remove_one request from higher up?
> >
> > There is a solution for this precise problem in the mthca driver (infiniband/hw/mthca/mthca_main.c
> > infiniband/hw/mthca/mthca_catas.c). In the mthca driver, we DO in fact use an "mthca_device_mutex"
> > for precisely the reason given in a. above. I see no reason not to do the same thing here.
> >
> > This requires:
> > 1. mlx4_init_one(), mlx4_remove_one() and mlx4_restart_one all grab an mlx4_device_mutex.
> > 2. new procedure __mlx4_remove_one(), which does not grab the mutex.
> >
> > Note that it is NOT enough to simply protect the removal operation. The protection must wrap the
> > ENTIRE restart operation (both removal and init), because allowing a remove in the middle of init_one
> > or restart_one would probably also cause a kernel Oops.
> >
> > 2. The intf_mutex is used with mlx4_un/register_device and mlx4_un/register_interface. unregister_device is
> > used both in remove_one and in mlx4_change_port_types. I would hesitate to grab that mutex for a more
> > global use. I think it is cleaner to add a device mutex (mlx4_device_mutex) for initializing/removing/
> > restarting the device.
> >
> > -Jack
> >
> Another thing -- what about the desired final state of the device? Say we do a remove one, and in the middle
> of this, we get a catas restart. The catas restart will wait until the remove-in-progress completes, and
> then will do its remove/init -- with the net result that the device is UP rather than DOWN.
>
> This implies that we need some sort of state machine here.
>
> -Jack