Re: [PATCH 1/3] s390: dasd extended error reporting module.

[Christoph Hellwig <hch@lst.de>]

On Wed, Feb 01, 2006 at 12:56:49PM +0100, Heiko Carstens wrote:
> From: Stefan Weinhuber <wein@de.ibm.com>
> 
> The DASD extended error reporting is a facility that allows to
> get detailed information about certain problems in the DASD I/O.
> This information can be used to implement fail-over applications
> that can recover these problems.
> This is a resubmit of this patch because at first submission it
> didn't get included due to Christoph's ioctl changes.
> Since these aren't in the -mm tree anymore this one should be
> merged now.

NACK again.  (although andrew unfortunately pushed this to Linus
already, it would be good if this went out again before 2.6.16).

If you really want this functionality in a separate module you need
to call try_module_get on the module structure of the eer module
everytime before calling into it.  You need to register a structure
ala

struct dasd_eer_ops {
	struct module *owner;
	void (*disable_on_device)(struct dasd_device *);
	void (*write_trigger)(struct dasd_eer_trigger *);
};

and then have wrappers in the core dasd code ala

eer_disable_on_device(struct dasd_device *dev)
{
	if (try_module_get(eer_ops->owner)) {
		eer_ops->write_trigger(dev);
		module_put(eer_ops->owner);
	}
}

the module_get/put on THIS_MODULE in the patch are completely broken.

the ioctl registration is also totally not acceptable.  firtly this
interface is absolutely suitable for sysfs, although that might only work
if it's in the main module.  But even if you go for ioctls it should be
on your separate char device and not use the broken dynamic ioctl
registration which _must_ not be used in any new code.

Besides that there's tons of the usual style issues on the code, please
use mutexes, please make lots of variables that could be static, etc.

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