[PATCH] MMC host class

[PATCH] MMC host class

[Pierre Ossman]

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

Create a mmc_host class to allow enumeration of MMC host controllers
even though they have no card(s) inserted.

Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>

(This will also allow cards to be enumerated by being able to find the
hosts.)

[-- Attachment #2: mmc-class.patch --]
[-- Type: text/x-patch, Size: 3039 bytes --]

Index: linux-wbsd/drivers/mmc/mmc.h
===================================================================
--- linux-wbsd/drivers/mmc/mmc.h	(revision 134)
+++ linux-wbsd/drivers/mmc/mmc.h	(working copy)
@@ -13,4 +13,6 @@
 void mmc_init_card(struct mmc_card *card, struct mmc_host *host);
 int mmc_register_card(struct mmc_card *card);
 void mmc_remove_card(struct mmc_card *card);
+int mmc_register_host(struct mmc_host *host);
+void mmc_unregister_host(struct mmc_host *host);
 #endif
Index: linux-wbsd/drivers/mmc/mmc_sysfs.c
===================================================================
--- linux-wbsd/drivers/mmc/mmc_sysfs.c	(revision 153)
+++ linux-wbsd/drivers/mmc/mmc_sysfs.c	(working copy)
@@ -20,6 +20,7 @@
 
 #define dev_to_mmc_card(d)	container_of(d, struct mmc_card, dev)
 #define to_mmc_driver(d)	container_of(d, struct mmc_driver, drv)
+#define cls_to_mmc_host(d)	container_of(d, struct mmc_host, class_dev)
 
 #define MMC_ATTR(name, fmt, args...)					\
 static ssize_t mmc_##name##_show (struct device *dev, char *buf)	\
@@ -224,14 +225,51 @@
 	put_device(&card->dev);
 }
 
+static void mmc_host_class_dev_release(struct class_device *dev)
+{
+}
+
+static struct class mmc_host_class = {
+	.name =		"mmc_host",
+	.release =	&mmc_host_class_dev_release,
+};
+
+/*
+ * Internal function. Register a new MMC host with the MMC class.
+ */
+int mmc_register_host(struct mmc_host *host)
+{
+	host->class_dev.dev = host->dev;
+	host->class_dev.class = &mmc_host_class;
+	strlcpy(host->class_dev.class_id, host->host_name, BUS_ID_SIZE);
+
+	return class_device_register(&host->class_dev);
+}
+
+/*
+ * Internal function. Unregister a MMC host with the MMC class.
+ */
+void mmc_unregister_host(struct mmc_host *host)
+{
+	class_device_unregister(&host->class_dev);
+}
 
 static int __init mmc_init(void)
 {
-	return bus_register(&mmc_bus_type);
+	int retval;
+	
+	retval = bus_register(&mmc_bus_type);
+	if (retval)
+		return retval;
+	retval = class_register(&mmc_host_class);
+	if (retval)
+		return retval;
+	return 0;
 }
 
 static void __exit mmc_exit(void)
 {
+	class_unregister(&mmc_host_class);
 	bus_unregister(&mmc_bus_type);
 }
 
Index: linux-wbsd/drivers/mmc/mmc.c
===================================================================
--- linux-wbsd/drivers/mmc/mmc.c	(revision 153)
+++ linux-wbsd/drivers/mmc/mmc.c	(working copy)
@@ -1196,6 +1196,8 @@
 	snprintf(host->host_name, sizeof(host->host_name),
 		 "mmc%d", host_num++);
 
+	mmc_register_host(host);
+	
 	mmc_power_off(host);
 	mmc_detect_change(host);
 
@@ -1220,6 +1222,8 @@
 
 		mmc_remove_card(card);
 	}
+	
+	mmc_unregister_host(host);
 
 	mmc_power_off(host);
 }
