Skip to contentSkip to conversation

ahci rmmodable and shouldn't

PATCH In review Pedro Francisco 4 participants 16 messages Sep 16, 2010 – Sep 23, 2010

Subsystems (2)
libata libata SATA AHCI platform devices support
lore
Share & tools
lore

Pedro Francisco

author
#0 lore raw

I reported this here for 2.6.35.4 -- http://lkml.org/lkml/2010/9/7/321 -- but still happens on latest -git.

rmmod ahci' works even if the module is being used (lsmod' thinks it's not being used but rmmoding ahci triggers filesystem erros and I've to force a reboot).

I've been told it's the same as plugging a cable. IMO that's what /sys/stuff/eject_or_similar is to, not `rmmod ahci'.

I *assume* it was caused by the split between libahci & ahci, since in 2.6.32 rmmod ahci' is impossible because it's marked as being in use. Didn't try rmmod -f ahci' but that's not the point.

I'm posting again about this because it doesn't make sense to me the actual behaviour.

TIA,

Signature · 1 line
Pedro
#1 lore raw
Quoted 2 lines · Pedro Francisco

I reported this here for 2.6.35.4 -- http://lkml.org/lkml/2010/9/7/321 -- but still happens on latest -git.

Please copy [email protected] on ATA issues. (done now)

Quoted 21 lines

rmmod ahci' works even if the module is being used (lsmod' thinks it's not being used but rmmoding ahci triggers filesystem erros and I've to force a reboot).

I've been told it's the same as plugging a cable. IMO that's what /sys/stuff/eject_or_similar is to, not `rmmod ahci'.

I *assume* it was caused by the split between libahci & ahci, since in 2.6.32
`rmmod ahci' is impossible because it's marked as being in use. Didn't try
`rmmod -f ahci' but that's not the point.

I'm posting again about this because it doesn't make sense to me the actual behaviour.

TIA,
--
Pedro
--

Signature · 1 line
~Randy
#2 lore raw

Hello,

Quoted 8 lines · Randy Dunlap
Quoted 8 lines

rmmod ahci' works even if the module is being used (lsmod' thinks it's not being used but rmmoding ahci triggers filesystem erros and I've to force a reboot).

I've been told it's the same as plugging a cable. IMO that's what /sys/stuff/eject_or_similar is to, not `rmmod ahci'.

Quoted 8 lines
Quoted 8 lines

I *assume* it was caused by the split between libahci & ahci, since in 2.6.32
`rmmod ahci' is impossible because it's marked as being in use. Didn't try
`rmmod -f ahci' but that's not the point.

I'm posting again about this because it doesn't make sense to me the actual behaviour.

Just don't rmmod a module which is serving a live filesystem. Why does it even matter?

Signature · 1 line
tejun
author
#3 lore raw
Quoted 3 lines · Tejun Heo

Just don't rmmod a module which is serving a live filesystem. Why does it even matter?

... Ok, let me argue another way. What if my mail was called: «lsmod shows false values ("Used by: 0") for module 'ahci'», would you agree it was an error and should therefore be fixed?

Signature · 1 line
Pedro
#4 lore raw

Hello,

Quoted 7 lines · Pedro Francisco
Quoted 2 lines · Tejun Heo

Just don't rmmod a module which is serving a live filesystem. Why does it even matter?

... Ok, let me argue another way. What if my mail was called: «lsmod shows false values ("Used by: 0") for module 'ahci'», would you agree it was an error and should therefore be fixed?

I think it works the same way for any other SCSI driver and it's pretty convenient during development. You can unload any network driver module while the network is still up too. Module reference count doesn't have any specific meaning of 'use' preassigned other than it's there to prevent unloading module while the code and data are being actively used by the processor. It's unfortunate that the way module reference count is used for libata is causing discomfort to you but I need much better reason to change it.

Thanks.

Signature · 1 line
tejun
author
#5 lore raw
Quoted 8 lines · Tejun Heo

