Re: [patch] add scsi changer driver

[Christoph Hellwig <hch@infradead.org>]

[this should go to linux-scsi]

> +#include <linux/version.h>

not needed.

> +#include <asm/system.h>

I doubt you'll need this one.

> +#include <asm/uaccess.h>
> +
> +#include <linux/chio.h>			/* here are all the ioctls */

<linux/*.h> should always go before <asm/*.h>

> +#define MAJOR_NR	SCSI_CHANGER_MAJOR

please kill this one

> +#include "scsi.h"

never use this header but always the <scsi/*.h>

> +MODULE_SUPPORTED_DEVICE("sch");

no needed thsese days

> +static int dt_id[CH_DT_MAX] = { [ 0 ... (CH_DT_MAX-1) ] = -1 };
> +static int dt_lun[CH_DT_MAX];
> +module_param_array(dt_id,  int, NULL, 0444);
> +module_param_array(dt_lun, int, NULL, 0444);
> +
> +/* tell the driver about vendor-specific slots */
> +static int vendor_firsts[CH_TYPES-4];
> +static int vendor_counts[CH_TYPES-4];
> +module_param_array(vendor_firsts, int, NULL, 0444);
> +module_param_array(vendor_counts, int, NULL, 0444);
> +
> +static char *vendor_labels[CH_TYPES-4] = {
> +	"v0", "v1", "v2", "v3"
> +};
> +// module_param_string_array(vendor_labels, NULL, 0444);
> +
> +#define dprintk(fmt, arg...)    if (debug) \
> +        printk(KERN_DEBUG "%s: " fmt, ch->name, ##arg)
> +#define vprintk(fmt, arg...)    if (verbose) \
> +        printk(KERN_INFO "%s: " fmt, ch->name, ##arg)
> +
> +/* ------------------------------------------------------------------- */

> +static int ioctl32_register(void)
> +{
> +	unsigned int i;
> +	int err;
> +
> +	for (i = 0; i < ARRAY_SIZE(ioctl32_cmds); i++) {
> +		err = register_ioctl32_conversion(ioctl32_cmds[i].cmd,NULL);
> +		if (err >= 0)
> +			ioctl32_cmds[i].reg++;
> +	}
> +	return 0;
> +}

please implement ->compat_ioctl instead.

> +	int errno, retries = 0, timeout;
> +	DECLARE_COMPLETION(wait);
> +	Scsi_Request *sr;
> +	
> +	sr = scsi_allocate_request(ch->device, GFP_ATOMIC);

wouldn't a GFP_KERNEL do just fine?

> +	if (NULL == sr)
> +		return -ENOMEM;

normal kernel style would be

	if (!s)
		return -ENOMEM;

> +	list_for_each(item,&ch_devlist) {
> +		tmp = list_entry(item, scsi_changer, list);

list_for_each_entry

> +	list_for_each(item,&ch_devlist) {
> +		tmp = list_entry(item, scsi_changer, list);

dito