Index: linux-wbsd/include/linux/mmc/host.h
===================================================================
--- linux-wbsd/include/linux/mmc/host.h	(revision 153)
+++ linux-wbsd/include/linux/mmc/host.h	(working copy)
@@ -69,6 +69,7 @@
 
 struct mmc_host {
 	struct device		*dev;
+	struct class_device	class_dev;
 	struct mmc_host_ops	*ops;
 	unsigned int		f_min;
 	unsigned int		f_max;

Re: [PATCH] MMC host class

[Russell King]

On Wed, Jul 13, 2005 at 05:52:52PM +0200, Pierre Ossman wrote:
> Create a mmc_host class to allow enumeration of MMC host controllers
> even though they have no card(s) inserted.
> 
> Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
> 
> (This will also allow cards to be enumerated by being able to find the
> hosts.)

> +static void mmc_host_class_dev_release(struct class_device *dev)
> +{
> +}
> +
> +static struct class mmc_host_class = {
> +	.name =		"mmc_host",
> +	.release =	&mmc_host_class_dev_release,
> +};

No no no no no.  Repeat after me ten times.  Empty or non-existant release
functions are bad and cause oopsen.  I will not create code which does
this.

> Index: linux-wbsd/include/linux/mmc/host.h
> ===================================================================
> --- linux-wbsd/include/linux/mmc/host.h	(revision 153)
> +++ linux-wbsd/include/linux/mmc/host.h	(working copy)
> @@ -69,6 +69,7 @@
>  
>  struct mmc_host {
>  	struct device		*dev;
> +	struct class_device	class_dev;
>  	struct mmc_host_ops	*ops;
>  	unsigned int		f_min;
>  	unsigned int		f_max;

What this means is that mmc_host itself becomes a refcounted sysfs
object which needs to follow the lifetime rules associated therewith.

Luckily, I thought about this earlier on, so there's a core mmc function
to allocate the beast, register it, unregister it, and finally free it.

The allocation function should initialise class_dev as much as possible.
The registration function should add the class device with the class
model.  The unregistration should remove the class device from the class
model, but _not_ free it.  The free function should drop the last
reference to the class device, which results in the remove function
(eventually) being called.  Finally, the remove function can free the
mmc_host.

Also note that since we have a class_dev, the mmc_host 'dev' field can
be removed.  However, we'll probably have to update the host drivers
to do this, so it should be a separate patch.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] MMC host class

[Pierre Ossman]

Russell King wrote:

>No no no no no.  Repeat after me ten times.  Empty or non-existant release
>functions are bad and cause oopsen.  I will not create code which does
>this.
>  
>

Sorry. I thought it was a generic cleanup function and since nothing was
allocated in the register function I didn't think it needed to do
anything. I tried to find some documentation about how classes were
handled but eventually had to resort to looking at other code. Perhaps I
should look at the documentation about kernel objects instead?

>What this means is that mmc_host itself becomes a refcounted sysfs
>object which needs to follow the lifetime rules associated therewith.
>
>Luckily, I thought about this earlier on, so there's a core mmc function
>to allocate the beast, register it, unregister it, and finally free it.
>
>The allocation function should initialise class_dev as much as possible.
>  
>

The name field cannot be initialised since it isn't generated until
registration. And I avoided filling in the other stuff at allocation so
that I could keep knowledge of mmc_host_class in mmc_sysfs.c.

>The registration function should add the class device with the class
>model.  The unregistration should remove the class device from the class
>model, but _not_ free it.  The free function should drop the last
>reference to the class device, which results in the remove function
>(eventually) being called.  Finally, the remove function can free the
>mmc_host.
>  
>

With the remove function you mean the .release in the class struct?

>Also note that since we have a class_dev, the mmc_host 'dev' field can
>be removed.  However, we'll probably have to update the host drivers
>to do this, so it should be a separate patch.
>
>  
>

I'll read up on kernel objects and sysfs and put together a new patch then.

Rgds
Pierre

Re: [PATCH] MMC host class

[Pierre Ossman]

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

Russell King wrote:

>The allocation function should initialise class_dev as much as possible.
>The registration function should add the class device with the class
>model.  The unregistration should remove the class device from the class
>model, but _not_ free it.  The free function should drop the last
>reference to the class device, which results in the remove function
>(eventually) being called.  Finally, the remove function can free the
>mmc_host.
>  
>

New patch according to above system. I've moved the naming a bit earlier
to avoid having a nameless kobj floating around.

>Also note that since we have a class_dev, the mmc_host 'dev' field can
>be removed.  However, we'll probably have to update the host drivers
>to do this, so it should be a separate patch.
>
>  
>

I believe there's a bit of abstraction to be gained from not poking
around inside the class_dev struct in too many places. It's not like
we're wasting any large amounts of memory.

Rgds
Pierre


[-- Attachment #2: mmc-class.patch --]
[-- Type: text/x-patch, Size: 3863 bytes --]

Index: linux-wbsd/drivers/mmc/mmc.h
===================================================================
--- linux-wbsd/drivers/mmc/mmc.h	(revision 134)
+++ linux-wbsd/drivers/mmc/mmc.h	(working copy)
@@ -13,4 +13,7 @@
 void mmc_init_card(struct mmc_card *card, struct mmc_host *host);
 int mmc_register_card(struct mmc_card *card);
 void mmc_remove_card(struct mmc_card *card);
+void mmc_init_host(struct mmc_host *host);
+int mmc_register_host(struct mmc_host *host);
+void mmc_unregister_host(struct mmc_host *host);
 #endif
Index: linux-wbsd/drivers/mmc/mmc_sysfs.c
===================================================================
--- linux-wbsd/drivers/mmc/mmc_sysfs.c	(revision 153)
+++ linux-wbsd/drivers/mmc/mmc_sysfs.c	(working copy)
@@ -20,6 +20,7 @@
 
 #define dev_to_mmc_card(d)	container_of(d, struct mmc_card, dev)
 #define to_mmc_driver(d)	container_of(d, struct mmc_driver, drv)
+#define cls_dev_to_mmc_host(d)	container_of(d, struct mmc_host, class_dev)
 
 #define MMC_ATTR(name, fmt, args...)					\
 static ssize_t mmc_##name##_show (struct device *dev, char *buf)	\
@@ -224,14 +225,64 @@
 	put_device(&card->dev);
 }
 
+static void mmc_host_class_dev_release(struct class_device *dev)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+	kfree(host);
+}
+
+static struct class mmc_host_class = {
+	.name =		"mmc_host",
+	.release =	&mmc_host_class_dev_release,
+};
+
+void mmc_init_host(struct mmc_host *host)
+{
+	static unsigned int host_num;
+
+	snprintf(host->host_name, sizeof(host->host_name),
+		 "mmc%d", host_num++);
+
+	host->class_dev.dev = host->dev;
+	host->class_dev.class = &mmc_host_class;
+	strlcpy(host->class_dev.class_id, host->host_name, BUS_ID_SIZE);
+	
+	class_device_initialize(&host->class_dev);
+	class_device_get(&host->class_dev);
+}
+
+/*
+ * Internal function. Register a new MMC host with the MMC class.
+ */
+int mmc_register_host(struct mmc_host *host)
+{
+	return class_device_add(&host->class_dev);
+}
+
+/*
+ * Internal function. Unregister a MMC host with the MMC class.
+ */
+void mmc_unregister_host(struct mmc_host *host)
+{
+	class_device_unregister(&host->class_dev);
+}
 
 static int __init mmc_init(void)
 {
-	return bus_register(&mmc_bus_type);
+	int retval;
+	
+	retval = bus_register(&mmc_bus_type);
+	if (retval)
+		return retval;
+	retval = class_register(&mmc_host_class);
+	if (retval)
+		return retval;
+	return 0;
 }
 
 static void __exit mmc_exit(void)
 {
+	class_unregister(&mmc_host_class);
 	bus_unregister(&mmc_bus_type);
 }
 