I think it works the same way for any other SCSI driver and it's pretty convenient during development. You can unload any network driver module while the network is still up too. Module reference count doesn't have any specific meaning of 'use' preassigned other than it's there to prevent unloading module while the code and data are being actively used by the processor. It's unfortunate that the way module reference count is used for libata is causing discomfort to you but I need much better reason to change it.

Discomfort? Not really, sorry if my stubbornness felt like it. No, I'm fine with it.

I just thought it was weird the change of behaviour from 2.6.32, plus felt that the "Used by" field by lsmod was incorrectly set at 0 since the module is, in fact, being used (e)

If you say both behaviours are expected and preferred, besides being consistent with other modules (something I didn't know), I really can't argue any more.

Thanks for the explanation and sorry for the noise :)

Signature · 1 line
Pedro
#6 lore raw

Hello,

Quoted 2 lines · Pedro Francisco

Discomfort? Not really, sorry if my stubbornness felt like it. No, I'm fine with it.

:-)

Quoted 3 lines

I just thought it was weird the change of behaviour from 2.6.32, plus felt that the "Used by" field by lsmod was incorrectly set at 0 since the module is, in fact, being used (e)

Hmmm... I don't recall the behavior changing from 2.6.32. Maybe I've been just speaking bullshit this whole time. Can you please post the output of lsmod? Maybe the difference is caused by the split between libahci and ahci?

Thanks.

Signature · 1 line
tejun
#7 lore raw

Hello, again.

Yeah, now I'm fairly sure I was spewing out complete bullshit. Sorry about that. Gees, why does spewing out bullshit always feel so right? Please feel free to call me an idiot. Can you please try the following patch and see whether it changes the module counting behavior?

Thanks.

drivers/ata/ahci.c +4 -0
index ff1c945..99d0e5a 100644
@@ -90,6 +90,10 @@ static int ahci_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
 static int ahci_pci_device_resume(struct pci_dev *pdev);
 #endif
 
