Skip to contentSkip to conversation

add PCI API to sysfs

PATCH In review Jesse Barnes 4 participants 10 messages Dec 20, 2004 – Dec 21, 2004

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 add PCI API to sysfs

Ok, how does this one look? It needs some obvious work, but is the approach of adding functionality to sysfs instead of /proc the right way to go? What I've done:

o add mmap support to bin files in sysfs
o make PCI resources available via mmap
o make legacy I/O and memory space available in sysfs

e.g.

0000:01:03.0/

|-- class
|-- config
|-- detach_state
|-- device
|-- driver -> ../../../bus/pci/drivers/qla1280
|-- host0
| <snip host stuff>
|-- irq
|-- legacy_io
|-- legacy_mem
|-- local_cpus
|-- resource
|-- resource0
|-- resource1
|-- rom
|-- subsystem_device
|-- subsystem_vendor

`-- vendor

Things that need work:
o documentation
o file placement
o more error checking (e.g. don't call attr->read() unless it exists!)

I'm not sure where we should put the legacy stuff. On sn2, each PCI root bus could have its own legacy files, but on other machines it might be a per-domain attribute. Maybe we could define the API to be such that applications have to walk upward from the device they're interested in to get to legacy stuff? Also, some arches (like ia64) don't allow mmap of I/O space, so I/O resources exported above will be useless unless I add read/write methods for I/O space.

Thanks,
Jesse

===== arch/ia64/pci/pci.c 1.59 vs edited =====

arch/ia64/pci/pci.c +118 -1
@@ -518,7 +518,7 @@
 	 * Leave vm_pgoff as-is, the PCI space address is the physical
 	 * address on this platform.
 	 */
-	vma->vm_flags |= (VM_SHM | VM_LOCKED | VM_IO);
+	vma->vm_flags |= (VM_SHM | VM_RESERVED | VM_IO);
 
 	if (write_combine)
 		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
@@ -530,6 +530,123 @@
 		return -EAGAIN;
 
 	return 0;
+}
+
+/**
+ * pci_mmap_legacy_page_range - map legacy memory space to userland
+ * @dev: device to map
+ * @vma: vma passed in by mmap
+ *
+ * Map legacy memory space for this device back to userspace using a machine
+ * vector to get the base address.
+ */
+int
+pci_mmap_legacy_page_range(struct pci_dev *dev, struct vm_area_struct *vma)
+{
+	unsigned long addr;
+	int ret;
+
+	if ((ret = pci_get_legacy_mem(dev, &addr)))
+		return ret;
+
+	vma->vm_pgoff += addr >> PAGE_SHIFT;
+
+	/*
+	 * Leave vm_pgoff as-is, the PCI space address is the physical
+	 * address on this platform.
+	 */
+	vma->vm_flags |= (VM_SHM | VM_RESERVED | VM_IO);
+
+	if (remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
+			    vma->vm_end - vma->vm_start, vma->vm_page_prot))
+		return -EAGAIN;
+
+	return 0;
+}
+
+/**
+ * ia64_pci_get_legacy_mem - generic legacy mem routine
+ * @dev: device pointer
+ * @addr: caller allocated variable for the base address
+ *
+ * Find the base of legacy memory for @dev.  This is typically the first
+ * megabyte of bus address space for @dev or is simply 0 on platforms whose
+ * chipsets support legacy I/O and memory routing.  Returns 0 on success
+ * or a standard error code on failure.
+ *
+ * This is the ia64 generic version of this routine.  Other platforms
+ * are free to override it with a machine vector.
+ */
+int ia64_pci_get_legacy_mem(struct pci_dev *dev, unsigned long *addr)
+{
+	*addr = 0;
+	return 0;
+}
+
+/**
+ * ia64_pci_legacy_read - read from legacy I/O space
+ * @dev: device to read
+ * @port: legacy port value
+ * @val: caller allocated storage for returned value
+ * @size: number of bytes to read
+ *
+ * Simply reads @size bytes from @port and puts the result in @val.
+ *
+ * Again, this (and the write routine) are generic versions that can be
+ * overridden by the platform.  This is necessary on platforms that don't
+ * support legacy I/O routing or that hard fail on legacy I/O timeouts.
+ */
+int ia64_pci_legacy_read(struct pci_dev *dev, u16 port, u32 *val, u8 size)
+{
+	int ret = size;
+
+	switch (size) {
+	case 1:
+		*val = inb(port);
+		break;
+	case 2:
+		*val = inw(port);
+		break;
+	case 4:
+		*val = inl(port);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+/**
+ * ia64_pci_legacy_write - perform a legacy I/O write
+ * @dev: device pointer
+ * @port: port to write
+ * @val: value to write
+ * @size: number of bytes to write from @val
+ *
+ * Simply writes @size bytes of @val to @port.
+ */
+int ia64_pci_legacy_write(struct pci_dev *dev, u16 port, u32 val, u8 size)
+{
+	int ret = 0;
+
+	switch (size) {
+	case 1:
+		outb(val, port);
+		break;
+	case 2:
+		outw(val, port);
+		break;
+	case 4:
+		outl(val, port);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
 }
 
 /**

===== arch/ia64/sn/pci/pci_dma.c 1.2 vs edited =====

arch/ia64/sn/pci/pci_dma.c +74 -0

77 lines not on this page · raw message

===== drivers/pci/pci-sysfs.c 1.13 vs edited =====

drivers/pci/pci-sysfs.c +166 -0

200 lines not on this page · raw message

===== fs/sysfs/bin.c 1.19 vs edited =====

fs/sysfs/bin.c +12 -2
@@ -92,6 +92,15 @@
 	return count;
 }
 
+static int mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct dentry *dentry = file->f_dentry;
+	struct bin_attribute *attr = to_bin_attr(dentry);
+	struct kobject *kobj = to_kobj(dentry->d_parent);
+
+	return attr->mmap(kobj, attr, vma);
+}
+
 static int open(struct inode * inode, struct file * file)
 {
 	struct kobject *kobj = sysfs_get_kobject(file->f_dentry->d_parent);
@@ -107,9 +116,9 @@
 		goto Done;
 
 	error = -EACCES;
-	if ((file->f_mode & FMODE_WRITE) && !attr->write)
+	if ((file->f_mode & FMODE_WRITE) && !(attr->write || attr->mmap))
 		goto Error;
-	if ((file->f_mode & FMODE_READ) && !attr->read)
+	if ((file->f_mode & FMODE_READ) && !(attr->read || attr->mmap))
 		goto Error;
 
 	error = -ENOMEM;
@@ -144,6 +153,7 @@
 struct file_operations bin_fops = {
 	.read		= read,
 	.write		= write,
+	.mmap		= mmap,
 	.llseek		= generic_file_llseek,
 	.open		= open,
 	.release	= release,

===== include/asm-ia64/machvec.h 1.29 vs edited =====

include/asm-ia64/machvec.h +24 -0
@@ -20,6 +20,7 @@
 struct irq_desc;
 struct page;
 struct mm_struct;
+struct pci_dev;
 
 typedef void ia64_mv_setup_t (char **);
 typedef void ia64_mv_cpu_init_t (void);
@@ -31,6 +32,11 @@
 typedef struct irq_desc *ia64_mv_irq_desc (unsigned int);
 typedef u8 ia64_mv_irq_to_vector (unsigned int);
 typedef unsigned int ia64_mv_local_vector_to_irq (u8);
+typedef int ia64_mv_pci_get_legacy_mem_t (struct pci_dev *, unsigned long *);
+typedef int ia64_mv_pci_legacy_read_t (struct pci_dev *, u16 port, u32 *val,
+				       u8 size);
+typedef int ia64_mv_pci_legacy_write_t (struct pci_dev *, u16 port, u32 val,
+					u8 size);
 
 /* DMA-mapping interface: */
 typedef void ia64_mv_dma_init (void);
@@ -125,6 +131,9 @@
 #  define platform_irq_desc		ia64_mv.irq_desc
 #  define platform_irq_to_vector	ia64_mv.irq_to_vector
 #  define platform_local_vector_to_irq	ia64_mv.local_vector_to_irq
+#  define platform_pci_get_legacy_mem	ia64_mv.pci_get_legacy_mem
+#  define platform_pci_legacy_read	ia64_mv.pci_legacy_read
+#  define platform_pci_legacy_write	ia64_mv.pci_legacy_write
 #  define platform_inb		ia64_mv.inb
 #  define platform_inw		ia64_mv.inw
 #  define platform_inl		ia64_mv.inl
@@ -172,6 +181,9 @@
 	ia64_mv_irq_desc *irq_desc;
 	ia64_mv_irq_to_vector *irq_to_vector;
 	ia64_mv_local_vector_to_irq *local_vector_to_irq;
+	ia64_mv_pci_get_legacy_mem_t *pci_get_legacy_mem;
+	ia64_mv_pci_legacy_read_t *pci_legacy_read;
+	ia64_mv_pci_legacy_write_t *pci_legacy_write;
 	ia64_mv_inb_t *inb;
 	ia64_mv_inw_t *inw;
 	ia64_mv_inl_t *inl;
@@ -215,6 +227,9 @@
 	platform_irq_desc,			\
 	platform_irq_to_vector,			\
 	platform_local_vector_to_irq,		\
+	platform_pci_get_legacy_mem,		\
+	platform_pci_legacy_read,		\
+	platform_pci_legacy_write,		\
 	platform_inb,				\
 	platform_inw,				\
 	platform_inl,				\
@@ -329,6 +344,15 @@
 #endif
 #ifndef platform_local_vector_to_irq
 # define platform_local_vector_to_irq	__ia64_local_vector_to_irq
+#endif
+#ifndef platform_pci_get_legacy_mem
+# define platform_pci_get_legacy_mem	ia64_pci_get_legacy_mem
+#endif
+#ifndef platform_pci_legacy_read
+# define platform_pci_legacy_read	ia64_pci_legacy_read
+#endif
+#ifndef platform_pci_legacy_write
+# define platform_pci_legacy_write	ia64_pci_legacy_write
 #endif
 #ifndef platform_inb
 # define platform_inb		__ia64_inb

===== include/asm-ia64/machvec_init.h 1.8 vs edited =====

include/asm-ia64/machvec_init.h +3 -0
@@ -5,6 +5,9 @@
 extern ia64_mv_irq_desc __ia64_irq_desc;
 extern ia64_mv_irq_to_vector __ia64_irq_to_vector;
 extern ia64_mv_local_vector_to_irq __ia64_local_vector_to_irq;
+extern ia64_mv_pci_get_legacy_mem_t ia64_pci_get_legacy_mem;
+extern ia64_mv_pci_legacy_read_t ia64_pci_legacy_read;
+extern ia64_mv_pci_legacy_write_t ia64_pci_legacy_write;
 
 extern ia64_mv_inb_t __ia64_inb;
 extern ia64_mv_inw_t __ia64_inw;

===== include/asm-ia64/machvec_sn2.h 1.16 vs edited =====

include/asm-ia64/machvec_sn2.h +6 -0
@@ -43,6 +43,9 @@
 extern ia64_mv_irq_desc sn_irq_desc;
 extern ia64_mv_irq_to_vector sn_irq_to_vector;
 extern ia64_mv_local_vector_to_irq sn_local_vector_to_irq;
+extern ia64_mv_pci_get_legacy_mem_t sn_pci_get_legacy_mem;
+extern ia64_mv_pci_legacy_read_t sn_pci_legacy_read;
+extern ia64_mv_pci_legacy_write_t sn_pci_legacy_write;
 extern ia64_mv_inb_t __sn_inb;
 extern ia64_mv_inw_t __sn_inw;
 extern ia64_mv_inl_t __sn_inl;
@@ -105,6 +108,9 @@
 #define platform_irq_desc		sn_irq_desc
 #define platform_irq_to_vector		sn_irq_to_vector
 #define platform_local_vector_to_irq	sn_local_vector_to_irq
+#define platform_pci_get_legacy_mem	sn_pci_get_legacy_mem
+#define platform_pci_legacy_read	sn_pci_legacy_read
+#define platform_pci_legacy_write	sn_pci_legacy_write
 #define platform_dma_init		machvec_noop
 #define platform_dma_alloc_coherent	sn_dma_alloc_coherent
 #define platform_dma_free_coherent	sn_dma_free_coherent

===== include/asm-ia64/pci.h 1.27 vs edited =====

include/asm-ia64/pci.h +6 -0
@@ -85,6 +85,12 @@
 #define HAVE_PCI_MMAP
 extern int pci_mmap_page_range (struct pci_dev *dev, struct vm_area_struct *vma,
 				enum pci_mmap_state mmap_state, int write_combine);
+#define HAVE_PCI_LEGACY
+extern int pci_mmap_legacy_page_range (struct pci_dev *dev,
+				       struct vm_area_struct *vma);
+#define pci_get_legacy_mem platform_pci_get_legacy_mem
+#define pci_legacy_read platform_pci_legacy_read
+#define pci_legacy_write platform_pci_legacy_write
 
 struct pci_window {
 	struct resource resource;

===== include/asm-ia64/sn/sn_sal.h 1.17 vs edited =====

include/asm-ia64/sn/sn_sal.h +46 -0

52 lines not on this page · raw message

===== include/linux/pci.h 1.142 vs edited =====

include/linux/pci.h +3 -0
@@ -539,6 +539,9 @@
 	u32		saved_config_space[16]; /* config space saved at suspend time */
 	struct bin_attribute *rom_attr; /* attribute descriptor for sysfs ROM entry */
 	int rom_attr_enabled;		/* has display of the rom attribute been enabled? */
+	struct bin_attribute *res_attr[DEVICE_COUNT_RESOURCE]; /* sysfs file for resources */
+	struct bin_attribute *legacy_io; /* sysfs files for legacy stuff */
+	struct bin_attribute *legacy_mem;
 #ifdef CONFIG_PCI_NAMES
 #define PCI_NAME_SIZE	96
 #define PCI_NAME_HALF	__stringify(43)	/* less than half to handle slop */

===== include/linux/sysfs.h 1.38 vs edited =====

include/linux/sysfs.h +5 -0
@@ -47,11 +47,16 @@
 
 #define attr_name(_attr) (_attr).attr.name
 
+struct vm_area_struct; /* circular dependencies? */
+
 struct bin_attribute {
 	struct attribute	attr;
 	size_t			size;
+	void			*private;
 	ssize_t (*read)(struct kobject *, char *, loff_t, size_t);
 	ssize_t (*write)(struct kobject *, char *, loff_t, size_t);
+	int (*mmap)(struct kobject *, struct bin_attribute *attr,
+		    struct vm_area_struct *vma);
 };
 
 struct sysfs_ops {
#1 lore raw
Quoted 7 lines · Jesse Barnes

Ok, how does this one look? It needs some obvious work, but is the approach of adding functionality to sysfs instead of /proc the right way to go? What I've done:

o add mmap support to bin files in sysfs
o make PCI resources available via mmap
o make legacy I/O and memory space available in sysfs

How about splitting this up into the different patches that each do one thing?

That way finding stuff like the following is easier:

Quoted 6 lines

===== fs/sysfs/bin.c 1.19 vs edited =====

fs/sysfs/bin.c +0 -0
@@ -92,6 +92,15 @@
 	return count;
 }

fs/sysfs/bin.c line 103

Quoted 10 lines
fs/sysfs/bin.c +9 -0
 
+static int mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct dentry *dentry = file->f_dentry;
+	struct bin_attribute *attr = to_bin_attr(dentry);
+	struct kobject *kobj = to_kobj(dentry->d_parent);
+
+	return attr->mmap(kobj, attr, vma);
+}
+

What happens if mmap is not set? oops...

also CC: to the sysfs and pci maintainer will jog his memory that he should be looking at these types of patches :)

thanks,

greg k-h

#2 lore raw
Quoted 11 lines · Greg KH
Quoted 7 lines · Jesse Barnes

Ok, how does this one look? It needs some obvious work, but is the approach of adding functionality to sysfs instead of /proc the right way to go? What I've done:

o add mmap support to bin files in sysfs
o make PCI resources available via mmap
o make legacy I/O and memory space available in sysfs

How about splitting this up into the different patches that each do one thing?

Sure, I forgot the diffstat too (doh!).

Quoted 11 lines

fs/sysfs/bin.c line 103

Quoted 9 lines
fs/sysfs/bin.c +9 -0
+static int mmap(struct file *file, struct vm_area_struct *vma)
+{
+ struct dentry *dentry = file->f_dentry;
+ struct bin_attribute *attr = to_bin_attr(dentry);
+ struct kobject *kobj = to_kobj(dentry->d_parent);
+
+ return attr->mmap(kobj, attr, vma);
+}
+

What happens if mmap is not set? oops...

Yeah, I mentioned that in "things to do" at the bottom, but I'm really looking for an "ack, this is a sane way to go" before I sink much more time into it.

Thanks,
Jesse

#3 lore raw
Quoted 3 lines

Yeah, I mentioned that in "things to do" at the bottom, but I'm really looking for an "ack, this is a sane way to go" before I sink much more time into it.

Overall, I like it, though I yet have to scrub the details. As far as the legacy space is concerned, I'd be as "generic" as possible and thus provide the legacy-files on a per-bus basis. That is each pci bus could expose those even if it's not a host bridge. It's an arch matter to actually implement them, and by default, I suppose archs would just treat all busses of a domain the same, but it leaves us with the necessary flexibility for setups with bridges that can remap the legacy space or that kind of thing.

Great work !

Ben.

#4 lore raw
Quoted 3 lines · Benjamin Herrenschmidt

treat all busses of a domain the same, but it leaves us with the necessary flexibility for setups with bridges that can remap the legacy space or that kind of thing.

Do any such bridges exist? I've never heard of them. io space accesses are implemented by the host bridge, so I think the most generic we need to support is one per host bridge, ie root bus.

Signature · 6 lines
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain
#5 lore raw
Quoted 8 lines · Matthew Wilcox
Quoted 3 lines · Benjamin Herrenschmidt

treat all busses of a domain the same, but it leaves us with the necessary flexibility for setups with bridges that can remap the legacy space or that kind of thing.

Do any such bridges exist? I've never heard of them. io space accesses are implemented by the host bridge, so I think the most generic we need to support is one per host bridge, ie root bus.

I've seen PCI->ISA stuffs that could remap the legacy ISA memory space at some random place, though don't ask me the details, I forgot.

Also, per-bus avoids the work of having to walk the tree up, etc... and doesn't make thing more complicated for the sysfs code so ...

Ben

#6 lore raw
In reply to Jesse Barnes
Quoted 4 lines · Jesse Barnes
Quoted 1 line

What happens if mmap is not set? oops...

Yeah, I mentioned that in "things to do" at the bottom, but I'm really looking for an "ack, this is a sane way to go" before I sink much more time into it.

I think this is a sane way to go.

How about sending me a patch just to add the mmap support to binary sysfs files now? I'll be glad to add that to my trees.

Then you can work on the pci stuff over time.

Sound good?

thanks,

greg k-h

#7 lore raw
Quoted 7 lines · Greg KH
Quoted 5 lines · Jesse Barnes
Quoted 1 line

What happens if mmap is not set? oops...

Yeah, I mentioned that in "things to do" at the bottom, but I'm really looking for an "ack, this is a sane way to go" before I sink much more time into it.

Quoted 8 lines

I think this is a sane way to go.

How about sending me a patch just to add the mmap support to binary sysfs files now? I'll be glad to add that to my trees.

Then you can work on the pci stuff over time.

Sound good?

Sure, coming right up.

Thanks,
Jesse

#8 lore raw
In reply to Greg KH
Quoted 3 lines

fs/sysfs/bin.c line 96

Quoted 3 lines
fs/sysfs/bin.c +2 -0
+static int mmap(struct file *file, struct vm_area_struct *vma)
+{

Also, just a style comment: I dislike when functions have such a "generic" name like mmap. I'd _much_ prefer something like pcisysfs_mmap or something like that. The simple name makes it confusing in System.map/kallsyms and thus when debugging.

#9 lore raw
Quoted 7 lines · Benjamin Herrenschmidt
Quoted 2 lines

fs/sysfs/bin.c line 96

Quoted 2 lines
fs/sysfs/bin.c +2 -0
+static int mmap(struct file *file, struct vm_area_struct *vma)
+{

Also, just a style comment: I dislike when functions have such a "generic" name like mmap. I'd _much_ prefer something like pcisysfs_mmap or something like that. The simple name makes it confusing in System.map/kallsyms and thus when debugging.

True, but I was just following the convention in bin.c. Maybe that could be cleaned up all at once with a separate patch? There's some whitespace I'd like to cleanup in pci-sysfs.c as well, so I could do both at once.

Jesse