dasd: remove dynamic ioctl registration

PATCH In review Christoph Hellwig 4 participants 14 messages Dec 16, 2005 – Jan 17, 2006

Subsystems (2)

s390 s390 DASD driver

lore

Share & tools

lore

Christoph Hellwig

author

2005-12-16 14:33 UTC #0 lore raw

PATCH 2/2 dasd: remove dynamic ioctl registration

dasd has some really messy code to allow submodule to register ioctl. Right now there are two cases: cmd and eckd.

cmd was merged into the main module in the last patchh, so we don't need the mechanism for it anymore.

eckd is actually broken right now because we allow calling the eckd ioctls on other dasd disciplines aswell, but they assume eckd-specific private data.

Fix this second issue by adding an ioctl method to the dasd_discipline structure.

Signed-off-by Christoph Hellwig <[email protected]>

drivers/s390/block/dasd.c +0 -4

Index: linux-2.6.15-rc5/drivers/s390/block/dasd.c
===================================================================
@@ -1762,7 +1762,6 @@
 #ifdef CONFIG_PROC_FS
 	dasd_proc_exit();
 #endif
-	dasd_ioctl_exit();
         if (dasd_page_cache != NULL) {
 		kmem_cache_destroy(dasd_page_cache);
 		dasd_page_cache = NULL;

@@ -2034,9 +2033,6 @@
 	rc = dasd_parse();
 	if (rc)
 		goto failed;
-	rc = dasd_ioctl_init();
-	if (rc)
-		goto failed;
 #ifdef CONFIG_PROC_FS
 	rc = dasd_proc_init();
 	if (rc)

drivers/s390/block/dasd_diag.c +1 -0

Index: linux-2.6.15-rc5/drivers/s390/block/dasd_diag.c
===================================================================
@@ -612,6 +612,7 @@
 	.free_cp = dasd_diag_free_cp,
 	.dump_sense = dasd_diag_dump_sense,
 	.fill_info = dasd_diag_fill_info,
+	.ioctl = dasd_generic_ioctl,
 };
 
 static int __init

drivers/s390/block/dasd_eckd.c +22 ~~-40~~

90 lines not on this page · raw message

drivers/s390/block/dasd_fba.c +1 -0

Index: linux-2.6.15-rc5/drivers/s390/block/dasd_fba.c
===================================================================
@@ -565,6 +565,7 @@
 	.free_cp = dasd_fba_free_cp,
 	.dump_sense = dasd_fba_dump_sense,
 	.fill_info = dasd_fba_fill_info,
+	.ioctl = dasd_generic_ioctl,
 };
 
 static int __init

drivers/s390/block/dasd_int.h +13 ~~-13~~

Index: linux-2.6.15-rc5/drivers/s390/block/dasd_int.h
===================================================================
@@ -69,15 +69,6 @@
  */
 struct dasd_device;
 
-typedef int (*dasd_ioctl_fn_t) (struct block_device *bdev, int no, long args);
-
-struct dasd_ioctl {
-	struct list_head list;
-	struct module *owner;
-	int no;
-	dasd_ioctl_fn_t handler;
-};
-
 typedef enum {
 	dasd_era_fatal = -1,	/* no chance to recover		     */
 	dasd_era_none = 0,	/* don't recover, everything alright */

@@ -272,6 +263,7 @@
         /* i/o control functions. */
 	int (*fill_geometry) (struct dasd_device *, struct hd_geometry *);
 	int (*fill_info) (struct dasd_device *, struct dasd_information2_t *);
+	int (*ioctl) (struct block_device, unsigned int, unsigned long);
 };
 
 extern struct dasd_discipline *dasd_diag_discipline_pointer;

@@ -490,6 +482,17 @@
 int dasd_generic_notify(struct ccw_device *, int);
 void dasd_generic_auto_online (struct ccw_driver *);
 
+/* externals in dasd_cmb.c */
+#ifdef CONFIG_DASD_CMB
+int  dasd_cmd_ioctl(struct block_device *, unsigned int, unsigned long);
+#else
+static inline int dasd_cmd_ioctl(struct block_device *bdev,
+		unsigned int cmd, unsigned long arg)
+{
+	return -ENOIOCTLCMD;
+}
+#endif
+
 /* externals in dasd_devmap.c */
 extern int dasd_max_devindex;
 extern int dasd_probeonly;

@@ -522,10 +525,7 @@
 void dasd_destroy_partitions(struct dasd_device *);
 
 /* externals in dasd_ioctl.c */
-int  dasd_ioctl_init(void);
-void dasd_ioctl_exit(void);
-int  dasd_ioctl_no_register(struct module *, int, dasd_ioctl_fn_t);
-int  dasd_ioctl_no_unregister(struct module *, int, dasd_ioctl_fn_t);
+int  dasd_generic_ioctl(struct block_device *, unsigned int, unsigned long);
 int  dasd_ioctl(struct inode *, struct file *, unsigned int, unsigned long);
 long dasd_compat_ioctl(struct file *, unsigned int, unsigned long);