Index: linux-wbsd/drivers/mmc/mmc.c
===================================================================
--- linux-wbsd/drivers/mmc/mmc.c	(revision 153)
+++ linux-wbsd/drivers/mmc/mmc.c	(working copy)
@@ -1178,6 +1178,8 @@
 		host->max_phys_segs = 1;
 		host->max_sectors = 1 << (PAGE_CACHE_SHIFT - 9);
 		host->max_seg_size = PAGE_CACHE_SIZE;
+		
+		mmc_init_host(host);
 	}
 
 	return host;
@@ -1191,10 +1193,7 @@
  */
 int mmc_add_host(struct mmc_host *host)
 {
-	static unsigned int host_num;
-
-	snprintf(host->host_name, sizeof(host->host_name),
-		 "mmc%d", host_num++);
+	mmc_register_host(host);
 
 	mmc_power_off(host);
 	mmc_detect_change(host);
@@ -1222,6 +1221,8 @@
 	}
 
 	mmc_power_off(host);
+	
+	mmc_unregister_host(host);
 }
 
 EXPORT_SYMBOL(mmc_remove_host);
@@ -1235,7 +1236,8 @@
 void mmc_free_host(struct mmc_host *host)
 {
 	flush_scheduled_work();
-	kfree(host);
+	
+	class_device_put(&host->class_dev);
 }
 
 EXPORT_SYMBOL(mmc_free_host);
Index: linux-wbsd/include/linux/mmc/host.h
===================================================================
--- linux-wbsd/include/linux/mmc/host.h	(revision 153)
+++ linux-wbsd/include/linux/mmc/host.h	(working copy)
@@ -69,6 +69,7 @@
 
 struct mmc_host {
 	struct device		*dev;
+	struct class_device	class_dev;
 	struct mmc_host_ops	*ops;
 	unsigned int		f_min;
 	unsigned int		f_max;

Re: [PATCH] MMC host class

[Russell King]

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

On Fri, Jul 15, 2005 at 10:21:43PM +0200, Pierre Ossman wrote:
> Russell King wrote:
> >Also note that since we have a class_dev, the mmc_host 'dev' field can
> >be removed.  However, we'll probably have to update the host drivers
> >to do this, so it should be a separate patch.
> 
> I believe there's a bit of abstraction to be gained from not poking
> around inside the class_dev struct in too many places. It's not like
> we're wasting any large amounts of memory.

I still don't like the needless duplication.  How about doing it this
way (see the attached patch.)

Note: I also intend to move MMC over to using an IDR for the host
numbers, which is why we need to setup the name at registration
time, not allocation time.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

[-- Attachment #2: 01-mmc_hostname.diff --]
[-- Type: text/plain, Size: 3585 bytes --]

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git orig/drivers/mmc/mmc.c linux/drivers/mmc/mmc.c
--- orig/drivers/mmc/mmc.c	Sun Apr 24 21:02:15 2005
+++ linux/drivers/mmc/mmc.c	Mon Jul 18 15:24:52 2005
@@ -361,7 +361,7 @@ static void mmc_decode_cid(struct mmc_ca
 
 	default:
 		printk("%s: card has unknown MMCA version %d\n",
-			card->host->host_name, card->csd.mmca_vsn);
+			mmc_hostname(card->host), card->csd.mmca_vsn);
 		mmc_card_set_bad(card);
 		break;
 	}
@@ -383,7 +383,7 @@ static void mmc_decode_csd(struct mmc_ca
 	csd_struct = UNSTUFF_BITS(resp, 126, 2);
 	if (csd_struct != 1 && csd_struct != 2) {
 		printk("%s: unrecognised CSD structure version %d\n",
-			card->host->host_name, csd_struct);
+			mmc_hostname(card->host), csd_struct);
 		mmc_card_set_bad(card);
 		return;
 	}
@@ -551,7 +551,7 @@ static void mmc_discover_cards(struct mm
 		}
 		if (err != MMC_ERR_NONE) {
 			printk(KERN_ERR "%s: error requesting CID: %d\n",
-				host->host_name, err);
+				mmc_hostname(host), err);
 			break;
 		}
 
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git orig/drivers/mmc/mmc_sysfs.c linux/drivers/mmc/mmc_sysfs.c
--- orig/drivers/mmc/mmc_sysfs.c	Tue Jun 21 07:04:35 2005
+++ linux/drivers/mmc/mmc_sysfs.c	Mon Jul 18 15:25:07 2005
@@ -206,7 +206,7 @@ void mmc_init_card(struct mmc_card *card
 int mmc_register_card(struct mmc_card *card)
 {
 	snprintf(card->dev.bus_id, sizeof(card->dev.bus_id),
-		 "%s:%04x", card->host->host_name, card->rca);
+		 "%s:%04x", mmc_hostname(card->host), card->rca);
 
 	return device_add(&card->dev);
 }
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git orig/drivers/mmc/mmci.c linux/drivers/mmc/mmci.c
--- orig/drivers/mmc/mmci.c	Sat Jul  2 22:27:45 2005
+++ linux/drivers/mmc/mmci.c	Mon Jul 18 15:25:32 2005
@@ -34,7 +34,7 @@
 
 #ifdef CONFIG_MMC_DEBUG
 #define DBG(host,fmt,args...)	\
-	pr_debug("%s: %s: " fmt, host->mmc->host_name, __func__ , args)
+	pr_debug("%s: %s: " fmt, mmc_hostname(host->mmc), __func__ , args)
 #else
 #define DBG(host,fmt,args...)	do { } while (0)
 #endif
