Skip to contentSkip to conversation

ide: /proc/ide/hd*/settings rework

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 5 messages Jul 27, 2008 – Jul 29, 2008

Subsystems (2)
SCSI libata
lore
Share & tools
lore

Bartlomiej Zolnierkiewicz

author
#0 lore raw

cover ide: /proc/ide/hd*/settings rework

Hi,

I finally dusted off /proc/ide/hd*/settings rework (this has been laying on my hdd and waiting for the better days for at least 1.5 year, draft version even longer like ~3 years... sigh).

The main motivation for doing it is that with infrastructure for private IDE subsystem requests from Elias Oltmanns (which was merged recently) and this patchset it should be possible (by using private requests for device settings) to make IDE locking code a lot saner and get rid of of ide_spin_wait_hwgroup()-ugliness completely.

Elias if you would like to take care of it please go ahead [ from a quick look it seems this would mostly require adding new request type, pointing rq->special to setting's ->get or ->set method and putting setting's type (read/write -> 1 bit) + argument (int) somewhere inside request but you probably know better ].

Patches #1-2 are a preparatory cleanups, the main rework is in patch #3.

diffstat:

 drivers/ide/ide-cd.c     |   20 +--
 drivers/ide/ide-disk.c   |   94 ++++++++-------
 drivers/ide/ide-floppy.c |   39 ++++--
 drivers/ide/ide-probe.c  |    2
 drivers/ide/ide-proc.c   |  285 ++++++++++++++---------------------------------
 drivers/ide/ide-tape.c   |   77 ++++++++----
 drivers/ide/ide.c        |   25 ++--
 drivers/scsi/ide-scsi.c  |   54 ++++++--
 include/linux/ide.h      |  112 ++++++++++++------
 9 files changed, 356 insertions(+), 352 deletions(-)
author
#1 lore raw

PATCH 1/3 ide: call ide_proc_register_driver() later

Call ide_proc_register_driver() in ide*_setup() (just before ide*_add_settings() call) instead of in ->probe method.

Despite being basically a preparation for /proc/ide/hd*/settings rework this is a nice cleanup in itself.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
against pata tree

 drivers/ide/ide-cd.c     |    5 ++---
 drivers/ide/ide-disk.c   |    4 ++--
 drivers/ide/ide-floppy.c |    4 ++--
 drivers/ide/ide-tape.c   |    3 +--
 drivers/scsi/ide-scsi.c  |    2 +-
 5 files changed, 8 insertions(+), 10 deletions(-)
drivers/ide/ide-cd.c +2 -3
Index: b/drivers/ide/ide-cd.c
===================================================================
@@ -1916,6 +1916,8 @@ static int ide_cdrom_setup(ide_drive_t *
 		cd->devinfo.handle = NULL;
 		return 1;
 	}
+
+	ide_proc_register_driver(drive, cd->driver);
 	ide_cdrom_add_settings(drive);
 	return 0;
 }

