[RFC 0/4] firewire: interface to remote memory (mem1394)

[RFC 0/4] firewire: interface to remote memory (mem1394)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]

The following series of four patches adds the possiblity for creating
device nodes for each node attached to the firewire bus and accessing
them (currently read-only) which yields access to the node's RAM, if
supported. This is useful for peeking into machines during debugging
(obviously only if you can attach a firewire cable). This is already
supported via the raw1394 interface, but surfacing this like /dev/mem is
advantageous because then user-space tools can work without
modification.

mem1394 itself is currently a bit limited and lacking on the
error-checking, it will be improved but I wanted to get some comments on
the current patches too. Write support will also be added.

The node interface and dynamic character device allocation are useful on
their own if another layer is ever introduced -- which has been under
discussion once a while to make something similar to raw1394 that gives
access only to certain nodes to separate users.

If you want to test this add a udev rule like the following:
  KERNEL=="fwmem-[0-9]*", NAME="fwmem-%s{device/guid}"
and then use
  dd if=/dev/fwmem-0x<GUID> bs=1K count=1024 of=/tmp/first-megabyte
to, for example, read the targets first megabyte of memory.

The patches (will be posted as follow-ups):
1/4: node interface
2/4: dynamic cdev allocation below firewire major
3/4: unconditionally export hpsb_send_packet_and_wait
4/4: add mem1394

Comments would be appreciated.

Thanks,
Johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[RFC 1/4] firewire: node interface

[Johannes Berg]

This patch adds just two small helper functions to allow other code to
register a struct class_interface to interface node entries.

diff --git a/drivers/ieee1394/nodemgr.c b/drivers/ieee1394/nodemgr.c
index 082c7fd..06f4544 100644
--- a/drivers/ieee1394/nodemgr.c
+++ b/drivers/ieee1394/nodemgr.c
@@ -1796,3 +1796,11 @@ void cleanup_ieee1394_nodemgr(void)
 	class_unregister(&nodemgr_ud_class);
 	class_unregister(&nodemgr_ne_class);
 }
+
+int hpsb_register_node_interface(struct class_interface *intf)
+{
+	intf->class = &nodemgr_ne_class;
+
+	return class_interface_register(intf);
+}
+EXPORT_SYMBOL_GPL(hpsb_register_node_interface);
diff --git a/drivers/ieee1394/nodemgr.h b/drivers/ieee1394/nodemgr.h
index 0b26616..d779f81 100644
--- a/drivers/ieee1394/nodemgr.h
+++ b/drivers/ieee1394/nodemgr.h
@@ -170,6 +170,14 @@ int hpsb_node_write(struct node_entry *n
 int hpsb_node_lock(struct node_entry *ne, u64 addr,
 		   int extcode, quadlet_t *data, quadlet_t arg);
 
+/*
+ * things like mem1394 are interfaces to nodes, thus
+ * allow them to register and unregister one.
+ */
+int hpsb_register_node_interface(struct class_interface *intf);
+static inline void hpsb_unregister_node_interface(struct class_interface *intf) {
+	class_interface_unregister(intf);
+}
 
 /* Iterate the hosts, calling a given function with supplied data for each
  * host. */

[RFC 2/4] firewire: dynamic cdev allocation below firewire major

[Johannes Berg]

This  patch implements dynamic minor number allocation below the 171
major allocated for ieee1394. Since on today's systems one doesn't need
to have fixed device numbers any more we could just use any, but it's
probably still useful to use the ieee1394 major number for any firewire
related devices (like mem1394).

diff --git a/drivers/ieee1394/ieee1394_core.c b/drivers/ieee1394/ieee1394_core.c
index 25ef5a8..17afc3b 100644
--- a/drivers/ieee1394/ieee1394_core.c
+++ b/drivers/ieee1394/ieee1394_core.c
@@ -29,10 +29,12 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/bitmap.h>
 #include <linux/bitops.h>
 #include <linux/kdev_t.h>
 #include <linux/skbuff.h>
 #include <linux/suspend.h>
+#include <linux/cdev.h>
 
 #include <asm/byteorder.h>
 #include <asm/semaphore.h>
@@ -1053,9 +1055,14 @@ static int hpsbpkt_thread(void *__hi)
 	complete_and_exit(&khpsbpkt_complete, 0);
 }
 