drivers/s390/block/dasd_ioctl.c +38 ~~-127~~

182 lines not on this page · raw message

drivers/s390/block/dasd_cmb.c +11 ~~-42~~

63 lines not on this page · raw message

Martin Schwidefsky

2005-12-16 14:58 UTC #1 lore raw

Quoted 2 lines · Christoph Hellwig

dasd has some really messy code to allow submodule to register ioctl. Right now there are two cases: cmd and eckd.

Wrong, at least four: cmf, eckd, err, and a binary only module from EMC. Now don't hit me for that binary module. But it has been there for 2.4 and we even reserved some ioctl numbers for them (240-255).

Quoted 2 lines

cmd was merged into the main module in the last patchh, so we don't need the mechanism for it anymore.

Seems resonable. The same could be done for the err module. Doesn't have to be a module, a config option is enough.

Quoted 6 lines

eckd is actually broken right now because we allow calling the eckd ioctls on other dasd disciplines aswell, but they assume eckd-specific private data.
Fix this second issue by adding an ioctl method to the dasd_discipline structure.

That can easily be fixed by adding a check in the ioctls as well. But a .ioctl entry in the discipline structure makes sense and would get rid of all dynamically added ioctls in our code. So I'm all in favor of it.

I would be cautious about ripping out the dynamic ioctls interface though. I have no idea if there still is an EMC module for 2.6 or other exploiters. It is an exported interface after all. It is not necessary to break these exploiters intentionally.

Signature · 6 lines

blue skies,
   Martin

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

Christoph Hellwig

author NAK

2005-12-16 15:01 UTC #2 lore raw

Quoted 7 lines · Martin Schwidefsky

Quoted 2 lines · Christoph Hellwig
dasd has some really messy code to allow submodule to register ioctl. Right now there are two cases: cmd and eckd.
Wrong, at least four: cmf, eckd, err, and a binary only module from EMC. Now don't hit me for that binary module. But it has been there for 2.4 and we even reserved some ioctl numbers for them (240-255).

NACK, binary modules are not a reason to keep broken things, rather one to fix it better sooner than later.

Quoted 5 lines

Quoted 2 lines
cmd was merged into the main module in the last patchh, so we don't need the mechanism for it anymore.
Seems resonable. The same could be done for the err module. Doesn't have to be a module, a config option is enough.

yes, it would clean up the err code a lot.

Quoted 6 lines

Quoted 2 lines
Fix this second issue by adding an ioctl method to the dasd_discipline structure.
That can easily be fixed by adding a check in the ioctls as well. But a .ioctl entry in the discipline structure makes sense and would get rid of all dynamically added ioctls in our code. So I'm all in favor of it.

Yepp. I generally prefer to not just fix things but rip out surrounding mess. Keeps code maintainable in the long run.

Quoted 4 lines

I would be cautious about ripping out the dynamic ioctls interface though. I have no idea if there still is an EMC module for 2.6 or other exploiters. It is an exported interface after all. It is not necessary to break these exploiters intentionally.

Yes, it is. Unrelated modules adding ioctls is a big no-way. Even more for binary modules. The EMC code deserves to be broken.

Martin Schwidefsky

2005-12-16 16:32 UTC #3 lore raw

Quoted 11 lines · Christoph Hellwig

Quoted 7 lines · Martin Schwidefsky
Quoted 2 lines · Christoph Hellwig
dasd has some really messy code to allow submodule to register ioctl. Right now there are two cases: cmd and eckd.
Wrong, at least four: cmf, eckd, err, and a binary only module from EMC. Now don't hit me for that binary module. But it has been there for 2.4 and we even reserved some ioctl numbers for them (240-255).
NACK, binary modules are not a reason to keep broken things, rather one to fix it better sooner than later.

I see your point. We shouldn't have introduced that interface in the first place but we really wanted to be able to run on EMC with decent speed.

Quoted 7 lines

Quoted 4 lines
I would be cautious about ripping out the dynamic ioctls interface though. I have no idea if there still is an EMC module for 2.6 or other exploiters. It is an exported interface after all. It is not necessary to break these exploiters intentionally.
Yes, it is. Unrelated modules adding ioctls is a big no-way. Even more for binary modules. The EMC code deserves to be broken.

Ok understood, but at least I have warn the EMC people about that change so that they can send a patch for the dasd driver ..

Signature · 6 lines

blue skies,
   Martin

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

Christoph Hellwig

author

2005-12-16 16:38 UTC #4 lore raw

Quoted 2 lines

Ok understood, but at least I have warn the EMC people about that change so that they can send a patch for the dasd driver ..

Sure, at this patch is a 2.6.16 thing, which is more than enough time for them. I'm happy to review any code they submit for kernel inclusion.

Ric Wheeler

2005-12-16 19:14 UTC #5 lore raw

Quoted 8 lines · Christoph Hellwig

Quoted 2 lines
Ok understood, but at least I have warn the EMC people about that change so that they can send a patch for the dasd driver ..
Sure, at this patch is a 2.6.16 thing, which is more than enough time for them. I'm happy to review any code they submit for kernel inclusion.

