proper replacements for ->proc_info
Subsystems (4)
PATCH 1/1 proper replacements for ->proc_info
Two new host template methods:
int (* show_info)(struct Scsi_Host *, struct seq_file *);
int (* store_info)(struct Scsi_Host *, const char *, size_t);First one is used for reading of /proc/scsi/<driver>/<host>, second for writing. They get an explicit hosy pointer instead of the integer number. Read side outputs into the simple version of the seq_file interface so lots of crappy string handling can go away, write side isn't much different from the old version, just properly split out.
->proc_info continues to work but I hope to move over all drivers before 2.6.
scsi_debug and aic7xxx are the example drivers in this patch, more will follow very soon.
btw, even this first patch already removes more code than it adds..
drivers/scsi/hosts.h +17 -7
@@ -63,13 +63,6 @@/* The pointer to the /proc/scsi directory entry */ struct proc_dir_entry *proc_dir; -/* proc-fs info function. -* Can be used to export driver statistics and other infos to the world -* outside the kernel ie. userspace and it also provides an interface -* to feed the driver with information. Check eata_dma_proc.c for reference -*/ -int (*proc_info)(char *,char **,off_t ,int ,int ,int ); -/* * The name pointer is a pointer to the name of the SCSI * device detected.
@@ -265,6 +258,23 @@*/ int (* bios_param)(struct scsi_device *,struct block_device *,sector_t ,int []); + +/* +* Show host information in /proc/scsi/<driver>/<host>. +* Optional. +*/ +int (* show_info)(struct Scsi_Host *,struct seq_file *); + +/* +* Allow user-commands to be written to /proc/scsi/<driver>/<host>. +* Optional. +*/ +int (* store_info)(struct Scsi_Host *,const char *,size_t ); + +/* +* Obsolete interface version of the two above. +*/ +int (* proc_info)(char *,char **,off_t ,int ,int ,int );/* * This determines if we will use a non-interrupt driven
drivers/scsi/scsi_debug.c +23 -34
78 lines not on this page · raw message
drivers/scsi/scsi_debug.h +4 -2
@@ -14,7 +14,8 @@static int scsi_debug_bus_reset (struct scsi_cmnd *);static int scsi_debug_device_reset (struct scsi_cmnd *);static int scsi_debug_host_reset (struct scsi_cmnd *); -static int scsi_debug_proc_info (char *,char **,off_t ,int ,int ,int ); +static int scsi_debug_store_info (struct Scsi_Host *,const char *,size_t ); +static int scsi_debug_show_info (struct Scsi_Host *,struct seq_file *);static const char *scsi_debug_info (struct Scsi_Host *);/*
@@ -25,7 +26,8 @@#define SCSI_DEBUG_MAX_CMD_LEN 16static Scsi_Host_Template sdebug_driver_template = { - .proc_info = scsi_debug_proc_info, + .show_info = scsi_debug_show_info, + .store_info = scsi_debug_store_info, .name ="SCSI DEBUG" , .info = scsi_debug_info, .slave_alloc = scsi_debug_slave_alloc,
drivers/scsi/scsi_proc.c +70 -12
101 lines not on this page · raw message
drivers/scsi/aic7xxx/aic7xxx_osm.c +5 -1
@@ -1268,7 +1268,8 @@Scsi_Host_Template aic7xxx_driver_template = { .module =THIS_MODULE , .name ="aic7xxx" , - .proc_info = ahc_linux_proc_info, + .show_info = ahc_linux_show_info, + .store_info = ahc_linux_store_info, .info = ahc_linux_info, .queuecommand = ahc_linux_queue, .eh_abort_handler = ahc_linux_abort,
@@ -4071,12 +4072,15 @@printf ("*): " ); else printf("%d): ", target); +#if 0 /* XXX(hch): need to come up with some fake seq_file stuff +for this. Should probably go into seq_file.c */ ahc_format_transinfo (&info, &tinfo->curr);if (info.pos < info.length) *info.buffer ='\0' ;else buf[info.length -1 ] ='\0' ;printf ("%s" , buf); +#endif break ; }case AC_SENT_BDR :
drivers/scsi/aic7xxx/aic7xxx_osm.h +5 -2
@@ -68,6 +68,7 @@#include <linux/smp_lock.h> #include <linux/version.h> #include <linux/module.h> +#include <linux/seq_file.h> #include <asm/byteorder.h> #include <asm/io.h>
@@ -673,7 +674,7 @@int pos; }; -void ahc_format_transinfo (struct info_str *info, +void ahc_format_transinfo (struct seq_file *s,struct ahc_transinfo *tinfo);/******************************** Locking *************************************/
@@ -1022,7 +1023,9 @@ (((dev_softc)->dma_mask = mask) && 0)#endif /**************************** Proc FS Support *********************************/ -int ahc_linux_proc_info (char *,char **,off_t ,int ,int ,int ); +int ahc_linux_show_info (struct Scsi_Host *host,struct seq_file *s); +int ahc_linux_store_info (struct Scsi_Host *host, +const char *buffer,size_t length);/*************************** Domain Validation ********************************/ #define AHC_DV_CMD (cmd) ((cmd)->scsi_done == ahc_linux_dv_complete)
===== drivers/scsi/aic7xxx/aic7xxx_proc.c 1.7 vs edited =====
drivers/scsi/aic7xxx/aic7xxx_proc.c +46 -121
254 lines not on this page · raw message
Quoted 4 lines · Christoph Hellwig
Two new host template methods:
int (* show_info)(struct Scsi_Host *, struct seq_file *); int (* store_info)(struct Scsi_Host *, const char *, size_t);
Well, this does look good, but by extension we could use something like this as a generic way to ship information to and from sysfs as well.
How about adding a "char *property" qualifier? If it's null you just dump the whole lot (for /proc) but if it has a value, it's that specific property from sysfs.
James
Quoted 7 lines · James Bottomley
Quoted 4 lines · Christoph Hellwig
Two new host template methods:
int (* show_info)(struct Scsi_Host *, struct seq_file *); int (* store_info)(struct Scsi_Host *, const char *, size_t);
Quoted 6 lines
Well, this does look good, but by extension we could use something like this as a generic way to ship information to and from sysfs as well.
How about adding a "char *property" qualifier? If it's null you just dump the whole lot (for /proc) but if it has a value, it's that specific property from sysfs.
For store_info? I don't think that's a good idea - most users of store_info should really be converted to sysfs insead, but this would break all kinds of existing apps. So we should add it but not rip out the procfs support. Mixing up procfs and sysfs code isn't a good idea anyway - this will lead to overloaded APIs again.
Quoted 7 lines · James Bottomley
Quoted 4 lines · Christoph Hellwig
Two new host template methods:
int (* show_info)(struct Scsi_Host *, struct seq_file *); int (* store_info)(struct Scsi_Host *, const char *, size_t);
Quoted 6 lines
Well, this does look good, but by extension we could use something like this as a generic way to ship information to and from sysfs as well.
How about adding a "char *property" qualifier? If it's null you just dump the whole lot (for /proc) but if it has a value, it's that specific property from sysfs.
Okay, after our off-list discussion here's a revised patch. This one removes ->store_info. Most of the users of writing to the proc file should be converted to sysfs for their parameters instead, and those that do other things than that need an explicit mangment misc device or something similar.
I've replaced aic7xx as example driver with 53c700 as aic7xxx falls into the category of a complex write handler above.
drivers/scsi/53c700.c +16 -30
@@ -124,7 +124,7 @@#include <linux/spinlock.h> #include <linux/completion.h> #include <linux/sched.h> -#include <linux/proc_fs.h> +#include <linux/seq_file.h> #include <asm/dma.h> #include <asm/system.h> #include <asm/io.h>
@@ -166,7 +166,7 @@ STATIC int NCR_700_bus_reset(Scsi_Cmnd * SCpnt); STATIC int NCR_700_dev_reset(Scsi_Cmnd * SCpnt); STATIC int NCR_700_host_reset(Scsi_Cmnd * SCpnt); -STATIC int NCR_700_proc_directory_info(char *, char **, off_t, int, int, int); +STATIC int NCR_700_show_info(struct Scsi_Host *host, struct seq_file *s); STATIC void NCR_700_chip_setup(struct Scsi_Host *host); STATIC void NCR_700_chip_reset(struct Scsi_Host *host); STATIC int NCR_700_slave_configure(Scsi_Device *SDpnt);
@@ -274,7 +274,7 @@ tpnt->sg_tablesize =NCR_700_SG_SEGMENTS ; tpnt->cmd_per_lun =NCR_700_CMD_PER_LUN ; tpnt->use_clustering =DISABLE_CLUSTERING ; - tpnt->proc_info = NCR_700_proc_directory_info; + tpnt->show_info = NCR_700_show_info; tpnt->slave_configure = NCR_700_slave_configure; tpnt->slave_destroy = NCR_700_slave_destroy; tpnt->use_blk_tcq =1 ;
@@ -1703,41 +1703,27 @@return retval; } -/* FIXME: Need to put some proc information in and plumb it -* into the scsi proc system */ STATIC int -NCR_700_proc_directory_info (char *proc_buf,char **startp, -off_t offset,int bytes_available, -int host_no,int write) +NCR_700_show_info (struct Scsi_Host *host,struct seq_file *s) { -static char buf[4096 ];/* 1 page should be sufficient */ -int len =0 ; -struct Scsi_Host *host;struct NCR_700_Host_Parameters *hostdata; -Scsi_Device *SDp; - - host =scsi_host_hn_get (host_no); -if (host ==NULL ) -return 0 ; +struct scsi_device *sdev; -if (write) { -/* FIXME: Clear internal statistics here */ -return 0 ; - } hostdata = (struct NCR_700_Host_Parameters *)host->hostdata[0 ]; - len +=sprintf (&buf[len],"Total commands outstanding: %d\n" , hostdata->command_slot_count); - len +=sprintf (&buf[len],"\ + +seq_printf (s,"Total commands outstanding: %d\n" , + hostdata->command_slot_count); +seq_printf (s,"\ Target Active Next Tag\n\ ====== ====== ========\n" ); -list_for_each_entry (SDp, &host->my_devices, siblings) { - len +=sprintf (&buf[len]," %2d:%2d %4d %4d\n" , SDp->id, SDp->lun,NCR_700_get_depth (SDp), SDp->current_tag); +list_for_each_entry (sdev, &host->my_devices, siblings) { +seq_printf (s," %2d:%2d %4d %4d\n" , + sdev->id, sdev->lun, +NCR_700_get_depth (sdev), + sdev->current_tag); } -if ((len -= offset) <=0 ) -return 0 ; -if (len > bytes_available) - len = bytes_available; -memcpy (proc_buf, buf + offset, len); -return len; + +return 0 ; }STATIC int
drivers/scsi/hosts.h +11 -7
@@ -63,13 +63,6 @@/* The pointer to the /proc/scsi directory entry */ struct proc_dir_entry *proc_dir; -/* proc-fs info function. -* Can be used to export driver statistics and other infos to the world -* outside the kernel ie. userspace and it also provides an interface -* to feed the driver with information. Check eata_dma_proc.c for reference -*/ -int (*proc_info)(char *,char **,off_t ,int ,int ,int ); -/* * The name pointer is a pointer to the name of the SCSI * device detected.
@@ -265,6 +258,17 @@*/ int (* bios_param)(struct scsi_device *,struct block_device *,sector_t ,int []); + +/* +* Show host information in /proc/scsi/<driver>/<host>. +* Optional. +*/ +int (* show_info)(struct Scsi_Host *,struct seq_file *); + +/* +* DON'T USE. +*/ +int (* proc_info)(char *,char **,off_t ,int ,int ,int );/* * This determines if we will use a non-interrupt driven
drivers/scsi/scsi_debug.c +6 -38
69 lines not on this page · raw message
drivers/scsi/scsi_debug.h +2 -2
@@ -14,7 +14,7 @@static int scsi_debug_bus_reset (struct scsi_cmnd *);static int scsi_debug_device_reset (struct scsi_cmnd *);static int scsi_debug_host_reset (struct scsi_cmnd *); -static int scsi_debug_proc_info (char *,char **,off_t ,int ,int ,int ); +static int scsi_debug_show_info (struct Scsi_Host *,struct seq_file *);static const char *scsi_debug_info (struct Scsi_Host *);/*
@@ -25,7 +25,7 @@#define SCSI_DEBUG_MAX_CMD_LEN 16static Scsi_Host_Template sdebug_driver_template = { - .proc_info = scsi_debug_proc_info, + .show_info = scsi_debug_show_info, .name ="SCSI DEBUG" , .info = scsi_debug_info, .slave_alloc = scsi_debug_slave_alloc,
drivers/scsi/scsi_proc.c +36 -12
67 lines not on this page · raw message
drivers/scsi/53c700.c line 1723
Quoted 5 lines · Christoph Hellwig
drivers/scsi/53c700.c +5
-0+list_for_each_entry (sdev, &host->my_devices, siblings) { +seq_printf (s," %2d:%2d %4d %4d\n" , + sdev->id, sdev->lun, +NCR_700_get_depth (sdev), + sdev->current_tag);
And that loop is protected by ...?
Quoted 9 lines · On Fri, Apr 25, 2003 at 11:41:37AM +0100…
drivers/scsi/53c700.c line 1723
Quoted 5 lines · Christoph Hellwig
drivers/scsi/53c700.c +5
-0+list_for_each_entry (sdev, &host->my_devices, siblings) { +seq_printf (s," %2d:%2d %4d %4d\n" , + sdev->id, sdev->lun, +NCR_700_get_depth (sdev), + sdev->current_tag);And that loop is protected by ...?
The right lock would be sdev->list_lock.
Quoted 3 lines · Christoph Hellwig
Quoted 1 line
And that loop is protected by ...?
The right lock would be
sdev->list_lock.
Actually it would probably be host->host_lock, not a per-sdev lock of course. Locking down the my_device traversal is on my TODO list but a separate thing, though.
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · Christoph Hellwig
Quoted 1 line
And that loop is protected by ...?
The right lock would be
sdev->list_lock.Actually it would probably be
host->host_lock, not a per-sdev lock of course. Locking down themy_devicetraversal is on my TODO list but a separate thing, though.
Oh, well.
Digging through all the (ab)users of ->my_devices I start to develop a really strong favour of just nuking /proc/scsi/<driver>/* stuff entirely. There's really not much in there that shouldn't better be done in sysfs.