+/* used further below, but needs to be here for initialisation */
+static spinlock_t used_minors_lock;
+
 static int __init ieee1394_init(void)
 {
 	int i, ret;
+	
+	spin_lock_init(&used_minors_lock);
 
 	skb_queue_head_init(&hpsbpkt_queue);
 
@@ -1197,6 +1204,47 @@ static void __exit ieee1394_cleanup(void
 module_init(ieee1394_init);
 module_exit(ieee1394_cleanup);
 
+/* dynamic minor allocation functions */
+static DECLARE_BITMAP(used_minors, IEEE1394_MINOR_DYNAMIC_COUNT);
+
+int hpsb_cdev_add(struct cdev *chardev)
+{
+	int minor, ret;
+
+	spin_lock(&used_minors_lock);
+	minor = find_first_zero_bit(used_minors, IEEE1394_MINOR_DYNAMIC_COUNT);
+	if (minor >= IEEE1394_MINOR_DYNAMIC_COUNT) {
+		spin_unlock(&used_minors_lock);
+		return -ENODEV;
+	}
+	set_bit(minor, used_minors);
+	spin_unlock(&used_minors_lock);
+
+	minor += IEEE1394_MINOR_DYNAMIC_FIRST;
+	ret = cdev_add(chardev, MKDEV(IEEE1394_MAJOR, minor), 1);
+	if (unlikely(ret)) {
+		spin_lock(&used_minors_lock);
+		clear_bit(minor-IEEE1394_MINOR_DYNAMIC_FIRST, used_minors);
+		spin_unlock(&used_minors_lock);
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(hpsb_cdev_add);
+
+void hpsb_cdev_del(struct cdev *chardev)
+{
+	dev_t dev;
+
+	BUG_ON(MAJOR(chardev->dev) != IEEE1394_MAJOR);
+	dev = chardev->dev;
+	cdev_del(chardev);
+
+	spin_lock(&used_minors_lock);
+	clear_bit(MINOR(dev) - IEEE1394_MINOR_DYNAMIC_FIRST, used_minors);
+	spin_unlock(&used_minors_lock);
+}
+EXPORT_SYMBOL_GPL(hpsb_cdev_del);
+
 /* Exported symbols */
 
 /** hosts.c **/
diff --git a/drivers/ieee1394/ieee1394_core.h b/drivers/ieee1394/ieee1394_core.h
index b354660..d248ed7 100644
--- a/drivers/ieee1394/ieee1394_core.h
+++ b/drivers/ieee1394/ieee1394_core.h
@@ -186,19 +186,38 @@ void hpsb_packet_received(struct hpsb_ho
  * 171:0-255, the various drivers must then cdev_add() their cdev
  * objects to handle their respective sub-regions.
  *
+ * Alternatively, drivers may use a dynamic minor number character
+ * device by using the functions hpsb_cdev_add and hpsb_cdev_del.
+ * hpsb_cdev_add requires an initialised struct cdev and will add
+ * it with cdev_add() automatically, reserving a new minor number
+ * for the new device (unless cdev_add() fails). It returns the
+ * status of cdev_add(), or -ENODEV if no minor could be allocated.
+ * 
+ * Currently 64 minor numbers are reserved for that, if necessary
+ * this number can be increased by simply adjusting the constant
+ * IEEE1394_MINOR_DYNAMIC_FIRST.
+ *
  * Minor device number block allocations:
  *
  * Block 0  (  0- 15)  raw1394
  * Block 1  ( 16- 31)  video1394
  * Block 2  ( 32- 47)  dv1394
  *
- * Blocks 3-14 free for future allocation
+ * Blocks 3-10 free for future allocation
  *
+ * Block 11 (176-191)  dynamic allocation region
+ * Block 12 (192-207)  dynamic allocation region
+ * Block 13 (208-223)  dynamic allocation region
+ * Block 14 (224-239)  dynamic allocation region
  * Block 15 (240-255)  reserved for drivers under development, etc.
  */
 
 #define IEEE1394_MAJOR			 171
 
+#define IEEE1394_MINOR_DYNAMIC_FIRST	176
+#define IEEE1394_MINOR_DYNAMIC_LAST	239
+#define IEEE1394_MINOR_DYNAMIC_COUNT	(IEEE1394_MINOR_DYNAMIC_LAST-IEEE1394_MINOR_DYNAMIC_FIRST+1)
+
 #define IEEE1394_MINOR_BLOCK_RAW1394	   0
 #define IEEE1394_MINOR_BLOCK_VIDEO1394	   1
 #define IEEE1394_MINOR_BLOCK_DV1394	   2
@@ -218,6 +237,11 @@ static inline unsigned char ieee1394_fil
 	return file->f_dentry->d_inode->i_cindex;
 }
 
+/* add a dynamic ieee1394 device */
+int hpsb_cdev_add(struct cdev *chardev);
+/* remove a dynamic ieee1394 device */
+void hpsb_cdev_del(struct cdev *chardev);
+
 extern int hpsb_disable_irm;
 
 /* Our sysfs bus entry */

[RFC 3/4] firewire: unconditionally export hpsb_send_packet_and_wait

[Johannes Berg]

mem1394 will need hpsb_send_packet_and_wait so it needs to be exported
unconditionally.

diff --git a/drivers/ieee1394/ieee1394_core.c b/drivers/ieee1394/ieee1394_core.c
index 17afc3b..5800534 100644
--- a/drivers/ieee1394/ieee1394_core.c
+++ b/drivers/ieee1394/ieee1394_core.c
@@ -589,6 +589,7 @@ int hpsb_send_packet_and_wait(struct hps
 
 	return retval;
 }
+EXPORT_SYMBOL(hpsb_send_packet_and_wait);
 
 static void send_packet_nocare(struct hpsb_packet *packet)
 {
@@ -1269,7 +1270,6 @@ EXPORT_SYMBOL(hpsb_packet_received);
 EXPORT_SYMBOL_GPL(hpsb_disable_irm);
 #ifdef CONFIG_IEEE1394_EXPORT_FULL_API
 EXPORT_SYMBOL(hpsb_send_phy_config);
-EXPORT_SYMBOL(hpsb_send_packet_and_wait);
 #endif
 
 /** ieee1394_transactions.c **/

[RFC 4/4] firewire: add mem1394

[Johannes Berg]

While the previous patches were purely infrastructure, this patch
actually adds the code using it: mem1394.

There are some open questions on a few things, maybe someone can help
out there.

diff --git a/drivers/ieee1394/Kconfig b/drivers/ieee1394/Kconfig
index 39142e2..cd7b28c 100644
--- a/drivers/ieee1394/Kconfig
+++ b/drivers/ieee1394/Kconfig
@@ -169,4 +169,21 @@ config IEEE1394_RAWIO
 	  To compile this driver as a module, say M here: the
 	  module will be called raw1394.
 
+config IEEE1394_MEMDEV
+	tristate "IEEE1394 memory device support"
+	depends on IEEE1394 && EXPERIMENTAL
+	help
+	  Say Y here if you want support for the ieee1394 memory device.
+	  This is useful for debugging systems attached via firewire
+	  since it usually allows you to read from and write to their memory,
+	  depending on the controller and machine setup.
+	  
+	  It differs from raw access (which allows the same usage) in that
+	  it provides devices nodes (usually called /dev/fwmem-<guid>) that can
+	  be read and written with any tool, as opposed to specialised tools
+	  required for raw1394.
+	  
+	  To compile this driver as a module, say M here: the
+	  module will be called mem1394.
+
 endmenu
diff --git a/drivers/ieee1394/Makefile b/drivers/ieee1394/Makefile
index 180bf82..7da4e21 100644
--- a/drivers/ieee1394/Makefile
+++ b/drivers/ieee1394/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_IEEE1394_RAWIO) += raw1394.
 obj-$(CONFIG_IEEE1394_SBP2) += sbp2.o
 obj-$(CONFIG_IEEE1394_DV1394) += dv1394.o
 obj-$(CONFIG_IEEE1394_ETH1394) += eth1394.o
+obj-$(CONFIG_IEEE1394_MEMDEV) += mem1394.o
 
 quiet_cmd_oui2c = OUI2C   $@
       cmd_oui2c = $(CONFIG_SHELL) $(src)/oui2c.sh < $< > $@
diff --git a/drivers/ieee1394/mem1394.c b/drivers/ieee1394/mem1394.c
new file mode 100644
index 0000000..e9d44a2
--- /dev/null
+++ b/drivers/ieee1394/mem1394.c
@@ -0,0 +1,277 @@
+/*
+ * IEEE 1394 for Linux
+ *
+ * Copyright (C) 2006 Johannes Berg <johannes@sipsolutions.net>
+ *
+ * This code is licensed under the GPL v2. See the file COPYING in the root
+ * directory of the kernel sources for details.
+ *
+ * This module provides a character device for each node attached
+ * to the bus and allows direct memory access on them.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/mm.h>
+#include <linux/cdev.h>
+
+#include "ieee1394.h"
+#include "ieee1394_core.h"
+#include "ieee1394_transactions.h"
+#include "mem1394.h"
+#include "nodemgr.h"
+#include "highlevel.h"
+
+static int mem1394_mmap(struct file * file, struct vm_area_struct * vma)
+{
+	return -ENOSYS;
+}
+
+static int mem1394_open(struct inode *inode, struct file *file)
+{
+	struct mem1394_file_info *fi;
+
+	fi = kzalloc(sizeof(*fi), GFP_KERNEL);
+	if (!fi)
+		return -ENOMEM;
+	file->private_data = fi;
+	fi->memdev = container_of(inode->i_cdev, struct mem1394_dev, cdev);
+	return 0;
+}
+
+/* This function essentially clones hpsb_read. Might be better
+ * to create a new hpsb_read_user function instead... */
+static int mem1394_read(struct file *file, char __user * buffer,
+			size_t count, loff_t *offset)
+{
+	struct mem1394_file_info *fi = (struct mem1394_file_info*)file->private_data;
+	/* lower levels only support count as a multiple of 4 */
+	size_t submitcount = (count + (4-1)) & ~(4-1);
+	int retval = 0;
+	struct hpsb_packet *packet;
+
+	/* this is a bit icky. I think I'll want to create a
+	 * "struct hpsb_node_class_interface" that you register
+	 * with nodemgr.c instead of registering the "struct class_interface"
+	 * directly. It would wrap around the "struct class_interface"
+	 * and handle things like this.
+	 *
+	 * This means it would call the node_class_interface's
+	 *  - "add" method whenever the device is fully there, and an
+	 *  - "update" method when it survived a bus reset, and the
+	 *  - "remove" method when it went away, also taking care of
+	 * debouncing, which the mem1394 interface currently doesn't handle.
+	 *
+	 * But I need advice on this. It'll probably works this way
+	 * but most likely not once this interface stuff gets more
+	 * use; I can imagine using it for scanners instead of raw1394
+	 * so that the kernel can validate that a user can only
+	 * access a certain scanner and not all 1394 devices on the bus.
+	 * In other words some 'raw1394intf' instead of 'raw1394' which
+	 * creates one character device per ieee1394 node for finer
+	 * grained access control.
+	 * That would definitely want to have debouncing etc.
+	 *
+	 * However, I don't fully understand the states node_entries go
+	 * through yet, so I'm not sure this should even be here!
+	 * Maybe it should be in open? But then the device could go
+	 * into limbo when it is already opened...
+	 *
+	 * Similarly, what happens if a node is suspended?
+	 */
+	if (fi->memdev->ne->in_limbo)
+		return -ENODEV;
+
+	if (count == 0)
+		return 0;
+
+	/* I wonder if it is possible to do DMA directly to the userspace buffer... */
+	packet = hpsb_make_readpacket(fi->memdev->ne->host, fi->memdev->ne->nodeid, *offset, submitcount);
+	if (!packet)
+		return -ENOENT;
+	
+	packet->generation = fi->memdev->ne->generation;
+	retval = hpsb_send_packet_and_wait(packet);
+	if (retval < 0)
+		goto out_free;
+
+	retval = hpsb_packet_success(packet);
+	
+	if (retval == 0) {
+		if (submitcount == 4) {
+			if (copy_to_user(buffer, &packet->header[3], count))
+				retval = -EFAULT;
+		} else {
+			if (copy_to_user(buffer, packet->data, count))
+				retval = -EFAULT;
+		}
+	}
+
+	if (retval == 0) {
+		retval = count;
+		*offset += count;
+	}
+
+ out_free:
+	hpsb_free_tlabel(packet);
+	hpsb_free_packet(packet);
+
+	return retval;
+}
+
+static int mem1394_release(struct inode *inode, struct file *file)
+{
+	struct mem1394_file_info *fi = (struct mem1394_file_info*)file->private_data;
+	
+	kfree(fi);
+
+	return 0;
+}
+
+static struct class *mem1394_sysfs_class;
+
+static struct file_operations mem1394_fops = {
+	.owner = THIS_MODULE,
+	.mmap = mem1394_mmap,
+	.open = mem1394_open,
+	.read = mem1394_read,
+	.release = mem1394_release,
+};
+
+static atomic_t mem1394_dev_ctr;
+
+static struct mem1394_dev * alloc_mem1394_dev(struct device *dev)
+{
+	struct mem1394_dev *result;
+	struct node_entry *ne = container_of(dev, struct node_entry, device);
+	int ret;
+	struct class_device * mem1394_class_member;
+
+	result = kzalloc(sizeof(*result), GFP_KERNEL);
+	if (!result)
+		return NULL;
+
+	cdev_init(&result->cdev, &mem1394_fops);
+	result->cdev.owner = THIS_MODULE;
+	result->ne = ne;
+
+	ret = hpsb_cdev_add(&result->cdev);
+	if (ret) {
+		printk(KERN_ERR "mem1394: failed to register character device!\n");
+		goto out_free;
+	}
+
+	atomic_inc(&mem1394_dev_ctr);
+	mem1394_class_member = class_device_create(mem1394_sysfs_class, NULL, result->cdev.dev,
+						dev, "fwmem-%d", atomic_read(&mem1394_dev_ctr));
+	if (IS_ERR(mem1394_class_member)) {
+		printk(KERN_WARNING "mem1394: class_device_create failed\n");
+	} else {
+		class_set_devdata(mem1394_class_member, result);
+	}
+	dev->driver_data = result;
+
+	return result;
+ out_free:
+	kfree(result);
+	return NULL;
+}
+
+static DEFINE_SPINLOCK(dev_list_lock);
+static LIST_HEAD(dev_list);
+
+static int mem1394_add(struct class_device *cl_dev, struct class_interface *cl_intf)
+{
+	struct mem1394_dev *memdev;
+
+	if (!cl_dev) {
+		printk("cl_dev not assigned\n");
+		return -ENOENT;
+	}
+	if (!cl_dev->dev) {
+		printk("cl_dev->dev not assigned\n");
+		return -ENONET;
+	}
+
+	memdev = alloc_mem1394_dev(cl_dev->dev);
+	if (!memdev)
+		return -ENOMEM;
+
+	spin_lock(&dev_list_lock);
+	list_add_tail(&memdev->list, &dev_list);
+	spin_unlock(&dev_list_lock);
+
+	/* need we do anything else? */
+	return 0;
+}
+
+static void mem1394_del(struct mem1394_dev *memdev)
+{
+	class_device_destroy(mem1394_sysfs_class, memdev->cdev.dev);
+
+	/* kill off everything that might be in progress */
+	/* TODO */
+
+	/* remove character device */
+	hpsb_cdev_del(&memdev->cdev);
+	kfree(memdev);
+}
+
+static void mem1394_remove(struct class_device *cl_dev, struct class_interface *cl_intf)
+{
+	struct mem1394_dev *memdev, *tmp, *found = NULL;
+	
+	/* find our memdev corresponding to the class device */
+	spin_lock(&dev_list_lock);
+	list_for_each_entry_safe(memdev, tmp, &dev_list, list) {
+		if (cl_dev->dev == memdev->dev) {
+			list_del(&memdev->list);
+			found = memdev;
+			break;
+		}
+	}
+	spin_unlock(&dev_list_lock);
+	if (!found)
+		return;
+
+	mem1394_del(found);
+}
+
+static struct class_interface mem1394_interface = {
+	.add		= mem1394_add,
+	.remove		= mem1394_remove,
+};
+                
+static int __init init_mem1394(void)
+{
+	int ret;
+	
+	spin_lock_init(&dev_list_lock);
+	
+	mem1394_sysfs_class = class_create(THIS_MODULE, "mem1394");
+	if (IS_ERR(mem1394_sysfs_class)) {
+		return PTR_ERR(mem1394_sysfs_class);
+	}
+
+	ret = hpsb_register_node_interface(&mem1394_interface);
+	return ret;
+}
+
+static void __exit cleanup_mem1394(void)
+{
+	struct mem1394_dev *memdev, *tmp;
+
+	hpsb_unregister_node_interface(&mem1394_interface);
+	list_for_each_entry_safe(memdev, tmp, &dev_list, list) {
+		list_del(&memdev->list);
+		mem1394_del(memdev);
+	}
+	class_destroy(mem1394_sysfs_class);
+}
+
+module_init(init_mem1394);
+module_exit(cleanup_mem1394);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Johannes Berg <johannes@sipsolutions.net>");
diff --git a/drivers/ieee1394/mem1394.h b/drivers/ieee1394/mem1394.h
new file mode 100644
index 0000000..e7cc8ab
--- /dev/null
+++ b/drivers/ieee1394/mem1394.h
@@ -0,0 +1,20 @@
+#ifndef IEEE1394_MEM1394_H
+#define IEEE1394_MEM1394_H
+
+#include <asm/types.h>
+#include <linux/cdev.h>
+#include <linux/list.h>
+#include "nodemgr.h"
+
+struct mem1394_dev {
+	struct device *dev;
+	struct node_entry *ne;
+	struct cdev cdev;
+	struct list_head list;
+};
+
+struct mem1394_file_info {
+	struct mem1394_dev *memdev;
+};
+
+#endif /* IEEE1394_MEM1394_H */

Re: [RFC 4/4] firewire: add mem1394

[Andy Wingo]

Hi Johannes,

On Thu, 2006-02-02 at 23:43 +0100, Johannes Berg wrote:
> +	spin_lock(&dev_list_lock);

Stupid question: are you sure that something coming from an interrupt
handler won't try to grab this lock? For example from a cable unplug?

Regards,
-- 
Andy Wingo
http://wingolog.org/

Re: [RFC 4/4] firewire: add mem1394

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

On Fri, 2006-02-03 at 12:35 +0100, Andy Wingo wrote:

> On Thu, 2006-02-02 at 23:43 +0100, Johannes Berg wrote:
> > +	spin_lock(&dev_list_lock);
> 
> Stupid question: are you sure that something coming from an interrupt
> handler won't try to grab this lock? For example from a cable unplug?

Yes, I'm pretty sure (but I hope some of the firewire experts will chime
in) -- but if you unplug or anything the node only goes into 'limbo' and
afaict if it is ever cleaned up then that comes from a thread context.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFC 4/4] firewire: add mem1394

[Andrew Morton]

Johannes Berg <johannes@sipsolutions.net> wrote:
>
> +config IEEE1394_MEMDEV
> +	tristate "IEEE1394 memory device support"
> +	depends on IEEE1394 && EXPERIMENTAL
> +	help
> +	  Say Y here if you want support for the ieee1394 memory device.
> +	  This is useful for debugging systems attached via firewire
> +	  since it usually allows you to read from and write to their memory,
> +	  depending on the controller and machine setup.

1394 is evil.  Does this mean that if a machine is completely
dead-and-crashed, we can still suck all its memory out over 1394 with no
cooperation from the dead machine's kernel?  If not, what limitations are
there?



Triviata:

> +static int mem1394_read(struct file *file, char __user * buffer,
> +			size_t count, loff_t *offset)
> +{
> +	struct mem1394_file_info *fi = (struct mem1394_file_info*)file->private_data;

Unneeded cast.

> +	packet = hpsb_make_readpacket(fi->memdev->ne->host, fi->memdev->ne->nodeid, *offset, submitcount);

xterm too big!

> +static int mem1394_release(struct inode *inode, struct file *file)
> +{
> +	struct mem1394_file_info *fi = (struct mem1394_file_info*)file->private_data;

Unneeded cast.

> +	

Adds trailing whitespace ;)

> +static struct mem1394_dev * alloc_mem1394_dev(struct device *dev)
> +{
> +	struct mem1394_dev *result;
> +	struct node_entry *ne = container_of(dev, struct node_entry, device);
> +	int ret;
> +	struct class_device * mem1394_class_member;

Inconsistent space-after-asterisk policy (no-space is preferred).

> +	mem1394_class_member = class_device_create(mem1394_sysfs_class, NULL, result->cdev.dev,
> +						dev, "fwmem-%d", atomic_read(&mem1394_dev_ctr));

My eyes!

> +	if (IS_ERR(mem1394_class_member)) {
> +		printk(KERN_WARNING "mem1394: class_device_create failed\n");
> +	} else {
> +		class_set_devdata(mem1394_class_member, result);
> +	}

Unneeded braces.

> +	if (IS_ERR(mem1394_sysfs_class)) {
> +		return PTR_ERR(mem1394_sysfs_class);
> +	}

Ditto.

Re: [RFC 4/4] firewire: add mem1394

[Kyle Moffett]

On Feb 05, 2006, at 03:43, Andrew Morton wrote:
> Johannes Berg <johannes@sipsolutions.net> wrote:
>>
>> +config IEEE1394_MEMDEV
>> +	tristate "IEEE1394 memory device support"
>> +	depends on IEEE1394 && EXPERIMENTAL
>> +	help
>> +	  Say Y here if you want support for the ieee1394 memory device.
>> +	  This is useful for debugging systems attached via firewire
>> +	  since it usually allows you to read from and write to their  
>> memory,
>> +	  depending on the controller and machine setup.
>
> 1394 is evil.  Does this mean that if a machine is completely dead- 
> and-crashed, we can still suck all its memory out over 1394 with no  
> cooperation from the dead machine's kernel?  If not, what  
> limitations are there?

I think you snipped too much of the description.  This was after the  
part you quoted:

> It differs from raw access (which allows the same usage) in that it  
> provides devices nodes (usually called /dev/fwmem-<guid>) that can  
> be read and written with any tool, as opposed to specialised tools  
> required for raw1394.

This seems to indicate that this is a _client_ for a IEEE1394 memory  
device, as opposed to a server.  Perhaps the description should be  
clarified, but I don't see any security issues (the kernel does not  
expose its own memory, it accesses the memory that another device is  
exposing).

Cheers,
Kyle Moffett

--
There is no way to make Linux robust with unreliable memory  
subsystems, sorry.  It would be like trying to make a human more  
robust with an unreliable O2 supply. Memory just has to work.
   -- Andi Kleen

Re: [RFC 4/4] firewire: add mem1394

[Stefan Richter]

Johannes Berg wrote:
> On Fri, 2006-02-03 at 12:35 +0100, Andy Wingo wrote:
>>On Thu, 2006-02-02 at 23:43 +0100, Johannes Berg wrote:
>>
>>>+	spin_lock(&dev_list_lock);
>>
>>Stupid question: are you sure that something coming from an interrupt
>>handler won't try to grab this lock? For example from a cable unplug?
> 
> Yes, I'm pretty sure (but I hope some of the firewire experts will chime
> in) -- but if you unplug or anything the node only goes into 'limbo' and
> afaict if it is ever cleaned up then that comes from a thread context.

The lock will only be taken in non-atomic context. In particular, if a 
mem1394 device is to be removed after cable unplug, the code will be run 
by knodemgrd.

What is more recommendable for mutual exclusion in non-atomic context 
(here also with very low probality of lock contention, given the current 
implementation of ieee1394) --- a mutex or a spinlock?
-- 
Stefan Richter
-=====-=-==- --=- --=-=
http://arcgraph.de/sr/

Re: [RFC 2/4] firewire: dynamic cdev allocation below firewire major

[Stefan Richter]

Johannes Berg wrote:
> This  patch implements dynamic minor number allocation below the 171
> major allocated for ieee1394. Since on today's systems one doesn't need
> to have fixed device numbers any more we could just use any, but it's
> probably still useful to use the ieee1394 major number for any firewire
> related devices (like mem1394).
[...]
> --- a/drivers/ieee1394/ieee1394_core.h
> +++ b/drivers/ieee1394/ieee1394_core.h
> @@ -186,19 +186,38 @@ void hpsb_packet_received(struct hpsb_ho
>   * 171:0-255, the various drivers must then cdev_add() their cdev
>   * objects to handle their respective sub-regions.
>   *
> + * Alternatively, drivers may use a dynamic minor number character
> + * device by using the functions hpsb_cdev_add and hpsb_cdev_del.
> + * hpsb_cdev_add requires an initialised struct cdev and will add
> + * it with cdev_add() automatically, reserving a new minor number
> + * for the new device (unless cdev_add() fails). It returns the
> + * status of cdev_add(), or -ENODEV if no minor could be allocated.

This comment should be moved to the implementations of respective 
functions and be formatted as described by 
Documentation/kernel-doc-nano-HOWTO.txt. (We should eventually check all 
exported ieee1394 symbols if they are documented that way.)

> + * Currently 64 minor numbers are reserved for that, if necessary
> + * this number can be increased by simply adjusting the constant
> + * IEEE1394_MINOR_DYNAMIC_FIRST.
> + *
>   * Minor device number block allocations:
>   *
>   * Block 0  (  0- 15)  raw1394
>   * Block 1  ( 16- 31)  video1394
>   * Block 2  ( 32- 47)  dv1394
>   *
> - * Blocks 3-14 free for future allocation
> + * Blocks 3-10 free for future allocation
>   *
> + * Block 11 (176-191)  dynamic allocation region
> + * Block 12 (192-207)  dynamic allocation region
> + * Block 13 (208-223)  dynamic allocation region
> + * Block 14 (224-239)  dynamic allocation region
>   * Block 15 (240-255)  reserved for drivers under development, etc.
>   */
>  
>  #define IEEE1394_MAJOR			 171
>  
> +#define IEEE1394_MINOR_DYNAMIC_FIRST	176
> +#define IEEE1394_MINOR_DYNAMIC_LAST	239
> +#define IEEE1394_MINOR_DYNAMIC_COUNT	(IEEE1394_MINOR_DYNAMIC_LAST-IEEE1394_MINOR_DYNAMIC_FIRST+1)
> +
>  #define IEEE1394_MINOR_BLOCK_RAW1394	   0
>  #define IEEE1394_MINOR_BLOCK_VIDEO1394	   1
>  #define IEEE1394_MINOR_BLOCK_DV1394	   2
> @@ -218,6 +237,11 @@ static inline unsigned char ieee1394_fil
>  	return file->f_dentry->d_inode->i_cindex;
>  }
>  
> +/* add a dynamic ieee1394 device */
> +int hpsb_cdev_add(struct cdev *chardev);
> +/* remove a dynamic ieee1394 device */
> +void hpsb_cdev_del(struct cdev *chardev);
[...]

Again, better no comment here than these two which are not very 
enlightening.
-- 
Stefan Richter
-=====-=-==- --=- --=-=
http://arcgraph.de/sr/

Re: [RFC 3/4] firewire: unconditionally export hpsb_send_packet_and_wait

[Stefan Richter]

Johannes Berg wrote:
> mem1394 will need hpsb_send_packet_and_wait so it needs to be exported
> unconditionally.
> 
> diff --git a/drivers/ieee1394/ieee1394_core.c b/drivers/ieee1394/ieee1394_core.c
> index 17afc3b..5800534 100644
> --- a/drivers/ieee1394/ieee1394_core.c
> +++ b/drivers/ieee1394/ieee1394_core.c
> @@ -589,6 +589,7 @@ int hpsb_send_packet_and_wait(struct hps
>  
>  	return retval;
>  }
> +EXPORT_SYMBOL(hpsb_send_packet_and_wait);
>  
>  static void send_packet_nocare(struct hpsb_packet *packet)
>  {
> @@ -1269,7 +1270,6 @@ EXPORT_SYMBOL(hpsb_packet_received);
>  EXPORT_SYMBOL_GPL(hpsb_disable_irm);
>  #ifdef CONFIG_IEEE1394_EXPORT_FULL_API
>  EXPORT_SYMBOL(hpsb_send_phy_config);
> -EXPORT_SYMBOL(hpsb_send_packet_and_wait);
>  #endif
>  
>  /** ieee1394_transactions.c **/

Leave the export down at the end of ieee1394_core.c among all other 
exports of ieee1394. Just move the export above the #ifdef.

Same for the two new exports by "[RFC 2/4] firewire: dynamic cdev 
allocation below firewire major": Place them at the end of ieee1394_core.c.
-- 
Stefan Richter
-=====-=-==- --=- --=-=
http://arcgraph.de/sr/

Re: [RFC 4/4] firewire: add mem1394

[Stefan Richter]

Johannes Berg wrote:
> While the previous patches were purely infrastructure, this patch
> actually adds the code using it: mem1394.
> 
> There are some open questions on a few things, maybe someone can help
> out there.
[...]
> +	/* this is a bit icky. I think I'll want to create a
> +	 * "struct hpsb_node_class_interface" that you register
> +	 * with nodemgr.c instead of registering the "struct class_interface"
> +	 * directly. It would wrap around the "struct class_interface"
> +	 * and handle things like this.
> +	 *
> +	 * This means it would call the node_class_interface's
> +	 *  - "add" method whenever the device is fully there, and an
> +	 *  - "update" method when it survived a bus reset, and the
> +	 *  - "remove" method when it went away, also taking care of
> +	 * debouncing, which the mem1394 interface currently doesn't handle.

I currently think so too.

> +	 * But I need advice on this. It'll probably works this way
> +	 * but most likely not once this interface stuff gets more
> +	 * use; I can imagine using it for scanners instead of raw1394
> +	 * so that the kernel can validate that a user can only
> +	 * access a certain scanner and not all 1394 devices on the bus.

Probably not. All devices (except perhaps custom embedded devices) which 
implement one or another high level protocol will always be accessed 
either by a protocol driver in kernelspace (like sbp2, eth1394, 
video1394) on top of a struct unit_directory, or by a driver or library 
in userspace on top of libraw1394/ raw1394. This is because such devices 
and protocols all implement the ISO/IEC 13213 CSR architecture.

> +	 * In other words some 'raw1394intf' instead of 'raw1394' which
> +	 * creates one character device per ieee1394 node for finer
> +	 * grained access control.
> +	 * That would definitely want to have debouncing etc.
> +	 *
> +	 * However, I don't fully understand the states node_entries go
> +	 * through yet, so I'm not sure this should even be here!
> +	 * Maybe it should be in open? But then the device could go
> +	 * into limbo when it is already opened...
> +	 *
> +	 * Similarly, what happens if a node is suspended?
> +	 */
[...]

When a node represented by a node_entry leaves the bus, the node_entry 
is "suspended" and "put into limbo", which is both the same for the 1394 
stack. The node_entry is only "removed" when forced by userspace through 
ieee1394's sysfs interface or when the ieee1394 driver module is 
unloaded. A unit_directory is either "suspended" or "removed", depending 
on what the protocol driver bound to the unit_directory implements. 
This behaviour of ieee1394 is currently not extensively used, but I plan 
to implement capability of sbp2 to survive transient disconnection on 
top of it.

I still haven't tested your driver yet and won't be able to do so during 
the next days...
-- 
Stefan Richter
-=====-=-==- --=- --=-=
http://arcgraph.de/sr/

Re: [RFC 4/4] firewire: add mem1394

[Stefan Richter]

I wrote:
(ohci1394's programming of physical DMA filters)
> The else clause is BTW bogus [...], but this is only 
> important if an IEEE 1394.1 bus bridge was present.

I had a closer look. Physical DMA _is_ reliably disabled for all nodes 
(including nodes on bridged buses) if ohci1394 was loaded with phys_dma=0.
-- 
Stefan Richter
-=====-=-==- --=- --=-=
http://arcgraph.de/sr/

Re: [RFC 4/4] firewire: add mem1394

[Andi Kleen]

Andrew Morton <akpm@osdl.org> writes:

> Johannes Berg <johannes@sipsolutions.net> wrote:
> >
> > +config IEEE1394_MEMDEV
> > +	tristate "IEEE1394 memory device support"
> > +	depends on IEEE1394 && EXPERIMENTAL
> > +	help
> > +	  Say Y here if you want support for the ieee1394 memory device.
> > +	  This is useful for debugging systems attached via firewire
> > +	  since it usually allows you to read from and write to their memory,
> > +	  depending on the controller and machine setup.
> 
> 1394 is evil.  Does this mean that if a machine is completely
> dead-and-crashed, we can still suck all its memory out over 1394 with no
> cooperation from the dead machine's kernel?  If not, what limitations are
> there?

Yes it can. BenH's firescope tool does this already using raw1394
(I have it working now on x86-64 too). I dont quite see the point
of adding another kernel driver for it though. This can be all
done fine in userspace.

-Andi

Re: [RFC 4/4] firewire: add mem1394

[Stefan Richter]

Andi Kleen wrote:
> BenH's firescope tool does this already using raw1394
> (I have it working now on x86-64 too). I dont quite see the point
> of adding another kernel driver for it though. This can be all
> done fine in userspace.

The point is to provide an interface like /dev/mem in order to use a 
wider range of debug/ forensics/ hacker tools than specialized 
libraw1394 clients. Sure enough, the benefit is small, but so is this 
driver's code footprint.
-- 
Stefan Richter
-=====-=-==- --=- --=-=
http://arcgraph.de/sr/

Re: [RFC 4/4] firewire: add mem1394

[Andi Kleen]

On Sunday 05 February 2006 21:50, Stefan Richter wrote:
> Andi Kleen wrote:
> > BenH's firescope tool does this already using raw1394
> > (I have it working now on x86-64 too). I dont quite see the point
> > of adding another kernel driver for it though. This can be all
> > done fine in userspace.
> 
> The point is to provide an interface like /dev/mem in order to use a 
> wider range of debug/ forensics/ hacker tools than specialized 
> libraw1394 clients. 

I don't see the benefit really. It can be as well provided by 
a userspace library 

Many of the debug tools don't even work on /dev/mem, but use
different interfaces (/proc/kcore, gdb remote protocol etc.) 

Also raw1394 could possibly be used to cause interrupts
on the target and also stop the target CPU this way.

-Andi

Re: [RFC 4/4] firewire: add mem1394

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1955 bytes --]

On Sun, 2006-02-05 at 15:19 +0100, Stefan Richter wrote:

> > +	 * But I need advice on this. It'll probably works this way
> > +	 * but most likely not once this interface stuff gets more
> > +	 * use; I can imagine using it for scanners instead of raw1394
> > +	 * so that the kernel can validate that a user can only
> > +	 * access a certain scanner and not all 1394 devices on the bus.
> 
> Probably not. All devices (except perhaps custom embedded devices) which 
> implement one or another high level protocol will always be accessed 
> either by a protocol driver in kernelspace (like sbp2, eth1394, 
> video1394) on top of a struct unit_directory, or by a driver or library 
> in userspace on top of libraw1394/ raw1394. This is because such devices 
> and protocols all implement the ISO/IEC 13213 CSR architecture.

You snipped too much :) At this point I was thinking of the raw1394
replacement that has finer grained access control which we talked about
in other threads too.

> > +	 * In other words some 'raw1394intf' instead of 'raw1394' which
> > +	 * creates one character device per ieee1394 node for finer
> > +	 * grained access control.
> > +	 * That would definitely want to have debouncing etc.


> When a node represented by a node_entry leaves the bus, the node_entry 
> is "suspended" and "put into limbo", which is both the same for the 1394 
> stack. The node_entry is only "removed" when forced by userspace through 
> ieee1394's sysfs interface or when the ieee1394 driver module is 
> unloaded. A unit_directory is either "suspended" or "removed", depending 
> on what the protocol driver bound to the unit_directory implements. 
> This behaviour of ieee1394 is currently not extensively used, but I plan 
> to implement capability of sbp2 to survive transient disconnection on 
> top of it.

Thanks for the explanation. I'll have to test my driver under the light
of this.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFC 3/4] firewire: unconditionally export hpsb_send_packet_and_wait

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Sun, 2006-02-05 at 14:42 +0100, Stefan Richter wrote:

> Leave the export down at the end of ieee1394_core.c among all other 
> exports of ieee1394. Just move the export above the #ifdef.
> 
> Same for the two new exports by "[RFC 2/4] firewire: dynamic cdev 
> allocation below firewire major": Place them at the end of ieee1394_core.c.

Somehow I thought we were supposed to now put the EXPORT_SYMBOL{,_GPL}
right after the declaration. I can post a patch to move all of them
instead just the few if desired, or change this patch.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFC 2/4] firewire: dynamic cdev allocation below firewire major

