Re: [PATCH 1/1] EDAC: fix edac core deadlock when removing a device

[Harry Ciao]

Hi Andrew,

Thanks for your response!

I think the change to edac_device_workq_function() is necessary, because 
edac_device_workq_teardown()'s call to cancel_delayed_work() won't 
ensure that the work won't be running, that's why it has to go on to 
call flush_workqueue() when cancel_delayed_work() returns zero, when the 
work has been added to edac_workqueue already, so the whole 
edac_workqueue has to be flushed to make sure this work completes before 
the edac device it belongs to could be safely removed.

I also add a printk to edac_device_workq_function():

static void edac_device_workq_function(struct work_struct *work_req)
{
struct delayed_work *d_work = (struct delayed_work *)work_req;
struct edac_device_ctl_info *edac_dev = to_edac_device_ctl_work(d_work);

mutex_lock(&device_ctls_mutex);

/* If we are being removed, bail out immediately */
if (edac_dev->op_state == OP_OFFLINE) {
printk(KERN_CRIT "+++HARRY+++ edac device is being removed, bail out 
from work!\n");
mutex_unlock(&device_ctls_mutex);
return;
}

/* Only poll controllers that are running polled and have a check */
if ((edac_dev->op_state == OP_RUNNING_POLL) &&
(edac_dev->edac_check != NULL)) {
edac_dev->edac_check(edac_dev);
}

mutex_unlock(&device_ctls_mutex);
...

and do a test to run pairs of insmod/rmmod about 10 times, as you could 
see from below logs that 2/10 chances that the work is running by 
edac_poller worker thread:

root@localhost:/root> insmod mv64x60_edac.ko
root@localhost:/root> rmmod mv64x60_edac.ko
root@localhost:/root> insmod mv64x60_edac.ko
root@localhost:/root> rmmod mv64x60_edac.ko
root@localhost:/root> insmod mv64x60_edac.ko
root@localhost:/root> rmmod mv64x60_edac.ko
root@localhost:/root> insmod mv64x60_edac.ko
root@localhost:/root> rmmod mv64x60_edac.ko
root@localhost:/root> insmod mv64x60_edac.ko
root@localhost:/root> rmmod mv64x60_edac.ko
+++HARRY+++ edac device is being removed, bail out from work!
root@localhost:/root> insmod mv64x60_edac.ko
root@localhost:/root> rmmod mv64x60_edac.ko
root@localhost:/root> insmod mv64x60_edac.ko
root@localhost:/root> rmmod mv64x60_edac.ko
root@localhost:/root> insmod mv64x60_edac.ko
root@localhost:/root> rmmod mv64x60_edac.ko
+++HARRY+++ edac device is being removed, bail out from work!
root@localhost:/root> insmod mv64x60_edac.ko
root@localhost:/root> rmmod mv64x60_edac.ko
root@localhost:/root> insmod mv64x60_edac.ko
root@localhost:/root> rmmod mv64x60_edac.ko
root@localhost:/root>

So edac_device_workq_function() should bail out immediately if the edac 
device that current work belongs to is being removed.

Best regards and Merry Chrismas!

Harry


Andrew Morton åé:
> On Thu, 18 Dec 2008 13:09:18 +0800
> Harry Ciao <qingtao.cao@xxxxxxxxxxxxx> wrote:
>
>   
> > To remove an edac device, all pending work must be complete. At that point, 
> > it is safe to remove the edac_dev structure.
> >
> > If the pending work is not properly cleared and proper care is not taken 
> > when waiting for it's completion, the following stack trace result:
> >
> > ...
> >
> > --- a/drivers/edac/edac_device.c
> > +++ b/drivers/edac/edac_device.c
> > @@ -394,6 +394,12 @@ static void edac_device_workq_function(struct work_struct *work_req)
> >  
> >  	mutex_lock(&device_ctls_mutex);
> >  
> > +	/* If we are being removed, bail out immediately */
> > +	if (edac_dev->op_state == OP_OFFLINE) {
> > +		mutex_unlock(&device_ctls_mutex);
> > +		return;
> > +	}
> > +
> >  	/* Only poll controllers that are running polled and have a check */
> >  	if ((edac_dev->op_state == OP_RUNNING_POLL) &&
> >  		(edac_dev->edac_check != NULL)) {
> > @@ -585,14 +591,14 @@ struct edac_device_ctl_info *edac_device_del_device(struct device *dev)
> >  	/* mark this instance as OFFLINE */
> >  	edac_dev->op_state = OP_OFFLINE;
> >  
> > -	/* clear workq processing on this instance */
> > -	edac_device_workq_teardown(edac_dev);
> > -
> >  	/* deregister from global list */
> >  	del_edac_device_from_global_list(edac_dev);
> >  
> >  	mutex_unlock(&device_ctls_mutex);
> >  
> > +	/* clear workq processing on this instance */
> > +	edac_device_workq_teardown(edac_dev);
> > +
> >  	/* Tear down the sysfs entries for this instance */
> >  	edac_device_remove_sysfs(edac_dev);
> >  
> >     
>
> Is the change to edac_device_workq_function() necessary? 
> edac_device_workq_teardown()'s call to cancel_delayed_work() will
> ensure that edac_device_workq_function() isn't running.
>
> Incidentally, edac_device_workq_teardown() is an open-coded
> cancel_delayed_work_sync().
>
>
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/