Hi Martin,

I am going to work with Carol (cc'ed below) to put out the module code that we have under GPL. We need to run it through our EMC (relatively newish) open source process to get official permission, but I don't expect any big hurdles.

Are you the right person for Carol to work with on the code structure, style, etc once we get the green light to move forward?

Regards,

ric

Martin Schwidefsky

2005-12-16 19:30 UTC #6 lore raw

On Fri, 2005-12-16 at 11:13 -0500, Ric Wheeler wrote:
Hi Ric,

Quoted 4 lines

I am going to work with Carol (cc'ed below) to put out the module code that we have under GPL. We need to run it through our EMC (relatively newish) open source process to get official permission, but I don't expect any big hurdles.

That is good news.

Quoted 2 lines

Are you the right person for Carol to work with on the code structure, style, etc once we get the green light to move forward?

Yes, sure. We should include Horst Hummel ([email protected]) to the discussion. He takes care of the dasd driver issues in our team.

Signature · 6 lines

blue skies,
   Martin

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

Christoph Hellwig

author

2006-01-06 11:02 UTC #7 lore raw

In reply to Christoph Hellwig

So can we please put this into -mm now so it can go to Linus for 2.6.16 still?

Ric Wheeler

2006-01-06 14:20 UTC #8 lore raw

Quoted 6 lines · Christoph Hellwig

So can we please put this into -mm now so it can go to Linus for 2.6.16 still?

If you could hold off just a couple of weeks, I hope that we can get through our EMC process junk and get the GPL'ed patch out to fix the symmetrix support part of this rolled in as well,

ric

Christoph Hellwig

author

2006-01-06 14:22 UTC #9 lore raw

Quoted 11 lines · Ric Wheeler

Quoted 6 lines · Christoph Hellwig
So can we please put this into -mm now so it can go to Linus for 2.6.16 still?
If you could hold off just a couple of weeks, I hope that we can get through our EMC process junk and get the GPL'ed patch out to fix the symmetrix support part of this rolled in as well,

Why? We never do things to support legally questionable binary modules. And on the practical side, does emc even ship modules for -mm release? What's the point of not putting it into -mm?

Ric Wheeler

2006-01-06 14:31 UTC #10 lore raw

Quoted 9 lines · Christoph Hellwig

Quoted 3 lines
If you could hold off just a couple of weeks, I hope that we can get through our EMC process junk and get the GPL'ed patch out to fix the symmetrix support part of this rolled in as well,
Why? We never do things to support legally questionable binary modules. And on the practical side, does emc even ship modules for -mm release? What's the point of not putting it into -mm?

No need for an EMC module, I think that we can issue a simple patch to dasd.c that removes the (silly) binary module that was there.

I would prefer that the clean up not break one of the few (and relatively common) devices supported by the dasd.c driver. If for no other reason, it would seem to make it more likely to be able test the existing patch properly.

ric

Martin Schwidefsky

2006-01-11 09:17 UTC #11 lore raw

Quoted 14 lines · Ric Wheeler

Quoted 11 lines · Christoph Hellwig
Quoted 5 lines
If you could hold off just a couple of weeks, I hope that we can get through our EMC process junk and get the GPL'ed patch out to fix the symmetrix support part of this rolled in as well,
Why? We never do things to support legally questionable binary modules. And on the practical side, does emc even ship modules for -mm release? What's the point of not putting it into -mm?

Quoted 7 lines

No need for an EMC module, I think that we can issue a simple patch to dasd.c that removes the (silly) binary module that was there.
I would prefer that the clean up not break one of the few (and relatively common) devices supported by the dasd.c driver. If for no other reason, it would seem to make it more likely to be able test the existing patch properly.

The patch we got from EMC is for 2.4 and in its current form would never have worked for 2.6 anyway. So 2.6 is already broken, no reason to hold off the ioctl removal patch. We'll come up with a cleaned up solution.

Christoph, please go ahead and push the patch to Andrew.

Signature · 6 lines

blue skies,
   Martin

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

Christoph Hellwig

author

2006-01-17 13:35 UTC #12 lore raw

Quoted 5 lines · Martin Schwidefsky

The patch we got from EMC is for 2.4 and in its current form would never have worked for 2.6 anyway. So 2.6 is already broken, no reason to hold off the ioctl removal patch. We'll come up with a cleaned up solution.
Christoph, please go ahead and push the patch to Andrew.

Andrew, should I resend the two patches or can you grab them from the mbox?

Andrew Morton

2006-01-17 20:50 UTC #13 lore raw

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · Martin Schwidefsky
The patch we got from EMC is for 2.4 and in its current form would never have worked for 2.6 anyway. So 2.6 is already broken, no reason to hold off the ioctl removal patch. We'll come up with a cleaned up solution.
Christoph, please go ahead and push the patch to Andrew.
Andrew, should I resend the two patches or can you grab them from the mbox?

I found them, and they still seem to apply.