Re: [PATCH 16/36] drivers edac mod move mc to use workq

[Andrew Morton <akpm@linux-foundation.org>]

On Sun, 3 Jun 2007 07:41:35 -0700 (PDT)
Doug Thompson <norsk5@yahoo.com> wrote:

> From:	Dave Jiang <djiang@mvista.com>
> 
> Move the memory controller object to work queue based implementation
> from the
> kernel thread based. 
> 
> ...
>
> +#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,20))

Please avoid doing this in the mainline kernel.

It'll diverge anyway.  It is all-round better for people to carry (small)
backport patches outside the kernel.org tree if needed.

> +/*
> + * handler for EDAC to check if NMI type handler has asserted
> interrupt
> + */
> +static int edac_mc_assert_error_check_and_clear(void)
> +{
> +	int vreg;
> +
> +	if(edac_op_state == EDAC_OPSTATE_POLL)
> +		return 1;

To quote Linus "`if' is not a function".  Se we use "if (" (multiple
instances).

> +	vreg = atomic_read(&edac_err_assert);
> +	if(vreg) {
> +		atomic_set(&edac_err_assert, 0);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * edac_mc_workq_function
> + *	performs the operation scheduled by a workq request
> + */
> +#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,20))
> +static void edac_mc_workq_function(struct work_struct *work_req)
> +{
> +	struct delayed_work *d_work = (struct delayed_work*) work_req;
> +	struct mem_ctl_info *mci = to_edac_mem_ctl_work(d_work);
> +#else
> +static void edac_mc_workq_function(void *ptr)
> +{
> +	struct mem_ctl_info *mci = (struct mem_ctl_info *) ptr;

Unneeded (and undesirable) cast.

> +#endif
> +
> +	mutex_lock(&mem_ctls_mutex);
> +
> +	/* Only poll controllers that are running polled and have a check */
> +	if (edac_mc_assert_error_check_and_clear() && (mci->edac_check !=
> NULL))
> +		mci->edac_check(mci);
> +
> +	/*
> +	 * FIXME: temp place holder for PCI checks,
> +	 * goes away when we break out PCI
> +	 */
> +	edac_pci_do_parity_check();
> +
> +	mutex_unlock(&mem_ctls_mutex);
> +
> +	/* Reschedule */
> +	queue_delayed_work(edac_workqueue, &mci->work,
> edac_mc_get_poll_msec());
> +}
> +	}
> +}
> +
> +/*
> + * edac_reset_delay_period
> + */
> +
> +void edac_reset_delay_period(struct mem_ctl_info *mci, unsigned long
> value)
> +{
> +	mutex_lock(&mem_ctls_mutex);
> +
> +	/* cancel the current workq request */
> +	edac_mc_workq_teardown(mci);
> +
> +	/* restart the workq request, with new delay value */
> +	edac_mc_workq_setup(mci, value);
> +
> +	mutex_unlock(&mem_ctls_mutex);
> +}

I suspect this is deadlocky, for the reasons described earlier.

>  /* Return 0 on success, 1 on failure.
>   * Before calling this function, caller must
>   * assign a unique value to mci->mc_idx.
> @@ -351,6 +454,16 @@ int edac_mc_add_mc(struct mem_ctl_info *
>  		goto fail1;
>  	}
>  
> +	/* If there IS a check routine, then we are running POLLED */
> +	if (mci->edac_check != NULL) {
> +		/* This instance is NOW RUNNING */
> +		mci->op_state = OP_RUNNING_POLL;
> +
> +		edac_mc_workq_setup(mci, edac_mc_get_poll_msec());
> +	} else {
> +		mci->op_state = OP_RUNNING_INTERRUPT;
> +	}
> +