@@ -2126,8 +2128,6 @@ static int ide_cd_probe(ide_drive_t *dri
 
 	ide_init_disk(g, drive);
 
-	ide_proc_register_driver(drive, &ide_cdrom_driver);
-
 	kref_init(&info->kref);
 
 	info->drive = drive;

@@ -2142,7 +2142,6 @@ static int ide_cd_probe(ide_drive_t *dri
 	g->driverfs_dev = &drive->gendev;
 	g->flags = GENHD_FL_CD | GENHD_FL_REMOVABLE;
 	if (ide_cdrom_setup(drive)) {
-		ide_proc_unregister_driver(drive, &ide_cdrom_driver);
 		ide_cd_release(&info->kref);
 		goto failed;
 	}
drivers/ide/ide-disk.c +2 -2
Index: b/drivers/ide/ide-disk.c
===================================================================
@@ -789,11 +789,13 @@ static inline void idedisk_add_settings(
 
 static void idedisk_setup(ide_drive_t *drive)
 {
+	struct ide_disk_obj *idkp = drive->driver_data;
 	ide_hwif_t *hwif = drive->hwif;
 	u16 *id = drive->id;
 	char *m = (char *)&id[ATA_ID_PROD];
 	unsigned long long capacity;
 
+	ide_proc_register_driver(drive, idkp->driver);
 	idedisk_add_settings(drive);
 
 	if (drive->id_read == 0)

@@ -1159,8 +1161,6 @@ static int ide_disk_probe(ide_drive_t *d
 
 	ide_init_disk(g, drive);
 
-	ide_proc_register_driver(drive, &idedisk_driver);
-
 	kref_init(&idkp->kref);
 
 	idkp->drive = drive;
drivers/ide/ide-floppy.c +2 -2
Index: b/drivers/ide/ide-floppy.c
===================================================================
@@ -1060,6 +1060,8 @@ static void idefloppy_setup(ide_drive_t 
 	}
 
 	(void) ide_floppy_get_capacity(drive);
+
+	ide_proc_register_driver(drive, floppy->driver);
 	idefloppy_add_settings(drive);
 }
 

@@ -1412,8 +1414,6 @@ static int ide_floppy_probe(ide_drive_t 
 
 	ide_init_disk(g, drive);
 
-	ide_proc_register_driver(drive, &idefloppy_driver);
-
 	kref_init(&floppy->kref);
 
 	floppy->drive = drive;
drivers/ide/ide-tape.c +1 -2
Index: b/drivers/ide/ide-tape.c
===================================================================
@@ -2512,6 +2512,7 @@ static void idetape_setup(ide_drive_t *d
 		tape->best_dsc_rw_freq * 1000 / HZ,
 		drive->using_dma ? ", DMA":"");
 
+	ide_proc_register_driver(drive, tape->driver);
 	idetape_add_settings(drive);
 }
 

@@ -2667,8 +2668,6 @@ static int ide_tape_probe(ide_drive_t *d
 
 	ide_init_disk(g, drive);
 
-	ide_proc_register_driver(drive, &idetape_driver);
-
 	kref_init(&tape->kref);
 
 	tape->drive = drive;
drivers/scsi/ide-scsi.c +1 -1
Index: b/drivers/scsi/ide-scsi.c
===================================================================
@@ -459,6 +459,7 @@ static void idescsi_setup (ide_drive_t *
 
 	drive->pc_callback = ide_scsi_callback;
 
+	ide_proc_register_driver(drive, scsi->driver);
 	idescsi_add_settings(drive);
 }
 

@@ -850,7 +851,6 @@ static int ide_scsi_probe(ide_drive_t *d
 	idescsi->host = host;
 	idescsi->disk = g;
 	g->private_data = &idescsi->driver;
-	ide_proc_register_driver(drive, &idescsi_driver);
 	err = 0;
 	idescsi_setup(drive, idescsi);
 	g->fops = &idescsi_ops;
author
#2 lore raw

PATCH 2/3 ide: preparations for /proc/ide/hd*/settings rework

In reply to Bartlomiej Zolnierkiewicz

After rework settings will be no longer created dynamically for each device so we need to make some fixups first.

* Use set_[ksettings,unmaskirq]() as a set function for ["keepsettings","unmaskirq"] setting.

* Allow writes to ["io_32bit","unmaskirq"] settings also when drive->no_[io_32bit,unmask] is set (this is checked later inside set_[io_32bit,unmaskirq]() anywyay and keeps consistency with the corresponding HDIO_SET_[32BIT,UNMASKINTR] ioctls).

* Use max possible multi sectors value (16) as an allowed max for "multcount" setting. set_multcount() set function checks against device's max possbile value anyway and it makes the proc setting consistent with the corresponding HDIO_SET_MULTCOUNT ioctl.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-disk.c |    3 +--
 drivers/ide/ide-proc.c |    6 +++---
 drivers/ide/ide.c      |    4 ++--
 include/linux/ide.h    |    2 ++
 4 files changed, 8 insertions(+), 7 deletions(-)
drivers/ide/ide-disk.c +1 -2
Index: b/drivers/ide/ide-disk.c
===================================================================
@@ -767,8 +767,7 @@ static void idedisk_add_settings(ide_dri
 			&drive->bios_sect, NULL);
 	ide_add_setting(drive, "address", SETTING_RW, TYPE_BYTE, 0, 2, 1, 1,
 			&drive->addressing, set_lba_addressing);
-	ide_add_setting(drive, "multcount", SETTING_RW, TYPE_BYTE, 0,
-			drive->id[ATA_ID_MAX_MULTSECT] & 0xff, 1, 1,
+	ide_add_setting(drive, "multcount", SETTING_RW, TYPE_BYTE, 0, 16, 1, 1,
 			&drive->mult_count, set_multcount);
 	ide_add_setting(drive, "nowerr", SETTING_RW, TYPE_BYTE, 0, 1, 1, 1,
 			&drive->nowerr, set_nowerr);
drivers/ide/ide-proc.c +3 -3
Index: b/drivers/ide/ide-proc.c
===================================================================
@@ -368,11 +368,11 @@ void ide_add_generic_settings (ide_drive
 /*
  *			  drive		setting name		read/write access				data type	min	max				mul_factor	div_factor	data pointer			set function
  */
-	__ide_add_setting(drive,	"io_32bit",		drive->no_io_32bit ? SETTING_READ : SETTING_RW,	TYPE_BYTE,	0,	1 + (SUPPORT_VLB_SYNC << 1),	1,		1,		&drive->io_32bit,		set_io_32bit,	0);
-	__ide_add_setting(drive,	"keepsettings",		SETTING_RW,					TYPE_BYTE,	0,	1,				1,		1,		&drive->keep_settings,		NULL,		0);
+	__ide_add_setting(drive,	"io_32bit",		SETTING_RW,					TYPE_BYTE,	0,	1 + (SUPPORT_VLB_SYNC << 1),	1,		1,		&drive->io_32bit,		set_io_32bit,	0);
+	__ide_add_setting(drive,	"keepsettings",		SETTING_RW,					TYPE_BYTE,	0,	1,				1,		1,		&drive->keep_settings,		set_ksettings,	0);
 	__ide_add_setting(drive,	"nice1",		SETTING_RW,					TYPE_BYTE,	0,	1,				1,		1,		&drive->nice1,			NULL,		0);
 	__ide_add_setting(drive,	"pio_mode",		SETTING_WRITE,					TYPE_BYTE,	0,	255,				1,		1,		NULL,				set_pio_mode,	0);
-	__ide_add_setting(drive,	"unmaskirq",		drive->no_unmask ? SETTING_READ : SETTING_RW,	TYPE_BYTE,	0,	1,				1,		1,		&drive->unmask,			NULL,		0);
+	__ide_add_setting(drive,	"unmaskirq",		SETTING_RW,					TYPE_BYTE,	0,	1,				1,		1,		&drive->unmask,			set_unmaskirq,	0);
 	__ide_add_setting(drive,	"using_dma",		SETTING_RW,					TYPE_BYTE,	0,	1,				1,		1,		&drive->using_dma,		set_using_dma,	0);
 	__ide_add_setting(drive,	"init_speed",		SETTING_RW,					TYPE_BYTE,	0,	70,				1,		1,		&drive->init_speed,		NULL,		0);
 	__ide_add_setting(drive,	"current_speed",	SETTING_RW,					TYPE_BYTE,	0,	70,				1,		1,		&drive->current_speed,		set_xfer_rate,	0);
drivers/ide/ide.c +2 -2
Index: b/drivers/ide/ide.c
===================================================================
@@ -305,7 +305,7 @@ int set_io_32bit(ide_drive_t *drive, int
 	return 0;
 }
 
-static int set_ksettings(ide_drive_t *drive, int arg)
+int set_ksettings(ide_drive_t *drive, int arg)
 {
 	if (arg < 0 || arg > 1)
 		return -EINVAL;

@@ -394,7 +394,7 @@ int set_pio_mode(ide_drive_t *drive, int
 	return 0;
 }
 
-static int set_unmaskirq(ide_drive_t *drive, int arg)
+int set_unmaskirq(ide_drive_t *drive, int arg)
 {
 	if (drive->no_unmask)
 		return -EPERM;
include/linux/ide.h +2 -0
Index: b/include/linux/ide.h
===================================================================
@@ -697,7 +697,9 @@ typedef struct ide_driver_s ide_driver_t
 extern struct mutex ide_setting_mtx;
 
 int set_io_32bit(ide_drive_t *, int);
+int set_ksettings(ide_drive_t *, int);
 int set_pio_mode(ide_drive_t *, int);
+int set_unmaskirq(ide_drive_t *, int);
 int set_using_dma(ide_drive_t *, int);
 
 /* ATAPI packet command flags */
author
#3 lore raw

PATCH 3/3 ide: /proc/ide/hd*/settings rework

In reply to Bartlomiej Zolnierkiewicz

* Add struct ide_devset, S_* flags, *DEVSET() & ide*_devset_*() macros.

* Add 'const struct ide_devset **settings' to ide_driver_t.

* Use 'const struct ide_devset **settings' in ide_drive_t instead of 'struct ide_settings_s *settings'. Then convert core code and device drivers to use struct ide_devset and co.:

- device settings are no longer allocated dynamically for each device but instead there is an unique struct ide_devset instance per setting

- device driver keeps the pointer to the table of pointers to its settings in ide_driver_t.settings

- generic settings are kept in ide_generic_setting[]

- ide_proc_[un]register_driver(), ide_find_setting_by_name(), ide_{read,write}_setting() and proc_ide_{read,write}_settings() are updated accordingly

- ide*_add_settings() are removed

* Remove no longer used __ide_add_setting(), ide_add_setting(), __ide_remove_setting() and auto_remove_settings().

* Remove no longer used TYPE_*, SETTING_*, ide_procset_t and ide_settings_t.

* ->keep_settings, ->using_dma, ->unmask, ->noflush, ->dsc_overlap, ->nice1, ->addressing, ->wcache and ->nowerr ide_drive_t fields can now be bitfield flags.

While at it:

* Rename ide_find_setting_by_name() to ide_find_setting().

* Rename write_wcache() to set_wcache().

There should be no functional changes caused by this patch.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-cd.c     |   15 +-
 drivers/ide/ide-disk.c   |   87 ++++++++------
 drivers/ide/ide-floppy.c |   35 +++--
 drivers/ide/ide-probe.c  |    2 
 drivers/ide/ide-proc.c   |  279 ++++++++++++++---------------------------------
 drivers/ide/ide-tape.c   |   74 ++++++++----
 drivers/ide/ide.c        |   21 ++-
 drivers/scsi/ide-scsi.c  |   52 ++++++--
 include/linux/ide.h      |  110 ++++++++++++------
 9 files changed, 340 insertions(+), 335 deletions(-)
drivers/ide/ide-cd.c +7 -8
Index: b/drivers/ide/ide-cd.c
===================================================================
@@ -1803,13 +1803,12 @@ static ide_proc_entry_t idecd_proc[] = {
 	{ NULL, 0, NULL, NULL }
 };
 
-static void ide_cdrom_add_settings(ide_drive_t *drive)
-{
-	ide_add_setting(drive, "dsc_overlap", SETTING_RW, TYPE_BYTE, 0, 1, 1, 1,
-			&drive->dsc_overlap, NULL);
-}
-#else
-static inline void ide_cdrom_add_settings(ide_drive_t *drive) { ; }
+ide_devset_rw(dsc_overlap, 0, 1, dsc_overlap);
+
+static const struct ide_devset *idecd_settings[] = {
+	&ide_devset_dsc_overlap,
+	NULL
+};
 #endif
 
 static const struct cd_list_entry ide_cd_quirks_list[] = {

@@ -1918,7 +1917,6 @@ static int ide_cdrom_setup(ide_drive_t *
 	}
 
 	ide_proc_register_driver(drive, cd->driver);
-	ide_cdrom_add_settings(drive);
 	return 0;
 }
 

@@ -1969,6 +1967,7 @@ static ide_driver_t ide_cdrom_driver = {
 	.error			= __ide_error,
 #ifdef CONFIG_IDE_PROC_FS
 	.proc			= idecd_proc,
+	.settings		= idecd_settings,
 #endif
 };
drivers/ide/ide-disk.c +48 -39

158 lines not on this page · raw message

drivers/ide/ide-floppy.c +23 -12
Index: b/drivers/ide/ide-floppy.c
===================================================================
@@ -1006,21 +1006,32 @@ static int idefloppy_identify_device(ide
 }
 
 #ifdef CONFIG_IDE_PROC_FS
-static void idefloppy_add_settings(ide_drive_t *drive)
+ide_devset_rw(bios_cyl,  0, 1023, bios_cyl);
+ide_devset_rw(bios_head, 0,  255, bios_head);
+ide_devset_rw(bios_sect, 0,   63, bios_sect);
+
+static int get_ticks(ide_drive_t *drive)
 {
 	idefloppy_floppy_t *floppy = drive->driver_data;
+	return floppy->ticks;
+}
 
-	ide_add_setting(drive, "bios_cyl", SETTING_RW, TYPE_INT, 0, 1023, 1, 1,
-			&drive->bios_cyl, NULL);
-	ide_add_setting(drive, "bios_head", SETTING_RW, TYPE_BYTE, 0, 255, 1, 1,
-			&drive->bios_head, NULL);
-	ide_add_setting(drive, "bios_sect", SETTING_RW,	TYPE_BYTE, 0,  63, 1, 1,
-			&drive->bios_sect, NULL);
-	ide_add_setting(drive, "ticks",	   SETTING_RW, TYPE_BYTE, 0, 255, 1, 1,
-			&floppy->ticks,	 NULL);
+static int set_ticks(ide_drive_t *drive, int arg)
+{
+	idefloppy_floppy_t *floppy = drive->driver_data;
+	floppy->ticks = arg;
+	return 0;
 }
-#else
-static inline void idefloppy_add_settings(ide_drive_t *drive) { ; }
+
+IDE_DEVSET(ticks, S_RW, 0, 255, get_ticks, set_ticks);
+
+static const struct ide_devset *idefloppy_settings[] = {
+	&ide_devset_bios_cyl,
+	&ide_devset_bios_head,
+	&ide_devset_bios_sect,
+	&ide_devset_ticks,
+	NULL
+};
 #endif
 
 static void idefloppy_setup(ide_drive_t *drive, idefloppy_floppy_t *floppy)

@@ -1062,7 +1073,6 @@ static void idefloppy_setup(ide_drive_t 
 	(void) ide_floppy_get_capacity(drive);
 
 	ide_proc_register_driver(drive, floppy->driver);
-	idefloppy_add_settings(drive);
 }
 
 static void ide_floppy_remove(ide_drive_t *drive)

@@ -1125,6 +1135,7 @@ static ide_driver_t idefloppy_driver = {
 	.error			= __ide_error,
 #ifdef CONFIG_IDE_PROC_FS
 	.proc			= idefloppy_proc,
+	.settings		= idefloppy_settings,
 #endif
 };
drivers/ide/ide-probe.c +0 -2
Index: b/drivers/ide/ide-probe.c
===================================================================
@@ -1332,8 +1332,6 @@ static void hwif_register_devices(ide_hw
 		if (!drive->present)
 			continue;
 
-		ide_add_generic_settings(drive);
-
 		snprintf(dev->bus_id, BUS_ID_SIZE, "%u.%u", hwif->index, i);
 		dev->parent = &hwif->gendev;
 		dev->bus = &ide_bus_type;
drivers/ide/ide-proc.c +87 -192

381 lines not on this page · raw message

drivers/ide/ide-tape.c +51 -23

92 lines not on this page · raw message

drivers/ide/ide.c +14 -7
Index: b/drivers/ide/ide.c
===================================================================
@@ -287,6 +287,8 @@ int ide_spin_wait_hwgroup (ide_drive_t *
 
 EXPORT_SYMBOL(ide_spin_wait_hwgroup);
 
+ide_devset_get(io_32bit, io_32bit);
+
 int set_io_32bit(ide_drive_t *drive, int arg)
 {
 	if (drive->no_io_32bit)

@@ -305,6 +307,8 @@ int set_io_32bit(ide_drive_t *drive, int
 	return 0;
 }
 
+ide_devset_get(ksettings, keep_settings);
+
 int set_ksettings(ide_drive_t *drive, int arg)
 {
 	if (arg < 0 || arg > 1)

@@ -318,6 +322,8 @@ int set_ksettings(ide_drive_t *drive, in
 	return 0;
 }
 
+ide_devset_get(using_dma, using_dma);
+
 int set_using_dma(ide_drive_t *drive, int arg)
 {
 #ifdef CONFIG_BLK_DEV_IDEDMA

@@ -394,6 +400,8 @@ int set_pio_mode(ide_drive_t *drive, int
 	return 0;
 }
 
+ide_devset_get(unmaskirq, unmask);
+
 int set_unmaskirq(ide_drive_t *drive, int arg)
 {
 	if (drive->no_unmask)

@@ -555,14 +563,13 @@ int generic_ide_ioctl(ide_drive_t *drive
 {
 	unsigned long flags;
 	ide_driver_t *drv;
-	int err = 0, (*setfunc)(ide_drive_t *, int);
-	u8 *val;
+	int err = 0, (*getfunc)(ide_drive_t *), (*setfunc)(ide_drive_t *, int);
 
 	switch (cmd) {
-	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;
-	case HDIO_GET_KEEPSETTINGS: val = &drive->keep_settings; goto read_val;
-	case HDIO_GET_UNMASKINTR:   val = &drive->unmask;	 goto read_val;
-	case HDIO_GET_DMA:	    val = &drive->using_dma;	 goto read_val;
+	case HDIO_GET_32BIT:	    getfunc = get_io_32bit;	 goto read_val;
+	case HDIO_GET_KEEPSETTINGS: getfunc = get_ksettings;	 goto read_val;
+	case HDIO_GET_UNMASKINTR:   getfunc = get_unmaskirq;	 goto read_val;
+	case HDIO_GET_DMA:	    getfunc = get_using_dma;	 goto read_val;
 	case HDIO_SET_32BIT:	    setfunc = set_io_32bit;	 goto set_val;
 	case HDIO_SET_KEEPSETTINGS: setfunc = set_ksettings;	 goto set_val;
 	case HDIO_SET_PIO_MODE:	    setfunc = set_pio_mode;	 goto set_val;

@@ -638,7 +645,7 @@ int generic_ide_ioctl(ide_drive_t *drive
 read_val:
 	mutex_lock(&ide_setting_mtx);
 	spin_lock_irqsave(&ide_lock, flags);
-	err = *val;
+	err = getfunc(drive);
 	spin_unlock_irqrestore(&ide_lock, flags);
 	mutex_unlock(&ide_setting_mtx);
 	return err >= 0 ? put_user(err, (long __user *)arg) : err;
drivers/scsi/ide-scsi.c +36 -16

70 lines not on this page · raw message

include/linux/ide.h +74 -36

189 lines not on this page · raw message

#4 lore raw
In reply to Bartlomiej Zolnierkiewicz
Quoted 12 lines · Bartlomiej Zolnierkiewicz

Hi,

I finally dusted off /proc/ide/hd*/settings rework (this has been laying on my hdd and waiting for the better days for at least 1.5 year, draft version even longer like ~3 years... sigh).

The main motivation for doing it is that with infrastructure for private IDE subsystem requests from Elias Oltmanns (which was merged recently) and this patchset it should be possible (by using private requests for device settings) to make IDE locking code a lot saner and get rid of of ide_spin_wait_hwgroup()-ugliness completely.

Quoted 5 lines

Elias if you would like to take care of it please go ahead [ from a quick look it seems this would mostly require adding new request type, pointing rq->special to setting's ->get or ->set method and putting setting's type (read/write -> 1 bit) + argument (int) somewhere inside request but you probably know better ].

Yes, I'll have a go at it. I haven't quite figured out yet whether I can (or should, for that matter) get rid of drive->special and convert the rest accordingly.

Anyway, I probably won't get it done before next week.

Regards,

Elias