@@ -541,7 +541,7 @@ static int mmci_probe(struct amba_device
 	mmc_add_host(mmc);
 
 	printk(KERN_INFO "%s: MMCI rev %x cfg %02x at 0x%08lx irq %d,%d\n",
-		mmc->host_name, amba_rev(dev), amba_config(dev),
+		mmc_hostname(mmc), amba_rev(dev), amba_config(dev),
 		dev->res.start, dev->irq[0], dev->irq[1]);
 
 	init_timer(&host->timer);
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git orig/drivers/mmc/wbsd.c linux/drivers/mmc/wbsd.c
--- orig/drivers/mmc/wbsd.c	Sat Jul  2 22:27:45 2005
+++ linux/drivers/mmc/wbsd.c	Mon Jul 18 15:25:48 2005
@@ -1796,7 +1796,7 @@ static int __devinit wbsd_init(struct de
 	
 	mmc_add_host(mmc);
 
-	printk(KERN_INFO "%s: W83L51xD", mmc->host_name);
+	printk(KERN_INFO "%s: W83L51xD", mmc_hostname(mmc));
 	if (host->chip_id != 0)
 		printk(" id %x", (int)host->chip_id);
 	printk(" at 0x%x irq %d", (int)host->base, (int)host->irq);
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git orig/include/linux/mmc/host.h linux/include/linux/mmc/host.h
--- orig/include/linux/mmc/host.h	Sun Apr 24 21:06:29 2005
+++ linux/include/linux/mmc/host.h	Mon Jul 18 15:26:55 2005
@@ -97,6 +97,7 @@ extern void mmc_free_host(struct mmc_hos
 
 #define mmc_priv(x)	((void *)((x) + 1))
 #define mmc_dev(x)	((x)->dev)
+#define mmc_hostname(x)	((x)->host_name)
 
 extern int mmc_suspend_host(struct mmc_host *, pm_message_t);
 extern int mmc_resume_host(struct mmc_host *);

[-- Attachment #3: 02-classdev.diff --]
[-- Type: text/plain, Size: 3671 bytes --]

diff -u linux/drivers/mmc/mmc.c linux/drivers/mmc/mmc.c
--- linux/drivers/mmc/mmc.c	Mon Jul 18 15:24:52 2005
+++ linux/drivers/mmc/mmc.c	Mon Jul 18 15:51:37 2005
@@ -828,15 +828,15 @@
  */
 int mmc_add_host(struct mmc_host *host)
 {
-	static unsigned int host_num;
+	int ret;
 
-	snprintf(host->host_name, sizeof(host->host_name),
-		 "mmc%d", host_num++);
-
-	mmc_power_off(host);
-	mmc_detect_change(host);
+	ret = mmc_register_host_sysfs(host);
+	if (ret == 0) {
+		mmc_power_off(host);
+		mmc_detect_change(host);
+	}
 
-	return 0;
+	return ret;
 }
 
 EXPORT_SYMBOL(mmc_add_host);
@@ -859,6 +859,8 @@
 	}
 
 	mmc_power_off(host);
+
+	mmc_unregister_host_sysfs(host);
 }
 
 EXPORT_SYMBOL(mmc_remove_host);
@@ -872,7 +874,7 @@
 void mmc_free_host(struct mmc_host *host)
 {
 	flush_scheduled_work();
-	kfree(host);
+	mmc_free_host_sysfs(host);
 }
 
 EXPORT_SYMBOL(mmc_free_host);
diff -u linux/drivers/mmc/mmc_sysfs.c linux/drivers/mmc/mmc_sysfs.c
--- linux/drivers/mmc/mmc_sysfs.c	Mon Jul 18 15:25:07 2005
+++ linux/drivers/mmc/mmc_sysfs.c	Mon Jul 18 16:33:46 2005
@@ -20,6 +20,7 @@
 
 #define dev_to_mmc_card(d)	container_of(d, struct mmc_card, dev)
 #define to_mmc_driver(d)	container_of(d, struct mmc_driver, drv)
+#define cls_dev_to_mmc_host(d)	container_of(d, struct mmc_host, class_dev)
 
 #define MMC_ATTR(name, fmt, args...)					\
 static ssize_t mmc_##name##_show (struct device *dev, struct device_attribute *attr, char *buf)	\
@@ -224,13 +225,64 @@
 }
 
 
+static void mmc_host_classdev_release(struct class_device *dev)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+	kfree(host);
+}
+
+static struct class mmc_host_class = {
+	.name		= "mmc_host",
+	.release	= mmc_host_classdev_release,
+};
+
+/*
+ * Internal function. Register a new MMC host with the MMC class.
+ */
+int mmc_register_host_sysfs(struct mmc_host *host)
+{
+	static unsigned int host_num;
+
+	snprintf(host->host_name, sizeof(host->host_name),
+		 "mmc%d", host_num++);
+
+	host->class_dev.dev = host->dev;
+	host->class_dev.class = &mmc_host_class;
+	strlcpy(host->class_dev.class_id, host->host_name, BUS_ID_SIZE);
+	return class_device_register(&host->class_dev);
+}
+
+/*
+ * Internal function. Unregister a MMC host with the MMC class.
+ */
+void mmc_unregister_host_sysfs(struct mmc_host *host)
+{
+	class_device_del(&host->class_dev);
+}
+
+/*
+ * Internal function. Free a MMC host.
+ */
+void mmc_free_host_sysfs(struct mmc_host *host)
+{
+	class_device_put(&host->class_dev);
+}
+
+
 static int __init mmc_init(void)
 {
-	return bus_register(&mmc_bus_type);
+	int ret = bus_register(&mmc_bus_type);
+	if (ret == 0) {
+		ret = class_register(&mmc_host_class);
+		if (ret)
+			bus_unregister(&mmc_bus_type);
+	}
+	return ret;
 }
 
 static void __exit mmc_exit(void)
 {
+	class_unregister(&mmc_host_class);
 	bus_unregister(&mmc_bus_type);
 }
 