+static struct scsi_host_template ahci_sht = {
+	AHCI_SHT("ahci"),
+};
+
 static struct ata_port_operations ahci_vt8251_ops = {
 	.inherits		= &ahci_ops,
 	.hardreset		= ahci_vt8251_hardreset,
drivers/ata/ahci.h +11 -1
index 474427b..e5fdeeb 100644
@@ -298,7 +298,17 @@ struct ahci_host_priv {
 
 extern int ahci_ignore_sss;
 
-extern struct scsi_host_template ahci_sht;
+extern struct device_attribute *ahci_shost_attrs[];
+extern struct device_attribute *ahci_sdev_attrs[];
+
+#define AHCI_SHT(drv_name)						\
+	ATA_NCQ_SHT(drv_name),						\
+	.can_queue		= AHCI_MAX_CMDS - 1,			\
+	.sg_tablesize		= AHCI_MAX_SG,				\
+	.dma_boundary		= AHCI_DMA_BOUNDARY,			\
+	.shost_attrs		= ahci_shost_attrs,			\
+	.sdev_attrs		= ahci_sdev_attrs
+
 extern struct ata_port_operations ahci_ops;
 
 void ahci_save_initial_config(struct device *dev,
drivers/ata/ahci_platform.c +5 -1
index 4e97f33..84b6432 100644
@@ -23,6 +23,10 @@
 #include <linux/ahci_platform.h>
 #include "ahci.h"
 
+static struct scsi_host_template ahci_platform_sht = {
+	AHCI_SHT("ahci_platform"),
+};
+
 static int __init ahci_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;

@@ -145,7 +149,7 @@ static int __init ahci_probe(struct platform_device *pdev)
 	ahci_print_info(host, "platform");
 
 	rc = ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
-			       &ahci_sht);
+			       &ahci_platform_sht);
 	if (rc)
 		goto err0;
drivers/ata/libahci.c +4 -12
index 68dc678..8eea309 100644
@@ -121,7 +121,7 @@ static DEVICE_ATTR(ahci_port_cmd, S_IRUGO, ahci_show_port_cmd, NULL);
 static DEVICE_ATTR(em_buffer, S_IWUSR | S_IRUGO,
 		   ahci_read_em_buffer, ahci_store_em_buffer);
 
-static struct device_attribute *ahci_shost_attrs[] = {
+struct device_attribute *ahci_shost_attrs[] = {
 	&dev_attr_link_power_management_policy,
 	&dev_attr_em_message_type,
 	&dev_attr_em_message,

@@ -132,22 +132,14 @@ static struct device_attribute *ahci_shost_attrs[] = {
 	&dev_attr_em_buffer,
 	NULL
 };
+EXPORT_SYMBOL_GPL(ahci_shost_attrs);
 
-static struct device_attribute *ahci_sdev_attrs[] = {
+struct device_attribute *ahci_sdev_attrs[] = {
 	&dev_attr_sw_activity,
 	&dev_attr_unload_heads,
 	NULL
 };
-
-struct scsi_host_template ahci_sht = {
-	ATA_NCQ_SHT("ahci"),
-	.can_queue		= AHCI_MAX_CMDS - 1,
-	.sg_tablesize		= AHCI_MAX_SG,
-	.dma_boundary		= AHCI_DMA_BOUNDARY,
-	.shost_attrs		= ahci_shost_attrs,
-	.sdev_attrs		= ahci_sdev_attrs,
-};
-EXPORT_SYMBOL_GPL(ahci_sht);
+EXPORT_SYMBOL_GPL(ahci_sdev_attrs);
 
 struct ata_port_operations ahci_ops = {
 	.inherits		= &sata_pmp_port_ops,
author
#8 lore raw
Quoted 10 lines · Tejun Heo

Hello, again.

Yeah, now I'm fairly sure I was spewing out complete bullshit. Sorry about that. Gees, why does spewing out bullshit always feel so right? Please feel free to call me an idiot. Can you please try the following patch and see whether it changes the module counting behavior?

Thanks.

-snip

To which tree shall I apply it? It currently fails both on Linus' linux-2.6 and
git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git .

#9 lore raw

Hello,

Quoted 3 lines · Pedro Francisco

To which tree shall I apply it? It currently fails both on Linus' linux-2.6 and
git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git .

Hmm... weird. It's against Linus' tree. Checking, yeap, it applies cleanly to 2.6.36-rc4, commit 2422084a (Merge branch 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/mattst88/alpha-2.6).

Thanks.

Signature · 1 line
tejun
#10 lore raw
Quoted 10 lines · Tejun Heo

Hello,

Quoted 3 lines · Pedro Francisco

To which tree shall I apply it? It currently fails both on Linus' linux-2.6 and
git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git .

Hmm... weird. It's against Linus' tree. Checking, yeap, it applies cleanly to 2.6.36-rc4, commit 2422084a (Merge branch 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/mattst88/alpha-2.6).

Attaching the patch just in case.

Signature · 1 line
tejun
drivers/ata/ahci.c +4 -0
index ff1c945..99d0e5a 100644
@@ -90,6 +90,10 @@ static int ahci_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
 static int ahci_pci_device_resume(struct pci_dev *pdev);
 #endif
 
+static struct scsi_host_template ahci_sht = {
+	AHCI_SHT("ahci"),
+};
+
 static struct ata_port_operations ahci_vt8251_ops = {
 	.inherits		= &ahci_ops,
 	.hardreset		= ahci_vt8251_hardreset,
drivers/ata/ahci.h +11 -1
index 474427b..e5fdeeb 100644
@@ -298,7 +298,17 @@ struct ahci_host_priv {
 
 extern int ahci_ignore_sss;
 
-extern struct scsi_host_template ahci_sht;
+extern struct device_attribute *ahci_shost_attrs[];
+extern struct device_attribute *ahci_sdev_attrs[];
+
+#define AHCI_SHT(drv_name)						\
+	ATA_NCQ_SHT(drv_name),						\
+	.can_queue		= AHCI_MAX_CMDS - 1,			\
+	.sg_tablesize		= AHCI_MAX_SG,				\
+	.dma_boundary		= AHCI_DMA_BOUNDARY,			\
+	.shost_attrs		= ahci_shost_attrs,			\
+	.sdev_attrs		= ahci_sdev_attrs
+
 extern struct ata_port_operations ahci_ops;
 
 void ahci_save_initial_config(struct device *dev,
drivers/ata/ahci_platform.c +5 -1
index 4e97f33..84b6432 100644
@@ -23,6 +23,10 @@
 #include <linux/ahci_platform.h>
 #include "ahci.h"
 
+static struct scsi_host_template ahci_platform_sht = {
+	AHCI_SHT("ahci_platform"),
+};
+
 static int __init ahci_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;

@@ -145,7 +149,7 @@ static int __init ahci_probe(struct platform_device *pdev)
 	ahci_print_info(host, "platform");
 
 	rc = ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
-			       &ahci_sht);
+			       &ahci_platform_sht);
 	if (rc)
 		goto err0;
drivers/ata/libahci.c +4 -12
index 68dc678..8eea309 100644
@@ -121,7 +121,7 @@ static DEVICE_ATTR(ahci_port_cmd, S_IRUGO, ahci_show_port_cmd, NULL);
 static DEVICE_ATTR(em_buffer, S_IWUSR | S_IRUGO,
 		   ahci_read_em_buffer, ahci_store_em_buffer);
 
-static struct device_attribute *ahci_shost_attrs[] = {
+struct device_attribute *ahci_shost_attrs[] = {
 	&dev_attr_link_power_management_policy,
 	&dev_attr_em_message_type,
 	&dev_attr_em_message,

@@ -132,22 +132,14 @@ static struct device_attribute *ahci_shost_attrs[] = {
 	&dev_attr_em_buffer,
 	NULL
 };
+EXPORT_SYMBOL_GPL(ahci_shost_attrs);
 
-static struct device_attribute *ahci_sdev_attrs[] = {
+struct device_attribute *ahci_sdev_attrs[] = {
 	&dev_attr_sw_activity,
 	&dev_attr_unload_heads,
 	NULL
 };
-
-struct scsi_host_template ahci_sht = {
-	ATA_NCQ_SHT("ahci"),
-	.can_queue		= AHCI_MAX_CMDS - 1,
-	.sg_tablesize		= AHCI_MAX_SG,
-	.dma_boundary		= AHCI_DMA_BOUNDARY,
-	.shost_attrs		= ahci_shost_attrs,
-	.sdev_attrs		= ahci_sdev_attrs,
-};
-EXPORT_SYMBOL_GPL(ahci_sht);
+EXPORT_SYMBOL_GPL(ahci_sdev_attrs);
 
 struct ata_port_operations ahci_ops = {
 	.inherits		= &sata_pmp_port_ops,
#11 lore raw
Quoted 8 lines · Tejun Heo
Quoted 7 lines · Tejun Heo

Hello,

Quoted 3 lines · Pedro Francisco

To which tree shall I apply it? It currently fails both on Linus' linux-2.6 and
git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git .

Quoted 5 lines
Quoted 3 lines

Hmm... weird. It's against Linus' tree. Checking, yeap, it applies cleanly to 2.6.36-rc4, commit 2422084a (Merge branch 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/mattst88/alpha-2.6).

Attaching the patch just in case.

I tried it on 2.6.35 kernel. The ahci module usage count now increases and decreases properly, at least from what I see here. That's definitely better than previous 2.6.35 variant.

Thanks!

/mjt

#12 lore raw

PATCH 1/1 ahci: fix module refcount breakage introduced by libahci split

libata depends on scsi_host_template for module reference counting and sht's should be owned by each low level driver. During libahci split, the sht was left with libahci.ko leaving the actual low level drivers not reference counted. This made ahci and ahci_platform always unloadable even while they're being actively used.

Fix it by defining AHCI_SHT() macro in ahci.h and defining a sht for each low level ahci driver.

stable: only applicable to 2.6.35.

Signed-off-by Tejun Heo <[email protected]>
Reported-by Michael Tokarev <[email protected]>
Cc [email protected]

---
Michael, thanks a lot for spotting it and bearing with me.

 drivers/ata/ahci.c          |    4 ++++
 drivers/ata/ahci.h          |   12 +++++++++++-
 drivers/ata/ahci_platform.c |    6 +++++-
 drivers/ata/libahci.c       |   16 ++++------------
 4 files changed, 24 insertions(+), 14 deletions(-)
drivers/ata/ahci.c +4 -0
index ff1c945..99d0e5a 100644
@@ -90,6 +90,10 @@ static int ahci_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
 static int ahci_pci_device_resume(struct pci_dev *pdev);
 #endif
 
+static struct scsi_host_template ahci_sht = {
+	AHCI_SHT("ahci"),
+};
+
 static struct ata_port_operations ahci_vt8251_ops = {
 	.inherits		= &ahci_ops,
 	.hardreset		= ahci_vt8251_hardreset,
drivers/ata/ahci.h +11 -1
index 474427b..e5fdeeb 100644
@@ -298,7 +298,17 @@ struct ahci_host_priv {
 
 extern int ahci_ignore_sss;
 
-extern struct scsi_host_template ahci_sht;
+extern struct device_attribute *ahci_shost_attrs[];
+extern struct device_attribute *ahci_sdev_attrs[];
+
+#define AHCI_SHT(drv_name)						\
+	ATA_NCQ_SHT(drv_name),						\
+	.can_queue		= AHCI_MAX_CMDS - 1,			\
+	.sg_tablesize		= AHCI_MAX_SG,				\
+	.dma_boundary		= AHCI_DMA_BOUNDARY,			\
+	.shost_attrs		= ahci_shost_attrs,			\
+	.sdev_attrs		= ahci_sdev_attrs
+
 extern struct ata_port_operations ahci_ops;
 
 void ahci_save_initial_config(struct device *dev,
drivers/ata/ahci_platform.c +5 -1
index 4e97f33..84b6432 100644
@@ -23,6 +23,10 @@
 #include <linux/ahci_platform.h>
 #include "ahci.h"
 
+static struct scsi_host_template ahci_platform_sht = {
+	AHCI_SHT("ahci_platform"),
+};
+
 static int __init ahci_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;

@@ -145,7 +149,7 @@ static int __init ahci_probe(struct platform_device *pdev)
 	ahci_print_info(host, "platform");
 
 	rc = ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
-			       &ahci_sht);
+			       &ahci_platform_sht);
 	if (rc)
 		goto err0;
drivers/ata/libahci.c +4 -12
index 68dc678..8eea309 100644
@@ -121,7 +121,7 @@ static DEVICE_ATTR(ahci_port_cmd, S_IRUGO, ahci_show_port_cmd, NULL);
 static DEVICE_ATTR(em_buffer, S_IWUSR | S_IRUGO,
 		   ahci_read_em_buffer, ahci_store_em_buffer);
 
-static struct device_attribute *ahci_shost_attrs[] = {
+struct device_attribute *ahci_shost_attrs[] = {
 	&dev_attr_link_power_management_policy,
 	&dev_attr_em_message_type,
 	&dev_attr_em_message,

@@ -132,22 +132,14 @@ static struct device_attribute *ahci_shost_attrs[] = {
 	&dev_attr_em_buffer,
 	NULL
 };
+EXPORT_SYMBOL_GPL(ahci_shost_attrs);
 
-static struct device_attribute *ahci_sdev_attrs[] = {
+struct device_attribute *ahci_sdev_attrs[] = {
 	&dev_attr_sw_activity,
 	&dev_attr_unload_heads,
 	NULL
 };
-
-struct scsi_host_template ahci_sht = {
-	ATA_NCQ_SHT("ahci"),
-	.can_queue		= AHCI_MAX_CMDS - 1,
-	.sg_tablesize		= AHCI_MAX_SG,
-	.dma_boundary		= AHCI_DMA_BOUNDARY,
-	.shost_attrs		= ahci_shost_attrs,
-	.sdev_attrs		= ahci_sdev_attrs,
-};
-EXPORT_SYMBOL_GPL(ahci_sht);
+EXPORT_SYMBOL_GPL(ahci_sdev_attrs);
 
 struct ata_port_operations ahci_ops = {
 	.inherits		= &sata_pmp_port_ops,
#13 lore raw

Re: [PATCH #upstream-fixes] ahci: fix module refcount breakage introduced by libahci split

Quoted 9 lines · Tejun Heo

libata depends on scsi_host_template for module reference counting and sht's should be owned by each low level driver. During libahci split, the sht was left with libahci.ko leaving the actual low level drivers not reference counted. This made ahci and ahci_platform always unloadable even while they're being actively used.

Fix it by defining AHCI_SHT() macro in ahci.h and defining a sht for each low level ahci driver.

Quoted 7 lines

stable: only applicable to 2.6.35.

Signed-off-by Tejun Heo <[email protected]>
Reported-by Michael Tokarev <[email protected]>
Cc [email protected]

---
Michael, thanks a lot for spotting it and bearing with me.

Um. All thanks goes to Pedro Francisco please, not to me - I just verified your patch on my system, it was Pedro who spotted and reported this. You may add my

Tested-Off-By Michael Tokarev <[email protected]>

, but not Reported-by... please ;)

Thanks!

/mjt

Tested-by Michael Tokarev
#14 lore raw

Re: [PATCH #upstream-fixes] ahci: fix module refcount breakage introduced by libahci split

Quoted 17 lines · Michael Tokarev
Quoted 9 lines · Tejun Heo

libata depends on scsi_host_template for module reference counting and sht's should be owned by each low level driver. During libahci split, the sht was left with libahci.ko leaving the actual low level drivers not reference counted. This made ahci and ahci_platform always unloadable even while they're being actively used.

Fix it by defining AHCI_SHT() macro in ahci.h and defining a sht for each low level ahci driver.

Quoted 7 lines

stable: only applicable to 2.6.35.

Signed-off-by Tejun Heo <[email protected]>
Reported-by Michael Tokarev <[email protected]>
Cc [email protected]

---
Michael, thanks a lot for spotting it and bearing with me.

Quoted 7 lines

Um. All thanks goes to Pedro Francisco please, not to me - I just verified your patch on my system, it was Pedro who spotted and reported this. You may add my

Tested-Off-By Michael Tokarev <[email protected]>

, but not Reported-by... please ;)

Oh, right. Jeff, can you please adjust tags to

Reported-by Pedro Francisco <[email protected]>
Tested-by Michael Tokarev <[email protected]>

Thanks.

Signature · 1 line
tejun
author
#15 lore raw
In reply to Tejun Heo
Quoted 8 lines · Tejun Heo
Quoted 7 lines · Tejun Heo

Hello,

Quoted 3 lines · Pedro Francisco

To which tree shall I apply it? It currently fails both on Linus' linux-2.6 and
git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git .

Quoted 5 lines
Quoted 3 lines

Hmm... weird. It's against Linus' tree. Checking, yeap, it applies cleanly to 2.6.36-rc4, commit 2422084a (Merge branch 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/mattst88/alpha-2.6).

Attaching the patch just in case.

I must have forgotten "-p1" since it now worked fine.

Anyway, the patch works on 2.6.36-rc4 :)

Sorry for the late reply but compiling and running it took a bit longer than expected.

Signature · 1 line
Pedro