lkml.org 
[lkml]   [2006]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/3] s390: dasd extended error reporting module.
From
Date
Christoph Hellwig <hch@lst.de> wrote on 04.02.2006 14:14:19:
>
> 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.

For the dynamically registered ioctls there is exactly such a
concept in place. So a try_module_get is done _before_ the
dasd_ioctl_set_eer function is called. That function again calls
dasd_eer_enable_on_device which does the try_module_get you complain
about. So, for each device on which the error reporing is enabled, a
try_module_get is done, and it should be safe for those devices to
call the respective notifier functions later on.

>
> 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.

I know that you don't like that dynamic ioctl interface, but I would
like to leave that discussion to Horst.
But besides that, the ioctl in question is needed on the DASD device,
since we want to enable or disable error reporting per DASD device.

Best Regards / Mit freundlichen Grüßen

Stefan Weinhuber

-------------------------------------------------------------------
IBM Deutschland Entwicklung GmbH
Linux for zSeries Development & Services

-
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/

\
 
 \ /
  Last update: 2006-02-06 19:26    [W:0.037 / U:0.396 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site