[Jody McIntyre]

On Thu, Feb 02, 2006 at 11:40:12PM +0100, Johannes Berg wrote:
> This  patch implements dynamic minor number allocation below the 171
> major allocated for ieee1394. Since on today's systems one doesn't need
> to have fixed device numbers any more we could just use any, but it's
> probably still useful to use the ieee1394 major number for any firewire
> related devices (like mem1394).

I don't really like this.  There's no benefit to using the 1394 major
number.  I'd rather see an improved alloc_chrdev_region() that does
something like this but for the whole kernel (currently it "wastes" an
entire major even if you only want 1 minor, and for what you're doing,
grabbing 1 minor at a time makes the most sense.)

I'll get to that someday unless someone beats me to it (tm) because it
will be necessary for the raw1394 security improvements I'm (slowly)
working on.  Unless someone convinces me that dynamic allocation under
171 is really the way to go.

Cheers,
Jody

> 
> diff --git a/drivers/ieee1394/ieee1394_core.c b/drivers/ieee1394/ieee1394_core.c
> index 25ef5a8..17afc3b 100644
> --- a/drivers/ieee1394/ieee1394_core.c
> +++ b/drivers/ieee1394/ieee1394_core.c
> @@ -29,10 +29,12 @@
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/bitmap.h>
>  #include <linux/bitops.h>
>  #include <linux/kdev_t.h>
>  #include <linux/skbuff.h>
>  #include <linux/suspend.h>
> +#include <linux/cdev.h>
>  
>  #include <asm/byteorder.h>
>  #include <asm/semaphore.h>
> @@ -1053,9 +1055,14 @@ static int hpsbpkt_thread(void *__hi)
>  	complete_and_exit(&khpsbpkt_complete, 0);
>  }
>  
> +/* used further below, but needs to be here for initialisation */
> +static spinlock_t used_minors_lock;
> +
>  static int __init ieee1394_init(void)
>  {
>  	int i, ret;
> +	
> +	spin_lock_init(&used_minors_lock);
>  
>  	skb_queue_head_init(&hpsbpkt_queue);
>  
> @@ -1197,6 +1204,47 @@ static void __exit ieee1394_cleanup(void
>  module_init(ieee1394_init);
>  module_exit(ieee1394_cleanup);
>  
> +/* dynamic minor allocation functions */
> +static DECLARE_BITMAP(used_minors, IEEE1394_MINOR_DYNAMIC_COUNT);
> +
> +int hpsb_cdev_add(struct cdev *chardev)
> +{
> +	int minor, ret;
> +
> +	spin_lock(&used_minors_lock);
> +	minor = find_first_zero_bit(used_minors, IEEE1394_MINOR_DYNAMIC_COUNT);
> +	if (minor >= IEEE1394_MINOR_DYNAMIC_COUNT) {
> +		spin_unlock(&used_minors_lock);
> +		return -ENODEV;
> +	}
> +	set_bit(minor, used_minors);
> +	spin_unlock(&used_minors_lock);
> +
> +	minor += IEEE1394_MINOR_DYNAMIC_FIRST;
> +	ret = cdev_add(chardev, MKDEV(IEEE1394_MAJOR, minor), 1);
> +	if (unlikely(ret)) {
> +		spin_lock(&used_minors_lock);
> +		clear_bit(minor-IEEE1394_MINOR_DYNAMIC_FIRST, used_minors);
> +		spin_unlock(&used_minors_lock);
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(hpsb_cdev_add);
> +
> +void hpsb_cdev_del(struct cdev *chardev)
> +{
> +	dev_t dev;
> +
> +	BUG_ON(MAJOR(chardev->dev) != IEEE1394_MAJOR);
> +	dev = chardev->dev;
> +	cdev_del(chardev);
> +
> +	spin_lock(&used_minors_lock);
> +	clear_bit(MINOR(dev) - IEEE1394_MINOR_DYNAMIC_FIRST, used_minors);
> +	spin_unlock(&used_minors_lock);
> +}
> +EXPORT_SYMBOL_GPL(hpsb_cdev_del);
> +
>  /* Exported symbols */
>  
>  /** hosts.c **/
> diff --git a/drivers/ieee1394/ieee1394_core.h b/drivers/ieee1394/ieee1394_core.h
> index b354660..d248ed7 100644
> --- a/drivers/ieee1394/ieee1394_core.h
> +++ b/drivers/ieee1394/ieee1394_core.h
> @@ -186,19 +186,38 @@ void hpsb_packet_received(struct hpsb_ho
>   * 171:0-255, the various drivers must then cdev_add() their cdev
>   * objects to handle their respective sub-regions.
>   *
> + * Alternatively, drivers may use a dynamic minor number character
> + * device by using the functions hpsb_cdev_add and hpsb_cdev_del.
> + * hpsb_cdev_add requires an initialised struct cdev and will add
> + * it with cdev_add() automatically, reserving a new minor number
> + * for the new device (unless cdev_add() fails). It returns the
> + * status of cdev_add(), or -ENODEV if no minor could be allocated.
> + * 
> + * Currently 64 minor numbers are reserved for that, if necessary
> + * this number can be increased by simply adjusting the constant
> + * IEEE1394_MINOR_DYNAMIC_FIRST.
> + *
>   * Minor device number block allocations:
>   *
>   * Block 0  (  0- 15)  raw1394
>   * Block 1  ( 16- 31)  video1394
>   * Block 2  ( 32- 47)  dv1394
>   *
> - * Blocks 3-14 free for future allocation
> + * Blocks 3-10 free for future allocation
>   *
> + * Block 11 (176-191)  dynamic allocation region
> + * Block 12 (192-207)  dynamic allocation region
> + * Block 13 (208-223)  dynamic allocation region
> + * Block 14 (224-239)  dynamic allocation region
>   * Block 15 (240-255)  reserved for drivers under development, etc.
>   */
>  
>  #define IEEE1394_MAJOR			 171
>  
> +#define IEEE1394_MINOR_DYNAMIC_FIRST	176
> +#define IEEE1394_MINOR_DYNAMIC_LAST	239
> +#define IEEE1394_MINOR_DYNAMIC_COUNT	(IEEE1394_MINOR_DYNAMIC_LAST-IEEE1394_MINOR_DYNAMIC_FIRST+1)
> +
>  #define IEEE1394_MINOR_BLOCK_RAW1394	   0
>  #define IEEE1394_MINOR_BLOCK_VIDEO1394	   1
>  #define IEEE1394_MINOR_BLOCK_DV1394	   2
> @@ -218,6 +237,11 @@ static inline unsigned char ieee1394_fil
>  	return file->f_dentry->d_inode->i_cindex;
>  }
>  
> +/* add a dynamic ieee1394 device */
> +int hpsb_cdev_add(struct cdev *chardev);
> +/* remove a dynamic ieee1394 device */
> +void hpsb_cdev_del(struct cdev *chardev);
> +
>  extern int hpsb_disable_irm;
>  
>  /* Our sysfs bus entry */
> 
> 
> -
> 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/

-- 

Re: [RFC 2/4] firewire: dynamic cdev allocation below firewire major

[Arjan van de Ven]

On Sun, 2006-02-12 at 22:51 -0500, Jody McIntyre wrote:
> On Thu, Feb 02, 2006 at 11:40:12PM +0100, Johannes Berg wrote:
> > This  patch implements dynamic minor number allocation below the 171
> > major allocated for ieee1394. Since on today's systems one doesn't need
> > to have fixed device numbers any more we could just use any, but it's
> > probably still useful to use the ieee1394 major number for any firewire
> > related devices (like mem1394).
> 
> I don't really like this.  There's no benefit to using the 1394 major
> number.  I'd rather see an improved alloc_chrdev_region() that does
> something like this but for the whole kernel (currently it "wastes" an
> entire major even if you only want 1 minor, and for what you're doing,
> grabbing 1 minor at a time makes the most sense.)


why bother? There's a LOT of majors nowadays (12 bits) so... what's the
problem with keeping the kernel side simple?
(it's not as if userspace needs to care about the exact numbers anyway
for almost everything)

Re: [RFC 2/4] firewire: dynamic cdev allocation below firewire major

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

On Mon, 2006-02-13 at 08:32 +0100, Arjan van de Ven wrote:
> > I don't really like this.  There's no benefit to using the 1394 major
> > number.  I'd rather see an improved alloc_chrdev_region() that does
> > something like this but for the whole kernel (currently it "wastes" an
> > entire major even if you only want 1 minor, and for what you're doing,
> > grabbing 1 minor at a time makes the most sense.)
> 
> why bother? There's a LOT of majors nowadays (12 bits) so... what's the
> problem with keeping the kernel side simple?
> (it's not as if userspace needs to care about the exact numbers anyway
> for almost everything)

Uh, ok. Seems pretty weird to effectively allocate 256 device numbers
for just a single device, but ok :)
I'll drop the patch and make it allocate a new major for every device
plugged in.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: [RFC 2/4] firewire: dynamic cdev allocation below firewire major

[Stefan Richter]

Johannes Berg wrote:
> Seems pretty weird to effectively allocate 256 device numbers
> for just a single device, but ok :)
> I'll drop the patch and make it allocate a new major for every device
> plugged in.

Your driver could internally dispatch 256 minor device numbers after
it got itself its own dynamic major number. This should even be
acceptable as a hard limit of mem1394 devices. One would need quite a
lot of 1394 cards (or 1394.1 bridges) to get access to 256 FireWire
nodes at once.
-- 
Stefan Richter
-=====-=-==- --=- -==-=
http://arcgraph.de/sr/

Re: [RFC 2/4] firewire: dynamic cdev allocation below firewire major

[Arjan van de Ven]

On Mon, 2006-02-13 at 13:02 +0100, Johannes Berg wrote:
> On Mon, 2006-02-13 at 08:32 +0100, Arjan van de Ven wrote:
> > > I don't really like this.  There's no benefit to using the 1394 major
> > > number.  I'd rather see an improved alloc_chrdev_region() that does
> > > something like this but for the whole kernel (currently it "wastes" an
> > > entire major even if you only want 1 minor, and for what you're doing,
> > > grabbing 1 minor at a time makes the most sense.)
> > 
> > why bother? There's a LOT of majors nowadays (12 bits) so... what's the
> > problem with keeping the kernel side simple?
> > (it's not as if userspace needs to care about the exact numbers anyway
> > for almost everything)
> 
> Uh, ok. Seems pretty weird to effectively allocate 256 device numbers
> for just a single device, but ok :)

it's not 256 it's 2^20.... but still :) 
(eg there are 20 bits to a minor, 12 to a major)

Re: [RFC 2/4] firewire: dynamic cdev allocation below firewire major

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 280 bytes --]

On Mon, 2006-02-13 at 22:10 +0100, Arjan van de Ven wrote:

> it's not 256 it's 2^20.... but still :) 
> (eg there are 20 bits to a minor, 12 to a major)

Umm, right. But I guess Stefan's right too, I should instead allocate a
single major and use it's minors.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]