Re: [PATCH 1/1] cciss: resubmit kernel scan thread for MSA2012

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

On Wed, 11 Mar 2009 11:17:33 -0500
"Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:
>

We haven't finished with you yet ;)

> Changed the logic in the while loop. The thread works, whenever I change my
> config the next IO to the storage returns the unit attention
> LUN_DATA_CHANGED. The thread fires off and either adds or removes the
> logical volume.
> 
> Signed-off-by: Mike Miller <mike.miller@hp.com>

That's not a suitable changelog for the patch.  Please maintain
changelogs alongside the patch, update them (if needed) with each
iteration and resend the full changelog each time.

> +static int scan_thread(ctlr_info_t *h)
> +{
> +	int rc;
> +	DECLARE_COMPLETION_ONSTACK(wait);
> +	h->rescan_wait = &wait;
> +
> +	while (!kthread_should_stop()) {
> +		rc = wait_for_completion_interruptible(&wait);
> +		if (!rc)
> +			rebuild_lun_table(h, 0);
> +	}
> +	return 0;
> +}

This will run rebuild_lun_table() in the case where the thread is being
asked to terminate.  Seems a bit peculiar, although hopefully harmless.

> +	snprintf(cciss_scan, 14, "cciss_scan%02d", i);
> +	hba[i]->cciss_scan_thread = kthread_run((void *)scan_thread, hba[i],
> +				cciss_scan);

kthread_run() takes printf-style arguments, so this can be

	hba[i]->cciss_scan_thread = kthread_run(scan_thread, hba[i],
					 "cciss_scan%02d", i);

and cciss_scan[] is removed.


And that void* cast of scan_thread is a bit grubby.  It would be
better to be more honest to the type system and do

static int scan_thread(void *data)
{
	ctlr_info_t *h = data;
	...
}