diff -u linux/include/linux/mmc/host.h linux/include/linux/mmc/host.h
--- linux/include/linux/mmc/host.h	Mon Jul 18 15:26:55 2005
+++ linux/include/linux/mmc/host.h	Mon Jul 18 15:44:56 2005
@@ -63,6 +63,7 @@
 
 struct mmc_host {
 	struct device		*dev;
+	struct class_device	class_dev;
 	struct mmc_host_ops	*ops;
 	unsigned int		f_min;
 	unsigned int		f_max;
only in patch2:
--- orig/drivers/mmc/mmc.h	Sun Apr 24 21:02:15 2005
+++ linux/drivers/mmc/mmc.h	Mon Jul 18 18:25:31 2005
@@ -13,4 +13,8 @@
 void mmc_init_card(struct mmc_card *card, struct mmc_host *host);
 int mmc_register_card(struct mmc_card *card);
 void mmc_remove_card(struct mmc_card *card);
+
+int mmc_register_host_sysfs(struct mmc_host *host);
+void mmc_unregister_host_sysfs(struct mmc_host *host);
+void mmc_free_host_sysfs(struct mmc_host *host);
 #endif

[-- Attachment #4: 03-classdev_hostname.diff --]
[-- Type: text/plain, Size: 1300 bytes --]

diff -u linux/drivers/mmc/mmc_sysfs.c linux/drivers/mmc/mmc_sysfs.c
--- linux/drivers/mmc/mmc_sysfs.c	Mon Jul 18 16:33:46 2005
+++ linux/drivers/mmc/mmc_sysfs.c	Mon Jul 18 18:27:04 2005
@@ -243,12 +243,11 @@
 {
 	static unsigned int host_num;
 
-	snprintf(host->host_name, sizeof(host->host_name),
-		 "mmc%d", host_num++);
-
 	host->class_dev.dev = host->dev;
 	host->class_dev.class = &mmc_host_class;
-	strlcpy(host->class_dev.class_id, host->host_name, BUS_ID_SIZE);
+	snprintf(host->class_dev.class_id, BUS_ID_SIZE,
+		 "mmc%d", host_num++);
+
 	return class_device_register(&host->class_dev);
 }
 
diff -u linux/include/linux/mmc/host.h linux/include/linux/mmc/host.h
--- linux/include/linux/mmc/host.h	Mon Jul 18 15:44:56 2005
+++ linux/include/linux/mmc/host.h	Mon Jul 18 18:28:51 2005
@@ -68,7 +68,6 @@
 	unsigned int		f_min;
 	unsigned int		f_max;
 	u32			ocr_avail;
-	char			host_name[8];
 
 	/* host specific block data */
 	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
@@ -98,7 +97,7 @@
 
 #define mmc_priv(x)	((void *)((x) + 1))
 #define mmc_dev(x)	((x)->dev)
-#define mmc_hostname(x)	((x)->host_name)
+#define mmc_hostname(x)	((x)->class_dev.class_id)
 
 extern int mmc_suspend_host(struct mmc_host *, pm_message_t);
 extern int mmc_resume_host(struct mmc_host *);

Re: [PATCH] MMC host class

[Pierre Ossman]

Russell King wrote:

>On Fri, Jul 15, 2005 at 10:21:43PM +0200, Pierre Ossman wrote:
>  
>
>>Russell King wrote:
>>    
>>
>>>Also note that since we have a class_dev, the mmc_host 'dev' field can
>>>be removed.  However, we'll probably have to update the host drivers
>>>to do this, so it should be a separate patch.
>>>      
>>>
>>I believe there's a bit of abstraction to be gained from not poking
>>around inside the class_dev struct in too many places. It's not like
>>we're wasting any large amounts of memory.
>>    
>>
>
>I still don't like the needless duplication.  How about doing it this
>way (see the attached patch.)
>  
>

The mmc_hostname macro seems like a good solution. It'll keep the
abstraction even if stuff needs to be moved around.

I see a problem with waiting until mmc_add_host() until initialising the
kobject though. If a driver calls mmc_alloc_host() and then
mmc_free_host(), perhaps because of some error, then the structure won't
be freed since we rely on release getting called. That's why I tried to
get the kobject stuff set up with the allocation.

Perhaps it is possible to test if a kobject is initialised and if not
free the structure directly?

Rgds
Pierre

Re: [PATCH] MMC host class

[Pierre Ossman]

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

Russell King wrote:

>
>I still don't like the needless duplication.  How about doing it this
>way (see the attached patch.)
>
>Note: I also intend to move MMC over to using an IDR for the host
>numbers, which is why we need to setup the name at registration
>time, not allocation time.
>
>  
>

This patch should cover the edge case of allocating but not registering
a host.

Rgds
Pierre


[-- Attachment #2: 04-safe_free.diff --]
[-- Type: text/x-patch, Size: 522 bytes --]

diff -uNp linux-2.6.13-rc6/drivers/mmc.orig/mmc.c linux-2.6.13-rc6/drivers/mmc/mmc.c
--- linux-2.6.13-rc6/drivers/mmc.orig/mmc.c	2005-08-08 13:29:53.000000000 +0200
+++ linux-2.6.13-rc6/drivers/mmc/mmc.c	2005-08-08 13:36:08.000000000 +0200
@@ -874,7 +874,11 @@ EXPORT_SYMBOL(mmc_remove_host);
 void mmc_free_host(struct mmc_host *host)
 {
 	flush_scheduled_work();
-	mmc_free_host_sysfs(host);
+
+	if (host->class_dev.class != NULL)
+		mmc_free_host_sysfs(host);
+	else
+		kfree(host);
 }
 
 EXPORT_SYMBOL(mmc_free_host);

Re: [PATCH] MMC host class

[Russell King]

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

On Mon, Aug 08, 2005 at 01:38:13PM +0200, Pierre Ossman wrote:
> Russell King wrote:
> >I still don't like the needless duplication.  How about doing it this
> >way (see the attached patch.)
> >
> >Note: I also intend to move MMC over to using an IDR for the host
> >numbers, which is why we need to setup the name at registration
> >time, not allocation time.
> 
> This patch should cover the edge case of allocating but not registering
> a host.

Hmm, I think I've gone back to preferring something similar to your
original approach actually.  I've also included the IDR patch.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

[-- Attachment #2: 01-mmc_hostname.diff --]
[-- Type: text/plain, Size: 3585 bytes --]

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git orig/drivers/mmc/mmc.c linux/drivers/mmc/mmc.c
--- orig/drivers/mmc/mmc.c	Sun Apr 24 21:02:15 2005
+++ linux/drivers/mmc/mmc.c	Mon Jul 18 15:24:52 2005
@@ -361,7 +361,7 @@ static void mmc_decode_cid(struct mmc_ca
 
 	default:
 		printk("%s: card has unknown MMCA version %d\n",
-			card->host->host_name, card->csd.mmca_vsn);
+			mmc_hostname(card->host), card->csd.mmca_vsn);
 		mmc_card_set_bad(card);
 		break;
 	}
@@ -383,7 +383,7 @@ static void mmc_decode_csd(struct mmc_ca
 	csd_struct = UNSTUFF_BITS(resp, 126, 2);
 	if (csd_struct != 1 && csd_struct != 2) {
 		printk("%s: unrecognised CSD structure version %d\n",
-			card->host->host_name, csd_struct);
+			mmc_hostname(card->host), csd_struct);
 		mmc_card_set_bad(card);
 		return;
 	}
@@ -551,7 +551,7 @@ static void mmc_discover_cards(struct mm
 		}
 		if (err != MMC_ERR_NONE) {
 			printk(KERN_ERR "%s: error requesting CID: %d\n",
-				host->host_name, err);
+				mmc_hostname(host), err);
 			break;
 		}
 
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git orig/drivers/mmc/mmc_sysfs.c linux/drivers/mmc/mmc_sysfs.c
--- orig/drivers/mmc/mmc_sysfs.c	Tue Jun 21 07:04:35 2005
+++ linux/drivers/mmc/mmc_sysfs.c	Mon Jul 18 15:25:07 2005
@@ -206,7 +206,7 @@ void mmc_init_card(struct mmc_card *card
 int mmc_register_card(struct mmc_card *card)
 {
 	snprintf(card->dev.bus_id, sizeof(card->dev.bus_id),
-		 "%s:%04x", card->host->host_name, card->rca);
+		 "%s:%04x", mmc_hostname(card->host), card->rca);
 
 	return device_add(&card->dev);
 }
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git orig/drivers/mmc/mmci.c linux/drivers/mmc/mmci.c
--- orig/drivers/mmc/mmci.c	Sat Jul  2 22:27:45 2005
+++ linux/drivers/mmc/mmci.c	Mon Jul 18 15:25:32 2005
@@ -34,7 +34,7 @@
 
 #ifdef CONFIG_MMC_DEBUG
 #define DBG(host,fmt,args...)	\
-	pr_debug("%s: %s: " fmt, host->mmc->host_name, __func__ , args)
+	pr_debug("%s: %s: " fmt, mmc_hostname(host->mmc), __func__ , args)
 #else
 #define DBG(host,fmt,args...)	do { } while (0)
 #endif
@@ -541,7 +541,7 @@ static int mmci_probe(struct amba_device
 	mmc_add_host(mmc);
 
 	printk(KERN_INFO "%s: MMCI rev %x cfg %02x at 0x%08lx irq %d,%d\n",
-		mmc->host_name, amba_rev(dev), amba_config(dev),
+		mmc_hostname(mmc), amba_rev(dev), amba_config(dev),
 		dev->res.start, dev->irq[0], dev->irq[1]);
 
 	init_timer(&host->timer);
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git orig/drivers/mmc/wbsd.c linux/drivers/mmc/wbsd.c
--- orig/drivers/mmc/wbsd.c	Sat Jul  2 22:27:45 2005
+++ linux/drivers/mmc/wbsd.c	Mon Jul 18 15:25:48 2005
@@ -1796,7 +1796,7 @@ static int __devinit wbsd_init(struct de
 	
 	mmc_add_host(mmc);
 
-	printk(KERN_INFO "%s: W83L51xD", mmc->host_name);
+	printk(KERN_INFO "%s: W83L51xD", mmc_hostname(mmc));
 	if (host->chip_id != 0)
 		printk(" id %x", (int)host->chip_id);
 	printk(" at 0x%x irq %d", (int)host->base, (int)host->irq);
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git orig/include/linux/mmc/host.h linux/include/linux/mmc/host.h
--- orig/include/linux/mmc/host.h	Sun Apr 24 21:06:29 2005
+++ linux/include/linux/mmc/host.h	Mon Jul 18 15:26:55 2005
@@ -97,6 +97,7 @@ extern void mmc_free_host(struct mmc_hos
 
 #define mmc_priv(x)	((void *)((x) + 1))
 #define mmc_dev(x)	((x)->dev)
+#define mmc_hostname(x)	((x)->host_name)
 
 extern int mmc_suspend_host(struct mmc_host *, pm_message_t);
 extern int mmc_resume_host(struct mmc_host *);

[-- Attachment #3: 02-classdev.diff --]
[-- Type: text/plain, Size: 4658 bytes --]

diff -u linux/drivers/mmc/mmc.c linux/drivers/mmc/mmc.c
--- linux/drivers/mmc/mmc.c	Mon Jul 18 15:24:52 2005
+++ linux/drivers/mmc/mmc.c	Mon Jul 18 15:51:37 2005
@@ -796,17 +796,13 @@ struct mmc_host *mmc_alloc_host(int extr
 {
 	struct mmc_host *host;
 
-	host = kmalloc(sizeof(struct mmc_host) + extra, GFP_KERNEL);
+	host = mmc_alloc_host_sysfs(extra, dev);
 	if (host) {
-		memset(host, 0, sizeof(struct mmc_host) + extra);
-
 		spin_lock_init(&host->lock);
 		init_waitqueue_head(&host->wq);
 		INIT_LIST_HEAD(&host->cards);
 		INIT_WORK(&host->detect, mmc_rescan, host);
 
-		host->dev = dev;
-
 		/*
 		 * By default, hosts do not support SGIO or large requests.
 		 * They have to set these according to their abilities.
@@ -828,15 +824,15 @@
  */
 int mmc_add_host(struct mmc_host *host)
 {
-	static unsigned int host_num;
+	int ret;
 
-	snprintf(host->host_name, sizeof(host->host_name),
-		 "mmc%d", host_num++);
-
-	mmc_power_off(host);
-	mmc_detect_change(host);
+	ret = mmc_add_host_sysfs(host);
+	if (ret == 0) {
+		mmc_power_off(host);
+		mmc_detect_change(host);
+	}
 
-	return 0;
+	return ret;
 }
 
 EXPORT_SYMBOL(mmc_add_host);
@@ -859,6 +855,7 @@
 	}
 
 	mmc_power_off(host);
+	mmc_remove_host_sysfs(host);
 }
 
 EXPORT_SYMBOL(mmc_remove_host);
@@ -872,7 +869,7 @@
 void mmc_free_host(struct mmc_host *host)
 {
 	flush_scheduled_work();
-	kfree(host);
+	mmc_free_host_sysfs(host);
 }
 
 EXPORT_SYMBOL(mmc_free_host);
diff -u linux/drivers/mmc/mmc_sysfs.c linux/drivers/mmc/mmc_sysfs.c
--- linux/drivers/mmc/mmc_sysfs.c	Mon Jul 18 15:25:07 2005
+++ linux/drivers/mmc/mmc_sysfs.c	Mon Jul 18 16:33:46 2005
@@ -20,6 +20,7 @@
 
 #define dev_to_mmc_card(d)	container_of(d, struct mmc_card, dev)
 #define to_mmc_driver(d)	container_of(d, struct mmc_driver, drv)
+#define cls_dev_to_mmc_host(d)	container_of(d, struct mmc_host, class_dev)
 
 #define MMC_ATTR(name, fmt, args...)					\
 static ssize_t mmc_##name##_show (struct device *dev, struct device_attribute *attr, char *buf)	\
@@ -224,13 +225,82 @@
 }
 
 
+static void mmc_host_classdev_release(struct class_device *dev)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+	kfree(host);
+}
+
+static struct class mmc_host_class = {
+	.name		= "mmc_host",
+	.release	= mmc_host_classdev_release,
+};
+
+/*
+ * Internal function. Allocate a new MMC host.
+ */
+struct mmc_host *mmc_alloc_host_sysfs(int extra, struct device *dev)
+{
+	struct mmc_host *host;
+
+	host = kmalloc(sizeof(struct mmc_host) + extra, GFP_KERNEL);
+	if (host) {
+		memset(host, 0, sizeof(struct mmc_host) + extra);
+
+		host->dev = dev;
+		host->class_dev.dev = host->dev;
+		host->class_dev.class = &mmc_host_class;
+		class_device_initialize(&host->class_dev);
+	}
+
+	return host;
+}
+
+/*
+ * Internal function. Register a new MMC host with the MMC class.
+ */
+int mmc_add_host_sysfs(struct mmc_host *host)
+{
+	static unsigned int host_num;
+
+	snprintf(host->host_name, sizeof(host->host_name),
+		 "mmc%d", host_num++);
+
+	strlcpy(host->class_dev.class_id, host->host_name, BUS_ID_SIZE);
+	return class_device_add(&host->class_dev);
+}
+
+/*
+ * Internal function. Unregister a MMC host with the MMC class.
+ */
+void mmc_remove_host_sysfs(struct mmc_host *host)
+{
+	class_device_del(&host->class_dev);
+}
+
+/*
+ * Internal function. Free a MMC host.
+ */
+void mmc_free_host_sysfs(struct mmc_host *host)
+{
+	class_device_put(&host->class_dev);
+}
+
+
 static int __init mmc_init(void)
 {
-	return bus_register(&mmc_bus_type);
+	int ret = bus_register(&mmc_bus_type);
+	if (ret == 0) {
+		ret = class_register(&mmc_host_class);
+		if (ret)
+			bus_unregister(&mmc_bus_type);
+	}
+	return ret;
 }
 
 static void __exit mmc_exit(void)
 {
+	class_unregister(&mmc_host_class);
 	bus_unregister(&mmc_bus_type);
 }
 
diff -u linux/include/linux/mmc/host.h linux/include/linux/mmc/host.h
--- linux/include/linux/mmc/host.h	Mon Jul 18 15:26:55 2005
+++ linux/include/linux/mmc/host.h	Mon Jul 18 15:44:56 2005
@@ -63,6 +63,7 @@
 
 struct mmc_host {
 	struct device		*dev;
+	struct class_device	class_dev;
 	struct mmc_host_ops	*ops;
 	unsigned int		f_min;
 	unsigned int		f_max;
only in patch2:
--- orig/drivers/mmc/mmc.h	Sun Apr 24 21:02:15 2005
+++ linux/drivers/mmc/mmc.h	Mon Jul 18 18:25:31 2005
@@ -13,4 +13,9 @@
 void mmc_init_card(struct mmc_card *card, struct mmc_host *host);
 int mmc_register_card(struct mmc_card *card);
 void mmc_remove_card(struct mmc_card *card);
+
+struct mmc_host *mmc_alloc_host_sysfs(int extra, struct device *dev);
+int mmc_add_host_sysfs(struct mmc_host *host);
+void mmc_remove_host_sysfs(struct mmc_host *host);
+void mmc_free_host_sysfs(struct mmc_host *host);
 #endif

[-- Attachment #4: 03-classdev_hostname.diff --]
[-- Type: text/plain, Size: 1216 bytes --]

diff -u linux/drivers/mmc/mmc_sysfs.c linux/drivers/mmc/mmc_sysfs.c
--- linux/drivers/mmc/mmc_sysfs.c	Mon Jul 18 16:33:46 2005
+++ linux/drivers/mmc/mmc_sysfs.c	Mon Jul 18 18:27:04 2005
@@ -263,10 +263,9 @@
 {
 	static unsigned int host_num;
 
-	snprintf(host->host_name, sizeof(host->host_name),
-		 "mmc%d", host_num++);
-
-	strlcpy(host->class_dev.class_id, host->host_name, BUS_ID_SIZE);
+	snprintf(host->class_dev.class_id, BUS_ID_SIZE,
+		 "mmc%d", host_num++);
+
 	return class_device_add(&host->class_dev);
 }
 
diff -u linux/include/linux/mmc/host.h linux/include/linux/mmc/host.h
--- linux/include/linux/mmc/host.h	Mon Jul 18 15:44:56 2005
+++ linux/include/linux/mmc/host.h	Mon Jul 18 18:28:51 2005
@@ -68,7 +68,6 @@
 	unsigned int		f_min;
 	unsigned int		f_max;
 	u32			ocr_avail;
-	char			host_name[8];
 
 	/* host specific block data */
 	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
@@ -98,7 +97,7 @@
 
 #define mmc_priv(x)	((void *)((x) + 1))
 #define mmc_dev(x)	((x)->dev)
-#define mmc_hostname(x)	((x)->host_name)
+#define mmc_hostname(x)	((x)->class_dev.class_id)
 
 extern int mmc_suspend_host(struct mmc_host *, pm_message_t);
 extern int mmc_resume_host(struct mmc_host *);

[-- Attachment #5: 04-classdev-idr.diff --]
[-- Type: text/plain, Size: 1869 bytes --]

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' orig/drivers/mmc/mmc_sysfs.c linux/drivers/mmc/mmc_sysfs.c
--- orig/drivers/mmc/mmc_sysfs.c	Sun Jul 24 20:00:07 2005
+++ linux/drivers/mmc/mmc_sysfs.c	Mon Jul 18 19:08:44 2005
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/device.h>
+#include <linux/idr.h>
 
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
@@ -236,6 +237,9 @@ static struct class mmc_host_class = {
 	.release	= mmc_host_classdev_release,
 };
 
+static DEFINE_IDR(mmc_host_idr);
+static DEFINE_SPINLOCK(mmc_host_lock);
+
 /*
  * Internal function. Allocate a new MMC host.
  */
@@ -261,10 +265,19 @@
  */
 int mmc_add_host_sysfs(struct mmc_host *host)
 {
-	static unsigned int host_num;
+	int err;
+
+	if (!idr_pre_get(&mmc_host_idr, GFP_KERNEL))
+		return -ENOMEM;
+
+	spin_lock(&mmc_host_lock);
+	err = idr_get_new(&mmc_host_idr, host, &host->index);
+	spin_unlock(&mmc_host_lock);
+	if (err)
+		return err;
 
 	snprintf(host->class_dev.class_id, BUS_ID_SIZE,
-		 "mmc%d", host_num++);
+		 "mmc%d", host->index);
 
 	return class_device_add(&host->class_dev);
 }
@@ -275,6 +288,10 @@ int mmc_add_host_sysfs(struct mmc_h
 void mmc_remove_host_sysfs(struct mmc_host *host)
 {
 	class_device_del(&host->class_dev);
+
+	spin_lock(&mmc_host_lock);
+	idr_remove(&mmc_host_idr, host->index);
+	spin_unlock(&mmc_host_lock);
 }
 
 /*
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' orig/include/linux/mmc/host.h linux/include/linux/mmc/host.h
--- orig/include/linux/mmc/host.h	Sun Jul 24 20:00:07 2005
+++ linux/include/linux/mmc/host.h	Mon Jul 18 19:07:22 2005
@@ -64,6 +64,7 @@ struct device;
 struct mmc_host {
 	struct device		*dev;
 	struct class_device	class_dev;
+	int			index;
 	struct mmc_host_ops	*ops;
 	unsigned int		f_min;
 	unsigned int		f_max;

Re: [PATCH] MMC host class

[Pierre Ossman]

Russell King wrote:

>
>Hmm, I think I've gone back to preferring something similar to your
>original approach actually.  I've also included the IDR patch.
>
>  
>

Ok. Just as long as it works. :)

My two concerns are:

* Things that assume there's a name for every kobject.
* Things that assume that a kobject's name cannot change.

The documentation isn't clear about the semantics in these cases. But
you have more experience with kobjects than I do so perhaps these are
obvious to you.

Otherwise I'm just waiting to see this committed.

Rgds
Pierre

Re: [PATCH] MMC host class

[Russell King]

On Fri, Aug 19, 2005 at 08:14:27AM +0200, Pierre Ossman wrote:
> * Things that assume there's a name for every kobject.

The name is assigned when the host is added to the mmc subsystem,
not when it's allocated.  This is the same behaviour as other
subsystems (eg, networking) and I don't see a problem with that.

> * Things that assume that a kobject's name cannot change.

The mmc host name will never change once its initially set.

> Otherwise I'm just waiting to see this committed.

Post 2.6.13, probably at akpm's discression.  As I mentioned in other
mails, I will be away from kernel work next week, which is probably
when 2.6.13 will be released.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core